OCPBUGS-59649, CNTRLPLANE-1398: feat(globalps): security enhancements on GlobalPullSecret feature by jparrill · Pull Request #6745 · openshift/hypershift

jparrill · 2025-09-03T08:03:32Z

What this PR does / why we need it

This PR includes several enhancements to the GlobalPullSecret feature:

Security:
- Mount pull secrets as volumes: DaemonSet reads secrets from mounted files instead of API calls
- Privileged mode: Maintained the Privileged mode on the DS, this does not work only with some capabilities enabled. Also documented appropriately.
- Mount pull secrets as volumes: DaemonSet reads secrets from mounted files instead of API calls
Performance:
- Configuration hash tracking: Add configuration hash to DaemonSet labels to trigger pod recreation
  on content changes
- Reduced API footprint: Removed controller from sync-global-pullsecret process and eliminated API
  calls in DaemonSet
Precedence Logic:
- Conflict resolution: GlobalPullSecret always preserves original pull secret entries in case of
  registry conflicts
- Clear documentation: Updated documentation to clearly explain precedence behavior with examples
Functional:
- Opt-in deployment strategy: DaemonSet node selection only on appropriate labeled nodes.

E2E adjustments

Fix node capacity and balancing issues
- Increase MaxFreeDifferenceRatioPercent to 70% for more permissive balancing
- Set m5.xlarge instance types to ensure adequate memory capacity
- Increase MaxNodesTotal from 4 to 6 nodes for larger scale testing
- TestAutoscalingBalancing expects at least 2+4, 3+3 or 4+2 nodes among the nodes

Added commit from #6736 to allow E2E testing to work as expected

Which issue(s) this PR fixes

openshift-ci · 2025-09-03T08:03:36Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci-robot · 2025-09-03T08:03:39Z

@jparrill: This pull request references Jira Issue OCPBUGS-59649, which is invalid:

expected the bug to target the "4.21.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In 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 · 2025-09-03T08:03:40Z

Caution

Review failed

The head commit changed during the review from 2bef710 to 722b0ad.

Walkthrough

Expand platform gating to Azure and AWS; refactor global pull-secret handling to maintain an "original" secret and optional merged "global" secret, remove RBAC, add config-seed labeling on the DaemonSet, fetch HostedControlPlane by name, and replace the controller-runtime syncer with a periodic file-driven sync loop. (50 words)

Changes

Cohort / File(s)	Summary
Platform gating `control-plane-operator/hostedclusterconfigoperator/cmd.go`	Register globalps controller when platform is Azure or AWS (was Azure-only).
GlobalPS controller core `control-plane-operator/hostedclusterconfigoperator/controllers/globalps/globalps.go`, `.../setup.go`, `.../globalps_test.go`	Add `hcpName` to Reconciler; fetch HostedControlPlane by namespace+name; always create an OriginalPullSecret in DataPlane `kube-system`; if user secret exists merge with original (original wins on conflicts); compute `configSeed` (hash) and pass original/global secret names + configSeed into DaemonSet reconciliation; update tests for original-precedence semantics.
Manifests / RBAC removal `control-plane-operator/hostedclusterconfigoperator/controllers/resources/manifests/pullsecret.go`	Remove RBAC Role/RoleBinding and ServiceAccount constructs; introduce `OriginalPullSecret()` and a Secret-producing `GlobalPullSecret()`; adjust secret names/types and drop RBAC helper functions.
DaemonSet / Pod wiring `control-plane-operator/hostedclusterconfigoperator/controllers/globalps/globalps.go` (reconcileDaemonSet)	Add Pod label `hypershift.openshift.io/globalps-config-hash` (configSeed); require mount of `original-pull-secret`, optionally mount `global-pull-secret`; remove ServiceAccount usage and set `automountServiceAccountToken=false`; simplify pod spec and adjust container args/volumes.
Sync tool refactor `sync-global-pullsecret/sync-global-pullsecret.go`, `sync-global-pullsecret/sync-global-pullsecret_test.go`	Replace controller-runtime reconciler with a periodic, signal-aware file-based syncer (`GlobalPullSecretSyncer`): structured logging, testable file IO hooks, read mounted original/global secret files, validate/merge as needed, write kubelet `config.json` atomically, restart kubelet via DBus with retries; update tests and add validateDockerConfigJSON suite.
E2E util adjustments `test/e2e/util/util.go`, `test/e2e/util/globalps.go`	Allow gating for Azure or AWS; remove DataPlane RBAC verification; update dummy secret data to include quay.io/hypershift; relax pod-failure handling for negative tests; simplify DaemonSet PodSpec to match controller changes.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Pre-merge checks and finishing touches and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 37.50% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The title explicitly mentions the introduction of security enhancements for the GlobalPullSecret feature and references the relevant JIRA issues, which aligns with the PR’s main changes such as mounting pull secrets as volumes, removing API calls, and hardening the DaemonSet.
Description Check	✅ Passed	The pull request description provides a clear summary of the security, performance, and precedence improvements to the GlobalPullSecret feature and cites the associated issues, which directly corresponds to the changes made in the code.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

jparrill · 2025-09-10T08:20:12Z

/test unit

jparrill · 2025-09-10T08:20:17Z

/test verify

jparrill · 2025-09-10T08:20:27Z

/test e2e-aws

jparrill · 2025-09-10T08:20:31Z

/test e2e-aks

cwbotbot · 2025-09-10T10:00:07Z

Test Results

e2e-aws

Status: ❌ FAIL
Started: 2025-09-10T08:20:50Z
View Job
View Job History

Failed Tests

Total failed tests: 21

TestAutoscaling
TestAutoscaling/ValidateHostedCluster
TestCreateCluster
TestCreateCluster/ValidateHostedCluster
TestCreateClusterCustomConfig

... and 16 more failed tests

e2e-aks

Status: ❌ FAIL
Started: 2025-09-10T08:20:53Z
View Job
View Job History

openshift-ci-robot · 2025-09-11T10:42:54Z

@jparrill: This pull request references Jira Issue OCPBUGS-59649, which is invalid:

expected the bug to target the "4.21.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

This pull request references CNTRLPLANE-1398 which is a valid jira issue.

Details

In 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-robot · 2025-11-11T00:40:39Z

/retest-required

Remaining retests: 0 against base HEAD 426c1fc and 2 for PR HEAD 143b261 in total

jparrill · 2025-11-11T08:00:36Z

/test e2e-aws

jparrill · 2025-11-11T11:55:43Z

Hey @gaol The only change included is the size of the nodes in the E2E + the client used to list the nodes in the GlobalPS controller. I've verified the right functioning already, but if you want to take a look again is fine.

openshift-ci-robot · 2025-11-11T16:35:08Z

@jparrill: This pull request references Jira Issue OCPBUGS-59649, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.21.0) matches configured target version for branch (4.21.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

No GitHub users were found matching the public email listed for the QA contact in Jira (lgao@redhat.com), skipping review request.

This pull request references CNTRLPLANE-1398 which is a valid jira issue.

Details

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

jparrill · 2025-11-11T17:03:07Z

/test e2e-aks

csrwng · 2025-11-11T22:21:28Z

/lgtm

openshift-ci-robot · 2025-11-11T22:23:43Z

@jparrill: This pull request references Jira Issue OCPBUGS-59649, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.21.0) matches configured target version for branch (4.21.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

No GitHub users were found matching the public email listed for the QA contact in Jira (lgao@redhat.com), skipping review request.

This pull request references CNTRLPLANE-1398 which is a valid jira issue.

Details

In 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-robot · 2025-11-11T22:24:55Z

@jparrill: This pull request references Jira Issue OCPBUGS-59649, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.21.0) matches configured target version for branch (4.21.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

No GitHub users were found matching the public email listed for the QA contact in Jira (lgao@redhat.com), skipping review request.

This pull request references CNTRLPLANE-1398 which is a valid jira issue.

Details

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

gaol · 2025-11-11T23:51:12Z

 type Reconciler struct {
 	cpClient               crclient.Client
 	kubeSystemSecretClient crclient.Client
+	nodeClient             crclient.Client


@jparrill I actually don't get it where this nodeClient is used ?

Sometimes Claude trolls me a lot..., thanks for the heads up, updating the PR

jparrill · 2025-11-12T06:16:48Z

/retest-required

…GlobalPullSecret - Add logic to preserve trailing newlines when updating kubelet config.json - Refactor EnsureGlobalPullSecret E2E test to run as proper subtest - Add validation for NodePool upgrade type compatibility This fix ensures kubelet config files maintain their original formatting when updated by the global pull secret syncer, preventing potential configuration inconsistencies. Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>

jparrill · 2025-11-12T09:19:53Z

Last changes:

Use nodeClient for node list
Fix test
Adjust the AutoscaleBalancing percentage to 70% (current percentage on 65-67% in latests E2E runs)

openshift-ci-robot · 2025-11-12T09:20:25Z

@jparrill: This pull request references Jira Issue OCPBUGS-59649, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.21.0) matches configured target version for branch (4.21.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

No GitHub users were found matching the public email listed for the QA contact in Jira (lgao@redhat.com), skipping review request.

This pull request references CNTRLPLANE-1398 which is a valid jira issue.

Details

In 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-robot · 2025-11-12T09:21:01Z

@jparrill: This pull request references Jira Issue OCPBUGS-59649, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.21.0) matches configured target version for branch (4.21.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

No GitHub users were found matching the public email listed for the QA contact in Jira (lgao@redhat.com), skipping review request.

This pull request references CNTRLPLANE-1398 which is a valid jira issue.

Details

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

jparrill · 2025-11-12T11:47:52Z

/retest-required

…est scale Improves autoscaling test robustness and scale to better validate behavior: Configuration changes: - Use RandomExpander instead of LeastWasteExpander for better distribution probability - Increase MaxFreeDifferenceRatioPercent to 70% for more permissive balancing - Set m5.xlarge instance types to ensure adequate memory capacity - Increase MaxNodesTotal from 4 to 6 nodes for larger scale testing Test validation improvements: - Increase workload from 4 to 6 jobs to match node scaling - Relax balancing check to accept 2+4, 3+3, 4+2 distributions (≥2 nodes per NodePool) - Reject extreme imbalances (≤1 nodes in any NodePool) - Update comments and log messages to reflect new expectations The cluster-autoscaler behavior is correct - it doesn't guarantee perfect balance, only reasonable distribution within the configured threshold. Test expectations now align with actual autoscaler behavior based on cluster state analysis. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>

openshift-ci-robot · 2025-11-12T14:30:36Z

@jparrill: This pull request references Jira Issue OCPBUGS-59649, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.21.0) matches configured target version for branch (4.21.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

No GitHub users were found matching the public email listed for the QA contact in Jira (lgao@redhat.com), skipping review request.

This pull request references CNTRLPLANE-1398 which is a valid jira issue.

Details

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

jparrill · 2025-11-12T18:35:06Z

/retest-required

jparrill · 2025-11-12T21:06:58Z

/test e2e-aks
non related test failed (NTO)

openshift-ci · 2025-11-13T00:11:28Z

@jparrill: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/okd-scos-e2e-aws-ovn	`6d6a827`	link	false	`/test okd-scos-e2e-aws-ovn`

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.

gaol · 2025-11-13T01:29:42Z

/verified by @gaol

openshift-ci-robot · 2025-11-13T01:29:53Z

@gaol: This PR has been marked as verified by @gaol.

Details

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

csrwng · 2025-11-13T14:42:02Z

/lgtm

openshift-ci-robot · 2025-11-13T18:22:24Z

@jparrill: Jira Issue Verification Checks: Jira Issue OCPBUGS-59649
✔️ This pull request was pre-merge verified.
✔️ All associated pull requests have merged.
✔️ All associated, merged pull requests were pre-merge verified.

Jira Issue OCPBUGS-59649 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓

Details

In 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-merge-robot · 2025-11-20T13:50:01Z

Fix included in accepted release 4.21.0-0.nightly-2025-11-15-144034

openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 3, 2025

openshift-ci-robot added jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Sep 3, 2025

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 9, 2025

jparrill force-pushed the OCPBUGS-59649 branch from 1106e90 to 2257739 Compare September 9, 2025 19:33

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 9, 2025

jparrill force-pushed the OCPBUGS-59649 branch 2 times, most recently from 68d44f7 to 9f5a84e Compare September 9, 2025 22:49

jparrill force-pushed the OCPBUGS-59649 branch from 9f5a84e to ed6005c Compare September 10, 2025 10:21

jparrill changed the title ~~OCPBUGS-59649: feat(globalps): security enhancements on GlobalPullSecret feature~~ OCPBUGS-59649, CNTRLPLANE-1398: feat(globalps): security enhancements on GlobalPullSecret feature Sep 11, 2025

gaol reviewed Nov 11, 2025

View reviewed changes

jparrill mentioned this pull request Nov 14, 2025

CNTRLPLANE-1710: feat(globalps): security enhancements on GlobalPullSecret feature #7234

Merged

jianlinliu mentioned this pull request Nov 14, 2025

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

Closed

wewang58 mentioned this pull request Nov 18, 2025

CNTRLPLANE-1137: Add e2e test for ARO HCP with Cilium Network Policies #7077

Closed

4 tasks

gaol mentioned this pull request Dec 9, 2025

CNTRLPLANE-1708: Backport Customer Global Pull Secret to OpenShift 4.18.z #7354

Merged

jhjaggars mentioned this pull request Dec 16, 2025

docs: add comprehensive code review standards from 10 PRs #7397

Closed

4 tasks

Uh oh!

Conversation

jparrill commented Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does / why we need it

E2E adjustments

Which issue(s) this PR fixes

Uh oh!

openshift-ci Bot commented Sep 3, 2025

Uh oh!

openshift-ci-robot commented Sep 3, 2025

What this PR does / why we need it

Which issue(s) this PR fixes

Uh oh!

coderabbitai Bot commented Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review failed

Walkthrough

Changes

Estimated code review effort

Pre-merge checks and finishing touches and finishing touches

Uh oh!

jparrill commented Sep 10, 2025

Uh oh!

jparrill commented Sep 10, 2025

Uh oh!

jparrill commented Sep 10, 2025

Uh oh!

jparrill commented Sep 10, 2025

Uh oh!

cwbotbot commented Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

e2e-aws

e2e-aks

Uh oh!

openshift-ci-robot commented Sep 11, 2025 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does / why we need it

Which issue(s) this PR fixes

Uh oh!

openshift-ci-robot commented Nov 11, 2025

Uh oh!

jparrill commented Nov 11, 2025

Uh oh!

jparrill commented Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Nov 11, 2025 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does / why we need it

E2E adjustments

Which issue(s) this PR fixes

Uh oh!

jparrill commented Nov 11, 2025

Uh oh!

csrwng commented Nov 11, 2025

Uh oh!

openshift-ci-robot commented Nov 11, 2025 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does / why we need it

E2E adjustments

Which issue(s) this PR fixes

Uh oh!

openshift-ci-robot commented Nov 11, 2025 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does / why we need it

E2E adjustments

Which issue(s) this PR fixes

Uh oh!

gaol Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

jparrill Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

jparrill commented Nov 12, 2025

Uh oh!

jparrill commented Nov 12, 2025

Uh oh!

jparrill commented Sep 3, 2025 •

edited

Loading

coderabbitai Bot commented Sep 3, 2025 •

edited

Loading

cwbotbot commented Sep 10, 2025 •

edited

Loading

openshift-ci-robot commented Sep 11, 2025 •

edited by openshift-ci Bot

Loading

jparrill commented Nov 11, 2025 •

edited

Loading

openshift-ci-robot commented Nov 11, 2025 •

edited by openshift-ci Bot

Loading

openshift-ci-robot commented Nov 11, 2025 •

edited by openshift-ci Bot

Loading

openshift-ci-robot commented Nov 11, 2025 •

edited by openshift-ci Bot

Loading

openshift-ci-robot commented Nov 12, 2025 •

edited by openshift-ci Bot

Loading

openshift-ci-robot commented Nov 12, 2025 •

edited by openshift-ci Bot

Loading

openshift-ci-robot commented Nov 12, 2025 •

edited by openshift-ci Bot

Loading