OCPCLOUD-3262: MAPI & CAPI MachineSet VAP #440
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
@theobarberbany: This pull request references OCPCLOUD-3262 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.22." or "openshift-4.22.", but it targets "openshift-4.21" instead. 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. |
|
Skipping CI for Draft Pull Request. |
📝 WalkthroughWalkthroughAdds many new ValidatingAdmissionPolicy and ValidatingAdmissionPolicyBinding resources guarding labels/annotations and authoritativeAPI transitions, extends admission error messaging, and adds extensive VAP-focused unit tests plus minor test message and constant refactors across Machine/MachineSet sync controllers. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant API_Server
participant VAP as ValidatingAdmissionPolicy
participant SyncController
participant MAPI
participant CAPI
User->>API_Server: create/update Machine or MachineSet
API_Server->>VAP: evaluate applicable policies/bindings
VAP-->>API_Server: allow / deny / warn (with message)
alt allowed
API_Server->>MAPI: persist MAPI resource (if applicable)
API_Server->>CAPI: persist CAPI resource (if applicable)
MAPI->>SyncController: event/reconcile
CAPI->>SyncController: event/reconcile
SyncController->>MAPI: perform sync mutations (if authoritative)
SyncController->>CAPI: perform sync mutations (if authoritative)
else denied
API_Server-->>User: rejection response (policy error)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 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 (4)
Comment |
theobarberbany
force-pushed
the
tb/3262
branch
from
5cf6514 to
5f594f8
Compare
openshift-merge-robot
added
the
needs-rebase
theobarberbany
force-pushed
the
tb/3262
branch
from
5f594f8 to
844e790
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
@theobarberbany: This pull request references OCPCLOUD-3262 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.22." or "openshift-4.22.", but it targets "openshift-4.21" instead. 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: 1
🤖 Fix all issues with AI agents
In @manifests/0000_30_cluster-api_09_admission-policies.yaml:
- Around line 245-271: The error messages are missing protected prefixes
referenced by the validation expressions; update the message for the label guard
(the rule guarding variables.newLabels/oldLabels in the expression that checks
startsWith('machine.openshift.io'), startsWith('kubernetes.io') and
contains('cluster.x-k8s.io/')) to include "cluster.x-k8s.io/*" alongside
"machine.openshift.io/*" and "kubernetes.io/*", and update the annotation guard
message (the rule using variables.newAnn/oldAnn that checks
startsWith('machine.openshift.io') and
contains('cluster.x-k8s.io'/'clusters.x-k8s.io')) to mention both
"cluster.x-k8s.io/*" and "clusters.x-k8s.io/*" in addition to
"machine.openshift.io/*"; mirror the wording used in the
cluster-api-machine-set-vap policy messages for consistency.
📜 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)
manifests/0000_30_cluster-api_09_admission-policies.yamlpkg/controllers/machinesetsync/machineset_vap_test.gopkg/controllers/machinesync/__debug_bin2952830453pkg/controllers/machinesync/__debug_bin4126191047
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/controllers/machinesetsync/machineset_vap_test.go (2)
pkg/conversion/mapi2capi/interface.go (1)
MachineSet(29-31)pkg/admissionpolicy/testutils/util.go (3)
AddSentinelValidation(185-190)UpdateVAPBindingNamespaces(216-239)VerifySentinelValidation(193-197)
🪛 Gitleaks (8.30.0)
pkg/controllers/machinesetsync/machineset_vap_test.go
[high] 428-428: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 440-440: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 681-681: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 693-693: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (9)
pkg/controllers/machinesetsync/machineset_vap_test.go (6)
50-50: LGTM!Good refactoring to extract
capiMachineSetBuilderas a reusable builder variable. This improves readability and allows the builder to be configured differently across test contexts.Also applies to: 102-108
427-435: Static analysis false positive: cluster name mistaken for API key.Gitleaks flags
ci-op-gs2k97d6-c9e33-2smphas a "Generic API Key", but this is an OpenShift CI-generated cluster identifier used in test fixtures. This is a false positive and can be safely ignored.The test setup for creating MAPI and CAPI MachineSet pairs with appropriate labels and annotations looks correct for validating the policy behavior.
Also applies to: 439-448
451-468: LGTM!Correct test logic: when
status.AuthoritativeAPIisMachineAPI, spec changes should be permitted since MAPI is the source of truth.
470-600: LGTM!Comprehensive test coverage for the ClusterAPI-authoritative scenario:
- Correctly verifies spec fields (except
authoritativeAPI) are locked- Validates protection of
machine.openshift.io/*labels and annotations- Tests the param-controlled label synchronization logic
603-706: LGTM!Good parallel test structure for CAPI MachineSet validation. The setup correctly:
- Configures the VAP binding with MAPI as param source and CAPI as target
- Sets
status.AuthoritativeAPIon the MAPI MachineSet (as the param)- Includes
cluster.x-k8s.io/cluster-namelabel in CAPI MachineSet fixtures for testing CAPI-specific label protectionThe cluster identifiers flagged by Gitleaks (lines 681, 693) are the same false positives noted earlier.
707-835: LGTM!Thorough test coverage for CAPI MachineSet behavior when Machine API is authoritative:
- Verifies spec changes are blocked on the CAPI side
- Tests
cluster.x-k8s.io/*label protections (lines 762-772)- Validates param-controlled label synchronization from MAPI to CAPI
manifests/0000_30_cluster-api_09_admission-policies.yaml (3)
161-177: LGTM!The binding configuration is correct:
parameterNotFoundAction: Allowensures MAPI functionality isn't blocked when no CAPI MachineSet exists- Namespace targeting and param references are appropriate
394-410: LGTM!Correct binding configuration with appropriate namespace targeting and param source inversion from the MAPI binding.
412-499: LGTM!The CAPI MachineSet policy is well-structured:
- Correctly checks
authoritativeAPIfrom the MAPI param (line 441)- Uses simpler
object.spec == oldObject.speccomparison (line 460) since CAPI MachineSets don't have anauthoritativeAPIfield to exclude- Error messages are complete and accurate
theobarberbany
force-pushed
the
tb/3262
branch
from
b944df0 to
aa3e93b
Compare
openshift-merge-robot
removed
the
needs-rebase
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
openshift-ci
bot
added
the
do-not-merge/work-in-progress
theobarberbany
force-pushed
the
tb/3262
branch
from
aa3e93b to
5a2249b
Compare
Locally seeing regular flake where we reconcile fast enough that we don't get the intermediary 'sync'd MAPI to CAPI' before we update to the stable state of 'sync'd CAPI to MAPI' we expect to see given the AuthoritativeAPI
theobarberbany
force-pushed
the
tb/3262
branch
from
5a2249b to
a70db0b
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
@theobarberbany: This pull request references OCPCLOUD-3262 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.22." or "openshift-4.22.", but it targets "openshift-4.21" instead. 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. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/controllers/machinesetsync/machineset_sync_controller_test.go (1)
1171-1179: Use the shared constant for the sync-direction message instead of hardcoding.The constants
messageSuccessfullySynchronizedCAPItoMAPIandmessageSuccessfullySynchronizedMAPItoCAPIalready exist inmachineset_sync_controller.goand are accessible from the test file in the same package. Replace the hardcoded string"Successfully synchronized CAPI MachineSet to MAPI"at line 1177 with the constantmessageSuccessfullySynchronizedCAPItoMAPI. This improves maintainability and prevents inconsistencies if the message ever needs to be updated.
🤖 Fix all issues with AI agents
In @pkg/controllers/machinesetsync/machineset_vap_test.go:
- Around line 351-604: Tests embed a gitleaks-flagged hard-coded cluster id
string ("ci-op-gs2k97d6-c9e33-2smph") used as a label value in the
mapiMachineSet/mapiMachineSetBuilder and capiMachineSet builders; replace these
literal cluster-id label values with a clearly non-secret placeholder (e.g.
"ci-cluster-placeholder" or similar) wherever they appear (including the other
occurrences noted in the review), updating references in mapiMachineSetBuilder,
capiMachineSetBuilder, and the Label checks that assert
"capi-param-controlled-label" so the tests still assert matching behaviour but
no longer contain a secret-looking string.
🧹 Nitpick comments (2)
manifests/0000_30_cluster-api_09_admission-policies.yaml (2)
159-281:machine-api-machine-set-vap: policy structure matches intent; consider whether/statusupdates should also be covered.Right now this only matches
resources: ["machinesets"]withoperations: ["UPDATE"]. If you also want to prevent privileged users from mutatingstatus.authoritativeAPI(or other status fields) directly, you likely also wantmachinesets/statusinresources.
393-499:cluster-api-machine-set-vap: good symmetry with MAPI-side policy; verify CEL optional-field features on target OCP/K8s.This policy relies on CEL optional-field helpers like
params.?status.?authoritativeAPI.orValue("")and map defaulting. Please confirm these CEL features are supported/enabled for the target branch payload (PR bot notes branch expects4.22.*).
📜 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.yamlpkg/controllers/machinesetsync/machineset_sync_controller_test.gopkg/controllers/machinesetsync/machineset_vap_test.go
🧰 Additional context used
🪛 Gitleaks (8.30.0)
pkg/controllers/machinesetsync/machineset_vap_test.go
[high] 431-431: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 443-443: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 832-832: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 844-844: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (1)
pkg/controllers/machinesetsync/machineset_vap_test.go (1)
50-110: Builder hoist to outer scope is clean and reduces shadowing.
Updates the protected labels and annotations messages for the CAPI and MAPI machine vaps to include all blocked prefixes.
|
@theobarberbany: This pull request references OCPCLOUD-3262 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.22." or "openshift-4.22.", but it targets "openshift-4.21" instead. 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. |
|
/jira-refresh |
damdo
left a comment
damdo
left a comment
There was a problem hiding this comment.
Thanks Theo, looks reasonable to me!
/approve
/lgtm
Still it may be worth having a second pair of eyes to check such a long PR
/hold
openshift-ci
bot
added
the
do-not-merge/hold
|
/assign @RadekManak @nrb |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: damdo 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
|
Ty dam! This is mostly a re-do of the existing Machine CAPI & MAPI VAPS, the logic is the same (apart from where the spec differs on machinesets, and without the |
|
/test e2e-aws-capi-techpreview |
|
/test regression-clusterinfra-aws-ipi-techpreview-capi |
|
ah @damdo we've got some bad e2es we'll need to fix up - i'll look at those tomorrow |
|
@theobarberbany: 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. |
|
@theobarberbany ack, ty |
In e2e we also changed |
@sunzhaohua2 this VAP blocks when we are modifying machineset.c , I think you are referring to modifications of machineset.m , which should be allowed , i tried manually it works. ( When machineset authoritativeAPI is mapi , you can change .spec.template.spec.authoritative to capi from mapi and vice versa , not when machineset authoritativeAPI is capi) |
|
Added one case https://polarion.engineering.redhat.com/polarion/#/project/OSE/workitem?id=OCP-87094 |
8 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.