Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🐛 (helm/v1-alpha): not scaffold webhooks conditionals manifests for projects without webhooks #4584

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+94 −36
Open

🐛 (helm/v1-alpha): not scaffold webhooks conditionals manifests for projects without webhooks #4584

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 0 additions & 24 deletions ...ok/src/getting-started/testdata/project/dist/chart/templates/certmanager/certificate.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions docs/book/src/getting-started/testdata/project/dist/chart/templates/manager/manager.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
89 changes: 79 additions & 10 deletions pkg/plugins/optional/helm/v1alpha/scaffolds/init.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{},
Expand All @@ -96,7 +97,7 @@ func (s *initScaffolder) Scaffold() error {
DeployImages: len(imagesEnvVars) > 0,
HasWebhooks: hasWebhooks,
},
&templatescertmanager.Certificate{},
&templatescertmanager.Certificate{HasWebhooks: hasWebhooks},
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry Camila, just wondered whether it is being updated due to scaffalding webhooks conditional issues.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are passing the value HasWebhooks here because then inside of the boilerplate to generate the Certs we just generate those which related to Webhook Sever only when we have Webhooks

&templatesmetrics.Service{},
&prometheus.Monitor{},
}
Expand All @@ -107,6 +108,11 @@ func (s *initScaffolder) Scaffold() error {
MutatingWebhooks: mutatingWebhooks,
ValidatingWebhooks: validatingWebhooks,
},
)
}

if hasWebhooks {
buildScaffold = append(buildScaffold,
&templateswebhooks.Service{},
)
}
Expand Down Expand Up @@ -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
}
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}
5 changes: 5 additions & 0 deletions ...nal/helm/v1alpha/scaffolds/internal/templates/chart-templates/cert-manager/certificate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -53,6 +56,7 @@ metadata:
namespace: {{ "{{ .Release.Namespace }}" }}
spec:
selfSigned: {}
{{- if .HasWebhooks }}
{{ "{{- if .Values.webhook.enable }}" }}
---
# Certificate for the webhook
Expand All @@ -77,6 +81,7 @@ spec:
name: selfsigned-issuer
secretName: webhook-server-cert
{{` + "`" + `{{- end }}` + "`" + `}}
{{- end }}
{{ "{{- if .Values.metrics.enable }}" }}
---
# Certificate for the metrics
Expand Down
8 changes: 8 additions & 0 deletions ...ins/optional/helm/v1alpha/scaffolds/internal/templates/chart-templates/manager/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }}" }}
Expand All @@ -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 }}" }}
Expand Down
Loading