feat: add conversion webhook to the controller#2274
Merged
Skarlso merged 8 commits intoApr 17, 2026
Conversation
✅ Deploy Preview for ocm-website canceled.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds optional CRD conversion webhooks with cert-manager-backed TLS: new webhook Service and Certificate templates, conditional CRD conversion and cert annotations, manager deployment changes to host the webhook, startup webhook registration and Hub/SetupWebhook method stubs, values/schema/docs, and a unit test asserting Hub implementation. Changes
Sequence DiagramsequenceDiagram
participant Client as API Client
participant Service as Kubernetes Service
participant Manager as Controller Manager<br/>Webhook Server
participant certmgr as cert-manager
participant CRD as CRD Conversion
certmgr->>Manager: Provision certificate (Secret)
Manager->>Service: Pod(s) expose webhook on 9443
Client->>Service: POST /convert (ConversionReview v1)
Service->>Manager: Route request to webhook handler
Manager->>CRD: Perform conversion logic
CRD->>Manager: Return converted object
Manager->>Client: Respond with ConversionReview result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
kubernetes/controller/chart/templates/crd/deployers.delivery.ocm.software.yaml (1)
7-23: Consider edge case: webhook enabled without cert-manager and without certSecret.When
webhook.enable=truebutcertManager.enable=false, the conversion webhook is configured but nocaBundleis provided in the CRD. Users must either:
- Manually patch the CRD with a
caBundle, or- Ensure
webhook.certSecretcontains a valid CA certificate that Kubernetes can use.Consider adding a chart note or validation to warn users about this configuration requirement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kubernetes/controller/chart/templates/crd/deployers.delivery.ocm.software.yaml` around lines 7 - 23, When .Values.webhook.enable is true but .Values.certManager.enable is false you currently create a conversion webhook without providing a caBundle; update the chart to handle this edge case by either (A) adding a Helm validation in values.schema.json that requires .Values.webhook.certSecret (or errors) when webhook.enable && !certManager.enable, or (B) enhance the template to populate conversion.webhook.clientConfig.caBundle from the secret referenced by .Values.webhook.certSecret if present, and add a clear NOTE in templates/NOTES.txt describing that users must supply webhook.certSecret or manually patch the CRD with a caBundle when certManager is disabled; reference .Values.webhook.enable, .Values.certManager.enable, .Values.webhook.certSecret, cert-manager.io/inject-ca-from and the conversion webhook block (path /convert) to locate the code to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@kubernetes/controller/chart/templates/webhook/service.yaml`:
- Around line 17-18: The Service selector in webhook/service.yaml is too broad
and was changed to include release-specific labels that do not exist on the
pods; update the manager Deployment
(kubernetes/controller/chart/templates/manager/manager.yaml) so its
Deployment.spec.selector.matchLabels and the
Deployment.spec.template.metadata.labels include app.kubernetes.io/instance and
app.kubernetes.io/name (matching the values used in the Service selector), and
then ensure the webhook Service selector uses those exact same label keys and
values so the Service can actually select the pods.
---
Nitpick comments:
In
`@kubernetes/controller/chart/templates/crd/deployers.delivery.ocm.software.yaml`:
- Around line 7-23: When .Values.webhook.enable is true but
.Values.certManager.enable is false you currently create a conversion webhook
without providing a caBundle; update the chart to handle this edge case by
either (A) adding a Helm validation in values.schema.json that requires
.Values.webhook.certSecret (or errors) when webhook.enable &&
!certManager.enable, or (B) enhance the template to populate
conversion.webhook.clientConfig.caBundle from the secret referenced by
.Values.webhook.certSecret if present, and add a clear NOTE in
templates/NOTES.txt describing that users must supply webhook.certSecret or
manually patch the CRD with a caBundle when certManager is disabled; reference
.Values.webhook.enable, .Values.certManager.enable, .Values.webhook.certSecret,
cert-manager.io/inject-ca-from and the conversion webhook block (path /convert)
to locate the code to change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e06cba40-1db7-4e7a-853c-e417722a3164
📒 Files selected for processing (40)
kubernetes/controller/chart/templates/crd/components.delivery.ocm.software.yamlkubernetes/controller/chart/templates/crd/deployers.delivery.ocm.software.yamlkubernetes/controller/chart/templates/crd/repositories.delivery.ocm.software.yamlkubernetes/controller/chart/templates/crd/resources.delivery.ocm.software.yamlkubernetes/controller/chart/templates/manager/manager.yamlkubernetes/controller/chart/templates/webhook/certificate.yamlkubernetes/controller/chart/templates/webhook/service.yamlkubernetes/controller/chart/values.schema.jsonkubernetes/controller/chart/values.yamlkubernetes/controller/config/crd/bases/delivery.ocm.software_components.yamlkubernetes/controller/config/crd/bases/delivery.ocm.software_deployers.yamlkubernetes/controller/config/crd/bases/delivery.ocm.software_repositories.yamlkubernetes/controller/config/crd/bases/delivery.ocm.software_resources.yamlkubernetes/controller/config/crd/kustomization.yamlkubernetes/controller/config/crd/kustomizeconfig.yamlkubernetes/controller/config/default/kustomization.yamlkubernetes/controller/config/rbac/component_editor_role.yamlkubernetes/controller/config/rbac/component_viewer_role.yamlkubernetes/controller/config/rbac/deployer_editor_role.yamlkubernetes/controller/config/rbac/deployer_viewer_role.yamlkubernetes/controller/config/rbac/kustomization.yamlkubernetes/controller/config/rbac/leader_election_role.yamlkubernetes/controller/config/rbac/leader_election_role_binding.yamlkubernetes/controller/config/rbac/replication_editor_role.yamlkubernetes/controller/config/rbac/replication_viewer_role.yamlkubernetes/controller/config/rbac/repository_editor_role.yamlkubernetes/controller/config/rbac/repository_viewer_role.yamlkubernetes/controller/config/rbac/resource_editor_role.yamlkubernetes/controller/config/rbac/resource_viewer_role.yamlkubernetes/controller/config/rbac/role.yamlkubernetes/controller/config/rbac/role_binding.yamlkubernetes/controller/config/rbac/service_account.yamlkubernetes/controller/config/samples/components.delivery.ocm.software_sample.yamlkubernetes/controller/config/samples/delivery_v1alpha1_component.yamlkubernetes/controller/config/samples/delivery_v1alpha1_ocmdeployer.yamlkubernetes/controller/config/samples/delivery_v1alpha1_ocmrepository.yamlkubernetes/controller/config/samples/delivery_v1alpha1_resource.yamlkubernetes/controller/config/samples/kustomization.yamlkubernetes/controller/config/samples/repository.delivery.ocm.software_sample.yamlkubernetes/controller/config/samples/resource.delivery.ocm.software_sample.yaml
💤 Files with no reviewable changes (31)
- kubernetes/controller/config/rbac/kustomization.yaml
- kubernetes/controller/config/samples/kustomization.yaml
- kubernetes/controller/config/default/kustomization.yaml
- kubernetes/controller/config/rbac/role_binding.yaml
- kubernetes/controller/config/samples/delivery_v1alpha1_component.yaml
- kubernetes/controller/config/rbac/replication_editor_role.yaml
- kubernetes/controller/config/rbac/service_account.yaml
- kubernetes/controller/config/crd/kustomization.yaml
- kubernetes/controller/config/samples/delivery_v1alpha1_ocmdeployer.yaml
- kubernetes/controller/config/samples/delivery_v1alpha1_ocmrepository.yaml
- kubernetes/controller/config/rbac/repository_viewer_role.yaml
- kubernetes/controller/config/rbac/deployer_editor_role.yaml
- kubernetes/controller/config/samples/delivery_v1alpha1_resource.yaml
- kubernetes/controller/config/rbac/leader_election_role.yaml
- kubernetes/controller/config/rbac/replication_viewer_role.yaml
- kubernetes/controller/config/rbac/deployer_viewer_role.yaml
- kubernetes/controller/config/rbac/component_viewer_role.yaml
- kubernetes/controller/config/rbac/leader_election_role_binding.yaml
- kubernetes/controller/config/samples/resource.delivery.ocm.software_sample.yaml
- kubernetes/controller/config/rbac/resource_editor_role.yaml
- kubernetes/controller/config/rbac/repository_editor_role.yaml
- kubernetes/controller/config/rbac/resource_viewer_role.yaml
- kubernetes/controller/config/rbac/component_editor_role.yaml
- kubernetes/controller/config/samples/repository.delivery.ocm.software_sample.yaml
- kubernetes/controller/config/crd/bases/delivery.ocm.software_repositories.yaml
- kubernetes/controller/config/rbac/role.yaml
- kubernetes/controller/config/crd/kustomizeconfig.yaml
- kubernetes/controller/config/crd/bases/delivery.ocm.software_resources.yaml
- kubernetes/controller/config/samples/components.delivery.ocm.software_sample.yaml
- kubernetes/controller/config/crd/bases/delivery.ocm.software_components.yaml
- kubernetes/controller/config/crd/bases/delivery.ocm.software_deployers.yaml
Contributor
Author
|
Hm, the kustomize fix I'm going to leave for a separate PR actually. I realised that it's more involved then just deleting it. |
a798324 to
2dfbcec
Compare
On-behalf-of: Gergely Brautigam <gergely.brautigam@sap.com> Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
2dfbcec to
fcdc013
Compare
On-behalf-of: Gergely Brautigam <gergely.brautigam@sap.com> Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
Contributor
Author
|
Selecting v1alpha1 for Hub for now. Once the new version comes we can implement the Conversion methods. |
On-behalf-of: Gergely Brautigam <gergely.brautigam@sap.com> Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
On-behalf-of: Gergely Brautigam <gergely.brautigam@sap.com> Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
kubernetes/controller/cmd/main.go (1)
308-323: Consider mirroring the webhook Helm toggle at runtime.These registrations are unconditional. A small env/flag guard here would keep the process behavior aligned with
webhook.enable, which makes feature-off smoke tests and chart behavior easier to reason about.♻️ Possible shape
+ if enableWebhooks { if err = (&v1alpha1.Component{}).SetupWebhookWithManager(mgr); err != nil { setupLog.Error(err, "unable to create webhook", "webhook", "Component") os.Exit(1) } if err = (&v1alpha1.Deployer{}).SetupWebhookWithManager(mgr); err != nil { setupLog.Error(err, "unable to create webhook", "webhook", "Deployer") os.Exit(1) } if err = (&v1alpha1.Repository{}).SetupWebhookWithManager(mgr); err != nil { setupLog.Error(err, "unable to create webhook", "webhook", "Repository") os.Exit(1) } if err = (&v1alpha1.Resource{}).SetupWebhookWithManager(mgr); err != nil { setupLog.Error(err, "unable to create webhook", "webhook", "Resource") os.Exit(1) } + }Also define
enableWebhooksfrom a flag/env earlier inmain()and wire the same switch from the chart.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kubernetes/controller/cmd/main.go` around lines 308 - 323, Add a runtime guard so webhooks are only registered when a new boolean flag/env variable (e.g., enableWebhooks) is true: define enableWebhooks earlier in main() (using the existing flag/env pattern used elsewhere), and wrap the four webhook registrations that call (&v1alpha1.Component{}).SetupWebhookWithManager(mgr), (&v1alpha1.Deployer{}).SetupWebhookWithManager(mgr), (&v1alpha1.Repository{}).SetupWebhookWithManager(mgr) and (&v1alpha1.Resource{}).SetupWebhookWithManager(mgr) in a single if enableWebhooks { ... } block (keeping the existing error handling inside) so runtime behavior mirrors the Helm chart webhook.enable toggle.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@kubernetes/controller/api/v1alpha1/conversion.go`:
- Around line 7-10: The current file implements Hub() on v1alpha1 types
(Component, Deployer, Repository, Resource) making v1alpha1 the conversion hub
and storage version; to avoid locking storage to an unstable version, either
remove these Hub() methods from v1alpha1 (so no hub is declared here) or change
them to point to the intended stable hub type (e.g., implement Hub() on the
stable v1 types and remove/replace the v1alpha1 Hub() implementations), and add
a brief comment documenting which version is intended as the canonical hub;
update the Hub() implementations for the specific types Component, Deployer,
Repository, and Resource accordingly.
---
Nitpick comments:
In `@kubernetes/controller/cmd/main.go`:
- Around line 308-323: Add a runtime guard so webhooks are only registered when
a new boolean flag/env variable (e.g., enableWebhooks) is true: define
enableWebhooks earlier in main() (using the existing flag/env pattern used
elsewhere), and wrap the four webhook registrations that call
(&v1alpha1.Component{}).SetupWebhookWithManager(mgr),
(&v1alpha1.Deployer{}).SetupWebhookWithManager(mgr),
(&v1alpha1.Repository{}).SetupWebhookWithManager(mgr) and
(&v1alpha1.Resource{}).SetupWebhookWithManager(mgr) in a single if
enableWebhooks { ... } block (keeping the existing error handling inside) so
runtime behavior mirrors the Helm chart webhook.enable toggle.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e7121267-2ada-4333-a7dc-fe9856941a70
📒 Files selected for processing (13)
kubernetes/controller/Taskfile.ymlkubernetes/controller/api/v1alpha1/conversion.gokubernetes/controller/chart/README.mdkubernetes/controller/chart/templates/crd/components.delivery.ocm.software.yamlkubernetes/controller/chart/templates/crd/deployers.delivery.ocm.software.yamlkubernetes/controller/chart/templates/crd/repositories.delivery.ocm.software.yamlkubernetes/controller/chart/templates/crd/resources.delivery.ocm.software.yamlkubernetes/controller/chart/templates/manager/manager.yamlkubernetes/controller/chart/templates/webhook/certificate.yamlkubernetes/controller/chart/templates/webhook/service.yamlkubernetes/controller/chart/values.schema.jsonkubernetes/controller/chart/values.yamlkubernetes/controller/cmd/main.go
✅ Files skipped from review due to trivial changes (7)
- kubernetes/controller/chart/README.md
- kubernetes/controller/Taskfile.yml
- kubernetes/controller/chart/values.yaml
- kubernetes/controller/chart/templates/crd/components.delivery.ocm.software.yaml
- kubernetes/controller/chart/templates/crd/repositories.delivery.ocm.software.yaml
- kubernetes/controller/chart/values.schema.json
- kubernetes/controller/chart/templates/manager/manager.yaml
jakobmoellerdev
approved these changes
Apr 15, 2026
8 tasks
Contributor
Author
|
Added followup for the cleanup of kustomization open-component-model/ocm-project#1030. |
morri-son
approved these changes
Apr 17, 2026
ocmbot2 Bot
pushed a commit
to morri-son/open-component-model
that referenced
this pull request
Apr 17, 2026
morri-son
pushed a commit
to morri-son/open-component-model
that referenced
this pull request
Apr 20, 2026
0606bcf accidentally clobbered the webhook conversion blocks added by open-component-model#2274 when adopting helm chart changes for the subscriber buffer flag. Restore all 4 CRD templates to upstream/main state. Signed-off-by: Gerald Morrison (SAP) <gerald.morrison@sap.com>
This was referenced Apr 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it
Adding a conversion webhook. The main problem is figuring out a Hub version. For now, this PR introduces the webhook, but doesn't introduce a Hub version. The Hub is the canonical version that spokes convert into. It should be AT LEAST v1. If we decide alpha to be a Hub version that's a volatile version that becomes load bearing, that's not desired.
Which issue(s) this PR fixes
open-component-model/ocm-project#983
Testing
How to test the changes
Verification
ocm