OCPBUGS-85519: Add Agent IRI credentials to global pull secret#6067
OCPBUGS-85519: Add Agent IRI credentials to global pull secret#6067sadasu wants to merge 2 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/severity-important
|
@sadasu: This pull request references Jira Issue OCPBUGS-85519, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughExports IRI credential helpers; InternalReleaseImage controller now syncs IRI credentials into the configured global pull secret and refactors event/filter logic; template controller no longer merges at render time; tests updated to expect creds already present in the global pull secret. ChangesIRI Credentials Migration to InternalReleaseImage Controller
Sequence Diagram(s) sequenceDiagram
participant InternalReleaseImageController
participant KubeAPI
participant GlobalPullSecret
participant IRIAuthSecret
InternalReleaseImageController->>KubeAPI: Get ControllerConfig (resolve pullSecret ref)
InternalReleaseImageController->>KubeAPI: Get GlobalPullSecret
InternalReleaseImageController->>KubeAPI: Get IRIAuthSecret
InternalReleaseImageController->>InternalReleaseImageController: ExtractIRICredentials(IRIAuthSecret, ControllerConfig)
InternalReleaseImageController->>InternalReleaseImageController: MergeIRIRegistryCredentialsIntoPullSecret(GlobalPullSecret.data, password, baseDomain)
InternalReleaseImageController->>KubeAPI: Update GlobalPullSecret (if changed)
🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels: Suggested reviewers:
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sadasu The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
sadasu
force-pushed
the
fix-iri-pull-secret
branch
from
eba73aa to
21b8d9e
Compare
|
@sadasu: This pull request references Jira Issue OCPBUGS-85519, which is valid. 3 validation(s) were run on this bug
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/controller/internalreleaseimage/internalreleaseimage_controller.go`:
- Around line 414-417: The reconcile only calls syncGlobalPullSecret but the
controller doesn't requeue on all resources that can stale the global pull
secret; update the controller's watches (SetupWithManager / Setup) so Reconcile
is triggered when the cluster pull secret (the Kubernetes Secret used as the
global pull secret) is created/updated/deleted, when ControllerConfig changes
(spec.pullSecret and spec.dns.baseDomain), and when any resource that can rotate
credentials (e.g., DockerRegistryKey) changes in addition to the existing
IRI-owned secrets. Concretely: add a watch for the cluster/global pull Secret
(by name/namespace or label) to enqueue affected InternalReleaseImage objects,
add a watch on ControllerConfig and filter to requeue when Spec.PullSecret or
Spec.DNS.BaseDomain changes, and ensure DockerRegistryKey and the two IRI-owned
secret watches remain; keep syncGlobalPullSecret as-is and rely on these
additional watches to ensure it runs whenever the global pull secret can become
stale.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0a3af203-7768-4e8d-b9bc-30a62398657d
📒 Files selected for processing (4)
pkg/controller/common/iri_secret_merger.gopkg/controller/internalreleaseimage/internalreleaseimage_controller.gopkg/controller/template/template_controller.gopkg/controller/template/template_controller_test.go
sadasu
force-pushed
the
fix-iri-pull-secret
branch
from
21b8d9e to
a2a957e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/controller/template/template_controller.go`:
- Around line 584-586: The Secret filter currently hard-codes the name
"pull-secret" so changes to a differently named global pull secret won't
re-enqueue the template controller; update filterSecret to compare the incoming
Secret's name against the configured value from ControllerConfig.Spec.PullSecret
(accessed via the controller's config instance, e.g., c.config.Spec.PullSecret
or controllerConfig variable) and namespace as appropriate, falling back to
"pull-secret" only if Spec.PullSecret is empty; ensure you reference the
controller's config in filterSecret (filterSecret,
ControllerConfig.Spec.PullSecret) so the watch reacts to the actual configured
secret name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: eed7f0d9-2394-4efe-ac5d-d3d662b75431
📒 Files selected for processing (4)
pkg/controller/common/iri_secret_merger.gopkg/controller/internalreleaseimage/internalreleaseimage_controller.gopkg/controller/template/template_controller.gopkg/controller/template/template_controller_test.go
|
/retest-required |
sadasu
force-pushed
the
fix-iri-pull-secret
branch
from
a2a957e to
3efb3e1
Compare
openshift-ci
Bot
added
the
needs-rebase
The IRI credentials were being added to /var/lib/kubelet/config.json which gave only kubelet the necessary credentials to pull images from the IRI registry. Fix now adds the IRI credentials to the global pull secret so that other components like the MCO could also pull images from this internal registry.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/controller/internalreleaseimage/internalreleaseimage_controller.go`:
- Around line 318-324: The deleteSecret handler does a direct type assertion on
the incoming obj which can be a cache.DeletedFinalStateUnknown tombstone and
will panic; update deleteSecret to detect if obj is a
cache.DeletedFinalStateUnknown, extract the actual Secret from tombstone.Obj (or
fall back when the cast fails), then proceed to call ctrl.filterSecret(secret)
and ctrl.queue.Add(ctrlcommon.InternalReleaseImageInstanceName) as before;
reference the deleteSecret function and reuse the existing filterSecret method
and ctrl.queue.Add call so behavior matches other delete handlers like
deleteInternalReleaseImage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 82ffd110-9096-496f-a2e8-260a8d8c61f5
📒 Files selected for processing (4)
pkg/controller/common/iri_secret_merger.gopkg/controller/internalreleaseimage/internalreleaseimage_controller.gopkg/controller/template/template_controller.gopkg/controller/template/template_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/controller/template/template_controller_test.go
- pkg/controller/common/iri_secret_merger.go
sadasu
force-pushed
the
fix-iri-pull-secret
branch
from
3efb3e1 to
2c82ee0
Compare
openshift-ci
Bot
removed
the
needs-rebase
syncGlobalPullSecret only runs during an IRI reconcile, but the IRI controller still requeues only on DockerRegistryKey changes and the two IRI-owned secrets. If the cluster pull secret is rotated/replaced, or ControllerConfig.Spec.PullSecret / the DNS base domain changes, the global secret can stay missing the IRI auth entries until some unrelated event happens.
sadasu
force-pushed
the
fix-iri-pull-secret
branch
from
2c82ee0 to
28bb4db
Compare
|
/retest-required |
|
/verified with help from @bfournie. The global pull secret on both the master and worker had contained the IRI credentials. |
|
@sadasu: The 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. |
|
/verified by @bfournie. The global pull secret on both the master and worker contain the IRI credentials after this fix. |
openshift-ci-robot
added
the
verified
|
@sadasu: This PR has been marked as verified by 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. |
|
/retest-required |
|
/test ? |
|
/test e2e-agent-compact-ipv4-iso-no-registry Better to get a green run on this job for this PR |
|
|
||
| if oldCfg.Spec.Images[templatectrl.DockerRegistryKey] == curCfg.Spec.Images[templatectrl.DockerRegistryKey] { | ||
| // Check if any fields relevant to syncGlobalPullSecret have changed | ||
| dockerRegistryChanged := oldCfg.Spec.Images[templatectrl.DockerRegistryKey] != curCfg.Spec.Images[templatectrl.DockerRegistryKey] |
There was a problem hiding this comment.
nit: (not blocking) this looks like a little bit too over-engineered. I do not expect a lot of updates on the (singleton) controller config resource (a simpler but less complicated code here could work as well very likely)
| } | ||
|
|
||
| // Check if this is the configured global pull secret | ||
| cfg, err := ctrl.ccLister.Get(ctrlcommon.ControllerConfigName) |
There was a problem hiding this comment.
Is there really a chance to have a different name for the global pull-secret? If not I'd prefer to remove this code and use a simpler check by name
| } | ||
|
|
||
| // Sync IRI credentials into the global pull secret | ||
| if err := ctrl.syncGlobalPullSecret(cconfig, iriRegistryCredentialsSecret); err != nil { |
There was a problem hiding this comment.
I think this design has been already discarded previously (see this thread) due the regeneration.
To avoid that @rwsu implemented the current approach (modify directly /var/lib/kubelet/config.json) which does not work as we discussed.
So the merge should happen directly in the point of the code where the global pull-secret is rendered (template controller)
|
@sadasu: 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. |
3 participants
The IRI credentials were being added to /var/lib/kubelet/config.json which gave only kubelet the necessary credentials to pull images from the IRI registry.
Fix now adds the IRI credentials to the global pull secret so that other components like the MCO could also pull images from this internal registry.
- What I did
- How to verify it
- Description for the changelog
Summary by CodeRabbit