[koptan] add cert-manager and webhook resources to helm chart - #10
Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends the charts/koptan Helm chart to support admission webhooks and TLS management via cert-manager, adding the necessary Services and cert-manager resources and wiring TLS-related flags/volumes into the controller Deployment.
Changes:
- Add
MutatingWebhookConfigurationandValidatingWebhookConfigurationresources (5 webhooks each) plus a dedicated webhookService. - Add cert-manager
IssuerandCertificateresources for webhook and metrics TLS. - Update the controller
Deploymentto expose webhook/metrics ports and mount cert secrets conditionally via newvalues.yamlflags.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| charts/koptan/values.yaml | Introduces webhook, certManager, and metrics configuration flags and defaults. |
| charts/koptan/templates/webhook-service.yaml | Adds a Service to route apiserver webhook traffic to port 9443. |
| charts/koptan/templates/validatingwebhookconfiguration.yaml | Adds ValidatingWebhookConfiguration entries and optional cert-manager CA injection annotation. |
| charts/koptan/templates/mutatingwebhookconfiguration.yaml | Adds MutatingWebhookConfiguration entries and optional cert-manager CA injection annotation. |
| charts/koptan/templates/metrics-service.yaml | Adds a metrics Service intended for secure metrics exposure. |
| charts/koptan/templates/issuer.yaml | Adds a self-signed cert-manager Issuer for generating certs. |
| charts/koptan/templates/certificate.yaml | Adds Certificates for webhook serving cert and metrics certs. |
| charts/koptan/templates/deployment.yaml | Wires webhook/metrics ports, args, and cert secret mounts into the controller deployment. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| webhook: | ||
| enabled: true | ||
|
|
||
| certManager: | ||
| enabled: true | ||
|
|
||
| metrics: | ||
| secure: true | ||
| port: 8443 |
There was a problem hiding this comment.
certManager.enabled defaults to true, but the chart does not declare a cert-manager dependency; installing on clusters without cert-manager CRDs will fail when rendering Issuer/Certificate resources. Consider defaulting certManager.enabled to false (and possibly webhook.enabled to false) or explicitly adding/documenting the dependency so the chart installs successfully by default.
| --- | ||
| apiVersion: cert-manager.io/v1 | ||
| kind: Certificate | ||
| metadata: | ||
| name: {{ include "koptan.fullname" . }}-metrics-certs | ||
| namespace: {{ .Release.Namespace }} | ||
| labels: | ||
| {{- include "koptan.labels" . | nindent 4 }} | ||
| spec: | ||
| dnsNames: | ||
| - {{ include "koptan.fullname" . }}-metrics-service.{{ .Release.Namespace }}.svc | ||
| - {{ include "koptan.fullname" . }}-metrics-service.{{ .Release.Namespace }}.svc.cluster.local | ||
| issuerRef: | ||
| kind: Issuer | ||
| name: {{ include "koptan.fullname" . }}-selfsigned-issuer | ||
| secretName: metrics-server-cert | ||
| {{- end }} |
There was a problem hiding this comment.
The metrics Certificate is rendered whenever certManager.enabled is true, even if metrics.secure is false (and metrics-service / metrics TLS mounts may not be rendered). Gate the metrics certificate on metrics.secure (or render the metrics Service/mounts whenever this certificate is created) to avoid creating unused resources and potential confusion.
| volumes: | ||
| {{- if and .Values.webhook.enabled .Values.certManager.enabled }} | ||
| - name: webhook-certs | ||
| secret: | ||
| secretName: webhook-server-cert | ||
| {{- end }} | ||
| {{- if and .Values.metrics.secure .Values.certManager.enabled }} | ||
| - name: metrics-certs | ||
| secret: | ||
| secretName: metrics-server-cert | ||
| optional: false | ||
| items: | ||
| - key: ca.crt | ||
| path: ca.crt | ||
| - key: tls.crt | ||
| path: tls.crt | ||
| - key: tls.key | ||
| path: tls.key | ||
| {{- end }} |
There was a problem hiding this comment.
If neither webhook-certs nor metrics-certs are enabled (e.g., certManager.enabled: false, or both webhook.enabled: false and metrics.secure: false), this template still renders a volumes: key with no list items, which becomes null in YAML/JSON and is invalid for PodSpec.volumes. Either omit the volumes block entirely when empty or render volumes: [] as a safe default.
| {{- if .Values.webhook.enabled }} | ||
| apiVersion: admissionregistration.k8s.io/v1 | ||
| kind: ValidatingWebhookConfiguration | ||
| metadata: | ||
| name: {{ include "koptan.fullname" . }}-validating-webhook-configuration | ||
| labels: | ||
| {{- include "koptan.labels" . | nindent 4 }} | ||
| {{- if .Values.certManager.enabled }} | ||
| annotations: | ||
| cert-manager.io/inject-ca-from: {{ .Release.Namespace }}/{{ include "koptan.fullname" . }}-serving-cert | ||
| {{- end }} | ||
| webhooks: | ||
| - admissionReviewVersions: | ||
| - v1 | ||
| clientConfig: | ||
| service: | ||
| name: {{ include "koptan.fullname" . }}-webhook-service | ||
| namespace: {{ .Release.Namespace }} | ||
| path: /validate-koptan-felukka-sh-v1alpha-goapp | ||
| failurePolicy: Fail | ||
| name: vgoapp-v1alpha.kb.io |
There was a problem hiding this comment.
When webhook.enabled is true but certManager.enabled is false, this webhook configuration renders without a CA bundle (no cert-manager.io/inject-ca-from and no caBundle), so the apiserver will be unable to establish TLS to the webhook and will block CREATE/UPDATE due to failurePolicy: Fail. Consider gating webhook configurations on certManager.enabled, or add values to supply caBundle/TLS secret when cert-manager is disabled.
| {{- if .Values.webhook.enabled }} | ||
| apiVersion: admissionregistration.k8s.io/v1 | ||
| kind: MutatingWebhookConfiguration | ||
| metadata: | ||
| name: {{ include "koptan.fullname" . }}-mutating-webhook-configuration | ||
| labels: | ||
| {{- include "koptan.labels" . | nindent 4 }} | ||
| {{- if .Values.certManager.enabled }} | ||
| annotations: | ||
| cert-manager.io/inject-ca-from: {{ .Release.Namespace }}/{{ include "koptan.fullname" . }}-serving-cert | ||
| {{- end }} | ||
| webhooks: | ||
| - admissionReviewVersions: | ||
| - v1 | ||
| clientConfig: | ||
| service: | ||
| name: {{ include "koptan.fullname" . }}-webhook-service | ||
| namespace: {{ .Release.Namespace }} | ||
| path: /mutate-koptan-felukka-sh-v1alpha-goapp | ||
| failurePolicy: Fail | ||
| name: mgoapp-v1alpha.kb.io |
There was a problem hiding this comment.
When webhook.enabled is true but certManager.enabled is false, this webhook configuration renders without a CA bundle (no cert-manager.io/inject-ca-from and no caBundle), so the apiserver will be unable to establish TLS to the webhook and will block CREATE/UPDATE due to failurePolicy: Fail. Consider gating webhook configurations on certManager.enabled, or add values to supply caBundle/TLS secret when cert-manager is disabled.
| {{- if .Values.metrics.secure }} | ||
| apiVersion: v1 | ||
| kind: Service | ||
| metadata: | ||
| name: {{ include "koptan.fullname" . }}-metrics-service | ||
| namespace: {{ .Release.Namespace }} | ||
| labels: | ||
| {{- include "koptan.labels" . | nindent 4 }} | ||
| spec: | ||
| ports: | ||
| - name: https | ||
| port: {{ .Values.metrics.port }} | ||
| protocol: TCP | ||
| targetPort: {{ .Values.metrics.port }} | ||
| selector: | ||
| {{- include "koptan.selectorLabels" . | nindent 4 }} | ||
| {{- end }} |
There was a problem hiding this comment.
metrics-service.yaml is gated only on metrics.secure, but the TLS cert volume/args are gated on certManager.enabled. If a user sets metrics.secure: true with certManager.enabled: false, the Service will still be created and the container will expose the port, but there is no chart path to provision TLS certs, which likely makes the 'secure' setting non-functional. Consider gating this Service (and the metrics port/args) on both metrics.secure and certManager.enabled, or introduce a separate flag/value for insecure metrics exposure.
|
@copilot review last commit |
The last commit (
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {{- if .Values.webhook.enabled }} | ||
| apiVersion: v1 | ||
| kind: Service | ||
| metadata: | ||
| name: {{ include "koptan.fullname" . }}-webhook-service |
There was a problem hiding this comment.
webhook.enabled can be true while certManager.enabled is false, but the chart will still create the webhook Service (and open port 9443 in the Deployment) without creating the webhook configurations/cert mounts. Either align the gating (e.g., gate the Service on the same condition as the webhook configs/certs) or support a non-cert-manager TLS option (user-provided Secret/caBundle).
| {{- if and .Values.webhook.enabled .Values.certManager.enabled }} | ||
| apiVersion: admissionregistration.k8s.io/v1 | ||
| kind: ValidatingWebhookConfiguration | ||
| metadata: | ||
| name: {{ include "koptan.fullname" . }}-validating-webhook-configuration | ||
| labels: | ||
| {{- include "koptan.labels" . | nindent 4 }} | ||
| {{- if .Values.certManager.enabled }} | ||
| annotations: | ||
| cert-manager.io/inject-ca-from: {{ .Release.Namespace }}/{{ include "koptan.fullname" . }}-serving-cert | ||
| {{- end }} |
There was a problem hiding this comment.
The template is gated by and .Values.webhook.enabled .Values.certManager.enabled, but the PR description/test plan indicate webhook resources should be controlled by webhook.enabled with only the cert-manager CA injection gated by certManager.enabled. As written, enabling the webhook without cert-manager renders no ValidatingWebhookConfiguration, and there’s no alternative caBundle/Secret-based path.
| {{- if and .Values.webhook.enabled .Values.certManager.enabled }} | ||
| apiVersion: admissionregistration.k8s.io/v1 | ||
| kind: MutatingWebhookConfiguration | ||
| metadata: | ||
| name: {{ include "koptan.fullname" . }}-mutating-webhook-configuration | ||
| labels: | ||
| {{- include "koptan.labels" . | nindent 4 }} | ||
| {{- if .Values.certManager.enabled }} | ||
| annotations: | ||
| cert-manager.io/inject-ca-from: {{ .Release.Namespace }}/{{ include "koptan.fullname" . }}-serving-cert | ||
| {{- end }} |
There was a problem hiding this comment.
Same gating issue as the validating webhook template: MutatingWebhookConfiguration is only rendered when both webhook.enabled and certManager.enabled are true, despite the description implying only CA injection depends on cert-manager. Consider rendering when webhook.enabled is true and conditionally setting either cert-manager.io/inject-ca-from or a user-provided caBundle.
| @@ -28,12 +30,31 @@ spec: | |||
| args: | |||
| - --leader-elect | |||
| - --health-probe-bind-address=:8081 | |||
| {{- if .Values.metrics.secure }} | |||
| - --metrics-bind-address=:{{ .Values.metrics.port }} | |||
| {{- end }} | |||
| {{- if $webhookCerts }} | |||
| - --webhook-cert-path=/tmp/k8s-webhook-server/serving-certs | |||
| {{- end }} | |||
| {{- if $metricsCerts }} | |||
| - --metrics-cert-path=/tmp/k8s-metrics-server/metrics-certs | |||
| {{- end }} | |||
There was a problem hiding this comment.
metrics.secure: true does not imply certManager.enabled: true, but the Deployment will still set --metrics-bind-address and open the metrics container port while cert mounts / metrics Service / metrics RBAC are only created when cert-manager is enabled. This creates a partial configuration that can break startup or expose an endpoint unexpectedly; consider enforcing the dependency with a Helm fail/required check or gating all related pieces on a single condition.
| apiVersion: v1 | ||
| kind: Service | ||
| metadata: | ||
| name: {{ include "koptan.fullname" . }}-webhook-service |
There was a problem hiding this comment.
Resource names are built as {{ include "koptan.fullname" . }}-webhook-service. Since koptan.fullname is already truncated to 63 chars, appending a suffix can exceed Kubernetes’ 63-char limit for Service names and render invalid manifests for long release names. Consider truncating after adding the suffix (or adding a helper for these derived names).
| name: {{ include "koptan.fullname" . }}-webhook-service | |
| name: {{ printf "%s-%s" (include "koptan.fullname" .) "webhook-service" | trunc 63 | trimSuffix "-" }} |
| apiVersion: v1 | ||
| kind: Service | ||
| metadata: | ||
| name: {{ include "koptan.fullname" . }}-metrics-service |
There was a problem hiding this comment.
Same potential name-length issue as the webhook Service: {{ include "koptan.fullname" . }}-metrics-service can exceed the 63-char Service name limit if the release/fullname is long. Truncate the final string (or use a helper) to keep the rendered name within Kubernetes limits.
| name: {{ include "koptan.fullname" . }}-metrics-service | |
| name: {{ include "koptan.fullname" . | trunc 47 | trimSuffix "-" }}-metrics-service |
| @@ -0,0 +1,11 @@ | |||
| {{- if .Values.certManager.enabled }} | |||
There was a problem hiding this comment.
Issuer is created whenever certManager.enabled is true, even if neither webhook.enabled nor metrics.secure are enabled (i.e., no Certificate resources will be rendered). Consider gating the Issuer on whether at least one certificate is needed to avoid installing unused cert-manager resources.
| {{- if .Values.certManager.enabled }} | |
| {{- if and .Values.certManager.enabled (or .Values.webhook.enabled .Values.metrics.secure) }} |
| apiVersion: rbac.authorization.k8s.io/v1 | ||
| kind: Role | ||
| metadata: | ||
| name: {{ include "koptan.fullname" . }}-leader-election-role |
There was a problem hiding this comment.
Role/RoleBinding names must be <=63 chars, but these are formed by appending a long suffix to koptan.fullname (already truncated to 63). For long release names this can exceed the limit and break installs; consider truncating after concatenating the suffix (or using a dedicated helper for leader-election resource names).
| name: {{ include "koptan.fullname" . }}-leader-election-role | |
| name: {{ printf "%s-leader-election-role" (include "koptan.fullname" .) | trunc 63 | trimSuffix "-" }} |
6067594 to
0ea7eb3
Compare
Closes #8 New templates: - certificate.yaml: cert-manager Certificates for webhook (serving-cert) and metrics (metrics-certs), gated on certManager.enabled + respective flags - issuer.yaml: self-signed cert-manager Issuer, gated on certManager.enabled - webhook-service.yaml: Service port 443->9443 for admission webhooks, gated on webhook.enabled - metrics-service.yaml: Service port 8443 for secure metrics, gated on metrics.secure + certManager.enabled - mutatingwebhookconfiguration.yaml: MutatingWebhookConfiguration for goapps, javaapps, slipways, voyages, dotnetapps with cert-manager CA injection annotation, gated on webhook.enabled + certManager.enabled - validatingwebhookconfiguration.yaml: ValidatingWebhookConfiguration for the same resources, same gates - leader-election-role.yaml: namespaced Role for coordination.k8s.io/leases, configmaps, and events (required for --leader-elect) - leader-election-rolebinding.yaml: binds leader election Role to service account - metrics-auth-clusterrole.yaml: ClusterRole + ClusterRoleBinding for tokenreviews and subjectaccessreviews (secure metrics auth), gated on metrics.secure + certManager.enabled - .gitignore: exclude .claude/ directory Updated templates: - deployment.yaml: conditionally expose webhook port 9443 and metrics port 8443, mount webhook-server-cert and metrics-server-cert volumes, and pass --webhook-cert-path / --metrics-cert-path / --metrics-bind-address args. Uses $webhookCerts/$metricsCerts variables to avoid empty volumes block. - values.yaml: add webhook.enabled (false), certManager.enabled (false, with install note), metrics.secure (false), metrics.port (8443) All new resources default to disabled (false) so the chart installs safely on clusters without cert-manager CRDs. Verified on Docker Desktop cluster: - cert-manager Issuer, Certificates, and TLS secrets created successfully - CA bundle injected into both webhook configurations (1504 chars) - Metrics server running on :8443 with TLS - Leader election working (no RBAC errors after Role addition)
0ea7eb3 to
8fa80a5
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {{- if .Values.metrics.secure }} | ||
| - --metrics-bind-address=:{{ .Values.metrics.port }} | ||
| {{- end }} | ||
| {{- if $webhookCerts }} | ||
| - --webhook-cert-path=/tmp/k8s-webhook-server/serving-certs | ||
| {{- end }} | ||
| {{- if $metricsCerts }} | ||
| - --metrics-cert-path=/tmp/k8s-metrics-server/metrics-certs | ||
| {{- end }} | ||
| image: "{{ .Values.controller.image.repository }}:{{ .Values.controller.image.tag | default .Chart.AppVersion }}" | ||
| imagePullPolicy: {{ .Values.controller.image.pullPolicy }} | ||
| ports: | ||
| - name: http | ||
| containerPort: 8080 | ||
| protocol: TCP | ||
| {{- if .Values.webhook.enabled }} | ||
| - name: webhook-server | ||
| containerPort: 9443 | ||
| protocol: TCP | ||
| {{- end }} | ||
| {{- if .Values.metrics.secure }} | ||
| - name: metrics | ||
| containerPort: {{ .Values.metrics.port }} | ||
| protocol: TCP |
There was a problem hiding this comment.
metrics.secure changes --metrics-bind-address to :{{ .Values.metrics.port }} even when certManager.enabled is false, but TLS is only configured when $metricsCerts is true (--metrics-cert-path + secret mount). With metrics.secure=true and certManager.enabled=false, metrics are likely served plaintext on the “secure” port and the default Service (templates/service.yaml) will still point at targetPort: http (8080), which won’t be listening anymore. Consider making --metrics-bind-address (and the metrics container port) conditional on $metricsCerts, or alternatively keep metrics on 8080 unless certs are configured and update/disable the default Service accordingly.
| {{- if .Values.webhook.enabled }} | ||
| apiVersion: v1 | ||
| kind: Service | ||
| metadata: | ||
| name: {{ include "koptan.fullname" . }}-webhook-service | ||
| namespace: {{ .Release.Namespace }} | ||
| labels: | ||
| {{- include "koptan.labels" . | nindent 4 }} | ||
| spec: | ||
| ports: | ||
| - port: 443 | ||
| protocol: TCP | ||
| targetPort: 9443 | ||
| selector: |
There was a problem hiding this comment.
This Service is rendered whenever webhook.enabled is true, but the webhook configurations/cert mounts currently require certManager.enabled too. If cert-manager is disabled, this can leave an exposed Service (443→9443) with no corresponding webhook configs (and potentially no TLS material). Consider aligning this condition with the chosen webhook configuration strategy (e.g., gate on $webhookCerts, or render webhook configs when webhook.enabled and allow a user-supplied cert/caBundle).
| {{- if .Values.certManager.enabled }} | ||
| apiVersion: cert-manager.io/v1 | ||
| kind: Issuer | ||
| metadata: | ||
| name: {{ include "koptan.fullname" . }}-selfsigned-issuer | ||
| namespace: {{ .Release.Namespace }} | ||
| labels: | ||
| {{- include "koptan.labels" . | nindent 4 }} | ||
| spec: | ||
| selfSigned: {} | ||
| {{- end }} |
There was a problem hiding this comment.
issuer.yaml renders an Issuer whenever certManager.enabled is true, even if neither webhook.enabled nor metrics.secure are enabled (so no Certificates are created). Consider gating the Issuer on and .Values.certManager.enabled (or .Values.webhook.enabled .Values.metrics.secure) to avoid creating unused resources.
Summary
MutatingWebhookConfigurationandValidatingWebhookConfigurationfor all 5 resource types (goapps, javaapps, slipways, voyages, dotnetapps), withcert-manager.io/inject-ca-fromannotation when cert-manager is enabledService(port 443 → 9443) and metricsService(port 8443)Issuer(self-signed) and twoCertificateresources:serving-cert(webhook) andmetrics-certsDeploymentto expose port 9443, mount cert volumes, and pass--webhook-cert-path/--metrics-cert-pathargsvalues.yamlflags:webhook.enabled,certManager.enabled,metrics.secureCloses #8
Test plan
helm lint ./charts/koptanpasseshelm template koptan ./charts/koptanrenders all 6 new resource kindswebhook.enabled: false→ webhook service and webhook configs not renderedcertManager.enabled: false→ Issuer, Certificates, CA annotations, and cert volume mounts not rendered🤖 Generated with Claude Code