Skip to content

TRT-2412: Revert #6745 "OCPBUGS-59649, CNTRLPLANE-1398: feat(globalps): security enhancements on GlobalPullSecret feature" - #7237

Closed
jianlinliu wants to merge 1 commit into
openshift:mainfrom
jianlinliu:revert-6745-1763116459932
Closed

TRT-2412: Revert #6745 "OCPBUGS-59649, CNTRLPLANE-1398: feat(globalps): security enhancements on GlobalPullSecret feature"#7237
jianlinliu wants to merge 1 commit into
openshift:mainfrom
jianlinliu:revert-6745-1763116459932

Conversation

@jianlinliu

@jianlinliu jianlinliu commented Nov 14, 2025

Copy link
Copy Markdown

Reverts #6745 ; tracked by TRT-2412

Per OpenShift policy, we are reverting this breaking change to get CI and/or nightly payloads flowing again.

This PR broke 4.21.0-0.nightly-2025-11-14-050926

To unrevert this, revert this PR, and layer an additional separate commit on top that addresses the problem. Before merging the unrevert, please run these jobs on the PR and check the result of these jobs to confirm the fix has corrected the problem:

Verification steps TBD

CC: @jparrill

PR created by Revertomatic™️

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Nov 14, 2025
@openshift-ci-robot

openshift-ci-robot commented Nov 14, 2025

Copy link
Copy Markdown

@jianlinliu: This pull request references TRT-2412 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 bug to target the "4.21.0" version, but no target version was set.

Details

In response to this:

Reverts #6745 ; tracked by TRT-2412

Per OpenShift policy, we are reverting this breaking change to get CI and/or nightly payloads flowing again.

This PR broke 4.21.0-0.nightly-2025-11-14-050926

To unrevert this, revert this PR, and layer an additional separate commit on top that addresses the problem. Before merging the unrevert, please run these jobs on the PR and check the result of these jobs to confirm the fix has corrected the problem:

Verification steps TBD

CC: @jparrill

PR created by Revertomatic™️

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

coderabbitai Bot commented Nov 14, 2025

Copy link
Copy Markdown
Contributor

Warning

Rate limit exceeded

@openshift-ci[bot] has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 24 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 9583569 and 4513b2e.

📒 Files selected for processing (14)
  • control-plane-operator/hostedclusterconfigoperator/controllers/globalps/globalps.go (9 hunks)
  • control-plane-operator/hostedclusterconfigoperator/controllers/globalps/globalps_test.go (1 hunks)
  • control-plane-operator/hostedclusterconfigoperator/controllers/globalps/setup.go (3 hunks)
  • control-plane-operator/hostedclusterconfigoperator/controllers/resources/manifests/pullsecret.go (2 hunks)
  • docs/content/how-to/common/global-pull-secret.md (4 hunks)
  • support/util/util.go (0 hunks)
  • support/util/util_test.go (0 hunks)
  • sync-global-pullsecret/sync-global-pullsecret.go (4 hunks)
  • sync-global-pullsecret/sync-global-pullsecret_test.go (6 hunks)
  • test/e2e/autoscaling_test.go (4 hunks)
  • test/e2e/create_cluster_test.go (1 hunks)
  • test/e2e/util/globalps.go (6 hunks)
  • test/e2e/util/hypershift_framework.go (0 hunks)
  • test/e2e/util/util.go (3 hunks)

Walkthrough

Replace the original sync tool with a controller-runtime reconciler, add RBAC-driven global pull-secret synchronization and a simplified DaemonSet, remove node-cache/node-count and node-labeling flows, and update tests, e2e utilities, manifests, and docs to reflect the new behavior.

Changes

Cohort / File(s) Summary
GlobalPullSecret Core Controller Logic
control-plane-operator/hostedclusterconfigoperator/controllers/globalps/globalps.go
Added reconcileGlobalPullSecretRBAC and RBAC imports; manage ServiceAccount/Role/RoleBinding for kube-system and openshift-config; removed node-labeling, original-pull-secret persistence and related helpers; simplified reconcileDaemonSet signature and DaemonSet spec/volumes; adjusted merge semantics to overwrite.
Controller Setup / Caches
control-plane-operator/hostedclusterconfigoperator/controllers/globalps/setup.go
Removed node cache, node client/informer/watcher and node-related setup; retained kube-system cache/informer and simplified reconciler initialization.
Manifest Generators / RBAC Manifests
control-plane-operator/hostedclusterconfigoperator/controllers/resources/manifests/pullsecret.go
Renamed OriginalPullSecret()GlobalPullSecret() (name/type change); added GlobalPullSecretSyncerServiceAccount(), GlobalPullSecretSyncerRole(ns string), and GlobalPullSecretSyncerRoleBinding(ns string) using rbacv1.
GlobalPullSecret Tests
control-plane-operator/hostedclusterconfigoperator/controllers/globalps/globalps_test.go
Simplified merge tests to verify overwrite behavior; removed node-labeling tests and other legacy scenarios.
Sync Tool → controller-runtime Reconciler
sync-global-pullsecret/sync-global-pullsecret.go
Replaced ticker/signal loop with a GlobalPullSecretReconciler and manager; added Reconcile, checkAndFixFile, DBus/restart helpers, isTargetSecret, writeFileFunc test hook, and new package-level constants/flags.
Sync Tool Tests
sync-global-pullsecret/sync-global-pullsecret_test.go
Reworked tests to use GlobalPullSecretReconciler and fake controller-runtime clients; added TestIsTargetSecret; updated signatures to accept context.Context; removed many legacy permutation tests.
Support Utilities Removed
support/util/util.go, support/util/util_test.go
Removed exported CountAvailableNodes and its tests.
E2E Test Utilities & Helpers
test/e2e/util/globalps.go, test/e2e/util/util.go
Removed KubeletConfigVerifierDaemonSet and DaemonSetManifest; added WaitForKubeletConfigVerifierDaemonSet and GetKubeletConfigVerifierLogs; updated DaemonSet spec (ServiceAccountName, automount token, resource requests); EnsureGlobalPullSecret now returns an error and flows updated to handle errors.
E2E Tests & Framework Changes
test/e2e/create_cluster_test.go, test/e2e/util/hypershift_framework.go, test/e2e/autoscaling_test.go
Added EnsureGlobalPullSecret call in TestCreateCluster; removed EnsureGlobalPullSecret invocation from framework teardown; adjusted autoscaling test parameters and expectations (smaller totals, dynamic node expectations, per-pool replica checks).
Documentation
docs/content/how-to/common/global-pull-secret.md
Simplified content: removed NodePool/InPlace specifics and many conflict scenarios; updated examples and stated new merge semantics (additional secret precedence); condensed RBAC/cleanup notes.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Areas requiring extra attention:

  • RBAC creation and cross-namespace Role/RoleBinding logic in globalps.go and manifest helpers.
  • reconcileDaemonSet PodSpec changes: hostPath mounts, volumes, service account, automount token, and resource adjustments.
  • sync-global-pullsecret reconciler: controller-runtime wiring, Reconcile/checkAndFixFile, DBus/restart logic, and test hook behavior.
  • Call-site updates for renamed/removed manifest functions (OriginalPullSecretGlobalPullSecret, removed helpers).
  • E2E changes: EnsureGlobalPullSecret signature update and altered readiness/logging helpers; validate test stability and expected behavior changes.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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

@openshift-ci

openshift-ci Bot commented Nov 14, 2025

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jianlinliu
Once this PR has been reviewed and has the lgtm label, please assign muraee for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

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

@openshift-ci openshift-ci Bot added area/control-plane-operator Indicates the PR includes changes for the control plane operator - in an OCP release area/documentation Indicates the PR includes changes for documentation area/hypershift-operator Indicates the PR includes changes for the hypershift operator and API - outside an OCP release area/testing Indicates the PR includes changes for e2e testing and removed do-not-merge/needs-area labels Nov 14, 2025
@jianlinliu

Copy link
Copy Markdown
Author

/close

@openshift-ci openshift-ci Bot closed this Nov 14, 2025
@openshift-ci

openshift-ci Bot commented Nov 14, 2025

Copy link
Copy Markdown
Contributor

@jianlinliu: Closed this PR.

Details

In response to this:

/close

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.

@jianlinliu

Copy link
Copy Markdown
Author

This issue is being fixed by #7236, no need a revert.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/control-plane-operator Indicates the PR includes changes for the control plane operator - in an OCP release area/documentation Indicates the PR includes changes for documentation area/hypershift-operator Indicates the PR includes changes for the hypershift operator and API - outside an OCP release area/testing Indicates the PR includes changes for e2e testing jira/valid-reference Indicates that this PR references a valid Jira ticket of any type.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants