OCPCLOUD-3170: Adds CAPI Machine VAP #361
Conversation
|
Pipeline controller notification |
WalkthroughAdds cluster-api-scoped ValidatingAdmissionPolicy and Binding enforcing Machine update constraints; broadens an existing policy to handle template-based specs and all cluster.x-k8s.io versions; refactors machine-sync tests with constants and a shared sentinel validation; introduces requeue-delay constants and naming fixes in MachineSyncReconciler. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client
participant APIServer as K8s API Server
participant Binding as VAP Binding (openshift-cluster-api)
participant Params as Machine API Params (machine.openshift.io)
participant VAP as ValidatingAdmissionPolicy
Client->>APIServer: CREATE/UPDATE Machine (cluster.x-*)
APIServer->>Binding: Resolve bindings for namespace
Binding-->>APIServer: policyName + paramRef
APIServer->>Params: Fetch referenced params
alt params found
APIServer->>VAP: Evaluate validations (object, oldObject, params)
VAP-->>APIServer: Allow or Deny (checks: authoritativeAPI, specPath, labels/annotations)
else params not found
note right of APIServer #D3E4CD: parameterNotFoundAction = Allow
APIServer-->>Client: Allow (unless other policies deny)
end
APIServer-->>Client: Final admission decision
sequenceDiagram
autonumber
participant Reconciler as MachineSyncReconciler
participant CAPI as Cluster API objects
participant MAPI as Machine API objects
Reconciler->>CAPI: Fetch Machine (cluster.x)
Reconciler->>MAPI: Fetch corresponding MAPI Machine/status
alt authoritativeAPI unknown
Reconciler->>Reconciler: log + requeue after requeueUnknownAuthoritativeAPI
else authoritativeAPI unexpected
Reconciler->>Reconciler: log + requeue after requeueUnexpectedAuthoritativeAPI
else infra progressing
Reconciler->>Reconciler: log + requeue after requeueInfraProgress
else stable
Reconciler->>CAPI: Update CAPI Machine to reflect MAPI (sync)
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
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 |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![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
🧹 Nitpick comments (3)
manifests/0000_30_cluster-api_09_admission-policies.yaml (3)
225-237: Fix inconsistent label pattern matching.Line 229 uses
k.contains('cluster.x-k8s.io/')while line 233 uses the same pattern. This is inconsistent with thestartsWith()pattern used for other prefixes and could match unintended labels.Apply this diff to use consistent prefix matching:
- (k.startsWith('machine.openshift.io') || k.startsWith('kubernetes.io') || k.contains('cluster.x-k8s.io/')) && + (k.startsWith('machine.openshift.io') || k.startsWith('kubernetes.io') || k.startsWith('cluster.x-k8s.io/')) &&- (k.startsWith('machine.openshift.io') || k.startsWith('kubernetes.io') || k.contains('cluster.x-k8s.io/')) && + (k.startsWith('machine.openshift.io') || k.startsWith('kubernetes.io') || k.startsWith('cluster.x-k8s.io/')) &&
239-251: Fix inconsistent annotation pattern matching.Similar to the label validation, lines 243 and 247 use
k.contains()for cluster.x-k8s.io patterns instead ofk.startsWith(), which could match unintended annotations.Apply this diff to use consistent prefix matching:
- (k.startsWith('machine.openshift.io') || k.contains('cluster.x-k8s.io') || k.contains('clusters.x-k8s.io')) && + (k.startsWith('machine.openshift.io') || k.startsWith('cluster.x-k8s.io') || k.startsWith('clusters.x-k8s.io')) &&- (k.startsWith('machine.openshift.io') || k.contains('cluster.x-k8s.io') || k.contains('clusters.x-k8s.io')) && + (k.startsWith('machine.openshift.io') || k.startsWith('cluster.x-k8s.io') || k.startsWith('clusters.x-k8s.io')) &&
237-237: Update error messages to reflect the expanded label/annotation protection.The error messages don't mention
cluster.x-k8s.ioorclusters.x-k8s.iopatterns that are now being protected.Apply this diff to update the error messages:
- message: "Cannot add, modify or delete any machine.openshift.io/* or kubernetes.io/* label. This is because status.authoritativeAPI is set to Machine API." + message: "Cannot add, modify or delete any machine.openshift.io/*, kubernetes.io/*, or cluster.x-k8s.io/* label. This is because status.authoritativeAPI is set to Machine API."- message: "Cannot add, modify or delete any machine.openshift.io/* or cluster.x-k8s.io or clusters.x-k8s.io annotation. This is because status.authoritativeAPI is set to Machine API." + message: "Cannot add, modify or delete any machine.openshift.io/*, cluster.x-k8s.io/*, or clusters.x-k8s.io/* annotation. This is because status.authoritativeAPI is set to Machine API."Also applies to: 251-251
📜 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(1 hunks)
🔇 Additional comments (5)
manifests/0000_30_cluster-api_09_admission-policies.yaml (5)
154-172: LGTM! ValidatingAdmissionPolicyBinding configuration looks correct.The binding properly targets the
openshift-cluster-apinamespace and references parameters fromopenshift-machine-apinamespace with appropriate fallback behavior usingparameterNotFoundAction: Allow.
174-183: LGTM! Policy metadata and parameter configuration are correct.The policy correctly targets Machine API objects (
machine.openshift.io/v1beta1) as parameters for validating Cluster API machines.
185-190: LGTM! Resource matching configuration is appropriate.The policy correctly targets Cluster API machines (
cluster.x-k8s.io/v1beta1/machines) for UPDATE operations only, which aligns with the intent to control modifications.
222-223: LGTM! Spec change restriction is appropriately strict.Unlike the Machine API policy which allows
spec.authoritativeAPIchanges, this policy completely locks the spec for Cluster API machines when Machine API is authoritative, which is the correct behavior.
253-260: LGTM! Parameter-controlled label validation logic is correct.The validation properly ensures that labels can only change to match the authoritative Machine API values, maintaining consistency between the two APIs.
theobarberbany
force-pushed
the
tb/update-machine-vap
branch
2 times, most recently
from
367a0e9 to
7c44697
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
manifests/0000_30_cluster-api_09_admission-policies.yaml (1)
203-203: AuthoritativeAPI equality check fixed.The condition now explicitly compares against "MachineAPI". This resolves the earlier presence-only check.
🧹 Nitpick comments (3)
pkg/controllers/machinesync/machine_sync_controller_test.go (1)
1160-1163: Avoid mixing WithName("") and WithGenerateName — build a fresh object instead.Setting Name to "" and then GenerateName is error‑prone; construct a new builder with only GenerateName.
- testMachine := capiMachineBuilder.WithName("").WithGenerateName("test-machine-").Build() + testMachine := clusterv1resourcebuilder. + Machine(). + WithNamespace(capiNamespace.GetName()). + WithGenerateName("test-machine-"). + Build()manifests/0000_30_cluster-api_09_admission-policies.yaml (2)
225-237: Use prefix match for cluster.x-k8s.io labels instead of substring.contains() can false‑positive on keys that merely include the substring. Use startsWith() like other prefixes.
- variables.newLabels.exists(k, - (k.startsWith('machine.openshift.io') || k.startsWith('kubernetes.io') || k.contains('cluster.x-k8s.io/')) && + variables.newLabels.exists(k, + (k.startsWith('machine.openshift.io') || k.startsWith('kubernetes.io') || k.startsWith('cluster.x-k8s.io')) && (variables.oldLabels[?k].orValue(null) != variables.newLabels[k]) ) || - variables.oldLabels.exists(k, - (k.startsWith('machine.openshift.io') || k.startsWith('kubernetes.io') || k.contains('cluster.x-k8s.io/')) && + variables.oldLabels.exists(k, + (k.startsWith('machine.openshift.io') || k.startsWith('kubernetes.io') || k.startsWith('cluster.x-k8s.io')) && !(k in variables.newLabels) )
239-251: Also switch annotation checks to prefix match.Aligns with label logic and avoids substring matches.
- variables.newAnn.exists(k, - (k.startsWith('machine.openshift.io') || k.contains('cluster.x-k8s.io') || k.contains('clusters.x-k8s.io')) && + variables.newAnn.exists(k, + (k.startsWith('machine.openshift.io') || k.startsWith('cluster.x-k8s.io') || k.startsWith('clusters.x-k8s.io')) && (variables.oldAnn[?k].orValue(null) != variables.newAnn[k]) ) || - variables.oldAnn.exists(k, - (k.startsWith('machine.openshift.io') || k.contains('cluster.x-k8s.io') || k.contains('clusters.x-k8s.io')) && + variables.oldAnn.exists(k, + (k.startsWith('machine.openshift.io') || k.startsWith('cluster.x-k8s.io') || k.startsWith('clusters.x-k8s.io')) && !(k in variables.newAnn) )
📜 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(9 hunks)
🔇 Additional comments (4)
pkg/controllers/machinesync/machine_sync_controller_test.go (3)
836-847: Good: centralized constants and shared sentinel validation.This de-duplicates test data and keeps assertions consistent.
969-971: Good: assert on the shared sentinel message.Keeps error checks stable against policy text changes.
1033-1036: Consistent use of testLabelValue across cases.Reduces string drift in expectations.
Also applies to: 1071-1074, 1109-1112
manifests/0000_30_cluster-api_09_admission-policies.yaml (1)
275-284: Nice: factoring spec path logic for Machine vs MachineSet.Makes the policy concise and less error‑prone.
theobarberbany
force-pushed
the
tb/update-machine-vap
branch
from
7c44697 to
9113184
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
manifests/0000_30_cluster-api_09_admission-policies.yaml (1)
200-204: Fixed: authoritativeAPI equality check is now correct and null‑safeSwitching to an explicit equality against "MachineAPI" (with optional chaining fallback) addresses the earlier logic bug. Looks good.
pkg/controllers/machinesync/machine_sync_controller_test.go (1)
1118-1135: Focused block removed (now Context) — resolves CI riskThanks for switching from FContext to Context; this prevents accidental focused runs in CI.
🧹 Nitpick comments (3)
manifests/0000_30_cluster-api_09_admission-policies.yaml (2)
225-238: Use startsWith for cluster.x-k8s.io label prefix checks (avoid over‑matching)contains("cluster.x-k8s.io/") will also match domains like topology.cluster.x-k8s.io/* unintentionally. Prefer startsWith for precise prefix semantics (aligns with other checks here).
Apply this diff:
- (k.startsWith('machine.openshift.io') || k.startsWith('kubernetes.io') || k.contains('cluster.x-k8s.io/')) && + (k.startsWith('machine.openshift.io') || k.startsWith('kubernetes.io') || k.startsWith('cluster.x-k8s.io')) && @@ - (k.startsWith('machine.openshift.io') || k.startsWith('kubernetes.io') || k.contains('cluster.x-k8s.io/')) && + (k.startsWith('machine.openshift.io') || k.startsWith('kubernetes.io') || k.startsWith('cluster.x-k8s.io')) &&
239-251: Align annotation prefix checks with startsWith and consistent messagingSame rationale as labels: contains("cluster.x-k8s.io") and contains("clusters.x-k8s.io") may over‑match. Use startsWith to target the intended prefixes. Also consider updating the message to include trailing /* for clarity, but that’s optional.
- (k.startsWith('machine.openshift.io') || k.contains('cluster.x-k8s.io') || k.contains('clusters.x-k8s.io')) && + (k.startsWith('machine.openshift.io') || k.startsWith('cluster.x-k8s.io') || k.startsWith('clusters.x-k8s.io')) && @@ - (k.startsWith('machine.openshift.io') || k.contains('cluster.x-k8s.io') || k.contains('clusters.x-k8s.io')) && + (k.startsWith('machine.openshift.io') || k.startsWith('cluster.x-k8s.io') || k.startsWith('clusters.x-k8s.io')) &&pkg/controllers/machinesync/machine_sync_controller_test.go (1)
1173-1183: Nit: comment misattributes the rejection causeThis failure is due to the appended sentinel validation, not the protected label prefixes (the test label isn’t under machine.openshift.io/ or cluster.x-k8s.io/).
Consider updating the comment to: “Setting the test sentinel label is blocked by the appended sentinel validation.”
📜 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(9 hunks)
🔇 Additional comments (18)
manifests/0000_30_cluster-api_09_admission-policies.yaml (4)
156-173: New Cluster API VAP and binding are well‑scoped
- Binding targets openshift-cluster-api and pulls params from openshift-machine-api with parameterNotFoundAction: Allow.
- Policy matches UPDATEs for cluster.x-k8s.io/v1beta1 Machines and gates validations behind param/name match plus SA exclusion.
LGTM.Also applies to: 175-184, 185-193
219-224: Spec lock for CAPI machines under MAPI authorityDenying any spec change when MAPI is authoritative is consistent with the design and test expectations. LGTM.
275-284: Generalized Machine vs MachineSet pathing looks goodThe machineSpec/specPath variable split cleanly supports both Machine and MachineSet resources. LGTM.
164-171: Minor: confirm ParamRef selector behavior is as intendedparamRef.selector: {} plus a name equality matchCondition typically works, but ParamRef evaluation semantics can vary across versions. Please confirm we never end up evaluating against multiple params for the same request (which could cause unexpected denials).
Would you like a quick probe test added that creates two param objects with the same name in different namespaces to ensure only the intended one is evaluated?
Also applies to: 41-49
pkg/controllers/machinesync/machine_sync_controller_test.go (14)
836-841: Test constants improve readability and reduce duplicationGood extraction of shared names and values.
843-847: Shared sentinel validation is tidy and reusableCentralizing the expression/message avoids drift across tests. LGTM.
875-882: Param‑controlled label seeded on MAPI machineThis neatly enables the param‑controlled label tests later. LGTM.
919-923: Patching VAP with sentinel validation via constant nameLooks good and matches later expectations on the sentinel message.
931-954: Binding patching to dynamic namespacesUpdating ParamRef.Namespace and NamespaceSelector to generated namespaces is correct for envtest. LGTM.
971-971: Assertion uses shared sentinel messageGood consistency; avoids brittle string duplication.
983-987: Spec update allowed under MAPI authorityTest reflects policy behavior correctly. LGTM.
1033-1037: Protected label test uses shared valueSolid coverage; minor duplication removed. LGTM.
1071-1075: Protected annotation test uses shared valueConsistent with previous test; LGTM.
1107-1112: Reject mixed spec update with spec.authoritativeAPIGood negative test for partial spec updates. LGTM.
1196-1200: CAPI spec change correctly rejected under MAPI authorityCovers the core contract from the new VAP. LGTM.
1203-1250: Comprehensive coverage of protected label behaviorsGood positive/negative coverage for machine.openshift.io and cluster.x-k8s.io prefixes. LGTM.
1252-1288: Protected annotations are thoroughly exercisedGood balance of modify/delete/empty‑string cases and an allowed case. LGTM.
1290-1305: Param‑controlled label rules validated both ways
- Accept when matching param value
- Reject when diverging
Looks correct.
theobarberbany
force-pushed
the
tb/update-machine-vap
branch
from
9113184 to
eee4f5a
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/controllers/machinesync/machine_sync_controller_test.go (1)
1519-1524: *Fix compile-time bug: don't compare a string to a corev1.Namespace — compare to its name and guard the index.capiNamespace is declared as "var capiNamespace *corev1.Namespace"; comparing expr.Values[i] (string) to capiNamespace is a type error. Replace the check with a string comparison (e.g. ensure len(expr.Values) > i && expr.Values[i] == capiNamespace.GetName()) and keep assigning namespace.Name.
File: pkg/controllers/machinesync/machine_sync_controller_test.go lines ~1543–1547.
♻️ Duplicate comments (1)
pkg/controllers/machinesync/machine_sync_controller_test.go (1)
1118-1135: Thanks for replacing the focused FContext with Context.Prevents accidental CI skips. Nothing else to change here.
🧹 Nitpick comments (7)
pkg/controllers/machinesync/machine_sync_controller_test.go (4)
881-881: Clarify test intent for param-controlled label.Consider a brief comment explaining this label is intentionally MAPI‑controlled and exercised later in CAPI VAP tests to avoid future confusion.
919-927: Wait for VAP ObservedGeneration correctly; minor: include a reason in By().Flow is sound. Optional: annotate the By() with what change is being observed to ease debugging.
1160-1166: Avoid setting an empty Name before GenerateName.WithName("") is redundant and may be misleading; GenerateName alone is sufficient.
Apply this diff:
- testMachine := capiMachineBuilder.WithName("").WithGenerateName("test-machine-").Build() + testMachine := capiMachineBuilder.WithGenerateName("test-machine-").Build()
1173-1179: Fix misleading comment: rejection is from the sentinel rule, not label guards."test-sentinel" is not a protected prefix; the failure here comes from the appended sentinel validation.
Apply this diff:
- // The sync controller will sync the labels from the MAPI Machine to the CAPI machine. - // The labels are protected (machine.openshift.io/cluster.x-k8s.io), - // and so setting the test sentinel label is blocked by the labels rule. + // The sync controller will sync labels; however, the "test-sentinel" label is not a protected prefix. + // The following mutation is expected to be blocked by the appended sentinel validation.manifests/0000_30_cluster-api_09_admission-policies.yaml (3)
192-204: AuthoritativeAPI check uses equality; consider simplifying optional chaining.Current expression is safe. If desired, simplify to avoid redundant orValue when params are present:
Apply this diff (behaviorally equivalent when params exist):
- expression: "params.?status.?authoritativeAPI.orValue(\"\") == \"MachineAPI\"" + expression: "params.?status.?authoritativeAPI == \"MachineAPI\""
225-238: Use startsWith for cluster.x-k8s.io label matching (precision/consistency).contains() may match unintended keys; startsWith('cluster.x-k8s.io/') is clearer and matches the tests/messages.
Apply this diff:
- (k.startsWith('machine.openshift.io') || k.startsWith('kubernetes.io') || k.contains('cluster.x-k8s.io/')) && + (k.startsWith('machine.openshift.io') || k.startsWith('kubernetes.io') || k.startsWith('cluster.x-k8s.io/')) && @@ - (k.startsWith('machine.openshift.io') || k.startsWith('kubernetes.io') || k.contains('cluster.x-k8s.io/')) && + (k.startsWith('machine.openshift.io') || k.startsWith('kubernetes.io') || k.startsWith('cluster.x-k8s.io/')) &&
239-252: Likewise for annotation prefixes: prefer startsWith.Aligns with label guard and reduces accidental matches.
Apply this diff:
- (k.startsWith('machine.openshift.io') || k.contains('cluster.x-k8s.io') || k.contains('clusters.x-k8s.io')) && + (k.startsWith('machine.openshift.io') || k.startsWith('cluster.x-k8s.io') || k.startsWith('clusters.x-k8s.io')) && @@ - (k.startsWith('machine.openshift.io') || k.contains('cluster.x-k8s.io') || k.contains('clusters.x-k8s.io')) && + (k.startsWith('machine.openshift.io') || k.startsWith('cluster.x-k8s.io') || k.startsWith('clusters.x-k8s.io')) &&
📜 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(9 hunks)
🔇 Additional comments (10)
pkg/controllers/machinesync/machine_sync_controller_test.go (8)
836-847: Good extraction of shared constants and sentinel validation.Centralizing names and the sentinel reduces duplication and brittle assertions.
931-955: Binding patching logic looks correct.ParamRef/namespaceSelector rewrites align with the test namespaces.
969-971: Nice assertion against the sentinel Message.Keeps tests resilient to validation ordering.
985-986: Use of testLabelValue improves consistency.Good replacement of magic strings.
1186-1200: Spec updates correctly denied when MAPI is authoritative.Assertion message matches the VAP.
1202-1250: Label guards for CAPI path look comprehensive.Covers modify/delete/empty-string cases and non-protected pass‑through.
1252-1288: Annotation guards for CAPI path look comprehensive.Protected prefixes are enforced; unrelated keys allowed.
1290-1305: Param-controlled label tests are correct and mirror the MAPI path.Matches policy behavior for CAPI respecting MAPI‑controlled labels.
manifests/0000_30_cluster-api_09_admission-policies.yaml (2)
156-173: Binding scope and parameter strategy look correct.Binding cluster-api-machine-vap to openshift-cluster-api with params from openshift-machine-api and Allow on missing params aligns with migration scenarios.
275-284: Nice refactor to machineSpec/specPath variables.CEL expressions are simpler and messages are clearer for Machines vs MachineSets.
|
/retitle OCPCLOUD-3170: Adds CAPI Machine VAP |
openshift-ci
bot
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@theobarberbany: This pull request references OCPCLOUD-3170 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 either version "4.21." or "openshift-4.21.", but it targets "openshift-4.20" instead. 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. |
|
@theobarberbany: This pull request references OCPCLOUD-3170 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 either version "4.21." or "openshift-4.21.", but it targets "openshift-4.20" instead. 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. |
|
@theobarberbany: This pull request references OCPCLOUD-3170 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 either version "4.21." or "openshift-4.21.", but it targets "openshift-4.20" instead. 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 unit |
|
@coderabbitai full review |
|
@coderabbitai review |
theobarberbany
force-pushed
the
tb/update-machine-vap
branch
2 times, most recently
from
1cb7771 to
40763c5
Compare
|
/test e2e-aws-capi-techpreview |
|
@theobarberbany: This pull request references OCPCLOUD-3170 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 either version "4.21." or "openshift-4.21.", but it targets "openshift-4.20" instead. 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. |
|
/assign @mdbooth @RadekManak |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@theobarberbany: This pull request references OCPCLOUD-3170 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 either version "4.21." or "openshift-4.21.", but it targets "openshift-4.20" instead. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/controllers/machinesync/machine_sync_controller.go (3)
1042-1048: Bug: always returns an error after removing finalizer (even on success).fmt.Errorf with %w on a nil error still returns a non-nil error. Return the actual changed flag and nil on success.
- _, err := util.RemoveFinalizer(ctx, r.Client, mapiMachine, SyncFinalizer) - - return false, fmt.Errorf("failed to remove finalizer: %w", err) + changed, err := util.RemoveFinalizer(ctx, r.Client, mapiMachine, SyncFinalizer) + if err != nil { + return false, fmt.Errorf("failed to remove finalizer: %w", err) + } + return changed, nil
1161-1167: Bug: always returns an error after removing finalizer (even on success).Same issue here; return changed, nil on success.
- _, err := util.RemoveFinalizer(ctx, r.Client, capiMachine, SyncFinalizer) - - return false, fmt.Errorf("failed to remove finalizer: %w", err) + changed, err := util.RemoveFinalizer(ctx, r.Client, capiMachine, SyncFinalizer) + if err != nil { + return false, fmt.Errorf("failed to remove finalizer: %w", err) + } + return changed, nil
1300-1313: Bug: compareMAPIMachines parses providerSpec from ‘a’ twice; never compares ‘b’.This prevents detection of providerSpec differences and can skip required updates.
- ps2, err := mapi2capi.AWSProviderSpecFromRawExtension(a.Spec.ProviderSpec.Value) + ps2, err := mapi2capi.AWSProviderSpecFromRawExtension(b.Spec.ProviderSpec.Value)
🧹 Nitpick comments (5)
pkg/controllers/machinesync/machine_sync_controller_test.go (1)
1050-1054: Minor test description nit.Wording: “rejects adding a new the 'machine-template-hash' label”.
- It("rejects adding a new the 'machine-template-hash' label", func() { + It("rejects adding a new 'machine-template-hash' label", func() {manifests/0000_30_cluster-api_09_admission-policies.yaml (4)
118-132: Message doesn’t reflect enforcement of cluster.x-k8s.io/ labels.*Validation blocks machine.openshift.io, kubernetes.io, and cluster.x-k8s.io labels, but the message omits cluster.x-k8s.io.
- message: "Cannot add, modify or delete any machine.openshift.io/* or kubernetes.io/* label. This is because status.authoritativeAPI is set to Cluster API." + message: "Cannot add, modify or delete any machine.openshift.io/*, kubernetes.io/* or cluster.x-k8s.io/* label. This is because status.authoritativeAPI is set to Cluster API."
132-145: Message doesn’t reflect enforcement of cluster.x‑k8s.io/clusters.x‑k8s.io annotations.The expression guards them, but the message mentions only machine.openshift.io.
- message: "Cannot add, modify or delete any machine.openshift.io/* annotation. This is because status.authoritativeAPI is set to Cluster API." + message: "Cannot add, modify or delete any machine.openshift.io/* or cluster.x-k8s.io or clusters.x-k8s.io annotation. This is because status.authoritativeAPI is set to Cluster API."
155-158: Trailing quote in error message.Fix stray apostrophe.
- message: "Setting the 'machine-template-hash' label is forbidden.'" + message: "Setting the 'machine-template-hash' label is forbidden"
266-268: Trailing quote in error message.Same nit here.
- message: "Setting the 'machine-template-hash' label is forbidden.'" + message: "Setting the 'machine-template-hash' label is forbidden"
📜 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 (3)
manifests/0000_30_cluster-api_09_admission-policies.yaml(3 hunks)pkg/controllers/machinesync/machine_sync_controller.go(11 hunks)pkg/controllers/machinesync/machine_sync_controller_test.go(12 hunks)
🔇 Additional comments (8)
pkg/controllers/machinesync/machine_sync_controller.go (2)
262-267: Good: removed unnecessary requeues for unknown/unexpected AuthoritativeAPI.This addresses prior feedback; the controller will reconcile on watched changes without periodic requeues.
549-557: Avoid returning a non-empty Result together with a possible error (controller-runtime warning).Split the condition patch error from the requeue return to prevent “non-empty Result with error” noise and retries.
- if synchronizationIsProgressing { - return ctrl.Result{RequeueAfter: requeueInfraProgress}, r.applySynchronizedConditionWithPatch(ctx, mapiMachine, corev1.ConditionUnknown, - reasonProgressingToCreateCAPIInfraMachine, progressingToSynchronizeMAPItoCAPI, nil) - } + if synchronizationIsProgressing { + if err := r.applySynchronizedConditionWithPatch( + ctx, mapiMachine, corev1.ConditionUnknown, reasonProgressingToCreateCAPIInfraMachine, progressingToSynchronizeMAPItoCAPI, nil, + ); err != nil { + return ctrl.Result{}, err + } + return ctrl.Result{RequeueAfter: requeueInfraProgress}, nil + }pkg/controllers/machinesync/machine_sync_controller_test.go (3)
195-201: Good: removed SyncPeriod usage and left it commented for reference.Avoids relying on resyncs; controller reacts to watched changes. Matches earlier feedback.
841-851: Nice consolidation: shared constants and sentinel validation.Reduces duplication and makes tests easier to maintain.
1129-1323: Good: unfocused Cluster API VAP tests, symmetric with MAPI path and param-controlled label checks.Covers both label/annotation protections and spec immutability under the respective authorities.
manifests/0000_30_cluster-api_09_admission-policies.yaml (3)
206-208: Correct equality check for authoritativeAPI.Using an explicit equality against "MachineAPI" fixes the earlier presence-only check.
189-195: Consider future-proofing apiVersions.If you intend to guard future CAPI versions (e.g., v1), use apiVersions: ["*"] like the other policies.
Do you want to scope this policy strictly to v1beta1, or broaden to ["*"] for forward compatibility?
283-292: Nice reuse via machineSpec/specPath variables.Cleanly handles both Machine and template specs with targeted messages.
|
@theobarberbany: This pull request references OCPCLOUD-3170 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 either version "4.21." or "openshift-4.21.", but it targets "openshift-4.20" instead. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/controllers/machinesync/machine_sync_controller.go (3)
1326-1334: ProviderSpec comparison bug: ps2 parsed from the wrong object.This causes providerSpec diffs to be missed, leading to stale objects not being updated.
Apply this diff:
- ps2, err := mapi2capi.AWSProviderSpecFromRawExtension(a.Spec.ProviderSpec.Value) + ps2, err := mapi2capi.AWSProviderSpecFromRawExtension(b.Spec.ProviderSpec.Value)
1060-1073: Nil-pointer risk and unconditional error return in deletion path.
- If capiMachine is nil and infraMachine is non‑nil, dereferencing capiMachine.DeletionTimestamp panics.
- Always returning an error after RemoveFinalizer (even when err is nil) is incorrect.
Apply this diff:
- if capiMachine == nil && util.IsNilObject(infraMachine) { + if capiMachine == nil && util.IsNilObject(infraMachine) { logger.Info("Cluster API machine and infra machine do not exist, removing corresponding Machine API machine sync finalizer") - // We don't have a capi machine or infra resouorces to clean up we can + // We don't have a CAPI machine or infra resources to clean up; we can // just let the MAPI operators function as normal, and remove our own sync // finalizer. - _, err := util.RemoveFinalizer(ctx, r.Client, mapiMachine, SyncFinalizer) - - return false, fmt.Errorf("failed to remove finalizer: %w", err) + if _, err := util.RemoveFinalizer(ctx, r.Client, mapiMachine, SyncFinalizer); err != nil { + return false, fmt.Errorf("failed to remove finalizer: %w", err) + } + return false, nil } - if capiMachine.DeletionTimestamp.IsZero() { + if capiMachine != nil && capiMachine.DeletionTimestamp.IsZero() { logger.Info("Machine API machine is being deleted, issuing deletion to corresponding Cluster API machine") if err := r.Client.Delete(ctx, capiMachine); err != nil { return false, fmt.Errorf("failed delete Cluster API machine: %w", err) } }
1182-1188: Unconditional error return after RemoveFinalizer (CAPI→MAPI deletion path).Returns an error even when none occurred.
Apply this diff:
- _, err := util.RemoveFinalizer(ctx, r.Client, capiMachine, SyncFinalizer) - - return false, fmt.Errorf("failed to remove finalizer: %w", err) + if _, err := util.RemoveFinalizer(ctx, r.Client, capiMachine, SyncFinalizer); err != nil { + return false, fmt.Errorf("failed to remove finalizer: %w", err) + } + return false, nil
🧹 Nitpick comments (4)
pkg/controllers/machinesync/machine_sync_controller.go (4)
427-436: Remove temporary debug logs.Drop “log X” Info lines (and commented log) or demote to logger.V(4).Info to keep production logs clean.
Apply this diff (sample for one location; repeat similarly across others):
- logger.Info("log 2")Also applies to: 493-495, 543-545, 549-551, 560-560, 567-567, 574-575, 1272-1300
83-87: Unused requeue constants.requeueUnknownAuthoritativeAPI and requeueUnexpectedAuthoritativeAPI are currently unused; remove or wire them up.
- requeueUnknownAuthoritativeAPI = 2 * time.Second - requeueUnexpectedAuthoritativeAPI = 2 * time.Second requeueInfraProgress = 1 * time.Second
225-225: Minor: fix typo in error message.Double colon after “machine”.
- return ctrl.Result{}, fmt.Errorf("failed to get Cluster API machine:: %w", err) + return ctrl.Result{}, fmt.Errorf("failed to get Cluster API machine: %w", err)
1038-1042: Typo in named return variable.Rename shouldReqeue → shouldRequeue for clarity.
-func (r *MachineSyncReconciler) reconcileMAPItoCAPIMachineDeletion(ctx context.Context, mapiMachine *machinev1beta1.Machine, capiMachine *clusterv1.Machine, infraMachine client.Object) (shouldReqeue bool, err error) { +func (r *MachineSyncReconciler) reconcileMAPItoCAPIMachineDeletion(ctx context.Context, mapiMachine *machinev1beta1.Machine, capiMachine *clusterv1.Machine, infraMachine client.Object) (shouldRequeue bool, err error) { - defer func() { - logger.Info("exiting reconcileMAPItoCAPIMachineDeletion", "shouldRequeue", shouldReqeue, "error", err) - }() + defer func() { + logger.Info("exiting reconcileMAPItoCAPIMachineDeletion", "shouldRequeue", shouldRequeue, "error", err) + }()
📜 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 (4)
pkg/controllers/machinesync/machine_sync_controller.go(22 hunks)pkg/controllers/machinesync/machine_sync_controller_test.go(12 hunks)pkg/controllers/machinesync/suite_test.go(3 hunks)pkg/util/watch_filters.go(3 hunks)
🔇 Additional comments (7)
pkg/controllers/machinesync/suite_test.go (1)
68-76: klog init looks good.FlagSet setup and verbosity configuration are fine for tests; switching to GinkgoLogr + ctrl logger is appropriate.
pkg/util/watch_filters.go (2)
39-44: Structured, consistent reconcile logs — good improvement.Clear, uniform keys (objectType/namespace/name) at V(4) will help triage.
86-97: Predicate logging without side effects — OK.Local result var with a detailed log before returning is fine and keeps behavior unchanged.
pkg/controllers/machinesync/machine_sync_controller_test.go (2)
921-977: Nicely centralized VAP test config.Using constants and a shared testSentinelValidation reduces duplication and tightens assertions.
1130-1334: Remove focused Ginkgo block (FContext) — it will skip other tests in CI.Replace FContext with Context so the suite runs all tests.
Apply this diff:
- FContext("cluster api vap tests", func() { + Context("cluster api vap tests", func() {pkg/controllers/machinesync/machine_sync_controller.go (2)
569-572: Avoid returning both a requeue Result and an error.controller-runtime warns when both are set. Apply the patch first; requeue only on success.
Apply this diff:
- if synchronizationIsProgressing { - return ctrl.Result{RequeueAfter: requeueInfraProgress}, r.applySynchronizedConditionWithPatch(ctx, mapiMachine, corev1.ConditionUnknown, - reasonProgressingToCreateCAPIInfraMachine, progressingToSynchronizeMAPItoCAPI, nil) - } + if synchronizationIsProgressing { + if err := r.applySynchronizedConditionWithPatch( + ctx, mapiMachine, corev1.ConditionUnknown, reasonProgressingToCreateCAPIInfraMachine, progressingToSynchronizeMAPItoCAPI, nil, + ); err != nil { + return ctrl.Result{}, err + } + return ctrl.Result{RequeueAfter: requeueInfraProgress}, nil + }
1133-1147: Remove unnecessary nil-guard — util.RemoveFinalizer already handles nilutil.RemoveFinalizer (pkg/util/finalizer.go) returns early when util.IsNilObject(obj) is true, so calling it with a nil infraMachine is safe; omit the proposed guard.
theobarberbany
force-pushed
the
tb/update-machine-vap
branch
2 times, most recently
from
b24a2d0 to
f7560b6
Compare
openshift-ci
bot
added
the
do-not-merge/work-in-progress
theobarberbany
force-pushed
the
tb/update-machine-vap
branch
3 times, most recently
from
0dcfc8a to
88a0272
Compare
- Adds CAPI Machine VAP, similar to the MAPI one, but for when MAPI is autoritative. - Adds tests - Updates machine sync reconciler to requeue after 2s of AuthoritativeAPI is empty (rather than the 10m default)
theobarberbany
force-pushed
the
tb/update-machine-vap
branch
from
88a0272 to
52c2b64
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
@theobarberbany: This pull request references OCPCLOUD-3170 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 either version "4.21." or "openshift-4.21.", but it targets "openshift-4.20" instead. 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. |
|
@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. |
5 participants
We expect the tests to fail, as we currently have a bug with how watch filters for infra machines are set up. We expect the CAPI Machine name will always equal the InfraMachine name. When we control the creation of the CAPI machine and infra machine, this holds. It may not always be the case if people bring their own CAPI machines. This will be addressed in a separate PR.
Summary by CodeRabbit
New Features
Tests
Bug Fixes
Style/Logging