-
Notifications
You must be signed in to change notification settings - Fork 36
CMP-3930: Include the required selectors to machineconfig to pass the ValidatingAdmissionPolicy #960
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
🤖 To deploy this PR, run the following command: |
|
/retest-required |
|
@xiaojiey: This pull request references CMP-3930 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. 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. |
|
/test e2e-aws-serial |
tests/e2e/framework/common.go
Outdated
| // This pool is still "invalid" for testing as no nodes match this selector | ||
| NodeSelector: &metav1.LabelSelector{ | ||
| MatchLabels: map[string]string{ | ||
| "node-role.kubernetes.io/e2e-invalid": "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Vincent056 I think we need NodeSelector to be empty to exercise the additional logic you added in 4961d0f correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- In newer OpenShift versions(4.21) have a ValidatingAdmissionPolicy that rejects MCPs without proper selectors. As a result it's no longer testable via e2e because you can't create such MCPs anymore due to ValidatingAdmissionPolicy enforcement.
% oc get clusterversion
NAME VERSION AVAILABLE PROGRESSING SINCE STATUS
version 4.21.0-0.nightly-2025-11-22-193140 True False 5h48m Cluster version is 4.21.0-0.nightly-2025-11-22-193140
% oc get ValidatingAdmissionPolicy custom-machine-config-pool-selector -o=jsonpath={.spec.validations} | jq -r
[
{
"expression": "( has(object.spec.machineConfigSelector.matchLabels) && ( (object.spec.machineConfigSelector.matchLabels[\"machineconfiguration.openshift.io/role\"] == \"master\") || (object.spec.machineConfigSelector.matchLabels[\"machineconfiguration.openshift.io/role\"] == \"worker\") || (object.spec.machineConfigSelector.matchLabels[\"machineconfiguration.openshift.io/role\"] == \"arbiter\") ) ) || ( has(object.spec.machineConfigSelector.matchExpressions) && ( (object.spec.machineConfigSelector.matchExpressions.exists( e, e.key == \"machineconfiguration.openshift.io/role\" && e.operator == \"In\" && \"worker\" in e.values) ) || (object.spec.machineConfigSelector.matchExpressions.exists( e, e.key == \"machineconfiguration.openshift.io/role\" && e.operator == \"NotIn\" && !(\"worker\" in e.values)) ) ) )",
"message": "All custom MachineConfigPools must inherit from the worker pool and must have a machineConfigSelector that matches for a 'machineconfiguration.openshift.io/role: worker' label"
}
]
- The logic from commit 4961d0f is still valuable for production environments where legacy/invalid MCPs without selectors might exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about we check whether ValidatingAdmissionPolicy exists or not, If not exists, use the legacy way without selectors; if exists use the minimal selectors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about we check whether ValidatingAdmissionPolicy exists or not, If not exists, use the legacy way without selectors; if exists use the minimal selectors.
That seems like the best option.
eb09393 to
5ad04b7
Compare
|
🤖 To deploy this PR, run the following command: |
|
/test e2e-aws-serial |
|
Cluster provisioning failed. /test e2e-aws-serial |
|
Looks like this worked as expected on a 4.17 cluster in CI: |
rhmdnd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
|
The profile bundle problem is affecting these test results, but we're chasing that down in a separate PR. |
rhmdnd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhmdnd, xiaojiey 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 |
|
/test e2e-aws-serial |
…gAdmissionPolicy Creates the pool with the appropriate selectors based on whether the validation policy exists Add ValidatingAdmissionPolicy scheme registration
5ad04b7 to
a1c0b1d
Compare
|
New changes are detected. LGTM label has been removed. |
|
/retest-required |
|
🤖 To deploy this PR, run the following command: |
|
/retest |
|
@xiaojiey: 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. |
Without this PR, for the
make e2e-serial, you will get the below error: