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 1 commit 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. |
WalkthroughThis PR refactors IRI registry credential handling from the template controller to the internalreleaseimage controller. Helper functions are exported for reuse, global pull secret syncing is implemented in the internalreleaseimage controller, and template controller logic that merged credentials at render time is removed. ChangesIRI Credentials Migration to InternalReleaseImage Controller
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
| // Sync IRI credentials into the global pull secret | ||
| if err := ctrl.syncGlobalPullSecret(cconfig, iriRegistryCredentialsSecret); err != nil { | ||
| return fmt.Errorf("could not sync global pull secret: %w", err) | ||
| } |
There was a problem hiding this comment.
Requeue on all inputs that can stale the global pull secret.
syncGlobalPullSecret only runs during an IRI reconcile, but this 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.
Also applies to: 422-491
🤖 Prompt for 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.
In `@pkg/controller/internalreleaseimage/internalreleaseimage_controller.go`
around lines 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.
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.
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
| // IRI registry credentials are now managed directly in the global pull secret | ||
| // by the IRI controller, so no merging is needed here. The global pull secret | ||
| // already contains IRI credentials when read above. |
There was a problem hiding this comment.
Don't assume the watched global pull secret is always named pull-secret.
With render-time merging gone, this path only converges when the global pull-secret update re-enqueues the template controller. filterSecret still hard-codes "pull-secret" instead of following ControllerConfig.Spec.PullSecret, so a differently named configured secret will not trigger a rerender and /var/lib/kubelet/config.json can stay stale until some unrelated enqueue.
Suggested change near filterSecret
func (ctrl *Controller) filterSecret(secret *corev1.Secret) {
- if secret.Name == "pull-secret" || secret.Name == ctrlcommon.InternalReleaseImageAuthSecretName {
+ if secret.Name == ctrlcommon.InternalReleaseImageAuthSecretName {
ctrl.enqueueController()
klog.Infof("Re-syncing ControllerConfig due to secret %s change", secret.Name)
+ return
+ }
+
+ cfg, err := ctrl.ccLister.Get(ctrlcommon.ControllerConfigName)
+ if err == nil && cfg.Spec.PullSecret != nil &&
+ secret.Namespace == cfg.Spec.PullSecret.Namespace &&
+ secret.Name == cfg.Spec.PullSecret.Name {
+ ctrl.enqueueController()
+ klog.Infof("Re-syncing ControllerConfig due to secret %s change", secret.Name)
}
}🤖 Prompt for 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.
In `@pkg/controller/template/template_controller.go` around lines 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.
|
/retest-required |
|
@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. |
2 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