OCPCLOUD-2997: Disallow authoritativeAPI swap when CAPI Machine is deleting or its infra is not ready #408
Conversation
|
Pipeline controller notification For optional jobs, comment |
openshift-ci-robot
added
the
jira/valid-reference
|
@theobarberbany: This pull request references OCPCLOUD-2997 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In 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. |
WalkthroughAdds a new ValidatingAdmissionPolicy and matching Binding (scoped to openshift-machine-api) that blocks Machine Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant MAPI as "OpenShift Machine API"
participant Admission as "Admission Controller"
participant VAP as "ValidatingAdmissionPolicy\nopenshift-mapi-authoritative-api-transition-...-and-not-deleting"
participant CAPI as "CAPI infra state"
Client->>MAPI: UPDATE Machine (spec.authoritativeAPI change)
MAPI->>Admission: submit admission request
Admission->>VAP: evaluate matchConstraints & matchConditions
VAP->>CAPI: read infra readiness & deletion status (variables)
alt infraReady AND not deleting
VAP-->>Admission: validation passes
Admission-->>MAPI: allow request
MAPI-->>Client: 200 OK
else infra not ready OR CAPI deleting
VAP-->>Admission: validation denies (CEL fails)
Admission-->>MAPI: deny request with message
MAPI-->>Client: 4xx Denied
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting 📒 Files selected for processing (2)
🔇 Additional comments (5)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Comment |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
@theobarberbany: This pull request references OCPCLOUD-2997 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In 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: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
manifests/0000_30_cluster-api_09_admission-policies.yaml(2 hunks)
🔇 Additional comments (2)
manifests/0000_30_cluster-api_09_admission-policies.yaml (2)
435-454: Policy binding structure looks correct.The ValidatingAdmissionPolicyBinding correctly targets the openshift-machine-api namespace with appropriate parameterNotFoundAction: Allow and references the corresponding policy. Structure and namespace scoping align with existing patterns in the file.
456-483: Policy definition structure is sound, pending variable reference fix.The ValidatingAdmissionPolicy correctly:
- Targets MAPI machines (machine.openshift.io v1beta1) on UPDATE operations
- Uses CAPI machines as params (cluster.x-k8s.io/v1beta1)
- Excludes service accounts via matchCondition
- Validates name matching between MAPI and CAPI machines
- Defines appropriate variables for infrastructure readiness and deletion status
Once the variable references in the validations are corrected (see previous comment), the policy structure will properly enforce the intended constraints.
theobarberbany
force-pushed
the
tb/prevent-authapi-change-if-unsuitable
branch
4 times, most recently
from
2007632 to
7211ded
Compare
theobarberbany
force-pushed
the
tb/prevent-authapi-change-if-unsuitable
branch
2 times, most recently
from
38d7255 to
b2e0a76
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
@theobarberbany: This pull request references OCPCLOUD-2997 which is a valid jira issue. In 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. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/controllers/machinesync/machine_sync_controller_test.go (1)
1879-2207: Comprehensive coverage of new VAP behavior; consider minor helper extractionThe new
"when validating MAPI authoritativeAPI transitions"context exercises the new VAP very well:
- Confirms denials for all “unsafe” CAPI states (infra not ready, missing status, deleting, mixed ready+deleting / not‑ready+non‑deleting).
- Confirms success paths (infraReady=true & not deleting, no CAPI counterpart, and non‑authoritative updates).
- Wires the binding namespaces and sentinel validation exactly like the existing VAP suites, so we know the policy is actually in play rather than relying on static YAML.
From a test‑design perspective this looks solid. If you want to trim duplication later, the repeated setup patterns (
setCAPIInfraReady(true/false),markCapiDeleting,setMapiAuthoritativeAPIStatusMachineAPIAndSpecClusterAPI) could be factored into small helpers, but that’s optional and in line with the rest of the file’s style.If you’d like, I can sketch small helper functions for the repeated “set infra / set deleting / flip authoritativeAPI” sequences to keep future scenarios easier to add.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
manifests/0000_30_cluster-api_09_admission-policies.yaml(2 hunks)pkg/controllers/machinesync/machine_sync_controller_test.go(1 hunks)
🔇 Additional comments (1)
manifests/0000_30_cluster-api_09_admission-policies.yaml (1)
482-555: New VAP correctly enforces infra‑ready & non‑deleting CAPI preconditions for MAPI authoritativeAPI changesThe
mapi-authapi-transition-requires-capi-readybinding/policy is wired consistently with the existing VAPs: it scopes validation toopenshift-machine-apiMAPI Machines, uses CAPI Machines as params, matches only name‑paired resources, and cleanly encodes the two required guards:
authAPIChanged && !capiInfraReadyis rejected, treating missing/absentstatus.infrastructureReadyas not ready via optional access +.orValue(false).authAPIChanged && capiDeletingis rejected usinghas(params.metadata.deletionTimestamp).The use of
parameterNotFoundAction: Allowplus the name match ensures Machines without a CAPI counterpart are not blocked, matching the tests. Variable references all use thevariables.prefix, so CEL evaluation should work as intended.If you want extra assurance, it’s worth running this VAP against a live envtest or dev cluster to confirm the interplay with the existing
openshift-prevent-migration-when-machine-updatingpolicy doesn’t introduce any unexpected double‑validation behavior around.spec.authoritativeAPIupdates.
theobarberbany
force-pushed
the
tb/prevent-authapi-change-if-unsuitable
branch
from
b2e0a76 to
5806dd9
Compare
|
@theobarberbany: This pull request references OCPCLOUD-2997 which is a valid jira issue. In 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. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/controllers/machinesync/machine_sync_controller_test.go (1)
1931-2207: Consider reducing repetition in authoritativeAPI transition test setupMany tests repeat the same pattern of “set
Status.AuthoritativeAPItoMachineAPIand then updateSpec.AuthoritativeAPI(and often CAPI infra/deletion state)”. If this block continues to grow, you might factor those sequences into small helpers or a table‑driven sub‑suite to keep individualItbodies shorter and the intent a bit clearer. Behavior is fine as is; this is just a maintainability tweak.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
manifests/0000_30_cluster-api_09_admission-policies.yaml(2 hunks)pkg/controllers/machinesync/machine_sync_controller_test.go(1 hunks)
🔇 Additional comments (2)
pkg/controllers/machinesync/machine_sync_controller_test.go (1)
1879-2207: VAP transition tests accurately capture infra/deletion semanticsThis context does a good job exercising all the important paths for the new policy: infraReady=false/missing/true, deletionTimestamp set/clear, mixed states, non‑auth changes, and the param‑not‑found case. The expectations on error substrings line up with the policy messages, and the use of
Eventually/k.Update(Status)is consistent with the rest of the suite. I don’t see correctness issues here.manifests/0000_30_cluster-api_09_admission-policies.yaml (1)
482-555: New mapi-authapi-transition-requires-capi-ready policy matches intended behaviorThe binding and policy wiring look correct: scope is
openshift-machine-api, params are CAPI Machines inopenshift-cluster-api(withparameterNotFoundAction: Allow), and match conditions mirror existing patterns. The CEL variables (authAPIChanged,capiInfraReady,capiDeleting) are correctly referenced viavariables.*, and the validations enforce exactly the intended rule: anyspec.authoritativeAPIchange is denied unless infra is ready and the CAPI Machine is not deleting, while non‑auth spec/metadata changes remain unaffected. This aligns with the added tests.
|
@theobarberbany: This pull request references OCPCLOUD-2997 which is a valid jira issue. In 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. |
|
/test e2e-aws-capi-techpreview |
|
/lgtm |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nrb The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/hold To coordinate merge with: #410 |
openshift-ci
bot
added
the
do-not-merge/hold
damdo
left a comment
damdo
left a comment
There was a problem hiding this comment.
Thanks
A couple of naming nits but LGTM
sigh this is what I get for using AI to help with naming lol Will update. |
…nfra is not ready
theobarberbany
force-pushed
the
tb/prevent-authapi-change-if-unsuitable
branch
from
5806dd9 to
0879a53
Compare
|
@theobarberbany: This pull request references OCPCLOUD-2997 which is a valid jira issue. In 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. |
|
Scheduling tests matching the |
|
/assign @miyadav for testing |
|
/retest-required |
|
functionally looks good , verified by test , will add label once tests passes in ci. |
|
@theobarberbany: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
6 participants
Ignore the coderabbit summary, it's wrong.
Ths adds a VAP on MAPI Machines that does not allow transitioning the
spec.AuthoritativeAPIif doing so would produce an unpredictable result: if the CAPI mirror is deleting, or if infrastructureReady is not true.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.