From 0bbf858cd9fb866a1052d39cec1a4c17679ad7ad Mon Sep 17 00:00:00 2001 From: Camila Macedo <7708031+camilamacedo86@users.noreply.github.com> Date: Fri, 28 Feb 2025 09:26:20 +0000 Subject: [PATCH] (helm/v1-alpha): Fix helm-chart scaffold by not scaffolding manifests with webhook conditions when webhooks are not used --- .../templates/certmanager/certificate.yaml | 24 ----- .../dist/chart/templates/manager/manager.yaml | 4 +- .../optional/helm/v1alpha/scaffolds/init.go | 89 ++++++++++++++++--- .../cert-manager/certificate.go | 5 ++ .../chart-templates/manager/manager.go | 8 ++ 5 files changed, 94 insertions(+), 36 deletions(-) diff --git a/docs/book/src/getting-started/testdata/project/dist/chart/templates/certmanager/certificate.yaml b/docs/book/src/getting-started/testdata/project/dist/chart/templates/certmanager/certificate.yaml index cf309d66456..aa1731782bf 100644 --- a/docs/book/src/getting-started/testdata/project/dist/chart/templates/certmanager/certificate.yaml +++ b/docs/book/src/getting-started/testdata/project/dist/chart/templates/certmanager/certificate.yaml @@ -9,30 +9,6 @@ metadata: namespace: {{ .Release.Namespace }} spec: selfSigned: {} -{{- if .Values.webhook.enable }} ---- -# Certificate for the webhook -apiVersion: cert-manager.io/v1 -kind: Certificate -metadata: - annotations: - {{- if .Values.crd.keep }} - "helm.sh/resource-policy": keep - {{- end }} - name: serving-cert - namespace: {{ .Release.Namespace }} - labels: - {{- include "chart.labels" . | nindent 4 }} -spec: - dnsNames: - - project.{{ .Release.Namespace }}.svc - - project.{{ .Release.Namespace }}.svc.cluster.local - - project-webhook-service.{{ .Release.Namespace }}.svc - issuerRef: - kind: Issuer - name: selfsigned-issuer - secretName: webhook-server-cert -{{- end }} {{- if .Values.metrics.enable }} --- # Certificate for the metrics diff --git a/docs/book/src/getting-started/testdata/project/dist/chart/templates/manager/manager.yaml b/docs/book/src/getting-started/testdata/project/dist/chart/templates/manager/manager.yaml index eb46406767e..2fecf33314f 100644 --- a/docs/book/src/getting-started/testdata/project/dist/chart/templates/manager/manager.yaml +++ b/docs/book/src/getting-started/testdata/project/dist/chart/templates/manager/manager.yaml @@ -49,7 +49,7 @@ spec: {{- toYaml .Values.controllerManager.container.resources | nindent 12 }} securityContext: {{- toYaml .Values.controllerManager.container.securityContext | nindent 12 }} - {{- if and .Values.certmanager.enable (or .Values.webhook.enable .Values.metrics.enable) }} + {{- if and .Values.certmanager.enable .Values.metrics.enable }} volumeMounts: {{- if and .Values.metrics.enable .Values.certmanager.enable }} - name: metrics-certs @@ -61,7 +61,7 @@ spec: {{- toYaml .Values.controllerManager.securityContext | nindent 8 }} serviceAccountName: {{ .Values.controllerManager.serviceAccountName }} terminationGracePeriodSeconds: {{ .Values.controllerManager.terminationGracePeriodSeconds }} - {{- if and .Values.certmanager.enable (or .Values.webhook.enable .Values.metrics.enable) }} + {{- if and .Values.certmanager.enable .Values.metrics.enable }} volumes: {{- if and .Values.metrics.enable .Values.certmanager.enable }} - name: metrics-certs diff --git a/pkg/plugins/optional/helm/v1alpha/scaffolds/init.go b/pkg/plugins/optional/helm/v1alpha/scaffolds/init.go index 1f0ef729a3a..ba579570e17 100644 --- a/pkg/plugins/optional/helm/v1alpha/scaffolds/init.go +++ b/pkg/plugins/optional/helm/v1alpha/scaffolds/init.go @@ -71,16 +71,17 @@ func (s *initScaffolder) Scaffold() error { imagesEnvVars := s.getDeployImagesEnvVars() + scaffold := machinery.NewScaffold(s.fs, + machinery.WithConfig(s.config), + ) + + // Found webhooks by looking at the config our scaffolds files mutatingWebhooks, validatingWebhooks, err := s.extractWebhooksFromGeneratedFiles() if err != nil { return fmt.Errorf("failed to extract webhooks: %w", err) } + hasWebhooks := hasWebhooks(s.config) || (len(mutatingWebhooks) > 0 && len(validatingWebhooks) > 0) - scaffold := machinery.NewScaffold(s.fs, - machinery.WithConfig(s.config), - ) - - hasWebhooks := len(mutatingWebhooks) > 0 || len(validatingWebhooks) > 0 buildScaffold := []machinery.Builder{ &github.HelmChartCI{}, &templates.HelmChart{}, @@ -96,7 +97,7 @@ func (s *initScaffolder) Scaffold() error { DeployImages: len(imagesEnvVars) > 0, HasWebhooks: hasWebhooks, }, - &templatescertmanager.Certificate{}, + &templatescertmanager.Certificate{HasWebhooks: hasWebhooks}, &templatesmetrics.Service{}, &prometheus.Monitor{}, } @@ -107,6 +108,11 @@ func (s *initScaffolder) Scaffold() error { MutatingWebhooks: mutatingWebhooks, ValidatingWebhooks: validatingWebhooks, }, + ) + } + + if hasWebhooks { + buildScaffold = append(buildScaffold, &templateswebhooks.Service{}, ) } @@ -254,7 +260,22 @@ func (s *initScaffolder) copyConfigFiles() error { for _, srcFile := range files { destFile := filepath.Join(dir.DestDir, filepath.Base(srcFile)) - err := copyFileWithHelmLogic(srcFile, destFile, dir.SubDir, s.config.GetProjectName()) + + hasConvertionalWebhook := false + if hasWebhooks(s.config) { + resources, err := s.config.GetResources() + if err != nil { + break + } + for _, res := range resources { + if res.HasConversionWebhook() { + hasConvertionalWebhook = true + break + } + } + } + + err := copyFileWithHelmLogic(srcFile, destFile, dir.SubDir, s.config.GetProjectName(), hasConvertionalWebhook) if err != nil { return err } @@ -266,7 +287,7 @@ func (s *initScaffolder) copyConfigFiles() error { // copyFileWithHelmLogic reads the source file, modifies the content for Helm, applies patches // to spec.conversion if applicable, and writes it to the destination -func copyFileWithHelmLogic(srcFile, destFile, subDir, projectName string) error { +func copyFileWithHelmLogic(srcFile, destFile, subDir, projectName string, hasConvertionalWebhook bool) error { if _, err := os.Stat(srcFile); os.IsNotExist(err) { log.Printf("Source file does not exist: %s", srcFile) return err @@ -351,8 +372,40 @@ func copyFileWithHelmLogic(srcFile, destFile, subDir, projectName string) error // If patch content exists, inject it under spec.conversion with Helm conditional if patchExists { conversionSpec := extractConversionSpec(patchContent) - contentStr = injectConversionSpecWithCondition(contentStr, conversionSpec) - hasWebhookPatch = true + // Projects scaffolded with old Kubebuilder versions does not have the conversion + // webhook properly generated because before 4.4.0 this feature was not fully addressed. + // The patch was added by default when should not. See the related fixes: + // + // Issue fixed in release 4.3.1: (which will cause the injection of webhook conditionals for projects without + // conversion webhooks) + // (kustomize/v2, go/v4): Corrected the generation of manifests under config/crd/patches + // to ensure the /convert service patch is only created for webhooks configured with --conversion. (#4280) + // + // Conversion webhook fully fixed in release 4.4.0: + // (kustomize/v2, go/v4): Fixed CA injection for conversion webhooks. Previously, the CA injection + // was applied incorrectly to all CRDs instead of only conversion types. The issue dates back to release 3.5.0 + // due to kustomize/v2-alpha changes. Now, conversion webhooks are properly generated. (#4254, #4282) + if len(conversionSpec) > 0 && !hasConvertionalWebhook { + log.Warn("\n" + + "============================================================\n" + + "| [WARNING] Webhook Patch Issue Detected |\n" + + "============================================================\n" + + "Webhook patch found, but no conversion webhook is configured for this project.\n\n" + + "Note: Older scaffolds have an issue where the conversion webhook patch was \n" + + " scaffolded by default, and conversion webhook injection was not properly limited \n" + + " to specific CRDs.\n\n" + + "Recommended Action:\n" + + " - Upgrade your project to the latest available version.\n" + + " - Consider using the 'alpha generate' command.\n\n" + + "The cert-manager injection and webhook conversion patch found for CRDs will\n" + + "be skipped and NOT added to the Helm chart.\n" + + "============================================================") + + hasWebhookPatch = false + } else { + contentStr = injectConversionSpecWithCondition(contentStr, conversionSpec) + hasWebhookPatch = true + } } // Inject annotations after "annotations:" in a single block without extra spaces @@ -489,3 +542,19 @@ func removeLabels(content string) string { return re.ReplaceAllString(content, "") } + +func hasWebhooks(c config.Config) bool { + // Get the list of resources + resources, err := c.GetResources() + if err != nil { + return false // If there's an error getting resources, assume no webhooks + } + + for _, res := range resources { + if res.HasDefaultingWebhook() || res.HasValidationWebhook() || res.HasConversionWebhook() { + return true + } + } + + return false +} diff --git a/pkg/plugins/optional/helm/v1alpha/scaffolds/internal/templates/chart-templates/cert-manager/certificate.go b/pkg/plugins/optional/helm/v1alpha/scaffolds/internal/templates/chart-templates/cert-manager/certificate.go index 7ea6b35c86e..01a90f4f8cb 100644 --- a/pkg/plugins/optional/helm/v1alpha/scaffolds/internal/templates/chart-templates/cert-manager/certificate.go +++ b/pkg/plugins/optional/helm/v1alpha/scaffolds/internal/templates/chart-templates/cert-manager/certificate.go @@ -27,6 +27,9 @@ var _ machinery.Template = &Certificate{} type Certificate struct { machinery.TemplateMixin machinery.ProjectNameMixin + + // HasWebhooks is true when webhooks were found in the config + HasWebhooks bool } // SetTemplateDefaults sets the default template configuration @@ -53,6 +56,7 @@ metadata: namespace: {{ "{{ .Release.Namespace }}" }} spec: selfSigned: {} +{{- if .HasWebhooks }} {{ "{{- if .Values.webhook.enable }}" }} --- # Certificate for the webhook @@ -77,6 +81,7 @@ spec: name: selfsigned-issuer secretName: webhook-server-cert {{` + "`" + `{{- end }}` + "`" + `}} +{{- end }} {{ "{{- if .Values.metrics.enable }}" }} --- # Certificate for the metrics diff --git a/pkg/plugins/optional/helm/v1alpha/scaffolds/internal/templates/chart-templates/manager/manager.go b/pkg/plugins/optional/helm/v1alpha/scaffolds/internal/templates/chart-templates/manager/manager.go index fc0d784c8ab..e6b2cdf36ba 100644 --- a/pkg/plugins/optional/helm/v1alpha/scaffolds/internal/templates/chart-templates/manager/manager.go +++ b/pkg/plugins/optional/helm/v1alpha/scaffolds/internal/templates/chart-templates/manager/manager.go @@ -114,7 +114,11 @@ spec: {{ "{{- toYaml .Values.controllerManager.container.resources | nindent 12 }}" }} securityContext: {{ "{{- toYaml .Values.controllerManager.container.securityContext | nindent 12 }}" }} +{{- if .HasWebhooks }} {{ "{{- if and .Values.certmanager.enable (or .Values.webhook.enable .Values.metrics.enable) }}" }} +{{- else }} + {{ "{{- if and .Values.certmanager.enable .Values.metrics.enable }}" }} +{{- end }} volumeMounts: {{- if .HasWebhooks }} {{ "{{- if and .Values.webhook.enable .Values.certmanager.enable }}" }} @@ -133,7 +137,11 @@ spec: {{ "{{- toYaml .Values.controllerManager.securityContext | nindent 8 }}" }} serviceAccountName: {{ "{{ .Values.controllerManager.serviceAccountName }}" }} terminationGracePeriodSeconds: {{ "{{ .Values.controllerManager.terminationGracePeriodSeconds }}" }} +{{- if .HasWebhooks }} {{ "{{- if and .Values.certmanager.enable (or .Values.webhook.enable .Values.metrics.enable) }}" }} +{{- else }} + {{ "{{- if and .Values.certmanager.enable .Values.metrics.enable }}" }} +{{- end }} volumes: {{- if .HasWebhooks }} {{ "{{- if and .Values.webhook.enable .Values.certmanager.enable }}" }}