Skip to content

[ROSAENG-61186] - fix: Allow OSC operator to manage MachineConfigs - #621

Merged
dustman9000 merged 1 commit into
openshift:masterfrom
xiaoyu74:rosaeng-61186-allow-osc-machineconfig
Sep 1, 2026
Merged

[ROSAENG-61186] - fix: Allow OSC operator to manage MachineConfigs#621
dustman9000 merged 1 commit into
openshift:masterfrom
xiaoyu74:rosaeng-61186-allow-osc-machineconfig

Conversation

@xiaoyu74

@xiaoyu74 xiaoyu74 commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

What

Allowlist the OSC controller identity below in the MCVW MachineConfig authorization.

system:serviceaccount:openshift-sandboxed-containers-operator:default

Why

  • When reconciling a KataConfig, the OpenShift Sandboxed Containers operator needs to create and modify MachineConfig resources.

  • The OSC controller currently runs as the default ServiceAccount in the fixed openshift-sandboxed-containers-operator namespace. Since this identity was not included in MCVW's trusted MachineConfig users, the requests were rejected by regular-user-validation.managed.openshift.io.

  • The exception matches the exact Kubernetes identity (namespace + ServiceAccount), so it does not allow default ServiceAccounts 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

Cluster: sa-gcpw/2ri1e9iikta9fap8lb3vqftcffl4ipup
Version: 4.22.3
Provider: GCP WIF
Test workload: registry.redhat.io/openshift4/ose-cli-rhel9:v4.22

Note:

  • Hive SyncSet was paused while testing the custom MCVW image.
  • Manually created temporary ClusterRole/ClusterRoleBinding granting the minimum required MachineConfig permissions.
  • To simulate the same identity used by the OSC controller without installing the full OSC stack, I manually created the namespace openshift-sandboxed-containers-operator and temporary ose-cli workload

Before - Using existing MCVW image

oc get pod -n openshift-sandboxed-containers-operator

NAME                       READY   STATUS    RESTARTS   AGE
rosaeng-61186-validation   1/1     Running   0          12s

bp-elevate exec \
  -n openshift-sandboxed-containers-operator \
  rosaeng-61186-validation \
  -- oc patch machineconfig "$TEST_MC" \
  --type=merge \
  --patch='{"metadata":{"annotations":{"rosaeng-61186-validation":"before"}}}' \
  --dry-run=server
Error from server (Forbidden): admission webhook "regular-user-validation.managed.openshift.io" denied the request: Prevented from accessing Red Hat managed resources. This is in an effort to prevent harmful actions that may cause unintended consequences or affect the stability of the cluster. If you have any questions about this, please reach out to Red Hat support at https://access.redhat.com/support
command terminated with exit code 1
ERRO[0004] exit status 1

Note: With the existing MCVW image, the OSC operator's default ServiceAccount in openshift-sandboxed-containers-operator was denied from modifying MachineConfig by MCVW

After - Build a new image with this PR

  • Deployed the new built MCVW image
oc get pods \
  -n openshift-validation-webhook \
  -o jsonpath='{range .items[*]}{.metadata.name}{"  "}{.spec.containers[0].image}{"\n"}{end}'

validation-webhook-2jcg4  quay.io/sbai_openshift/managed-cluster-validating-webhooks:rosaeng-61186-8e5df14
  • dry-run the MachineConfig Patch
bp-elevate exec \
  -n openshift-sandboxed-containers-operator \
  rosaeng-61186-validation \
  -- /bin/bash -c "
    SERVER=\"https://\${KUBERNETES_SERVICE_HOST}:\${KUBERNETES_SERVICE_PORT_HTTPS}\"
    TOKEN=\"\$(cat /var/run/secrets/kubernetes.io/serviceaccount/token)\"
    CA=\"/var/run/secrets/kubernetes.io/serviceaccount/ca.crt\"

    oc \
      --server=\"\${SERVER}\" \
      --token=\"\${TOKEN}\" \
      --certificate-authority=\"\${CA}\" \
      patch machineconfig '${TEST_MC}' \
      --type=merge \
      --patch='{\"metadata\":{\"annotations\":{\"rosaeng-61186-validation\":\"after\"}}}' \
      --dry-run=server
  "
machineconfig.machineconfiguration.openshift.io/00-master patched

Note: PR-built image allowed the same server-side dry-run request, confirming the intended MCVW authorization change

For reviewers

@coderabbitai

coderabbitai Bot commented Aug 31, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: a57f3696-3d98-44db-928d-efffc7a0690b

📥 Commits

Reviewing files that changed from the base of the PR and between bb0c8d2 and 8e5df14.

📒 Files selected for processing (2)
  • pkg/webhooks/regularuser/common/regularuser.go
  • pkg/webhooks/regularuser/common/regularuser_test.go

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


Walkthrough

The 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.

Changes

OSC MachineConfig authorization

Layer / File(s) Summary
Allowlist entry and authorization tests
pkg/webhooks/regularuser/common/regularuser.go, pkg/webhooks/regularuser/common/regularuser_test.go
The allowlist permits system:serviceaccount:openshift-sandboxed-containers-operator:default. Tests verify the exact identity is allowed and similar identities remain denied.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Merge Risk: ⚪ Minimal · up to 8e5df

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)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names ✅ Passed The changed tests use three fixed string identifiers: machineconfig-osc-serviceaccount, machineconfig-osc-serviceaccount-wrong-namespace, and machineconfig-osc-serviceaccount-wrong-sa. The file …
Test Structure And Quality ✅ Passed PASS: The changed tests use the repository's existing table-driven testing.T pattern, not Ginkgo It blocks. Each added case covers one identity/authorization behavior. The tests send in-memory web…
Microshift Test Compatibility ✅ Passed PASS — The pull request adds standard Go unit-test cases, not new Ginkgo e2e tests. regularuser_test.go imports testing and uses runRegularuserTests with locally constructed admission requests. …
Single Node Openshift (Sno) Test Compatibility ✅ Passed The added tests are table-driven Go unit tests under TestMachineConfig using testing.T, not Ginkgo e2e tests. They only construct admission requests with ServiceAccount identities and assert autho…
Topology-Aware Scheduling Compatibility ✅ Passed 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, rep…
Ote Binary Stdout Contract ✅ Passed PASS: The pull request changes only the machineConfigUsers slice and adds data-driven cases inside TestMachineConfig. The new production initializer contains only a string and comments. The new te…
Ipv6 And Disconnected Network Test Compatibility ✅ Passed The added tests are standard Go testing table cases in TestMachineConfig, not new Ginkgo e2e tests. They only construct local admission requests and assert authorization results. The added code co…
No-Weak-Crypto ✅ Passed 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 c…
Container-Privileges ✅ Passed 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 Kubern…
No-Sensitive-Data-In-Logs ✅ Passed 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, …
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: allowing the OpenShift Sandboxed Containers operator to manage MachineConfig resources.
Full details: Stable And Deterministic Test Names

Explanation

The changed tests use three fixed string identifiers: machineconfig-osc-serviceaccount, machineconfig-osc-serviceaccount-wrong-namespace, and machineconfig-osc-serviceaccount-wrong-sa. The file uses Go testing, not Ginkgo, and runRegularuserTests does not create dynamic subtest names. No changed test title contains a generated value, timestamp, node name, random namespace, IP address, or other run-dependent data.

Full details: Test Structure And Quality

Explanation

PASS: The changed tests use the repository's existing table-driven testing.T pattern, not Ginkgo It blocks. Each added case covers one identity/authorization behavior. The tests send in-memory webhook requests through testutils and create no cluster resources, so BeforeEach/AfterEach and cluster timeouts do not apply. Shared t.Fatalf messages include the test ID and request/response details, which provides useful failure context. The pattern matches nearby webhook tests.

Full details: Microshift Test Compatibility

Explanation

PASS — The pull request adds standard Go unit-test cases, not new Ginkgo e2e tests. regularuser_test.go imports testing and uses runRegularuserTests with locally constructed admission requests. It has no Ginkgo constructs, MicroShift checks, cluster clients, or live API calls. Therefore the MicroShift Test Compatibility check does not apply.

Full details: Single Node Openshift (Sno) Test Compatibility

Explanation

The added tests are table-driven Go unit tests under TestMachineConfig using testing.T, not Ginkgo e2e tests. They only construct admission requests with ServiceAccount identities and assert authorization results. They do not use nodes, scheduling, replicas, topology, failover, draining, or other multi-node behavior. The SNO compatibility check is therefore not applicable.

Full details: Topology-Aware Scheduling Compatibility

Explanation

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 Contract

Explanation

PASS: The pull request changes only the machineConfigUsers slice and adds data-driven cases inside TestMachineConfig. The new production initializer contains only a string and comments. The new test cases contain no stdout writes. The diff adds no fmt.Print*, klog, RunSpecs setup, BeforeSuite, AfterSuite, SynchronizedBeforeSuite, TestMain, or init() output code.

Full details: Ipv6 And Disconnected Network Test Compatibility

Explanation

The added tests are standard Go testing table cases in TestMachineConfig, not new Ginkgo e2e tests. They only construct local admission requests and assert authorization results. The added code contains no IPv4 assumptions or external connectivity requirements. The existing Kubernetes documentation URL is only a comment and is not accessed by the test.

Full details: No-Weak-Crypto

Explanation

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 slices.Contains.

Full details: Container-Privileges

Explanation

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 privileged: true, hostPID, hostNetwork, hostIPC, SYS_ADMIN, or allowPrivilegeEscalation: true. Therefore, the custom check has no applicable failure condition.

Full details: No-Sensitive-Data-In-Logs

Explanation

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)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@xiaoyu74 xiaoyu74 changed the title fix: ROSAENG-61186 Allow OSC operator to manage MachineConfigs fix: [ROSAENG-61186] Allow OSC operator to manage MachineConfigs Aug 31, 2026
@xiaoyu74 xiaoyu74 changed the title fix: [ROSAENG-61186] Allow OSC operator to manage MachineConfigs [ROSAENG-61186] - fix: Allow OSC operator to manage MachineConfigs Aug 31, 2026
@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 31, 2026
@xiaoyu74

Copy link
Copy Markdown
Contributor Author

/retest

@openshift-ci

openshift-ci Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

@xiaoyu74: all tests passed!

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

@xiaoyu74

Copy link
Copy Markdown
Contributor Author

/retest

1 similar comment
@jsztuka

jsztuka commented Aug 31, 2026

Copy link
Copy Markdown

/retest

@dustman9000

Copy link
Copy Markdown
Member

/override "Konflux kflux-prd-rh03 / managed-cluster-validating-webhooks-pko-on-pull-request"

@openshift-ci

openshift-ci Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

@dustman9000: Overrode contexts on behalf of dustman9000: Konflux kflux-prd-rh03 / managed-cluster-validating-webhooks-pko-on-pull-request

Details

In response to this:

/override "Konflux kflux-prd-rh03 / managed-cluster-validating-webhooks-pko-on-pull-request"

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.

@dustman9000

Copy link
Copy Markdown
Member

/lgtm

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Aug 31, 2026
@openshift-ci

openshift-ci Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

[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

Details Needs approval from an approver in each of these files:
  • OWNERS [dustman9000,xiaoyu74]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@samanthajayasinghe

Copy link
Copy Markdown
Contributor

/override "Konflux kflux-prd-rh03 / managed-cluster-validating-webhooks-pko-on-pull-request"

@openshift-ci

openshift-ci Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

@samanthajayasinghe: /override requires failed status contexts, check run or a prowjob name to operate on.
The following unknown contexts/checkruns were given:

  • Konflux kflux-prd-rh03 / managed-cluster-validating-webhooks-pko-on-pull-request

Only the following failed contexts/checkruns were expected:

  • CodeRabbit
  • Konflux kflux-prd-rh03 / managed-cluster-validating-webhooks-on-pull-request
  • Red Hat Konflux / managed-cluster-validating-webhooks-enterprise-contract / managed-cluster-validating-webhooks-pko
  • Red Hat Konflux / managed-cluster-validating-webhooks-enterprise-contract / pr group
  • auto-merge
  • ci/prow/e2e-binary-build-success
  • ci/prow/pr-check
  • pull-ci-openshift-managed-cluster-validating-webhooks-master-e2e-binary-build-success
  • pull-ci-openshift-managed-cluster-validating-webhooks-master-pr-check
  • tide

If you are trying to override a checkrun that has a space in it, you must put a double quote on the context.

Details

In response to this:

/override "Konflux kflux-prd-rh03 / managed-cluster-validating-webhooks-pko-on-pull-request"

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.

@samanthajayasinghe

Copy link
Copy Markdown
Contributor

/override "Konflux kflux-prd-rh03 / managed-cluster-validating-webhooks-pko-on-pull-request"

@openshift-ci

openshift-ci Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

@samanthajayasinghe: /override requires failed status contexts, check run or a prowjob name to operate on.
The following unknown contexts/checkruns were given:

  • Konflux kflux-prd-rh03 / managed-cluster-validating-webhooks-pko-on-pull-request

Only the following failed contexts/checkruns were expected:

  • CodeRabbit
  • Konflux kflux-prd-rh03 / managed-cluster-validating-webhooks-on-pull-request
  • Red Hat Konflux / managed-cluster-validating-webhooks-enterprise-contract / managed-cluster-validating-webhooks-pko
  • Red Hat Konflux / managed-cluster-validating-webhooks-enterprise-contract / pr group
  • auto-merge
  • ci/prow/e2e-binary-build-success
  • ci/prow/pr-check
  • pull-ci-openshift-managed-cluster-validating-webhooks-master-e2e-binary-build-success
  • pull-ci-openshift-managed-cluster-validating-webhooks-master-pr-check
  • tide

If you are trying to override a checkrun that has a space in it, you must put a double quote on the context.

Details

In response to this:

/override "Konflux kflux-prd-rh03 / managed-cluster-validating-webhooks-pko-on-pull-request"

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.

@xiaoyu74

xiaoyu74 commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

/override "Konflux kflux-prd-rh03 / managed-cluster-validating-webhooks-pko-on-pull-request"

@openshift-ci

openshift-ci Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

@xiaoyu74: /override requires failed status contexts, check run or a prowjob name to operate on.
The following unknown contexts/checkruns were given:

  • Konflux kflux-prd-rh03 / managed-cluster-validating-webhooks-pko-on-pull-request

Only the following failed contexts/checkruns were expected:

  • CodeRabbit
  • Konflux kflux-prd-rh03 / managed-cluster-validating-webhooks-on-pull-request
  • Red Hat Konflux / managed-cluster-validating-webhooks-enterprise-contract / managed-cluster-validating-webhooks-pko
  • Red Hat Konflux / managed-cluster-validating-webhooks-enterprise-contract / pr group
  • auto-merge
  • ci/prow/e2e-binary-build-success
  • ci/prow/pr-check
  • pull-ci-openshift-managed-cluster-validating-webhooks-master-e2e-binary-build-success
  • pull-ci-openshift-managed-cluster-validating-webhooks-master-pr-check
  • tide

If you are trying to override a checkrun that has a space in it, you must put a double quote on the context.

Details

In response to this:

/override "Konflux kflux-prd-rh03 / managed-cluster-validating-webhooks-pko-on-pull-request"

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.

@dustman9000
dustman9000 merged commit 584e095 into openshift:master Sep 1, 2026
9 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants