From be97e19b4a8a9cb3118a610fce1075eeffcfe26e Mon Sep 17 00:00:00 2001 From: Camila Macedo Date: Tue, 10 Sep 2024 20:17:25 +0100 Subject: [PATCH] fix: improve CronJob and multiversion tutorial by refining replaces without overwriting webhook markers Ensure we are more precise with operations to avoid overwriting the default scaffolding. Previously, the markers in the scaffold were unintentionally overwritten when injecting code and documentation, which could override crucial changes. This commit ensures the fix applied in PR #4122 is preserved by refining the tutorial instructions to avoid overwriting webhook markers while still injecting the necessary information. Either we should not export the consts used in the hack/docs since those are internal implementations, therefore the consts used in this area affected have their name changed --- .../project/api/v1/cronjob_webhook.go | 25 ++++----- .../project/config/webhook/manifests.yaml | 41 -------------- .../project/api/v1/cronjob_webhook.go | 25 ++++----- .../project/config/webhook/manifests.yaml | 41 -------------- .../cronjob-tutorial/generate_cronjob.go | 47 ++++++++-------- .../webhook_implementation.go | 53 ++++++++++--------- 6 files changed, 71 insertions(+), 161 deletions(-) diff --git a/docs/book/src/cronjob-tutorial/testdata/project/api/v1/cronjob_webhook.go b/docs/book/src/cronjob-tutorial/testdata/project/api/v1/cronjob_webhook.go index 47f015baa58..db59768a51a 100644 --- a/docs/book/src/cronjob-tutorial/testdata/project/api/v1/cronjob_webhook.go +++ b/docs/book/src/cronjob-tutorial/testdata/project/api/v1/cronjob_webhook.go @@ -66,13 +66,8 @@ This marker is responsible for generating a mutating webhook manifest. The meaning of each marker can be found [here](/reference/markers/webhook.md). */ -// +kubebuilder:webhook:path=/mutate-batch-tutorial-kubebuilder-io-v1-cronjob,mutating=true,failurePolicy=fail,groups=batch.tutorial.kubebuilder.io,resources=cronjobs,verbs=create;update,versions=v1,name=mcronjob.kb.io,sideEffects=None,admissionReviewVersions=v1 - /* -We use the `webhook.CustomDefaulter` interface to set defaults to our CRD. -A webhook will automatically be served that calls this defaulting. - -The `Default` method is expected to mutate the receiver, setting the defaults. +This marker is responsible for generating a mutation webhook manifest. */ // +kubebuilder:webhook:path=/mutate-batch-tutorial-kubebuilder-io-v1-cronjob,mutating=true,failurePolicy=fail,sideEffects=None,groups=batch.tutorial.kubebuilder.io,resources=cronjobs,verbs=create;update,versions=v1,name=mcronjob-v1.kb.io,admissionReviewVersions=v1 @@ -94,6 +89,13 @@ type CronJobCustomDefaulter struct { var _ webhook.CustomDefaulter = &CronJobCustomDefaulter{} +/* +We use the `webhook.CustomDefaulter`interface to set defaults to our CRD. +A webhook will automatically be served that calls this defaulting. + +The `Default`method is expected to mutate the receiver, setting the defaults. +*/ + // Default implements webhook.CustomDefaulter so a webhook will be registered for the Kind CronJob. func (d *CronJobCustomDefaulter) Default(ctx context.Context, obj runtime.Object) error { cronjob, ok := obj.(*CronJob) @@ -125,12 +127,6 @@ func (r *CronJob) Default() { } } -/* -This marker is responsible for generating a validating webhook manifest. -*/ - -// +kubebuilder:webhook:verbs=create;update;delete,path=/validate-batch-tutorial-kubebuilder-io-v1-cronjob,mutating=false,failurePolicy=fail,groups=batch.tutorial.kubebuilder.io,resources=cronjobs,versions=v1,name=vcronjob.kb.io,sideEffects=None,admissionReviewVersions=v1 - /* We can validate our CRD beyond what's possible with declarative validation. Generally, declarative validation should be sufficient, but @@ -153,8 +149,9 @@ Here, however, we just use the same shared validation for `ValidateCreate` and validate anything on deletion. */ -// NOTE: The 'path' attribute must follow a specific pattern and should not be modified directly here. -// Modifying the path for an invalid path can cause API server errors; failing to locate the webhook. +/* +This marker is responsible for generating a validation webhook manifest. +*/ // +kubebuilder:webhook:path=/validate-batch-tutorial-kubebuilder-io-v1-cronjob,mutating=false,failurePolicy=fail,sideEffects=None,groups=batch.tutorial.kubebuilder.io,resources=cronjobs,verbs=create;update,versions=v1,name=vcronjob-v1.kb.io,admissionReviewVersions=v1 // +kubebuilder:object:generate=false diff --git a/docs/book/src/cronjob-tutorial/testdata/project/config/webhook/manifests.yaml b/docs/book/src/cronjob-tutorial/testdata/project/config/webhook/manifests.yaml index d0d39428467..92fc8109797 100644 --- a/docs/book/src/cronjob-tutorial/testdata/project/config/webhook/manifests.yaml +++ b/docs/book/src/cronjob-tutorial/testdata/project/config/webhook/manifests.yaml @@ -24,26 +24,6 @@ webhooks: resources: - cronjobs sideEffects: None -- admissionReviewVersions: - - v1 - clientConfig: - service: - name: webhook-service - namespace: system - path: /mutate-batch-tutorial-kubebuilder-io-v1-cronjob - failurePolicy: Fail - name: mcronjob.kb.io - rules: - - apiGroups: - - batch.tutorial.kubebuilder.io - apiVersions: - - v1 - operations: - - CREATE - - UPDATE - resources: - - cronjobs - sideEffects: None --- apiVersion: admissionregistration.k8s.io/v1 kind: ValidatingWebhookConfiguration @@ -70,24 +50,3 @@ webhooks: resources: - cronjobs sideEffects: None -- admissionReviewVersions: - - v1 - clientConfig: - service: - name: webhook-service - namespace: system - path: /validate-batch-tutorial-kubebuilder-io-v1-cronjob - failurePolicy: Fail - name: vcronjob.kb.io - rules: - - apiGroups: - - batch.tutorial.kubebuilder.io - apiVersions: - - v1 - operations: - - CREATE - - UPDATE - - DELETE - resources: - - cronjobs - sideEffects: None diff --git a/docs/book/src/multiversion-tutorial/testdata/project/api/v1/cronjob_webhook.go b/docs/book/src/multiversion-tutorial/testdata/project/api/v1/cronjob_webhook.go index 40c435ffd46..98bdafe18c8 100644 --- a/docs/book/src/multiversion-tutorial/testdata/project/api/v1/cronjob_webhook.go +++ b/docs/book/src/multiversion-tutorial/testdata/project/api/v1/cronjob_webhook.go @@ -70,13 +70,8 @@ This marker is responsible for generating a mutating webhook manifest. The meaning of each marker can be found [here](/reference/markers/webhook.md). */ -// +kubebuilder:webhook:path=/mutate-batch-tutorial-kubebuilder-io-v1-cronjob,mutating=true,failurePolicy=fail,groups=batch.tutorial.kubebuilder.io,resources=cronjobs,verbs=create;update,versions=v1,name=mcronjob.kb.io,sideEffects=None,admissionReviewVersions=v1 - /* -We use the `webhook.CustomDefaulter` interface to set defaults to our CRD. -A webhook will automatically be served that calls this defaulting. - -The `Default` method is expected to mutate the receiver, setting the defaults. +This marker is responsible for generating a mutation webhook manifest. */ // +kubebuilder:webhook:path=/mutate-batch-tutorial-kubebuilder-io-v1-cronjob,mutating=true,failurePolicy=fail,sideEffects=None,groups=batch.tutorial.kubebuilder.io,resources=cronjobs,verbs=create;update,versions=v1,name=mcronjob-v1.kb.io,admissionReviewVersions=v1 @@ -98,6 +93,13 @@ type CronJobCustomDefaulter struct { var _ webhook.CustomDefaulter = &CronJobCustomDefaulter{} +/* +We use the `webhook.CustomDefaulter`interface to set defaults to our CRD. +A webhook will automatically be served that calls this defaulting. + +The `Default`method is expected to mutate the receiver, setting the defaults. +*/ + // Default implements webhook.CustomDefaulter so a webhook will be registered for the Kind CronJob. func (d *CronJobCustomDefaulter) Default(ctx context.Context, obj runtime.Object) error { cronjob, ok := obj.(*CronJob) @@ -129,12 +131,6 @@ func (r *CronJob) Default() { } } -/* -This marker is responsible for generating a validating webhook manifest. -*/ - -// +kubebuilder:webhook:verbs=create;update;delete,path=/validate-batch-tutorial-kubebuilder-io-v1-cronjob,mutating=false,failurePolicy=fail,groups=batch.tutorial.kubebuilder.io,resources=cronjobs,versions=v1,name=vcronjob.kb.io,sideEffects=None,admissionReviewVersions=v1 - /* We can validate our CRD beyond what's possible with declarative validation. Generally, declarative validation should be sufficient, but @@ -157,8 +153,9 @@ Here, however, we just use the same shared validation for `ValidateCreate` and validate anything on deletion. */ -// NOTE: The 'path' attribute must follow a specific pattern and should not be modified directly here. -// Modifying the path for an invalid path can cause API server errors; failing to locate the webhook. +/* +This marker is responsible for generating a validation webhook manifest. +*/ // +kubebuilder:webhook:path=/validate-batch-tutorial-kubebuilder-io-v1-cronjob,mutating=false,failurePolicy=fail,sideEffects=None,groups=batch.tutorial.kubebuilder.io,resources=cronjobs,verbs=create;update,versions=v1,name=vcronjob-v1.kb.io,admissionReviewVersions=v1 // +kubebuilder:object:generate=false diff --git a/docs/book/src/multiversion-tutorial/testdata/project/config/webhook/manifests.yaml b/docs/book/src/multiversion-tutorial/testdata/project/config/webhook/manifests.yaml index d9fc8e530c5..801980c73c4 100644 --- a/docs/book/src/multiversion-tutorial/testdata/project/config/webhook/manifests.yaml +++ b/docs/book/src/multiversion-tutorial/testdata/project/config/webhook/manifests.yaml @@ -24,26 +24,6 @@ webhooks: resources: - cronjobs sideEffects: None -- admissionReviewVersions: - - v1 - clientConfig: - service: - name: webhook-service - namespace: system - path: /mutate-batch-tutorial-kubebuilder-io-v1-cronjob - failurePolicy: Fail - name: mcronjob.kb.io - rules: - - apiGroups: - - batch.tutorial.kubebuilder.io - apiVersions: - - v1 - operations: - - CREATE - - UPDATE - resources: - - cronjobs - sideEffects: None - admissionReviewVersions: - v1 clientConfig: @@ -90,27 +70,6 @@ webhooks: resources: - cronjobs sideEffects: None -- admissionReviewVersions: - - v1 - clientConfig: - service: - name: webhook-service - namespace: system - path: /validate-batch-tutorial-kubebuilder-io-v1-cronjob - failurePolicy: Fail - name: vcronjob.kb.io - rules: - - apiGroups: - - batch.tutorial.kubebuilder.io - apiVersions: - - v1 - operations: - - CREATE - - UPDATE - - DELETE - resources: - - cronjobs - sideEffects: None - admissionReviewVersions: - v1 clientConfig: diff --git a/hack/docs/internal/cronjob-tutorial/generate_cronjob.go b/hack/docs/internal/cronjob-tutorial/generate_cronjob.go index 5fea5c3c86c..63515ab6d63 100644 --- a/hack/docs/internal/cronjob-tutorial/generate_cronjob.go +++ b/hack/docs/internal/cronjob-tutorial/generate_cronjob.go @@ -433,7 +433,7 @@ func (sp *Sample) updateWebhook() { // nolint:unused // log is for logging in this package. -`, WebhookIntro) +`, webhookIntro) hackutils.CheckError("fixing cronjob_webhook.go", err) err = pluginutil.InsertCode( @@ -447,8 +447,14 @@ Then, we set up the webhook with the manager. err = pluginutil.ReplaceInFile( filepath.Join(sp.ctx.Dir, "api/v1/cronjob_webhook.go"), - `// TODO(user): EDIT THIS FILE! THIS IS SCAFFOLDING FOR YOU TO OWN!`, WebhookMarker) - hackutils.CheckError("fixing cronjob_webhook.go by replacing TODO", err) + `// TODO(user): EDIT THIS FILE! THIS IS SCAFFOLDING FOR YOU TO OWN!`, webhooksNoticeMarker) + hackutils.CheckError("fixing cronjob_webhook.go by replacing note about path attribute", err) + + err = pluginutil.ReplaceInFile( + filepath.Join(sp.ctx.Dir, "api/v1/cronjob_webhook.go"), + `// NOTE: The 'path' attribute must follow a specific pattern and should not be modified directly here. +// Modifying the path for an invalid path can cause API server errors; failing to locate the webhook.`, explanationValidateCRD) + hackutils.CheckError("fixing cronjob_webhook.go by replacing note about path attribute", err) err = pluginutil.ReplaceInFile( filepath.Join(sp.ctx.Dir, "api/v1/cronjob_webhook.go"), @@ -474,7 +480,9 @@ Then, we set up the webhook with the manager. err = pluginutil.ReplaceInFile( filepath.Join(sp.ctx.Dir, "api/v1/cronjob_webhook.go"), `// TODO(user): fill in your defaulting logic. -`, WebhookDefaultingSettings) + + return nil +}`, webhookDefaultingSettings) hackutils.CheckError("fixing cronjob_webhook.go by adding logic", err) err = pluginutil.ReplaceInFile( @@ -482,8 +490,7 @@ Then, we set up the webhook with the manager. `// TODO(user): fill in your validation logic upon object creation. return nil, nil`, - ` - return nil, cronjob.validateCronJob()`) + `return nil, cronjob.validateCronJob()`) hackutils.CheckError("fixing cronjob_webhook.go by fill in your validation", err) err = pluginutil.ReplaceInFile( @@ -491,31 +498,19 @@ Then, we set up the webhook with the manager. `// TODO(user): fill in your validation logic upon object update. return nil, nil`, - ` - return nil, cronjob.validateCronJob()`) + `return nil, cronjob.validateCronJob()`) hackutils.CheckError("fixing cronjob_webhook.go by adding validation logic upon object update", err) - err = pluginutil.InsertCode( - filepath.Join(sp.ctx.Dir, "api/v1/cronjob_webhook.go"), - `// TODO(user): fill in your validation logic upon object deletion. - - return nil, nil -}`, WebhookValidateSpec) - - hackutils.CheckError("fixing cronjob_webhook.go upon object deletion", err) - err = pluginutil.ReplaceInFile( filepath.Join(sp.ctx.Dir, "api/v1/cronjob_webhook.go"), - `validate anything on deletion. -*/ - - return nil -}`, `validate anything on deletion. -*/ - -`) - hackutils.CheckError("fixing cronjob_webhook.go by removing wrong return nil", err) + `// Default implements webhook.CustomDefaulter so a webhook will be registered for the Kind CronJob.`, + customInterfaceDefaultInfo) + hackutils.CheckError("fixing cronjob_webhook.go by adding validation logic upon object update", err) + err = pluginutil.AppendCodeAtTheEnd( + filepath.Join(sp.ctx.Dir, "api/v1/cronjob_webhook.go"), + webhookValidateSpecMethods) + hackutils.CheckError("adding validation spec methods at the end", err) } func (sp *Sample) updateSuiteTest() { diff --git a/hack/docs/internal/cronjob-tutorial/webhook_implementation.go b/hack/docs/internal/cronjob-tutorial/webhook_implementation.go index ba7ec40e004..46bde114dea 100644 --- a/hack/docs/internal/cronjob-tutorial/webhook_implementation.go +++ b/hack/docs/internal/cronjob-tutorial/webhook_implementation.go @@ -16,7 +16,7 @@ limitations under the License. package cronjob -const WebhookIntro = `"sigs.k8s.io/controller-runtime/pkg/webhook/admission" +const webhookIntro = `"sigs.k8s.io/controller-runtime/pkg/webhook/admission" ) // +kubebuilder:docs-gen:collapse=Go imports @@ -27,26 +27,7 @@ Next, we'll setup a logger for the webhooks. ` -const WebhookMarker = `/* -Notice that we use kubebuilder markers to generate webhook manifests. -This marker is responsible for generating a mutating webhook manifest. - -The meaning of each marker can be found [here](/reference/markers/webhook.md). -*/ - -// +kubebuilder:webhook:path=/mutate-batch-tutorial-kubebuilder-io-v1-cronjob,mutating=true,failurePolicy=fail,groups=batch.tutorial.kubebuilder.io,resources=cronjobs,verbs=create;update,versions=v1,name=mcronjob.kb.io,sideEffects=None,admissionReviewVersions=v1 - -/* -We use the` + " `" + `webhook.CustomDefaulter` + "`" + ` interface to set defaults to our CRD. -A webhook will automatically be served that calls this defaulting. - -The` + " `" + `Default` + "`" + ` method is expected to mutate the receiver, setting the defaults. -*/ -` - -const WebhookDefaultingSettings = ` - - // Set default values +const webhookDefaultingSettings = `// Set default values cronjob.Default() return nil @@ -68,13 +49,22 @@ func (r *CronJob) Default() { *r.Spec.FailedJobsHistoryLimit = 1 } } +` +const webhooksNoticeMarker = ` /* -This marker is responsible for generating a validating webhook manifest. +Notice that we use kubebuilder markers to generate webhook manifests. +This marker is responsible for generating a mutating webhook manifest. + +The meaning of each marker can be found [here](/reference/markers/webhook.md). */ -// +kubebuilder:webhook:verbs=create;update;delete,path=/validate-batch-tutorial-kubebuilder-io-v1-cronjob,mutating=false,failurePolicy=fail,groups=batch.tutorial.kubebuilder.io,resources=cronjobs,versions=v1,name=vcronjob.kb.io,sideEffects=None,admissionReviewVersions=v1 +/* +This marker is responsible for generating a mutation webhook manifest. +*/ +` +const explanationValidateCRD = ` /* We can validate our CRD beyond what's possible with declarative validation. Generally, declarative validation should be sufficient, but @@ -96,8 +86,21 @@ Here, however, we just use the same shared validation for` + " `" + `ValidateCre ` + "`" + `ValidateUpdate` + "`" + `. And we do nothing in` + " `" + `ValidateDelete` + "`" + `, since we don't need to validate anything on deletion. */ -` -const WebhookValidateSpec = ` + +/* +This marker is responsible for generating a validation webhook manifest. +*/` + +const customInterfaceDefaultInfo = `/* +We use the ` + "`" + `webhook.CustomDefaulter` + "`" + `interface to set defaults to our CRD. +A webhook will automatically be served that calls this defaulting. + +The ` + "`" + `Default` + "`" + `method is expected to mutate the receiver, setting the defaults. +*/ + +// Default implements webhook.CustomDefaulter so a webhook will be registered for the Kind CronJob.` + +const webhookValidateSpecMethods = ` /* We validate the name and the spec of the CronJob. */