OCPCLOUD-3263: CAPI MachineSet creation validation #437
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@theobarberbany: This pull request references OCPCLOUD-3263 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.22.0" version, but no target version was set. 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-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
WalkthroughAdds ValidatingAdmissionPolicy and ValidatingAdmissionPolicyBinding resources to enforce creation controls on CAPI Machine and MachineSet resources, correcting status path expressions and gating creation based on authoritativeAPI state and pause conditions. Corresponding test suite validates machine set creation scenarios across authorization modes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
theobarberbany
force-pushed
the
tb/3263
branch
3 times, most recently
from
fea3348 to
a0c6760
Compare
|
@theobarberbany: This pull request references OCPCLOUD-3263 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.22.0" version, but no target version was set. 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-ci
bot
removed
the
do-not-merge/work-in-progress
|
@theobarberbany: This pull request references OCPCLOUD-3263 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.22.0" version, but no target version was set. 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: 0
🧹 Nitpick comments (2)
pkg/controllers/machinesetsync/machineset_vap_test.go (1)
391-391: Potential nil builder usage.
mapiMachineSetBuilderis declared at line 352 but not initialized with a namespace before line 391. The code attempts to call.WithNamespace()on a zero-value builder. While this might work if the builder handles zero values gracefully, it's inconsistent with the pattern in the existing test at lines 289-290 where the builder is freshly created.🔎 Suggested fix
- By("Configuring the MAPI MachineSet Builder") - mapiMachineSetBuilder = mapiMachineSetBuilder.WithNamespace(mapiNamespace.Name) + By("Configuring the MAPI MachineSet Builder") + mapiMachineSetBuilder = machinev1resourcebuilder.MachineSet(). + WithNamespace(mapiNamespace.Name)manifests/0000_30_cluster-api_09_admission-policies.yaml (1)
531-536: Minor: Comment references "Machine" instead of "MachineSet".The comment at line 531 says "parameter Machine (MAPI)" but this policy is for MachineSet resources. While this doesn't affect functionality, it could cause confusion during maintenance.
🔎 Suggested fix
- # True when the **parameter Machine (MAPI)** is already paused + # True when the **parameter MachineSet (MAPI)** is already paused - name: mapiPaused
📜 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(1 hunks)pkg/controllers/machinesetsync/machineset_vap_test.go(2 hunks)pkg/controllers/machinesync/machine_sync_controller_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/controllers/machinesync/machine_sync_controller_test.go (2)
pkg/admissionpolicy/testutils/util.go (3)
AddSentinelValidation(185-190)UpdateVAPBindingNamespaces(216-239)VerifySentinelValidation(193-197)pkg/conversion/mapi2capi/interface.go (1)
Machine(24-26)
🔇 Additional comments (5)
pkg/controllers/machinesync/machine_sync_controller_test.go (2)
1643-1692: Test setup follows established patterns.The new test context for "Validate creation of CAPI machine" mirrors the structure of existing VAP tests in this file, including:
- Loading and patching the VAP to include UPDATE operations for easier testing
- Updating the VAP binding namespaces
- Creating sentinel resources and verifying sentinel validation
1694-1781: Test coverage for CAPI machine creation validation is comprehensive.The test cases properly validate:
- Creation allowed when no MAPI counterpart exists (parameterNotFoundAction=Allow)
- Denial of unpaused CAPI machine when MAPI has authoritativeAPI=MachineAPI
- Allowance of CAPI machine with paused annotation when MAPI is authoritative
- Denial when MAPI has authoritativeAPI=ClusterAPI but is not paused
- Allowance when MAPI machine has Paused condition set
pkg/controllers/machinesetsync/machineset_vap_test.go (1)
405-493: Test coverage for CAPI MachineSet creation validation is comprehensive.The test cases properly validate the same scenarios as the Machine tests:
- Creation allowed when no MAPI counterpart exists
- Denial of unpaused CAPI machineset when MAPI has authoritativeAPI=MachineAPI
- Allowance with paused annotation
- Denial when MAPI has authoritativeAPI=ClusterAPI but is not paused
- Allowance when MAPI machineset has Paused condition
manifests/0000_30_cluster-api_09_admission-policies.yaml (2)
402-478: New CAPI Machine creation validation policy looks correct.The policy:
- Properly excludes service account requests (machine-api-controllers, cluster-capi-operator)
- Uses parameterNotFoundAction: Allow to avoid blocking when no MAPI counterpart exists
- Checks both MachineAPI and ClusterAPI authoritative states with appropriate pause requirements
- CEL expressions correctly handle optional fields with
.orValue()
480-556: CAPI MachineSet creation validation policy implementation is correct.The policy mirrors the Machine creation policy with appropriate adjustments:
- Correct paramKind (MachineSet)
- Correct resource rules targeting machinesets
- Same service account exclusions
- Consistent validation logic for pause requirements
theobarberbany
force-pushed
the
tb/3263
branch
from
a0c6760 to
9a011dc
Compare
|
@theobarberbany: This pull request references OCPCLOUD-3263 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.22.0" version, but no target version was set. 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
♻️ Duplicate comments (2)
manifests/0000_30_cluster-api_09_admission-policies.yaml (2)
477-477: Remove unnecessary second optional chaining operator.Based on learnings, only the first
?is needed when using the optional chaining operator. The expressionparams.?status.?authoritativeAPIshould beparams.?status.authoritativeAPI.🔎 Proposed fix
- messageExpression: "'Can\\'t create Cluster API Machine ' + object.metadata.name + ' as a Machine API Machine with status.authoritativeAPI: ' + params.?status.?authoritativeAPI.orValue('<unset>') + ' already exists and is not paused. '" + messageExpression: "'Can\\'t create Cluster API Machine ' + object.metadata.name + ' as a Machine API Machine with status.authoritativeAPI: ' + params.?status.authoritativeAPI.orValue('<unset>') + ' already exists and is not paused. '"Based on learnings, this follows the pattern identified in the previous review.
555-555: Remove unnecessary second optional chaining operator.Based on learnings, only the first
?is needed when using the optional chaining operator. The expressionparams.?status.?authoritativeAPIshould beparams.?status.authoritativeAPI.🔎 Proposed fix
- messageExpression: "'Can\\'t create Cluster API machine set ' + object.metadata.name + ' as a Machine API machine set with status.authoritativeAPI: ' + params.?status.?authoritativeAPI.orValue('<unset>') + ' already exists and is not paused. '" + messageExpression: "'Can\\'t create Cluster API machine set ' + object.metadata.name + ' as a Machine API machine set with status.authoritativeAPI: ' + params.?status.authoritativeAPI.orValue('<unset>') + ' already exists and is not paused. '"Based on learnings, this follows the pattern identified in the previous review.
🧹 Nitpick comments (1)
manifests/0000_30_cluster-api_09_admission-policies.yaml (1)
419-478: Consider extracting common validation logic to reduce duplication.The Machine and MachineSet validation policies (lines 419-478 and 497-556) share nearly identical structure with only minor differences in resource types and message wording. While this duplication ensures correctness and clarity, consider whether CEL expressions support composition patterns that could reduce maintenance burden.
Also applies to: 497-556
📜 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(1 hunks)pkg/controllers/machinesetsync/machineset_vap_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/controllers/machinesetsync/machineset_vap_test.go (2)
pkg/admissionpolicy/testutils/util.go (3)
AddSentinelValidation(185-190)UpdateVAPBindingNamespaces(216-239)VerifySentinelValidation(193-197)pkg/conversion/mapi2capi/interface.go (1)
MachineSet(29-31)
🔇 Additional comments (3)
pkg/controllers/machinesetsync/machineset_vap_test.go (2)
32-32: LGTM!The
metav1import is correctly added to support themetav1.Now()call on line 481 for settingLastTransitionTime.
350-494: Well-structured test coverage for CAPI MachineSet creation validation.The test comprehensively covers the three key scenarios:
- Creation when no MAPI counterpart exists (parameterNotFoundAction=Allow)
- Denial when authoritativeAPI=MachineAPI and CAPI is unpaused
- Proper handling when authoritativeAPI=ClusterAPI based on MAPI pause state
The structure mirrors the machine creation tests, ensuring consistency across the codebase.
manifests/0000_30_cluster-api_09_admission-policies.yaml (1)
402-556: Well-designed admission policies for CAPI creation validation.The new policies correctly implement the creation validation logic:
- Bindings properly configure
parameterNotFoundAction: Allowto permit creation when no corresponding MAPI resource exists- Variables clearly define authoritative API states and pause conditions
- Validations enforce the correct pause-state requirements based on the authoritative API
The structure aligns well with existing policies and provides comprehensive protection for CAPI resource creation.
theobarberbany
force-pushed
the
tb/3263
branch
from
9a011dc to
ea29ded
Compare
|
@theobarberbany: This pull request references OCPCLOUD-3263 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.22.0" version, but no target version was set. 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: 0
♻️ Duplicate comments (2)
manifests/0000_30_cluster-api_09_admission-policies.yaml (2)
477-477: Remove redundant safe-navigation operator.Line 477 uses two
?operators (params.?status.?authoritativeAPI) when only one is needed. The safe-navigation should only be applied afterparams.🔎 Proposed fix
- messageExpression: "'Can\\'t create Cluster API Machine ' + object.metadata.name + ' as a Machine API Machine with status.authoritativeAPI: ' + params.?status.?authoritativeAPI.orValue('<unset>') + ' already exists and is not paused. '" + messageExpression: "'Can\\'t create Cluster API Machine ' + object.metadata.name + ' as a Machine API Machine with status.authoritativeAPI: ' + params.?status.authoritativeAPI.orValue('<unset>') + ' already exists and is not paused. '"Based on past review comments, the safe-navigation operator pattern should be
params.?status.authoritativeAPI(as correctly used in lines 447 and 451).
555-555: Remove redundant safe-navigation operator.Line 555 uses two
?operators (params.?status.?authoritativeAPI) when only one is needed. The safe-navigation should only be applied afterparams.🔎 Proposed fix
- messageExpression: "'Can\\'t create Cluster API machine set ' + object.metadata.name + ' as a Machine API machine set with status.authoritativeAPI: ' + params.?status.?authoritativeAPI.orValue('<unset>') + ' already exists and is not paused. '" + messageExpression: "'Can\\'t create Cluster API machine set ' + object.metadata.name + ' as a Machine API machine set with status.authoritativeAPI: ' + params.?status.authoritativeAPI.orValue('<unset>') + ' already exists and is not paused. '"Based on past review comments, the safe-navigation operator pattern should be
params.?status.authoritativeAPI(as correctly used in lines 525 and 529).
🧹 Nitpick comments (1)
pkg/controllers/machinesetsync/machineset_vap_test.go (1)
397-397: Rename variable to match the resource type.The variable is named
capiSentinelMachinebut creates aMachineSet. Consider renaming tocapiSentinelMachineSetfor clarity.🔎 Proposed fix
- capiSentinelMachine := clusterv1resourcebuilder.MachineSet().WithName("sentinel-machineset").WithNamespace(capiNamespace.Name).Build() - Eventually(k8sClient.Create(ctx, capiSentinelMachine)).Should(Succeed()) + capiSentinelMachineSet := clusterv1resourcebuilder.MachineSet().WithName("sentinel-machineset").WithNamespace(capiNamespace.Name).Build() + Eventually(k8sClient.Create(ctx, capiSentinelMachineSet)).Should(Succeed()) - Eventually(k.Get(capiSentinelMachine)).Should(Succeed()) + Eventually(k.Get(capiSentinelMachineSet)).Should(Succeed()) - admissiontestutils.VerifySentinelValidation(k, capiSentinelMachine, timeout) + admissiontestutils.VerifySentinelValidation(k, capiSentinelMachineSet, timeout)
📜 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(1 hunks)pkg/controllers/machinesetsync/machineset_vap_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/controllers/machinesetsync/machineset_vap_test.go (1)
pkg/admissionpolicy/testutils/util.go (3)
AddSentinelValidation(185-190)UpdateVAPBindingNamespaces(216-239)VerifySentinelValidation(193-197)
🔇 Additional comments (4)
pkg/controllers/machinesetsync/machineset_vap_test.go (2)
32-32: LGTM!The import addition and test context setup follow established patterns in the codebase.
Also applies to: 350-352
405-494: LGTM!The test cases comprehensively cover the validation scenarios:
- Creating CAPI MachineSet without MAPI counterpart (parameterNotFoundAction=Allow)
- Paused/unpaused states for both authoritativeAPI values
- Error message expectations align with the VAP policy definitions
manifests/0000_30_cluster-api_09_admission-policies.yaml (2)
402-417: LGTM!The ValidatingAdmissionPolicyBinding is properly configured with namespace selectors and parameterNotFoundAction=Allow, which correctly allows CAPI Machine creation when no corresponding MAPI Machine exists.
480-495: LGTM!The MachineSet creation binding mirrors the Machine creation binding with appropriate resource type changes, maintaining consistency across the admission policy set.
theobarberbany
force-pushed
the
tb/3263
branch
from
ea29ded to
a2fd054
Compare
|
@theobarberbany: This pull request references OCPCLOUD-3263 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.22.0" version, but no target version was set. 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 |
|
/test unit |
theobarberbany
force-pushed
the
tb/3263
branch
from
a2fd054 to
4e77276
Compare
|
@theobarberbany: This pull request references OCPCLOUD-3263 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.22.0" version, but no target version was set. 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: 0
🧹 Nitpick comments (2)
pkg/controllers/machinesetsync/machineset_vap_test.go (1)
397-398: Minor: Variable naming inconsistency.The variable
capiSentinelMachineis a MachineSet, not a Machine. Consider renaming tocapiSentinelMachineSetfor consistency with the MAPI sentinel variable naming.🔎 Suggested rename
- capiSentinelMachine := clusterv1resourcebuilder.MachineSet().WithName("sentinel-machineset").WithNamespace(capiNamespace.Name).Build() - Eventually(k8sClient.Create(ctx, capiSentinelMachine)).Should(Succeed()) + capiSentinelMachineSet := clusterv1resourcebuilder.MachineSet().WithName("sentinel-machineset").WithNamespace(capiNamespace.Name).Build() + Eventually(k8sClient.Create(ctx, capiSentinelMachineSet)).Should(Succeed()) - Eventually(k.Get(capiSentinelMachine)).Should(Succeed()) + Eventually(k.Get(capiSentinelMachineSet)).Should(Succeed()) - admissiontestutils.VerifySentinelValidation(k, capiSentinelMachine, timeout) + admissiontestutils.VerifySentinelValidation(k, capiSentinelMachineSet, timeout)manifests/0000_30_cluster-api_09_admission-policies.yaml (1)
531-532: Copy-paste error in comment.The comment says "parameter Machine (MAPI)" but this policy is for MachineSets. This should be corrected for clarity.
🔎 Suggested fix
- # True when the **parameter Machine (MAPI)** is already paused + # True when the **parameter MachineSet (MAPI)** is already paused
📜 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(3 hunks)pkg/controllers/machinesetsync/machineset_vap_test.go(2 hunks)
🔇 Additional comments (6)
pkg/controllers/machinesetsync/machineset_vap_test.go (2)
32-32: LGTM!The
metav1import is correctly added for themetav1.Now()usage in the Paused condition'sLastTransitionTimeon line 481.
405-493: LGTM!The test cases comprehensively cover the validation scenarios:
parameterNotFoundAction=Allowwhen no MAPI counterpart exists- Denial of unpaused CAPI MachineSet when MAPI has
authoritativeAPI=MachineAPI- Allowance when CAPI has paused annotation
- Denial when MAPI has
authoritativeAPI=ClusterAPIbut is not paused- Allowance when MAPI has Paused condition
The status updates correctly target the MAPI MachineSet's
status.authoritativeAPIandstatus.conditions, matching the VAP expressions.manifests/0000_30_cluster-api_09_admission-policies.yaml (4)
207-207: LGTM!The expression correctly uses
params.?status.authoritativeAPI.orValue("")with only the first?for optional chaining. This addresses the previous review feedback.
447-477: LGTM!The expressions for
authoritativeAPIMAPIandauthoritativeAPICAPIcorrectly use optional chaining withorValue("")for safe status field access. The updatedmessageExpressionon line 477 properly references the status path.
479-495: LGTM!The binding correctly configures:
matchResourcestargetingopenshift-cluster-apinamespace for CAPI MachineSetsparamRefreferencingopenshift-machine-apifor MAPI MachineSet parametersparameterNotFoundAction: Allowto permit creation when no MAPI counterpart existsThis mirrors the machine creation binding appropriately.
547-556: LGTM!The validation expressions correctly implement the creation rules:
- When MAPI is authoritative (
authoritativeAPIMAPI), CAPI MachineSet must be paused- When CAPI is authoritative (
authoritativeAPICAPI), MAPI MachineSet must be pausedThe message expressions clearly communicate the denial reasons. The logic mirrors the Machine creation policy appropriately.
|
/lgtm |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JoelSpeed 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
|
@theobarberbany: The following test 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. |
|
/retest-required |
|
/verified by units and e2es |
openshift-ci-robot
added
the
verified
|
@theobarberbany: 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. |
|
🤞 those two jobs are no longer permafailing |
|
/override ci/prow/okd-scos-images |
|
/override okd-scos-images |
|
@theobarberbany: Overrode contexts on behalf of theobarberbany: ci/prow/okd-scos-images 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. |
|
@theobarberbany: /override requires failed status contexts, check run or a prowjob name to operate on.
Only the following failed contexts/checkruns were expected:
If you are trying to override a checkrun that has a space in it, you must put a double quote on the context. 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. |
openshift-merge-bot
bot
merged commit 433fd8b
into
openshift:main
3 participants
Should merge after #412 as it's stacked.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.