Skip to content

Commit e1333e2

Browse files
(helm/v1-alpha): Fix helm-chart scaffold by not scaffolding manifests with webhook conditions when webhooks are not used
1 parent 8c817f5 commit e1333e2

File tree

7 files changed

+97
-36
lines changed

7 files changed

+97
-36
lines changed

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

-24
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

+2-2
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/config/interface.go

+2
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,8 @@ type Config interface {
7676
AddResource(res resource.Resource) error
7777
// UpdateResource adds the provided resource if it was not present, modifies it if it was already present.
7878
UpdateResource(res resource.Resource) error
79+
// HasWebhooks returns true if any stored resource has webhooks.
80+
HasWebhooks() bool
7981

8082
// HasGroup checks if the provided group is the same as any of the tracked resources.
8183
HasGroup(group string) bool

pkg/config/v3/config.go

+17
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,23 @@ func (c Cfg) HasResource(gvk resource.GVK) bool {
172172
return found
173173
}
174174

175+
// HasWebhooks checks if any stored resource has associated webhooks.
176+
func (c Cfg) HasWebhooks() bool {
177+
// Get the list of resources
178+
resources, err := c.GetResources()
179+
if err != nil {
180+
return false // If there's an error getting resources, assume no webhooks
181+
}
182+
183+
for _, res := range resources {
184+
if res.HasDefaultingWebhook() || res.HasValidationWebhook() || res.HasConversionWebhook() {
185+
return true
186+
}
187+
}
188+
189+
return false
190+
}
191+
175192
// GetResource implements config.Config
176193
func (c Cfg) GetResource(gvk resource.GVK) (resource.Resource, error) {
177194
for _, res := range c.Resources {

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

+63-10
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 := s.config.HasWebhooks() || (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 s.config.HasWebhooks() {
115+
buildScaffold = append(buildScaffold,
110116
&templateswebhooks.Service{},
111117
)
112118
}
@@ -243,7 +249,22 @@ func (s *initScaffolder) copyConfigFiles() error {
243249

244250
for _, srcFile := range files {
245251
destFile := filepath.Join(dir.DestDir, filepath.Base(srcFile))
246-
err := copyFileWithHelmLogic(srcFile, destFile, dir.SubDir, s.config.GetProjectName())
252+
253+
hasConvertionalWebhook := false
254+
if s.config.HasWebhooks() {
255+
resources, err := s.config.GetResources()
256+
if err != nil {
257+
break
258+
}
259+
for _, res := range resources {
260+
if res.HasConversionWebhook() {
261+
hasConvertionalWebhook = true
262+
break
263+
}
264+
}
265+
}
266+
267+
err := copyFileWithHelmLogic(srcFile, destFile, dir.SubDir, s.config.GetProjectName(), hasConvertionalWebhook)
247268
if err != nil {
248269
return err
249270
}
@@ -255,7 +276,7 @@ func (s *initScaffolder) copyConfigFiles() error {
255276

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

347400
// Inject annotations after "annotations:" in a single block without extra spaces

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

+5
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

+8
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)