OCPBUGS-105193: extract HCCO webhook validation into a dedicated controller - #9239
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci-robot
added
the
jira/valid-reference
|
@bryan-cox: This pull request references Jira Issue OCPBUGS-105193, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-bug
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe operator now registers a dedicated webhook validation controller. The controller watches validating and mutating webhook configurations and control-plane Services. It builds disallowed service URLs and deletes matching guest webhook configurations. Storage reconciliation no longer performs this validation. Tests cover filtering, deletion, preservation, not-found handling, and error propagation. The end-to-end test waits up to three minutes for deletion. Sequence Diagram(s)sequenceDiagram
participant WebhookConfiguration
participant WebhookValidationController
participant ControlPlaneServices
participant GuestWebhookConfiguration
WebhookConfiguration->>WebhookValidationController: trigger webhook event
WebhookValidationController->>ControlPlaneServices: list services and build disallowed URLs
WebhookValidationController->>GuestWebhookConfiguration: get configuration
WebhookValidationController->>GuestWebhookConfiguration: delete configuration when a URL matches
Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
approved
|
@bryan-cox: This pull request references Jira Issue OCPBUGS-105193, which is valid. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
control-plane-operator/hostedclusterconfigoperator/controllers/webhookvalidation/webhookvalidation.go (2)
67-71: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider an explicit type discriminator instead of
req.Namespace.
Reconcilereads the webhook kind fromreq.Namespace. Both watched resources are cluster-scoped, so the field is unused and the encoding works. However, the contract is implicit and lives insetup.go. A reader ofReconcilealone cannot tell thatNamespaceis a kind selector.An alternative is one controller instance per webhook kind, each with its own
reconcilerholding awebhookTypefield. That removes the map lookup and the silent no-op path for unknown values.This is optional. The current code is documented by the comment on Lines 21-23.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@control-plane-operator/hostedclusterconfigoperator/controllers/webhookvalidation/webhookvalidation.go` around lines 67 - 71, Optionally refactor Reconcile to use an explicit webhookType field on each reconciler instance, configured by setup.go with one controller per webhook kind. Replace the webhookTypesByName lookup and unknown-namespace no-op path with direct use of the reconciler’s webhookType, while preserving existing reconciliation behavior.
122-129: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueSubstring matching on the bare service name can over-match.
buildDisallowedURLsaddshttps://<serviceName>with no delimiter.isAllowedWebhookURLthen usesstrings.Contains. A hosted-cluster webhook that points at an unrelated external host whose name begins with a control-plane service name is deleted. Example: a control-plane Service namedetcd-clientmakeshttps://etcd-client.corp.example.comdisallowed.This logic appears to be carried over from the previous implementation, so it is likely pre-existing and out of scope for this PR. Confirm that the intent is a prefix-with-delimiter match rather than a plain substring match.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@control-plane-operator/hostedclusterconfigoperator/controllers/webhookvalidation/webhookvalidation.go` around lines 122 - 129, Update isAllowedWebhookURL to avoid plain substring matching against bare service names; match only URLs where the service-name prefix is followed by an appropriate delimiter or otherwise represents the intended host. Preserve allowing unrelated external hosts such as etcd-client.corp.example.com, and keep buildDisallowedURLs’ existing inputs unchanged.control-plane-operator/hostedclusterconfigoperator/controllers/webhookvalidation/webhookvalidation_test.go (1)
255-268: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd table cases for the remaining URL forms and the unknown webhook type.
The current cases exercise only the bare
https://<serviceName>form.buildDisallowedURLsalso produceshttps://<name>.<ns>.svcandhttps://<name>.<ns>.svc.cluster.local. Those two forms are only covered indirectly throughTestIsAllowedWebhookURL, which bypassesbuildDisallowedURLs.Two gaps remain:
- No case uses a webhook URL in the
.svcor.svc.cluster.localform.- No case supplies an unrecognized value in
req.Namespace, so the early return atwebhookvalidation.goLines 68-71 is untested.A third gap is the Delete error path in
reconcileWebhook. Aninterceptor.Funcs{Delete: ...}test would mirror the existing Get and List error tests.🧪 Proposed additional table cases
{ name: "When webhook config does not exist, it should return without error", webhookType: validatingType.name, cpServices: []corev1.Service{ { ObjectMeta: metav1.ObjectMeta{ Name: "etcd-client", Namespace: hcpNamespace, }, }, }, guestObjects: []client.Object{}, reconcileName: "nonexistent-webhook", }, + { + name: "When validating webhook targets a CP service by cluster-local FQDN, it should delete the webhook", + webhookType: validatingType.name, + cpServices: []corev1.Service{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "etcd-client", + Namespace: hcpNamespace, + }, + }, + }, + guestObjects: []client.Object{ + &admissionregistrationv1.ValidatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "fqdn-validating-webhook"}, + Webhooks: []admissionregistrationv1.ValidatingWebhook{ + { + Name: "fqdn.webhook.io", + ClientConfig: admissionregistrationv1.WebhookClientConfig{ + URL: ptr.To("https://etcd-client." + hcpNamespace + ".svc.cluster.local:2379"), + }, + }, + }, + }, + }, + reconcileName: "fqdn-validating-webhook", + expectWebhookGone: true, + }, + { + name: "When the request carries an unknown webhook type, it should preserve the webhook", + webhookType: "unknown", + cpServices: []corev1.Service{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "etcd-client", + Namespace: hcpNamespace, + }, + }, + }, + guestObjects: []client.Object{ + &admissionregistrationv1.ValidatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{Name: "untouched-webhook"}, + Webhooks: []admissionregistrationv1.ValidatingWebhook{ + { + Name: "untouched.webhook.io", + ClientConfig: admissionregistrationv1.WebhookClientConfig{URL: ptr.To("https://etcd-client:2379")}, + }, + }, + }, + }, + reconcileName: "untouched-webhook", + },The unknown-type case needs the assertion helpers to resolve a concrete type. Either assert inline for that case or default
wtNametovalidatingType.nameinassertWebhookExists.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@control-plane-operator/hostedclusterconfigoperator/controllers/webhookvalidation/webhookvalidation_test.go` around lines 255 - 268, Extend the webhook reconciliation table tests to cover both https://<name>.<ns>.svc and https://<name>.<ns>.svc.cluster.local URLs, plus an unrecognized req.Namespace value that exercises the early return. Add a reconcileWebhook case using interceptor.Funcs.Delete to verify Delete errors. Update assertWebhookExists or the unknown-type assertion so it resolves a concrete webhook type, defaulting wtName to validatingType.name where appropriate.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@control-plane-operator/hostedclusterconfigoperator/cmd.go`:
- Line 78: The controller list used for IBMCloud must include
webhook-validation. Update the explicit controller list in the hosted control
plane deployment configuration to include the webhookvalidation controller, and
regenerate or update the corresponding IBMCloud fixture while preserving the
existing controller entries.
In
`@control-plane-operator/hostedclusterconfigoperator/controllers/webhookvalidation/setup.go`:
- Around line 33-39: Extend the controller setup around the existing
ValidatingWebhookConfiguration and MutatingWebhookConfiguration watches to also
watch control-plane Service events through opts.CPCluster.GetCache(). Enqueue
affected webhook configurations when matching Services change, ensuring
buildDisallowedURLs is reevaluated without waiting for the long cache resync;
alternatively, return a bounded RequeueAfter.
---
Nitpick comments:
In
`@control-plane-operator/hostedclusterconfigoperator/controllers/webhookvalidation/webhookvalidation_test.go`:
- Around line 255-268: Extend the webhook reconciliation table tests to cover
both https://<name>.<ns>.svc and https://<name>.<ns>.svc.cluster.local URLs,
plus an unrecognized req.Namespace value that exercises the early return. Add a
reconcileWebhook case using interceptor.Funcs.Delete to verify Delete errors.
Update assertWebhookExists or the unknown-type assertion so it resolves a
concrete webhook type, defaulting wtName to validatingType.name where
appropriate.
In
`@control-plane-operator/hostedclusterconfigoperator/controllers/webhookvalidation/webhookvalidation.go`:
- Around line 67-71: Optionally refactor Reconcile to use an explicit
webhookType field on each reconciler instance, configured by setup.go with one
controller per webhook kind. Replace the webhookTypesByName lookup and
unknown-namespace no-op path with direct use of the reconciler’s webhookType,
while preserving existing reconciliation behavior.
- Around line 122-129: Update isAllowedWebhookURL to avoid plain substring
matching against bare service names; match only URLs where the service-name
prefix is followed by an appropriate delimiter or otherwise represents the
intended host. Preserve allowing unrelated external hosts such as
etcd-client.corp.example.com, and keep buildDisallowedURLs’ existing inputs
unchanged.
🪄 Autofix
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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: df0f3c87-a67f-41bf-a459-cc133329b083
📒 Files selected for processing (7)
control-plane-operator/hostedclusterconfigoperator/cmd.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/webhookvalidation/setup.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/webhookvalidation/webhookvalidation.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/webhookvalidation/webhookvalidation_test.gotest/e2e/v2/tests/hosted_cluster_security_test.go
💤 Files with no reviewable changes (2)
- control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go
- control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go
81a7168 to
957288f
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
957288f to
b898189
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@control-plane-operator/hostedclusterconfigoperator/controllers/webhookvalidation/setup.go`:
- Around line 67-95: Update serviceToWebhookHandler to enqueue a fixed
reconciliation request for Service events instead of listing webhook
configurations in the map function. Move both ValidatingWebhookConfigurationList
and MutatingWebhookConfigurationList operations into Reconcile, return either
guestClient.List error, and enqueue each discovered webhook configuration there
so transient failures trigger reconciliation retries.
🪄 Autofix
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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 6c6c2c14-42fd-4b62-ac96-79317ed691d3
📒 Files selected for processing (7)
control-plane-operator/hostedclusterconfigoperator/cmd.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/webhookvalidation/setup.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/webhookvalidation/webhookvalidation.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/webhookvalidation/webhookvalidation_test.gotest/e2e/v2/tests/hosted_cluster_security_test.go
💤 Files with no reviewable changes (2)
- control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go
- control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- test/e2e/v2/tests/hosted_cluster_security_test.go
- control-plane-operator/hostedclusterconfigoperator/cmd.go
- control-plane-operator/hostedclusterconfigoperator/controllers/webhookvalidation/webhookvalidation.go
- control-plane-operator/hostedclusterconfigoperator/controllers/webhookvalidation/webhookvalidation_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9239 +/- ##
==========================================
+ Coverage 44.96% 44.98% +0.01%
==========================================
Files 778 780 +2
Lines 97444 97528 +84
==========================================
+ Hits 43819 43870 +51
- Misses 50602 50636 +34
+ Partials 3023 3022 -1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
b898189 to
56b9d36
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
control-plane-operator/hostedclusterconfigoperator/controllers/webhookvalidation/webhookvalidation_test.go (2)
39-80: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a host-boundary case.
The table covers exact and FQDN matches but not a host that only begins with a Service name. Add a case for
https://etcd-client-external.example.comagainsthttps://etcd-client. With the currentstrings.Containsimplementation that case returnsfalse, which documents the over-broad matching flagged inwebhookvalidation.go.💚 Proposed test case
{ name: "When disallowed list is empty, it should return true", disallowedURLs: []string{}, url: "https://anything", expected: true, }, + { + name: "When URL host only shares a prefix with a disallowed service name, it should return true", + disallowedURLs: []string{"https://etcd-client"}, + url: "https://etcd-client-external.example.com", + expected: true, + },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@control-plane-operator/hostedclusterconfigoperator/controllers/webhookvalidation/webhookvalidation_test.go` around lines 39 - 80, Add a table-driven case to TestIsAllowedWebhookURL where disallowedURLs contains “https://etcd-client” and the URL is “https://etcd-client-external.example.com”, expecting true. Keep the case focused on verifying that a hostname merely beginning with the disallowed service name is allowed.
357-366: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for aggregated per-webhook errors.
The sentinel table covers a failing
Listbut not a failingDeleteduring bulk reconciliation.reconcileAllWebhookscollects per-webhook errors witherrors.Joinand continues the loop. Add a case with two disallowed configurations and aDeleteinterceptor that fails for one of them. Assert that the returned error mentions the failing configuration and that the other configuration is deleted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@control-plane-operator/hostedclusterconfigoperator/controllers/webhookvalidation/webhookvalidation_test.go` around lines 357 - 366, The webhook validation tests need coverage for aggregated Delete errors during bulk reconciliation. Add a case with two disallowed configurations, make the Delete interceptor fail for exactly one configuration, and verify reconcileAllWebhooks returns an error mentioning that configuration while confirming the other configuration is deleted.Source: Coding guidelines
control-plane-operator/hostedclusterconfigoperator/controllers/webhookvalidation/setup.go (1)
47-52: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider a predicate on the control-plane Service watch.
This watch has no predicate. Every Service event in the HCP namespace, including status-only updates and periodic cache resyncs, enqueues two sentinel requests. Each sentinel lists all webhook configurations of one type. Only Service creation, deletion, name changes, and changes to the
AllowGuestWebhooksServiceLabellabel affect the disallowed set.Add a predicate that admits create and delete events and admits update events only when the label set changes.
♻️ Proposed predicate
+ servicePredicate := predicate.Funcs{ + UpdateFunc: func(e event.UpdateEvent) bool { + _, oldAllowed := e.ObjectOld.GetLabels()[hyperv1.AllowGuestWebhooksServiceLabel] + _, newAllowed := e.ObjectNew.GetLabels()[hyperv1.AllowGuestWebhooksServiceLabel] + return oldAllowed != newAllowed + }, + } - if err := c.Watch(source.Kind[client.Object](opts.CPCluster.GetCache(), &corev1.Service{}, serviceEventHandler())); err != nil { + if err := c.Watch(source.Kind[client.Object](opts.CPCluster.GetCache(), &corev1.Service{}, serviceEventHandler(), servicePredicate)); err != nil { return fmt.Errorf("failed to watch control plane Services: %w", err) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@control-plane-operator/hostedclusterconfigoperator/controllers/webhookvalidation/setup.go` around lines 47 - 52, Update the Service watch in the setup flow around serviceEventHandler to apply a predicate that accepts all create and delete events, while accepting update events only when the AllowGuestWebhooksServiceLabel value changes; reject status-only updates and resync events.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@control-plane-operator/hostedclusterconfigoperator/controllers/webhookvalidation/webhookvalidation.go`:
- Around line 142-168: The URL matching in isAllowedWebhookURL is over-broad:
update buildDisallowedURLs and isAllowedWebhookURL in
control-plane-operator/hostedclusterconfigoperator/controllers/webhookvalidation/webhookvalidation.go
(lines 142-168) to return bare hostnames, parse each webhook URL, and compare
url.Hostname() for exact equality instead of using strings.Contains. In
control-plane-operator/hostedclusterconfigoperator/controllers/webhookvalidation/webhookvalidation_test.go
(lines 39-80), add coverage confirming https://etcd-client-external.example.com
remains allowed when etcd-client is disallowed, and revise the existing
substring case for hostname equality semantics.
---
Nitpick comments:
In
`@control-plane-operator/hostedclusterconfigoperator/controllers/webhookvalidation/setup.go`:
- Around line 47-52: Update the Service watch in the setup flow around
serviceEventHandler to apply a predicate that accepts all create and delete
events, while accepting update events only when the
AllowGuestWebhooksServiceLabel value changes; reject status-only updates and
resync events.
In
`@control-plane-operator/hostedclusterconfigoperator/controllers/webhookvalidation/webhookvalidation_test.go`:
- Around line 39-80: Add a table-driven case to TestIsAllowedWebhookURL where
disallowedURLs contains “https://etcd-client” and the URL is
“https://etcd-client-external.example.com”, expecting true. Keep the case
focused on verifying that a hostname merely beginning with the disallowed
service name is allowed.
- Around line 357-366: The webhook validation tests need coverage for aggregated
Delete errors during bulk reconciliation. Add a case with two disallowed
configurations, make the Delete interceptor fail for exactly one configuration,
and verify reconcileAllWebhooks returns an error mentioning that configuration
while confirming the other configuration is deleted.
🪄 Autofix
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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: ca483eb0-318f-43f7-b03c-22ddcf10627b
📒 Files selected for processing (7)
control-plane-operator/hostedclusterconfigoperator/cmd.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/webhookvalidation/setup.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/webhookvalidation/webhookvalidation.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/webhookvalidation/webhookvalidation_test.gotest/e2e/v2/tests/hosted_cluster_security_test.go
💤 Files with no reviewable changes (2)
- control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go
- control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- test/e2e/v2/tests/hosted_cluster_security_test.go
- control-plane-operator/hostedclusterconfigoperator/cmd.go
Move webhook validation logic from the monolithic HCCO resources controller into its own webhookvalidation package with dedicated watches, reconciler, and tests. Service events now enqueue sentinel requests so that webhook config listing happens inside Reconcile where errors are retried by the work queue, rather than silently discarding list failures in the event handler. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
56b9d36 to
c9cb6d2
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Local TestingTested locally against an AWS dev cluster with a hosted cluster ( Setup
E2E TestRan the KUBECONFIG=<mgmt-kubeconfig> \
E2E_HOSTED_CLUSTER_NAME=<hc-name> \
E2E_HOSTED_CLUSTER_NAMESPACE=clusters \
bin/test-e2e-v2 --ginkgo.focus='\[Feature:WebhookValidation\]' -test.vResults: 10/10 passes, each completing in 0.40–0.48 seconds. Previously this test had a 47% pass rate with 60+ second delays waiting for the monolithic resources controller to cycle. HCCO LogsConfirmed the new controller registers at startup and all three watches are active: The resources controller continues reconciling normally alongside the new controller — no regressions observed. |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
| } | ||
|
|
||
| // Webhook configs are cluster-scoped so Namespace is normally empty; we repurpose it | ||
| // to carry the webhook kind ("validating"/"mutating") so Reconcile targets only the type that fired. |
There was a problem hiding this comment.
It's clever but it redefines the semantics of the core controller constructs in service of an optimization and makes it harder to understand internally... not necessarily opposed but was there some empirical analysis that led to the decision to make the design so abstract given there are only 2 types of webhooks and I'm curious how many resources there are in a typical cluster
There was a problem hiding this comment.
Would you like me to drop the webhookType abstraction and just have two controllers (or one reconciler with two explicit code paths), each watching its own type directly. That eliminates both the namespace hack and the function-pointer indirection.
There was a problem hiding this comment.
Note, I don't think the above is blocking feedback, if what's here is well tested and working I say go with it
There was a problem hiding this comment.
Two controllers seems like it would be pretty straightforward FWIW
|
This makes sense to me and the test plan looks good, will give it a tag but also a hold in case someone else wants an opportunity to look before merge /lgtm |
openshift-ci
Bot
added
the
do-not-merge/hold
|
Scheduling tests matching the |
|
/retest |
Test Resultse2e-aws
e2e-aks
|
|
/test e2e-v2-aws |
|
/hold cancel No further PR comments to address |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/test e2e-v2-gke |
1 similar comment
|
/test e2e-v2-gke |
|
/verified by e2e passing |
openshift-ci-robot
added
the
verified
|
@bryan-cox: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@bryan-cox: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@bryan-cox: Jira Issue OCPBUGS-105193: Some pull requests linked via external trackers have merged: The following pull request, linked via external tracker, has not merged:
All associated pull requests must be merged or unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with Jira Issue OCPBUGS-105193 has not been moved to the MODIFIED state. This PR is marked as verified. If the remaining PRs listed above are marked as verified before merging, the issue will automatically be moved to VERIFIED after all of the changes from the PRs are available in an accepted nightly payload. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
4 participants
What this PR does / why we need it:
Extracts
ensureGuestAdmissionWebhooksAreValid()from the monolithic HCCO resources controller into a dedicatedwebhook-validationcontroller. The[Feature:WebhookValidation]e2e test has a 47% pass rate one2e-v2-azure-self-managedbecause webhook validation runs at the tail of a 15+ sub-reconciler chain, causing 60+ second delays between webhook creation and deletion.The new controller watches
ValidatingWebhookConfigurationandMutatingWebhookConfigurationdirectly, so reconciliation triggers immediately on webhook events. It encodes the webhook type in the request namespace field soReconcile()targets only the type that fired, avoiding a redundant Get for the other kind.Which issue(s) this PR fixes:
Fixes https://issues.redhat.com/browse/OCPBUGS-105193
Special notes for your reviewer:
labels.Everything()(no label filter) — seeoperator/config.go:129-130— so the resources controller retains its watches forensureResourceCreationIsBlockedChecklist:
Always review AI generated responses prior to use.
Generated with Claude Code via openshift-developer plugin
Summary by CodeRabbit
Bug Fixes
Tests