Skip to content

Commit d90d095

Browse files
authored
Merge pull request #4584 from camilamacedo86/fix-helm-no-webhook
🐛 (helm/v1-alpha): not scaffold webhooks conditionals manifests for projects without webhooks
2 parents 3ae9604 + 8e0dc31 commit d90d095

File tree

5 files changed

+94
-36
lines changed

5 files changed

+94
-36
lines changed

docs/book/src/getting-started/testdata/project/dist/chart/templates/certmanager/certificate.yaml

Lines changed: 0 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -9,30 +9,6 @@ metadata:
99
namespace: {{ .Release.Namespace }}
1010
spec:
1111
selfSigned: {}
12-
{{- if .Values.webhook.enable }}
13-
---
14-
# Certificate for the webhook
15-
apiVersion: cert-manager.io/v1
16-
kind: Certificate
17-
metadata:
18-
annotations:
19-
{{- if .Values.crd.keep }}
20-
"helm.sh/resource-policy": keep
21-
{{- end }}
22-
name: serving-cert
23-
namespace: {{ .Release.Namespace }}
24-
labels:
25-
{{- include "chart.labels" . | nindent 4 }}
26-
spec:
27-
dnsNames:
28-
- project.{{ .Release.Namespace }}.svc
29-
- project.{{ .Release.Namespace }}.svc.cluster.local
30-
- project-webhook-service.{{ .Release.Namespace }}.svc
31-
issuerRef:
32-
kind: Issuer
33-
name: selfsigned-issuer
34-
secretName: webhook-server-cert
35-
{{- end }}
3612
{{- if .Values.metrics.enable }}
3713
---
3814
# Certificate for the metrics

docs/book/src/getting-started/testdata/project/dist/chart/templates/manager/manager.yaml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ spec:
4949
{{- toYaml .Values.controllerManager.container.resources | nindent 12 }}
5050
securityContext:
5151
{{- toYaml .Values.controllerManager.container.securityContext | nindent 12 }}
52-
{{- if and .Values.certmanager.enable (or .Values.webhook.enable .Values.metrics.enable) }}
52+
{{- if and .Values.certmanager.enable .Values.metrics.enable }}
5353
volumeMounts:
5454
{{- if and .Values.metrics.enable .Values.certmanager.enable }}
5555
- name: metrics-certs
@@ -61,7 +61,7 @@ spec:
6161
{{- toYaml .Values.controllerManager.securityContext | nindent 8 }}
6262
serviceAccountName: {{ .Values.controllerManager.serviceAccountName }}
6363
terminationGracePeriodSeconds: {{ .Values.controllerManager.terminationGracePeriodSeconds }}
64-
{{- if and .Values.certmanager.enable (or .Values.webhook.enable .Values.metrics.enable) }}
64+
{{- if and .Values.certmanager.enable .Values.metrics.enable }}
6565
volumes:
6666
{{- if and .Values.metrics.enable .Values.certmanager.enable }}
6767
- name: metrics-certs

pkg/plugins/optional/helm/v1alpha/scaffolds/init.go

Lines changed: 79 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -71,16 +71,17 @@ func (s *initScaffolder) Scaffold() error {
7171

7272
imagesEnvVars := s.getDeployImagesEnvVars()
7373

74+
scaffold := machinery.NewScaffold(s.fs,
75+
machinery.WithConfig(s.config),
76+
)
77+
78+
// Found webhooks by looking at the config our scaffolds files
7479
mutatingWebhooks, validatingWebhooks, err := s.extractWebhooksFromGeneratedFiles()
7580
if err != nil {
7681
return fmt.Errorf("failed to extract webhooks: %w", err)
7782
}
83+
hasWebhooks := hasWebhooksWith(s.config) || (len(mutatingWebhooks) > 0 && len(validatingWebhooks) > 0)
7884

79-
scaffold := machinery.NewScaffold(s.fs,
80-
machinery.WithConfig(s.config),
81-
)
82-
83-
hasWebhooks := len(mutatingWebhooks) > 0 || len(validatingWebhooks) > 0
8485
buildScaffold := []machinery.Builder{
8586
&github.HelmChartCI{},
8687
&templates.HelmChart{},
@@ -96,7 +97,7 @@ func (s *initScaffolder) Scaffold() error {
9697
DeployImages: len(imagesEnvVars) > 0,
9798
HasWebhooks: hasWebhooks,
9899
},
99-
&templatescertmanager.Certificate{},
100+
&templatescertmanager.Certificate{HasWebhooks: hasWebhooks},
100101
&templatesmetrics.Service{},
101102
&prometheus.Monitor{},
102103
}
@@ -107,6 +108,11 @@ func (s *initScaffolder) Scaffold() error {
107108
MutatingWebhooks: mutatingWebhooks,
108109
ValidatingWebhooks: validatingWebhooks,
109110
},
111+
)
112+
}
113+
114+
if hasWebhooks {
115+
buildScaffold = append(buildScaffold,
110116
&templateswebhooks.Service{},
111117
)
112118
}
@@ -255,7 +261,22 @@ func (s *initScaffolder) copyConfigFiles() error {
255261

256262
for _, srcFile := range files {
257263
destFile := filepath.Join(dir.DestDir, filepath.Base(srcFile))
258-
err := copyFileWithHelmLogic(srcFile, destFile, dir.SubDir, s.config.GetProjectName())
264+
265+
hasConvertionalWebhook := false
266+
if hasWebhooksWith(s.config) {
267+
resources, err := s.config.GetResources()
268+
if err != nil {
269+
break
270+
}
271+
for _, res := range resources {
272+
if res.HasConversionWebhook() {
273+
hasConvertionalWebhook = true
274+
break
275+
}
276+
}
277+
}
278+
279+
err := copyFileWithHelmLogic(srcFile, destFile, dir.SubDir, s.config.GetProjectName(), hasConvertionalWebhook)
259280
if err != nil {
260281
return err
261282
}
@@ -267,7 +288,7 @@ func (s *initScaffolder) copyConfigFiles() error {
267288

268289
// copyFileWithHelmLogic reads the source file, modifies the content for Helm, applies patches
269290
// to spec.conversion if applicable, and writes it to the destination
270-
func copyFileWithHelmLogic(srcFile, destFile, subDir, projectName string) error {
291+
func copyFileWithHelmLogic(srcFile, destFile, subDir, projectName string, hasConvertionalWebhook bool) error {
271292
if _, err := os.Stat(srcFile); os.IsNotExist(err) {
272293
log.Printf("Source file does not exist: %s", srcFile)
273294
return err
@@ -352,8 +373,40 @@ func copyFileWithHelmLogic(srcFile, destFile, subDir, projectName string) error
352373
// If patch content exists, inject it under spec.conversion with Helm conditional
353374
if patchExists {
354375
conversionSpec := extractConversionSpec(patchContent)
355-
contentStr = injectConversionSpecWithCondition(contentStr, conversionSpec)
356-
hasWebhookPatch = true
376+
// Projects scaffolded with old Kubebuilder versions does not have the conversion
377+
// webhook properly generated because before 4.4.0 this feature was not fully addressed.
378+
// The patch was added by default when should not. See the related fixes:
379+
//
380+
// Issue fixed in release 4.3.1: (which will cause the injection of webhook conditionals for projects without
381+
// conversion webhooks)
382+
// (kustomize/v2, go/v4): Corrected the generation of manifests under config/crd/patches
383+
// to ensure the /convert service patch is only created for webhooks configured with --conversion. (#4280)
384+
//
385+
// Conversion webhook fully fixed in release 4.4.0:
386+
// (kustomize/v2, go/v4): Fixed CA injection for conversion webhooks. Previously, the CA injection
387+
// was applied incorrectly to all CRDs instead of only conversion types. The issue dates back to release 3.5.0
388+
// due to kustomize/v2-alpha changes. Now, conversion webhooks are properly generated. (#4254, #4282)
389+
if len(conversionSpec) > 0 && !hasConvertionalWebhook {
390+
log.Warn("\n" +
391+
"============================================================\n" +
392+
"| [WARNING] Webhook Patch Issue Detected |\n" +
393+
"============================================================\n" +
394+
"Webhook patch found, but no conversion webhook is configured for this project.\n\n" +
395+
"Note: Older scaffolds have an issue where the conversion webhook patch was \n" +
396+
" scaffolded by default, and conversion webhook injection was not properly limited \n" +
397+
" to specific CRDs.\n\n" +
398+
"Recommended Action:\n" +
399+
" - Upgrade your project to the latest available version.\n" +
400+
" - Consider using the 'alpha generate' command.\n\n" +
401+
"The cert-manager injection and webhook conversion patch found for CRDs will\n" +
402+
"be skipped and NOT added to the Helm chart.\n" +
403+
"============================================================")
404+
405+
hasWebhookPatch = false
406+
} else {
407+
contentStr = injectConversionSpecWithCondition(contentStr, conversionSpec)
408+
hasWebhookPatch = true
409+
}
357410
}
358411

359412
// Inject annotations after "annotations:" in a single block without extra spaces
@@ -490,3 +543,19 @@ func removeLabels(content string) string {
490543

491544
return re.ReplaceAllString(content, "")
492545
}
546+
547+
func hasWebhooksWith(c config.Config) bool {
548+
// Get the list of resources
549+
resources, err := c.GetResources()
550+
if err != nil {
551+
return false // If there's an error getting resources, assume no webhooks
552+
}
553+
554+
for _, res := range resources {
555+
if res.HasDefaultingWebhook() || res.HasValidationWebhook() || res.HasConversionWebhook() {
556+
return true
557+
}
558+
}
559+
560+
return false
561+
}

pkg/plugins/optional/helm/v1alpha/scaffolds/internal/templates/chart-templates/cert-manager/certificate.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,9 @@ var _ machinery.Template = &Certificate{}
2727
type Certificate struct {
2828
machinery.TemplateMixin
2929
machinery.ProjectNameMixin
30+
31+
// HasWebhooks is true when webhooks were found in the config
32+
HasWebhooks bool
3033
}
3134

3235
// SetTemplateDefaults sets the default template configuration
@@ -53,6 +56,7 @@ metadata:
5356
namespace: {{ "{{ .Release.Namespace }}" }}
5457
spec:
5558
selfSigned: {}
59+
{{- if .HasWebhooks }}
5660
{{ "{{- if .Values.webhook.enable }}" }}
5761
---
5862
# Certificate for the webhook
@@ -77,6 +81,7 @@ spec:
7781
name: selfsigned-issuer
7882
secretName: webhook-server-cert
7983
{{` + "`" + `{{- end }}` + "`" + `}}
84+
{{- end }}
8085
{{ "{{- if .Values.metrics.enable }}" }}
8186
---
8287
# Certificate for the metrics

pkg/plugins/optional/helm/v1alpha/scaffolds/internal/templates/chart-templates/manager/manager.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,11 @@ spec:
114114
{{ "{{- toYaml .Values.controllerManager.container.resources | nindent 12 }}" }}
115115
securityContext:
116116
{{ "{{- toYaml .Values.controllerManager.container.securityContext | nindent 12 }}" }}
117+
{{- if .HasWebhooks }}
117118
{{ "{{- if and .Values.certmanager.enable (or .Values.webhook.enable .Values.metrics.enable) }}" }}
119+
{{- else }}
120+
{{ "{{- if and .Values.certmanager.enable .Values.metrics.enable }}" }}
121+
{{- end }}
118122
volumeMounts:
119123
{{- if .HasWebhooks }}
120124
{{ "{{- if and .Values.webhook.enable .Values.certmanager.enable }}" }}
@@ -133,7 +137,11 @@ spec:
133137
{{ "{{- toYaml .Values.controllerManager.securityContext | nindent 8 }}" }}
134138
serviceAccountName: {{ "{{ .Values.controllerManager.serviceAccountName }}" }}
135139
terminationGracePeriodSeconds: {{ "{{ .Values.controllerManager.terminationGracePeriodSeconds }}" }}
140+
{{- if .HasWebhooks }}
136141
{{ "{{- if and .Values.certmanager.enable (or .Values.webhook.enable .Values.metrics.enable) }}" }}
142+
{{- else }}
143+
{{ "{{- if and .Values.certmanager.enable .Values.metrics.enable }}" }}
144+
{{- end }}
137145
volumes:
138146
{{- if .HasWebhooks }}
139147
{{ "{{- if and .Values.webhook.enable .Values.certmanager.enable }}" }}

0 commit comments

Comments
 (0)