OCPBUGS-84815: fix: scope webhook to capi namespace, remove unused webhook endpoints#543
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/valid-reference
|
@damdo: This pull request references Jira Issue OCPBUGS-84815, which is invalid:
Comment 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. |
|
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:
WalkthroughRemoved several validating webhooks and constrained the remaining Cluster validation webhook to the ChangesWebhook manifest changes
E2E tests and test harness wiring
Sequence Diagram(s)sequenceDiagram
participant Test as Test Harness
participant KubeAPI as kube-apiserver
participant Webhook as ClusterAPI ValidatingWebhook
participant Etcd as Kubernetes Storage
Test->>KubeAPI: CREATE Cluster in "default" namespace
KubeAPI->>Webhook: AdmissionReview (validation request)
Webhook-->>KubeAPI: No match (namespaceSelector) — Allowed
KubeAPI->>Etcd: Persist Cluster
Etcd-->>KubeAPI: Persisted
KubeAPI-->>Test: 201 Created
Test->>KubeAPI: CREATE Cluster in "openshift-cluster-api" namespace
KubeAPI->>Webhook: AdmissionReview (validation request)
Webhook-->>KubeAPI: Reject with Forbidden (if spec invalid)
KubeAPI-->>Test: 4xx Admission error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 9 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
|
/cherry-pick release-4.22 |
|
@damdo: once the present PR merges, I will cherry-pick it on top of 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 kubernetes-sigs/prow repository. |
|
/pipeline auto |
|
Pipeline controller notification The |
|
/pipeline required |
|
Scheduling tests matching the |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
2 similar comments
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
I'd really like to see an e2e for this. It could be really simple: create and delete a Cluster in the I understand the temptation to do the e2e later, but we should resist it. |
damdo
force-pushed
the
namespace-scope-vwc-webhooks
branch
from
ccc62de to
94cc3c7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@e2e/webhook_test.go`:
- Around line 57-100: Add an unconditional cleanup that always tries to delete
the test Cluster to avoid cross-run flakes: after the BeforeAll that sets up the
cluster (symbol: cluster) add an AfterEach or AfterAll block which, regardless
of test outcome, attempts to delete the Cluster (call cl.Delete(ctx, cluster) or
wrap with client.IgnoreNotFound/DeleteIfExists) and swallow
NotFound/AlreadyDeleted errors; ensure the cleanup runs even if create/update
failed by guarding on cluster != nil and apply the same cleanup pattern to the
other group of specs referenced (the tests around lines 117-149) so leftover
objects cannot cause "AlreadyExists" on subsequent runs.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 6677217d-dd8b-46b3-b345-abcfd1d18219
📒 Files selected for processing (3)
e2e/e2e_common.goe2e/webhook_test.gomanifests/0000_30_cluster-api_10_webhooks.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- manifests/0000_30_cluster-api_10_webhooks.yaml
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
/test e2e-aws-capi-techpreview |
damdo
force-pushed
the
namespace-scope-vwc-webhooks
branch
from
1a51ad1 to
49a160c
Compare
|
/test e2e-aws-capi-techpreview |
|
/retest |
see: https://redhat.atlassian.net/browse/OCPBUGS-84815 On unsupported platforms (e.g. IBMCloud), the openshift-capi-controllers ValidatingWebhookConfiguration is deployed by CVO but the webhook server does not start, causing Cluster creation in any namespace to fail with "connection refused". This blocks MCE/ACM which install CAPI independently and create Cluster objects in their own namespace. Scope the remaining Cluster webhook to the openshift-cluster-api namespace via namespaceSelector so it no longer intercepts requests in other namespaces. Remove the coreprovider, infrastructureprovider, and clusterctl provider webhook entries which had no corresponding handler code registered and were for CRDs no longer deployed by this operator.
Verify that the openshift-capi-controllers ValidatingWebhookConfiguration is correctly scoped to the managed namespace. The negative test creates a Cluster API Cluster in the "default" namespace that deliberately violates the webhook's validation rules (unsupported infrastructureRef kind and mismatched cluster name). It asserts that create, update, and delete operations all succeed, proving the webhook does not intercept Cluster API calls outside the managed namespace. This is critical for layered product operators (e.g. MCE/ACM) that install Cluster API independently and create Cluster objects in their own namespaces, and for unsupported platforms where the webhook server does not start. The positive test verifies the counterpart: on supported platforms, creating a non-compliant Cluster API Cluster inside the managed namespace is rejected by the webhook.
damdo
force-pushed
the
namespace-scope-vwc-webhooks
branch
from
49a160c to
185c6c5
Compare
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mdbooth 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
the
approved
|
Scheduling tests matching the |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@damdo: This pull request references Jira Issue OCPBUGS-84815, which is valid. The bug has been moved to the POST state. 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. |
|
/cherry-pick release-4.22 |
|
@damdo: once the present PR merges, I will cherry-pick it on top of 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 kubernetes-sigs/prow repository. |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
@damdo: The following tests failed, say
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. |
|
/verified by new E2Es |
openshift-ci-robot
added
the
verified
|
@damdo: 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. |
openshift-merge-bot
Bot
merged commit a500393
into
openshift:main
|
@damdo: Jira Issue Verification Checks: Jira Issue OCPBUGS-84815 Jira Issue OCPBUGS-84815 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is 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. |
|
@damdo: #543 failed to apply on top of branch "release-4.22": 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 kubernetes-sigs/prow repository. |
7 participants
See: https://redhat.atlassian.net/browse/OCPBUGS-84815
On unsupported platforms (e.g. IBMCloud), the openshift-capi-controllers ValidatingWebhookConfiguration is deployed by CVO but the webhook server does not start, causing Cluster creation in any namespace to fail with "connection refused". This blocks MCE/ACM which install CAPI independently and create Cluster objects in their own namespace.
Scope the remaining Cluster webhook to the openshift-cluster-api namespace via namespaceSelector so it no longer intercepts requests in other namespaces.
Remove the coreprovider, infrastructureprovider, and clusterctl provider webhook entries which had no corresponding handler code registered and were for CRDs no longer deployed by this operator.
Summary by CodeRabbit
Chores
Tests