[ROSAENG-61186] - fix: Allow OSC operator to manage MachineConfigs - #621
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe regular-user-validation webhook now permits the OpenShift Sandboxed Containers operator’s default ServiceAccount to create MachineConfig resources. Tests cover the allowed identity and two denied identities. ChangesOSC MachineConfig authorization
Estimated code review effort: 1 (Trivial) | ~5 minutes Merge Risk: ⚪ Minimal · up to This change narrowly allows the OSC controller identity to manage the required MachineConfig resources; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
Full details: Stable And Deterministic Test NamesExplanation The changed tests use three fixed string identifiers: Full details: Test Structure And QualityExplanation PASS: The changed tests use the repository's existing table-driven Full details: Microshift Test CompatibilityExplanation PASS — The pull request adds standard Go unit-test cases, not new Ginkgo e2e tests. Full details: Single Node Openshift (Sno) Test CompatibilityExplanation The added tests are table-driven Go unit tests under Full details: Topology-Aware Scheduling CompatibilityExplanation The pull request changes only the MachineConfig authorization allowlist and its unit tests. The diff adds one service-account identity and authorization test cases. It adds no deployment manifest, replica setting, affinity, topology spread constraint, node selector, toleration, or other scheduling constraint. The topology-aware scheduling check is therefore not triggered. Full details: Ote Binary Stdout ContractExplanation PASS: The pull request changes only the Full details: Ipv6 And Disconnected Network Test CompatibilityExplanation The added tests are standard Go Full details: No-Weak-CryptoExplanation PASS. The pull request changes only the MachineConfig ServiceAccount allowlist, comments, and related authorization tests. The added diff contains no MD5, SHA1, DES, 3DES, RC4, Blowfish, ECB, custom crypto, or non-constant-time secret comparison usage. The authorization function performs an exact string membership check with Full details: Container-PrivilegesExplanation The pull request changes only Go source and Go tests. The diff adds a ServiceAccount string to a MachineConfig authorization allowlist and adds authorization test cases. It adds no container or Kubernetes manifest, and no added line contains Full details: No-Sensitive-Data-In-LogsExplanation The pull request adds one MachineConfig ServiceAccount allowlist entry, explanatory comments, and authorization tests. It adds no logging calls or log fields, and no passwords, tokens, API keys, PII, session IDs, internal hostnames, or customer data to logs. Existing logs that include admission requests are unchanged; the new authorized request path returns without logging. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/retest |
|
@xiaoyu74: all tests passed! 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 |
1 similar comment
|
/retest |
|
/override "Konflux kflux-prd-rh03 / managed-cluster-validating-webhooks-pko-on-pull-request" |
|
@dustman9000: Overrode contexts on behalf of dustman9000: Konflux kflux-prd-rh03 / managed-cluster-validating-webhooks-pko-on-pull-request 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. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dustman9000, xiaoyu74 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 |
|
/override "Konflux kflux-prd-rh03 / managed-cluster-validating-webhooks-pko-on-pull-request" |
|
@samanthajayasinghe: /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. |
|
/override "Konflux kflux-prd-rh03 / managed-cluster-validating-webhooks-pko-on-pull-request" |
|
@samanthajayasinghe: /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. |
|
/override "Konflux kflux-prd-rh03 / managed-cluster-validating-webhooks-pko-on-pull-request" |
|
@xiaoyu74: /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. |
What
Allowlist the OSC controller identity below in the MCVW MachineConfig authorization.
Why
When reconciling a KataConfig, the OpenShift Sandboxed Containers operator needs to create and modify MachineConfig resources.
The OSC controller currently runs as the
defaultServiceAccount in the fixedopenshift-sandboxed-containers-operatornamespace. Since this identity was not included in MCVW's trusted MachineConfig users, the requests were rejected byregular-user-validation.managed.openshift.io.The exception matches the exact Kubernetes identity (
namespace + ServiceAccount), so it does not allowdefaultServiceAccounts from other namespaces or other ServiceAccounts in the OSC namespace.OSD-GCP is the reported scenario for this issue. The MachineConfig authorization path is shared across applicable Classic managed clusters, with no provider-specific logic introduced by this change.
Local E2E Validation
Test env with a staging OSD GCP WIF cluster
Note:
SyncSet was paused while testing the custom MCVW image.ClusterRole/ClusterRoleBindinggranting the minimum required MachineConfig permissions.OSC controllerwithout installing the full OSC stack, I manually created the namespaceopenshift-sandboxed-containers-operatorand temporaryose-cliworkloadBefore - Using existing MCVW image
Note: With the existing MCVW image, the OSC operator's default ServiceAccount in
openshift-sandboxed-containers-operatorwas denied from modifying MachineConfig by MCVWAfter - Build a new image with this PR
MachineConfigPatchNote: PR-built image allowed the same server-side dry-run request, confirming the intended MCVW authorization change
For reviewers