OCPCLOUD-3262: MAPI & CAPI MachineSet VAP#440
OCPCLOUD-3262: MAPI & CAPI MachineSet VAP#440theobarberbany wants to merge 5 commits intoopenshift:mainfrom
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 new ValidatingAdmissionPolicy and Binding resources to enforce and mirror authoritativeAPI transitions, expands protected label/annotation namespaces to include cluster.x-k8s.io/*, and updates unit and e2e tests and helpers to separate and verify template vs. spec authoritative switching and sync behavior. 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 (policy message)
alt allowed
API_Server->>MAPI: persist MAPI resource (if applicable)
API_Server->>CAPI: persist CAPI resource (if applicable)
MAPI->>SyncController: enqueue/reconcile
CAPI->>SyncController: enqueue/reconcile
SyncController->>MAPI: apply sync mutations when authoritative
SyncController->>CAPI: apply sync mutations when 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
🧪 Generate unit tests (beta)
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 Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Failure to add the new IP will result in interrupted reviews. 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
2 times, most recently
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. |
|
Scheduling tests matching the |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@e2e/machine_migration_vap_tests.go`:
- Around line 32-35: The test's expected VAP annotation error string
(vapProtectedAnnotationMessage) is out of date; update the
vapProtectedAnnotationMessage constant to match the current policy text by
including both "cluster.x-k8s.io/*" and "clusters.x-k8s.io/*" patterns (i.e.,
change the string assigned to vapProtectedAnnotationMessage so it reads the
full, updated policy message exactly as the policy returns).
In `@e2e/machineset_migration_helpers.go`:
- Around line 93-99: The failure message in
switchMachineSetTemplateAuthoritativeAPI contains a typo ("tempplate"); update
the fmt string passed to Eventually(...).Should(...) to correct "tempplate" to
"template" so the error reads "Failed to update MachineSet %s template
AuthoritativeAPI", keeping the same mapiMachineSet.Name interpolation and
leaving function logic and identifiers
(switchMachineSetTemplateAuthoritativeAPI, mapiMachineSet, machineAuthority)
unchanged.
a44f194 to
d78dede
Compare
|
@theobarberbany: This pull request references OCPCLOUD-3262 which is a valid jira issue. 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. |
theobarberbany
force-pushed
the
tb/3262
branch
from
d78dede to
cafe037
Compare
|
/lgtm |
|
@theobarberbany: This pull request references OCPCLOUD-3262 which is a valid jira issue. 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. |
|
Scheduling tests matching the |
|
when creaing a machineset Or creating a machineset, just set |
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
Updates the protected labels and annotations messages for the CAPI and MAPI machine vaps to include all blocked prefixes.
This change splits changing the machineset.spec.AuthoritativeAPI from the changing of the machineset.spec.template.spec.AuthoritativeAPI. This is to handle the new CAPI & MAPI MachineSet VAPs
theobarberbany
force-pushed
the
tb/3262
branch
from
cafe037 to
9bae54e
Compare
|
/test e2e-aws-capi-techpreview |
|
@theobarberbany: This pull request references OCPCLOUD-3262 which is a valid jira issue. 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
🤖 Fix all issues with AI agents
In `@manifests/0000_30_cluster-api_09_admission-policies.yaml`:
- Around line 225-238: The immutability check in the expression named
specLockedExceptAuthoritativeAPI is missing the authoritativeAPI field; update
the array inside specLockedExceptAuthoritativeAPI to include
[object.spec.?authoritativeAPI, oldObject.spec.?authoritativeAPI] alongside the
existing entries so authoritativeAPI is compared the same way as
replicas/minReadySeconds/deletePolicy/selector/template, or alternatively add an
explicit comment explaining why authoritativeAPI is intentionally excluded;
locate the specLockedExceptAuthoritativeAPI expression and add the
authoritativeAPI tuple to the checked list to enforce immutability.
|
Retest this, change spec.authoritativeAPI from MachineAPI to ClusterAPI work as expected. |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
/pipeline required |
|
Scheduling tests matching the |
|
@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. |
9 participants
Summary by CodeRabbit
New Features
Tests