AGENT-1449: Add single-phase IRI registry credential rotation - #5810
AGENT-1449: Add single-phase IRI registry credential rotation#5810rwsu wants to merge 8 commits into
Conversation
|
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:
WalkthroughThe PR implements htpasswd-based authentication for the internal release image registry, introducing secret reconciliation logic to sync htpasswd content with password fields, refactoring auth token handling in the daemon, and adding comprehensive test coverage including e2e credential rotation scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 9 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
rwsu
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@rwsu: This pull request references AGENT-1449 which is a valid jira issue. 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/controller/internalreleaseimage/internalreleaseimage_controller.go`:
- Around line 478-510: The code in mergeIRIAuthIntoPullSecret reads
cconfig.Spec.DNS.Spec.BaseDomain without guarding for a nil DNS pointer; add a
nil check at the start of mergeIRIAuthIntoPullSecret to handle missing DNS
(e.g., if cconfig.Spec.DNS == nil) and return a clear error (or fallback
behavior) instead of dereferencing; update references to baseDomain to use the
validated value so the function never panics when ControllerConfig has no DNS
configured.
In `@pkg/controller/internalreleaseimage/internalreleaseimage_renderer.go`:
- Around line 121-122: r.iriAuthSecret may be nil but the code unconditionally
reads r.iriAuthSecret.Data["htpasswd"] into iriHtpasswd; add a nil guard before
that access (e.g., check if r.iriAuthSecret != nil) and handle the nil case
explicitly — either return an error from the renderer function or set
iriHtpasswd to a safe default and log/propagate the missing secret; update the
struct comment only if you change the invariant to make iriAuthSecret required.
Ensure you modify the code paths that use iriHtpasswd to handle the new
nil/empty case consistently.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 33ab74c5-20ae-4aee-8f24-35db4c8537b4
⛔ Files ignored due to path filters (3)
vendor/golang.org/x/crypto/bcrypt/base64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/bcrypt/bcrypt.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (15)
pkg/apihelpers/apihelpers.gopkg/controller/bootstrap/bootstrap.gopkg/controller/common/constants.gopkg/controller/internalreleaseimage/internalreleaseimage_bootstrap.gopkg/controller/internalreleaseimage/internalreleaseimage_bootstrap_test.gopkg/controller/internalreleaseimage/internalreleaseimage_controller.gopkg/controller/internalreleaseimage/internalreleaseimage_controller_test.gopkg/controller/internalreleaseimage/internalreleaseimage_helpers_test.gopkg/controller/internalreleaseimage/internalreleaseimage_renderer.gopkg/controller/internalreleaseimage/pullsecret.gopkg/controller/internalreleaseimage/pullsecret_test.gopkg/controller/internalreleaseimage/templates/master/files/iri-registry-auth-htpasswd.yamlpkg/controller/internalreleaseimage/templates/master/files/usr-local-bin-load-registry-image-sh.yamlpkg/controller/internalreleaseimage/templates/master/units/iri-registry.service.yamltest/e2e-iri/iri_test.go
| func (ctrl *Controller) mergeIRIAuthIntoPullSecret(cconfig *mcfgv1.ControllerConfig, authSecret *corev1.Secret) error { | ||
| password := string(authSecret.Data["password"]) | ||
| if password == "" { | ||
| return fmt.Errorf("IRI auth secret %s/%s has empty password", authSecret.Namespace, authSecret.Name) | ||
| } | ||
|
|
||
| baseDomain := cconfig.Spec.DNS.Spec.BaseDomain | ||
|
|
||
| // Fetch current pull secret from openshift-config | ||
| pullSecret, err := ctrl.kubeClient.CoreV1().Secrets(ctrlcommon.OpenshiftConfigNamespace).Get( | ||
| context.TODO(), ctrlcommon.GlobalPullSecretName, metav1.GetOptions{}) | ||
| if err != nil { | ||
| return fmt.Errorf("could not get pull-secret: %w", err) | ||
| } | ||
|
|
||
| mergedBytes, err := MergeIRIAuthIntoPullSecret(pullSecret.Data[corev1.DockerConfigJsonKey], password, baseDomain) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // No change needed | ||
| if bytes.Equal(mergedBytes, pullSecret.Data[corev1.DockerConfigJsonKey]) { | ||
| return nil | ||
| } | ||
|
|
||
| pullSecret.Data[corev1.DockerConfigJsonKey] = mergedBytes | ||
| _, err = ctrl.kubeClient.CoreV1().Secrets(ctrlcommon.OpenshiftConfigNamespace).Update( | ||
| context.TODO(), pullSecret, metav1.UpdateOptions{}) | ||
| if err == nil { | ||
| klog.Infof("Updated pull secret with IRI registry auth credentials from secret %s/%s (uid=%s, resourceVersion=%s)", authSecret.Namespace, authSecret.Name, authSecret.UID, authSecret.ResourceVersion) | ||
| } | ||
| return err | ||
| } |
There was a problem hiding this comment.
Potential nil pointer dereference on cconfig.Spec.DNS.
Line 484 accesses cconfig.Spec.DNS.Spec.BaseDomain without checking if DNS is nil. If the ControllerConfig doesn't have DNS configured, this will panic.
🛡️ Proposed fix: add nil guard
func (ctrl *Controller) mergeIRIAuthIntoPullSecret(cconfig *mcfgv1.ControllerConfig, authSecret *corev1.Secret) error {
password := string(authSecret.Data["password"])
if password == "" {
return fmt.Errorf("IRI auth secret %s/%s has empty password", authSecret.Namespace, authSecret.Name)
}
+ if cconfig.Spec.DNS == nil {
+ return fmt.Errorf("ControllerConfig DNS not configured, cannot determine IRI registry host")
+ }
baseDomain := cconfig.Spec.DNS.Spec.BaseDomain📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (ctrl *Controller) mergeIRIAuthIntoPullSecret(cconfig *mcfgv1.ControllerConfig, authSecret *corev1.Secret) error { | |
| password := string(authSecret.Data["password"]) | |
| if password == "" { | |
| return fmt.Errorf("IRI auth secret %s/%s has empty password", authSecret.Namespace, authSecret.Name) | |
| } | |
| baseDomain := cconfig.Spec.DNS.Spec.BaseDomain | |
| // Fetch current pull secret from openshift-config | |
| pullSecret, err := ctrl.kubeClient.CoreV1().Secrets(ctrlcommon.OpenshiftConfigNamespace).Get( | |
| context.TODO(), ctrlcommon.GlobalPullSecretName, metav1.GetOptions{}) | |
| if err != nil { | |
| return fmt.Errorf("could not get pull-secret: %w", err) | |
| } | |
| mergedBytes, err := MergeIRIAuthIntoPullSecret(pullSecret.Data[corev1.DockerConfigJsonKey], password, baseDomain) | |
| if err != nil { | |
| return err | |
| } | |
| // No change needed | |
| if bytes.Equal(mergedBytes, pullSecret.Data[corev1.DockerConfigJsonKey]) { | |
| return nil | |
| } | |
| pullSecret.Data[corev1.DockerConfigJsonKey] = mergedBytes | |
| _, err = ctrl.kubeClient.CoreV1().Secrets(ctrlcommon.OpenshiftConfigNamespace).Update( | |
| context.TODO(), pullSecret, metav1.UpdateOptions{}) | |
| if err == nil { | |
| klog.Infof("Updated pull secret with IRI registry auth credentials from secret %s/%s (uid=%s, resourceVersion=%s)", authSecret.Namespace, authSecret.Name, authSecret.UID, authSecret.ResourceVersion) | |
| } | |
| return err | |
| } | |
| func (ctrl *Controller) mergeIRIAuthIntoPullSecret(cconfig *mcfgv1.ControllerConfig, authSecret *corev1.Secret) error { | |
| password := string(authSecret.Data["password"]) | |
| if password == "" { | |
| return fmt.Errorf("IRI auth secret %s/%s has empty password", authSecret.Namespace, authSecret.Name) | |
| } | |
| if cconfig.Spec.DNS == nil { | |
| return fmt.Errorf("ControllerConfig DNS not configured, cannot determine IRI registry host") | |
| } | |
| baseDomain := cconfig.Spec.DNS.Spec.BaseDomain | |
| // Fetch current pull secret from openshift-config | |
| pullSecret, err := ctrl.kubeClient.CoreV1().Secrets(ctrlcommon.OpenshiftConfigNamespace).Get( | |
| context.TODO(), ctrlcommon.GlobalPullSecretName, metav1.GetOptions{}) | |
| if err != nil { | |
| return fmt.Errorf("could not get pull-secret: %w", err) | |
| } | |
| mergedBytes, err := MergeIRIAuthIntoPullSecret(pullSecret.Data[corev1.DockerConfigJsonKey], password, baseDomain) | |
| if err != nil { | |
| return err | |
| } | |
| // No change needed | |
| if bytes.Equal(mergedBytes, pullSecret.Data[corev1.DockerConfigJsonKey]) { | |
| return nil | |
| } | |
| pullSecret.Data[corev1.DockerConfigJsonKey] = mergedBytes | |
| _, err = ctrl.kubeClient.CoreV1().Secrets(ctrlcommon.OpenshiftConfigNamespace).Update( | |
| context.TODO(), pullSecret, metav1.UpdateOptions{}) | |
| if err == nil { | |
| klog.Infof("Updated pull secret with IRI registry auth credentials from secret %s/%s (uid=%s, resourceVersion=%s)", authSecret.Namespace, authSecret.Name, authSecret.UID, authSecret.ResourceVersion) | |
| } | |
| return err | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/controller/internalreleaseimage/internalreleaseimage_controller.go`
around lines 478 - 510, The code in mergeIRIAuthIntoPullSecret reads
cconfig.Spec.DNS.Spec.BaseDomain without guarding for a nil DNS pointer; add a
nil check at the start of mergeIRIAuthIntoPullSecret to handle missing DNS
(e.g., if cconfig.Spec.DNS == nil) and return a clear error (or fallback
behavior) instead of dereferencing; update references to baseDomain to use the
validated value so the function never panics when ControllerConfig has no DNS
configured.
|
@rwsu: This pull request references AGENT-1449 which is a valid jira issue. 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. |
|
@rwsu: This pull request references AGENT-1449 which is a valid jira issue. 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. |
|
@rwsu: This pull request references AGENT-1449 which is a valid jira issue. 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. |
openshift-ci
Bot
added
the
needs-rebase
5bf9b3c to
91b03d0
Compare
openshift-ci
Bot
removed
the
needs-rebase
There was a problem hiding this comment.
♻️ Duplicate comments (2)
pkg/controller/internalreleaseimage/internalreleaseimage_controller.go (1)
538-544:⚠️ Potential issue | 🟠 MajorPotential nil pointer dereference on
cconfig.Spec.DNS.Line 544 accesses
cconfig.Spec.DNS.Spec.BaseDomainwithout checking ifDNSis nil. If the ControllerConfig doesn't have DNS configured, this will panic.func (ctrl *Controller) mergeIRIAuthIntoPullSecret(cconfig *mcfgv1.ControllerConfig, authSecret *corev1.Secret) error { password := string(authSecret.Data["password"]) if password == "" { return fmt.Errorf("IRI auth secret %s/%s has empty password", authSecret.Namespace, authSecret.Name) } + if cconfig.Spec.DNS == nil { + return fmt.Errorf("ControllerConfig DNS not configured, cannot determine IRI registry host") + } baseDomain := cconfig.Spec.DNS.Spec.BaseDomain🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/internalreleaseimage/internalreleaseimage_controller.go` around lines 538 - 544, In mergeIRIAuthIntoPullSecret, avoid a nil pointer dereference by checking cconfig.Spec.DNS before accessing cconfig.Spec.DNS.Spec.BaseDomain; if cconfig.Spec.DNS is nil, either return a descriptive error or use a sensible default/behavior consistent with the controller (e.g., return fmt.Errorf("ControllerConfig missing DNS configuration") or proceed with an empty baseDomain), then use the validated baseDomain value for the rest of the function.pkg/controller/internalreleaseimage/internalreleaseimage_renderer.go (1)
121-122:⚠️ Potential issue | 🟠 MajorNil pointer dereference risk if
iriAuthSecretis nil.Line 43 documents
iriAuthSecretas "may be nil", but line 121 unconditionally accessesr.iriAuthSecret.Data["htpasswd"]. The bootstrap code path can pass nil foririAuthSecret, which would cause a panic.Add a nil guard before accessing the secret data:
+ var iriHtpasswd string + if r.iriAuthSecret != nil { + iriHtpasswd = string(r.iriAuthSecret.Data["htpasswd"]) + } - iriHtpasswd := string(r.iriAuthSecret.Data["htpasswd"])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/internalreleaseimage/internalreleaseimage_renderer.go` around lines 121 - 122, The code unconditionally reads r.iriAuthSecret.Data["htpasswd"] into iriHtpasswd which can panic because r.iriAuthSecret may be nil; update the iriHtpasswd initialization in internalreleaseimage_renderer.go (where iriHtpasswd is set) to first check r.iriAuthSecret != nil and only read Data["htpasswd"] when non-nil, otherwise set iriHtpasswd to an appropriate empty/default value or handle the nil case so no nil pointer dereference occurs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/controller/internalreleaseimage/internalreleaseimage_controller.go`:
- Around line 538-544: In mergeIRIAuthIntoPullSecret, avoid a nil pointer
dereference by checking cconfig.Spec.DNS before accessing
cconfig.Spec.DNS.Spec.BaseDomain; if cconfig.Spec.DNS is nil, either return a
descriptive error or use a sensible default/behavior consistent with the
controller (e.g., return fmt.Errorf("ControllerConfig missing DNS
configuration") or proceed with an empty baseDomain), then use the validated
baseDomain value for the rest of the function.
In `@pkg/controller/internalreleaseimage/internalreleaseimage_renderer.go`:
- Around line 121-122: The code unconditionally reads
r.iriAuthSecret.Data["htpasswd"] into iriHtpasswd which can panic because
r.iriAuthSecret may be nil; update the iriHtpasswd initialization in
internalreleaseimage_renderer.go (where iriHtpasswd is set) to first check
r.iriAuthSecret != nil and only read Data["htpasswd"] when non-nil, otherwise
set iriHtpasswd to an appropriate empty/default value or handle the nil case so
no nil pointer dereference occurs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d91576d8-1b24-47cb-816b-8cf2eb827572
⛔ Files ignored due to path filters (3)
vendor/golang.org/x/crypto/bcrypt/base64.gois excluded by!vendor/**,!**/vendor/**vendor/golang.org/x/crypto/bcrypt/bcrypt.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (15)
pkg/apihelpers/apihelpers.gopkg/controller/bootstrap/bootstrap.gopkg/controller/common/constants.gopkg/controller/internalreleaseimage/internalreleaseimage_bootstrap.gopkg/controller/internalreleaseimage/internalreleaseimage_bootstrap_test.gopkg/controller/internalreleaseimage/internalreleaseimage_controller.gopkg/controller/internalreleaseimage/internalreleaseimage_controller_test.gopkg/controller/internalreleaseimage/internalreleaseimage_helpers_test.gopkg/controller/internalreleaseimage/internalreleaseimage_renderer.gopkg/controller/internalreleaseimage/pullsecret.gopkg/controller/internalreleaseimage/pullsecret_test.gopkg/controller/internalreleaseimage/templates/master/files/iri-registry-auth-htpasswd.yamlpkg/controller/internalreleaseimage/templates/master/files/usr-local-bin-load-registry-image-sh.yamlpkg/controller/internalreleaseimage/templates/master/units/iri-registry.service.yamltest/e2e-iri/iri_test.go
✅ Files skipped from review due to trivial changes (4)
- pkg/controller/internalreleaseimage/templates/master/files/iri-registry-auth-htpasswd.yaml
- pkg/controller/common/constants.go
- pkg/controller/internalreleaseimage/pullsecret_test.go
- pkg/controller/internalreleaseimage/internalreleaseimage_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/controller/internalreleaseimage/templates/master/files/usr-local-bin-load-registry-image-sh.yaml
- pkg/apihelpers/apihelpers.go
- pkg/controller/internalreleaseimage/internalreleaseimage_bootstrap.go
- pkg/controller/internalreleaseimage/internalreleaseimage_bootstrap_test.go
91b03d0 to
199098a
Compare
|
/test e2e-aws-ovn |
|
/verified by @rwsu and with TestIRIAuth_CredentialRotation e2e test |
|
@rwsu: 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. |
openshift-ci-robot
added
the
verified
199098a to
56ec3ab
Compare
openshift-ci-robot
removed
the
verified
|
/test e2e-agent-compact-ipv4-iso-no-registry |
|
@rwsu: This pull request references AGENT-1449 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 story to target either version "5.0." or "openshift-5.0.", but it targets "openshift-4.22" instead. 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. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
pkg/controller/internalreleaseimage/internalreleaseimage_renderer.go (1)
121-124:⚠️ Potential issue | 🔴 CriticalGuard against nil/missing IRI auth secret before reading
htpasswd.Line 123 dereferences
r.iriRegistryCredentialsSecretunconditionally; this can panic (or render empty auth) when bootstrap inputs are incomplete. Fail fast with a clear error instead of implicit dereference.🛠️ Proposed fix
func (r *Renderer) newRenderContext() (*renderContext, error) { iriTLSKey, err := r.extractTLSCertFieldFromSecret(r.iriSecret, "tls.key") if err != nil { return nil, err } iriTLSCert, err := r.extractTLSCertFieldFromSecret(r.iriSecret, "tls.crt") if err != nil { return nil, err } - // iriRegistryCredentialsSecret is always non-nil here: the IRI controller - // fetches it and fails loudly if not found (auth is mandatory). - iriHtpasswd := string(r.iriRegistryCredentialsSecret.Data["htpasswd"]) + if r.iriRegistryCredentialsSecret == nil { + return nil, fmt.Errorf("missing secret %q", ctrlcommon.InternalReleaseImageAuthSecretName) + } + iriHtpasswdRaw, found := r.iriRegistryCredentialsSecret.Data["htpasswd"] + if !found || len(iriHtpasswdRaw) == 0 { + return nil, fmt.Errorf("cannot find non-empty htpasswd in secret %s", r.iriRegistryCredentialsSecret.Name) + } + iriHtpasswd := string(iriHtpasswdRaw) return &renderContext{ DockerRegistryImage: r.cconfig.Spec.Images[templatectrl.DockerRegistryKey],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/internalreleaseimage/internalreleaseimage_renderer.go` around lines 121 - 124, Check r.iriRegistryCredentialsSecret for nil and ensure Data contains the "htpasswd" key before dereferencing it to build iriHtpasswd; if the secret is nil or the key is missing, return an explicit error (or fail fast) with a clear message instead of proceeding. Update the code that currently sets iriHtpasswd from r.iriRegistryCredentialsSecret.Data["htpasswd"] to perform a nil check on r.iriRegistryCredentialsSecret and a presence check on the "htpasswd" entry, and return a descriptive error from the surrounding function (referencing iriRegistryCredentialsSecret and iriHtpasswd) when either check fails.
🧹 Nitpick comments (3)
pkg/controller/common/iri_secret_merger.go (1)
46-48: Add a defensive nil guard forfgHandlerin the lister-based constructor.Line 47 dereferences
fgHandlerdirectly; unlikeNewIRISecretMergerFromObjects, this path will panic if nil is passed.Suggested fix
- if !fgHandler.Enabled(features.FeatureGateNoRegistryClusterInstall) { + if fgHandler == nil || !fgHandler.Enabled(features.FeatureGateNoRegistryClusterInstall) { return "", "", errIRIDisabled }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/common/iri_secret_merger.go` around lines 46 - 48, The lister-based constructor dereferences fgHandler inside the resolve closure (resolve: func() (string, string, error) { if !fgHandler.Enabled(...)) which will panic if fgHandler is nil; add a defensive nil check at the start of that resolve closure (or in the lister-based constructor before creating the closure) that returns a clear error (e.g., a new errNilFeatureGateHandler or a descriptive fmt.Errorf) instead of dereferencing fgHandler, and ensure callers handle this error similarly to other constructor error paths (mirroring NewIRISecretMergerFromObjects behavior).pkg/controller/internalreleaseimage/internalreleaseimage_helpers_test.go (1)
245-256: Set pull-secret type in test fixture to match real objects.The helper currently omits
Type, which can hide type-validation regressions in consumers.🔧 Proposed test-fixture adjustment
func pullSecret() *secretBuilder { return &secretBuilder{ obj: &corev1.Secret{ ObjectMeta: v1.ObjectMeta{ Namespace: ctrlcommon.OpenshiftConfigNamespace, Name: ctrlcommon.GlobalPullSecretName, }, + Type: corev1.SecretTypeDockerConfigJson, Data: map[string][]byte{ corev1.DockerConfigJsonKey: []byte(`{"auths":{"quay.io":{"auth":"dGVzdDp0ZXN0"}}}`), }, }, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/internalreleaseimage/internalreleaseimage_helpers_test.go` around lines 245 - 256, The test fixture returned by pullSecret() omits the Secret.Type which can hide type-validation regressions; update pullSecret (and the secretBuilder's obj of type *corev1.Secret) to set Type: corev1.SecretTypeDockerConfigJson (matching real pull-secret objects) so tests exercise the same validation paths as production consumers.pkg/controller/template/template_controller.go (1)
152-158: Harden constructor against mixed nil informer inputs.If
iriInformeris non-nil whileiriSecretsInformeris nil, Line 154 will panic oniriSecretsInformer.Lister(). Add an explicit guard and fall back to a merger without secret lister (or log and disable merge).🧱 Proposed defensive guard
- if iriInformer != nil { + if iriInformer != nil && iriSecretsInformer != nil { ctrl.iriInformerSynced = iriInformer.Informer().HasSynced ctrl.iriMerger = ctrlcommon.NewIRISecretMerger(iriSecretsInformer.Lister(), ctrl.ccLister, iriInformer.Lister(), fgHandler) + } else if iriInformer != nil { + ctrl.iriInformerSynced = iriInformer.Informer().HasSynced + klog.Warning("iriInformer configured without iriSecretsInformer; disabling IRI credential merge") + ctrl.iriMerger = ctrlcommon.NewIRISecretMerger(nil, ctrl.ccLister, iriInformer.Lister(), fgHandler) } else { ctrl.iriInformerSynced = func() bool { return true } ctrl.iriMerger = ctrlcommon.NewIRISecretMerger(nil, ctrl.ccLister, nil, fgHandler) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/template/template_controller.go` around lines 152 - 158, The constructor assumes iriSecretsInformer is non-nil when iriInformer is non-nil and will panic calling iriSecretsInformer.Lister(); update the branch that handles a non-nil iriInformer to check iriSecretsInformer for nil and pass nil for the secret lister to ctrlcommon.NewIRISecretMerger if absent (or otherwise disable merging/log an error), while still setting ctrl.iriInformerSynced = iriInformer.Informer().HasSynced and using iriInformer.Lister() and fgHandler; ensure the symmetric else case remains for when iriInformer is nil.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/controller/internalreleaseimage/internalreleaseimage_controller.go`:
- Around line 286-299: The secret event handlers (the add/update functions
shown) only filter by secret.Name (ctrlcommon.InternalReleaseImageTLSSecretName
and ctrlcommon.InternalReleaseImageAuthSecretName) and must also verify the
secret is in the controller’s target namespace to avoid reacting to same-name
secrets cluster-wide; update both the add and update handlers (the functions
containing the shown name checks) to also compare secret.Namespace against the
controller’s configured namespace field (e.g., ctrl.namespace or
ctrl.watchNamespace or the constant used for the IRI namespace) and return early
unless it matches, then proceed to log and enqueue as before.
In `@pkg/controller/internalreleaseimage/internalreleaseimage_registry_auth.go`:
- Around line 64-65: The Update call for the Secret is using a hardcoded
namespace (ctrlcommon.MCONamespace); change it to use the secret's actual
namespace (authSecret.Namespace) so the Update uses
kubeClient.CoreV1().Secrets(authSecret.Namespace).Update(...). Locate the call
in internalreleaseimage_registry_auth.go (the result, err :=
kubeClient.CoreV1().Secrets(...).Update(...) line) and replace the hardcoded
namespace with authSecret.Namespace to keep namespace usage dynamic and
consistent.
In `@pkg/daemon/internalreleaseimage/iriregistry.go`:
- Around line 58-79: The readIRIAuthToken function should treat a missing auth
entry as non-fatal so anonymous registry access is allowed: keep returning
errors for os.ReadFile and json.Unmarshal failures, but change the end of
readIRIAuthToken (the check of dockerConfig.Auths[registryHostPort]) to return
an empty string and nil error when no entry exists or entry.Auth is empty
instead of returning an error; this preserves the current behavior when an auth
token is present (return entry.Auth, nil) while enabling the unauthenticated
fallback.
---
Duplicate comments:
In `@pkg/controller/internalreleaseimage/internalreleaseimage_renderer.go`:
- Around line 121-124: Check r.iriRegistryCredentialsSecret for nil and ensure
Data contains the "htpasswd" key before dereferencing it to build iriHtpasswd;
if the secret is nil or the key is missing, return an explicit error (or fail
fast) with a clear message instead of proceeding. Update the code that currently
sets iriHtpasswd from r.iriRegistryCredentialsSecret.Data["htpasswd"] to perform
a nil check on r.iriRegistryCredentialsSecret and a presence check on the
"htpasswd" entry, and return a descriptive error from the surrounding function
(referencing iriRegistryCredentialsSecret and iriHtpasswd) when either check
fails.
---
Nitpick comments:
In `@pkg/controller/common/iri_secret_merger.go`:
- Around line 46-48: The lister-based constructor dereferences fgHandler inside
the resolve closure (resolve: func() (string, string, error) { if
!fgHandler.Enabled(...)) which will panic if fgHandler is nil; add a defensive
nil check at the start of that resolve closure (or in the lister-based
constructor before creating the closure) that returns a clear error (e.g., a new
errNilFeatureGateHandler or a descriptive fmt.Errorf) instead of dereferencing
fgHandler, and ensure callers handle this error similarly to other constructor
error paths (mirroring NewIRISecretMergerFromObjects behavior).
In `@pkg/controller/internalreleaseimage/internalreleaseimage_helpers_test.go`:
- Around line 245-256: The test fixture returned by pullSecret() omits the
Secret.Type which can hide type-validation regressions; update pullSecret (and
the secretBuilder's obj of type *corev1.Secret) to set Type:
corev1.SecretTypeDockerConfigJson (matching real pull-secret objects) so tests
exercise the same validation paths as production consumers.
In `@pkg/controller/template/template_controller.go`:
- Around line 152-158: The constructor assumes iriSecretsInformer is non-nil
when iriInformer is non-nil and will panic calling iriSecretsInformer.Lister();
update the branch that handles a non-nil iriInformer to check iriSecretsInformer
for nil and pass nil for the secret lister to ctrlcommon.NewIRISecretMerger if
absent (or otherwise disable merging/log an error), while still setting
ctrl.iriInformerSynced = iriInformer.Informer().HasSynced and using
iriInformer.Lister() and fgHandler; ensure the symmetric else case remains for
when iriInformer is nil.
🪄 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: 14b0550d-2255-47f9-a2aa-67688c9d4de9
📒 Files selected for processing (24)
cmd/machine-config-controller/start.goinstall/0000_80_machine-config_00_service.yamlpkg/apihelpers/apihelpers.gopkg/controller/bootstrap/bootstrap.gopkg/controller/common/constants.gopkg/controller/common/iri_secret_merger.gopkg/controller/common/iri_secret_merger_test.gopkg/controller/internalreleaseimage/internalreleaseimage_bootstrap.gopkg/controller/internalreleaseimage/internalreleaseimage_bootstrap_test.gopkg/controller/internalreleaseimage/internalreleaseimage_controller.gopkg/controller/internalreleaseimage/internalreleaseimage_controller_test.gopkg/controller/internalreleaseimage/internalreleaseimage_helpers_test.gopkg/controller/internalreleaseimage/internalreleaseimage_registry_auth.gopkg/controller/internalreleaseimage/internalreleaseimage_renderer.gopkg/controller/internalreleaseimage/templates/master/files/iri-registry-auth-htpasswd.yamlpkg/controller/internalreleaseimage/templates/master/files/usr-local-bin-load-registry-image-sh.yamlpkg/controller/internalreleaseimage/templates/master/units/iri-registry.service.yamlpkg/controller/template/template_controller.gopkg/controller/template/template_controller_test.gopkg/daemon/internalreleaseimage/internalreleaseimage_manager.gopkg/daemon/internalreleaseimage/internalreleaseimage_manager_test.gopkg/daemon/internalreleaseimage/iriregistry.gotest/e2e-bootstrap/bootstrap_test.gotest/e2e-iri/iri_test.go
✅ Files skipped from review due to trivial changes (2)
- pkg/controller/common/constants.go
- pkg/controller/internalreleaseimage/templates/master/files/iri-registry-auth-htpasswd.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/controller/internalreleaseimage/templates/master/files/usr-local-bin-load-registry-image-sh.yaml
- pkg/controller/internalreleaseimage/templates/master/units/iri-registry.service.yaml
- test/e2e-iri/iri_test.go
| // readIRIAuthToken reads the base64-encoded auth token for the IRI registry | ||
| // from the kubelet auth file (/var/lib/kubelet/config.json). | ||
| func readIRIAuthToken(registryHostPort string) (string, error) { | ||
| data, err := os.ReadFile(constants.KubeletAuthFile) | ||
| if err != nil { | ||
| return "", fmt.Errorf("could not read %s for IRI registry auth: %w", constants.KubeletAuthFile, err) | ||
| } | ||
|
|
||
| var dockerConfig struct { | ||
| Auths map[string]struct { | ||
| Auth string `json:"auth"` | ||
| } `json:"auths"` | ||
| } | ||
| if err := json.Unmarshal(data, &dockerConfig); err != nil { | ||
| return "", fmt.Errorf("could not parse %s for IRI registry auth: %w", constants.KubeletAuthFile, err) | ||
| } | ||
|
|
||
| if entry, ok := dockerConfig.Auths[registryHostPort]; ok && entry.Auth != "" { | ||
| return entry.Auth, nil | ||
| } | ||
| return "", fmt.Errorf("no auth entry found for %s in %s", registryHostPort, constants.KubeletAuthFile) | ||
| } |
There was a problem hiding this comment.
Handle missing auth entry as non-fatal to preserve unauthenticated fallback.
Right now, if the kubelet auth file exists but has no matching entry, Line 78 returns an error and blocks registry checks entirely. That conflicts with the empty-token behavior described in comments and makes anonymous access impossible.
Proposed fix
func readIRIAuthToken(registryHostPort string) (string, error) {
data, err := os.ReadFile(constants.KubeletAuthFile)
if err != nil {
+ if os.IsNotExist(err) {
+ // No kubelet auth file: allow anonymous registry access.
+ return "", nil
+ }
return "", fmt.Errorf("could not read %s for IRI registry auth: %w", constants.KubeletAuthFile, err)
}
@@
- if entry, ok := dockerConfig.Auths[registryHostPort]; ok && entry.Auth != "" {
+ if entry, ok := dockerConfig.Auths[registryHostPort]; ok {
return entry.Auth, nil
}
- return "", fmt.Errorf("no auth entry found for %s in %s", registryHostPort, constants.KubeletAuthFile)
+ // No matching auth entry: continue unauthenticated.
+ return "", nil
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/daemon/internalreleaseimage/iriregistry.go` around lines 58 - 79, The
readIRIAuthToken function should treat a missing auth entry as non-fatal so
anonymous registry access is allowed: keep returning errors for os.ReadFile and
json.Unmarshal failures, but change the end of readIRIAuthToken (the check of
dockerConfig.Auths[registryHostPort]) to return an empty string and nil error
when no entry exists or entry.Auth is empty instead of returning an error; this
preserves the current behavior when an auth token is present (return entry.Auth,
nil) while enabling the unauthenticated fallback.
| // template controller would block forever. | ||
| var iriSecretsInformer coreinformersv1.SecretInformer | ||
| var iriInformer mcfginformersv1alpha1.InternalReleaseImageInformer | ||
| if ctx.FeatureGatesHandler.Enabled(features.FeatureGateNoRegistryClusterInstall) { |
There was a problem hiding this comment.
nit: wouldn't be better to append them to controller rather than adding nil entries?
| // are rotated the pull secret rendered into 00-master/00-worker is updated. | ||
| // Both informers are nil when the NoRegistryClusterInstall feature gate is | ||
| // off (the CRD doesn't exist on those clusters). | ||
| if iriSecretsInformer != nil { |
There was a problem hiding this comment.
Probably it works, but I'd find easier to conditionally constraint this code on a featuregate handler check (ie, only when IRI is enabled)
| } | ||
| var iriReg *iriRegistry | ||
| if registryErr == nil { | ||
| iriReg = newIRIRegistry(i.nodeName, i.registryClient, authToken) |
There was a problem hiding this comment.
This looks fragile, since by design the registry ctor hard-coded iriRegistryHost and iriRegistryPort usage. So no need to invoke readIRIAuthToken outside newIRIRegistry method, it'd be safer to push it down within newIRIRegistry (it already has all the elements to create the token) and have everything centralize there.
Also, as an additional bonus, we'll keep the main sync method simpler and with fewer lines of code (to help the readability and maintenance)
|
|
||
| // readIRIAuthToken reads the base64-encoded auth token for the IRI registry | ||
| // from the kubelet auth file (/var/lib/kubelet/config.json). | ||
| func readIRIAuthToken(registryHostPort string) (string, error) { |
There was a problem hiding this comment.
let's make it part of the type
| // The MCD pod runs on the host network and can reach api-int:22625, making | ||
| // this approach work in CI where the port is not reachable from the test runner. | ||
| // Returns the HTTP status code string (e.g. "200", "401"). | ||
| func curlIRIRegistry(t *testing.T, cs *framework.ClientSet, node corev1.Node, baseDomain string, extraArgs ...string) string { |
There was a problem hiding this comment.
This works and I'd not consider it a blocking point for the current PR, but it feels a little bit hacky and fragile.
IMHO a more robust cert rotation test would be:
- Create a new pod with image pull policy set to Always. Launch it and ensure it goes running.
- Change the IRI credentials
- Repeat step 1
openshift-ci
Bot
removed
the
do-not-merge/hold
6bcc98d to
7f2b2e4
Compare
openshift-ci-robot
removed
the
verified
|
/test e2e-agent-compact-ipv4-iso-no-registry unit |
|
/test unit |
|
/lgtm |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: andfasano, rwsu 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 |
|
/test e2e-aws-ovn-upgrade |
|
/verified by e2e-agent-compact-ipv4-iso-no-registry TestIRIAuth_CredentialRotation iri-e2e test |
openshift-ci-robot
added
the
verified
|
@rwsu: 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. |
Implement credential rotation that accepts brief registry downtime. When an admin updates iriAuthSecret.Data["password"], the controller: 1. Detects the mismatch between password and htpasswd (via bcrypt compare) 2. Generates a new bcrypt hash and updates iriAuthSecret.Data["htpasswd"] 3. Re-renders the master MachineConfig with the new htpasswd 4. MCD rolls out the updated MC; brief downtime for IRI registry during rollout is accepted Key changes: - Add kubeClient field to IRI controller (needed to update auth secret) - Add reconcileHtpasswd to detect password/htpasswd mismatch and regenerate the bcrypt hash; moved to internalreleaseimage_registry_auth.go alongside the bcrypt helpers (generateHtpasswdEntry, HtpasswdMatchesPassword) - Add NoneStatusAction for /etc/iri-registry/auth/htpasswd in NodeDisruptionPolicy (distribution registry re-reads htpasswd on mtime change, no restart needed) - Add unit tests for reconcileHtpasswd - Add e2e test for the full rotation flow (TestIRIAuth_CredentialRotation); uses ExecCmdOnNode via MCD pod to reach api-int:22625 in CI Assisted-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Move readIRIAuthToken from a standalone function into a method on iriRegistry (readAuthToken), and have newIRIRegistry call it internally rather than requiring the caller to resolve credentials beforehand. newIRIRegistry now returns (*iriRegistry, error) and accepts an optional authTokenOverride used in tests; in production the override is always empty and the token is read from the kubelet auth file at construction time. The manager sync path shrinks from 7 lines to 3. Assisted-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…space - addSecret/updateSecret now check namespace (MCONamespace) before name, preventing same-name secrets in other namespaces from triggering noisy IRI requeues - reconcileHtpasswd uses authSecret.Namespace instead of the hardcoded MCONamespace constant when updating the secret Assisted-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…emplate controller Replace if iriSecretsInformer != nil / if iriInformer != nil guards with fgHandler.Enabled(FeatureGateNoRegistryClusterInstall) checks, making the intent explicit: IRI event handlers and the merger are only wired when the feature gate is on, not as a side-effect of nil informers being passed. The nil-informer approach in start.go is preserved as it correctly prevents the informers from starting on clusters where the CRD is not installed. Assisted-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Add verifyCanPullFromIRI helper that creates a pod with imagePullPolicy:Always using the IRI release image (pulled from the local IRI registry, not quay.io) and verifies the kubelet can authenticate and pull it. This exercises the full kubelet credential lookup path (/var/lib/kubelet/config.json) rather than just raw HTTP auth via curl exec. Add getIRIReleasePullSpec helper that queries /v2/openshift/release-images/tags/list on the IRI registry and constructs the local pullspec (api-int.<baseDomain>:22625/openshift/release-images:<version-tag>). Add pre-rotation and post-rotation pull checks to TestIRIAuth_CredentialRotation. The existing curlIRIRegistry checks are retained for old-credential rejection verification. Assisted-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…and lister use Evaluate NoRegistryClusterInstall feature gate once at NewIRISecretMerger construction time rather than re-reading it live inside the resolve closure. When the gate is off at New() time, nil listers are passed. If the live gate check returned true on a subsequent Merge() call (e.g. TechPreview FeatureGate synced after controller creation in bootstrap-unit tests), calling Get() on a nil lister would panic. Capturing the gate state at construction time makes the enabled/disabled decision consistent with the lister setup. Assisted-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…edentials rejected
The old-credential 401 check after rotation was running immediately after
observing a single api-int 200 from curlIRIRegistry. Since api-int is a VIP
that load-balances across masters, this only proved one backend had the new
htpasswd; the 401 probe could land on an unrotated master and return 200.
Wait for WaitForPoolCompleteAny("master") before the old-credential assertion
to ensure all masters have applied the new htpasswd before we check rejection.
Assisted-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Three fixes for reliability of the post-rotation verifyCanPullFromIRI check: 1. Retry getIRIReleasePullSpec until tags are available. After credential restores the IRI registry can take a moment to stabilize; querying tags immediately can return an empty list causing a spurious test failure. 2. Wait for /var/lib/kubelet/config.json to contain the new IRI credentials before creating the pull-test pod. Credential rotation triggers two sequential MC rollouts (02-master for htpasswd, 00-master for pull secret); WaitForPoolCompleteAny returns after the first, so without this wait the pod is created before the pull secret is updated. 3. Retry the pull-test pod if it hits ImagePullBackOff. CRI-O can cache authentication failures briefly; deleting and recreating the pod forces a fresh authentication attempt with the updated credentials. Assisted-by: Claude Sonnet 4.6 <noreply@anthropic.com>
7f2b2e4 to
6c65328
Compare
openshift-ci-robot
removed
the
verified
|
New changes are detected. LGTM label has been removed. |
|
/test e2e-agent-compact-ipv4-iso-no-registry |
|
@rwsu: 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. |
yuqi-zhang
left a comment
yuqi-zhang
left a comment
There was a problem hiding this comment.
Logically seems fine to me. I think my main concern would be whether this would cause a rapid multi-rollout, since in your description, you have
Re-renders the master MachineConfig with the new htpasswd
Updates the global pull secret with the new credentials
which are separate steps managed by two separate controllers I think? In theory, they happen one after the other, with both of them being disruptionless, but there is a very small chance that another change could also be happening in the meantime, getting you into a state like:
- change one happens, we get a new rendered MC with
/etc/iri-registry/auth/htpasswdchange - during the 5-second render delay, change 2 happens with some other cluster change, causing a new rendered MC with pull secret changes + more, which is not disruptionless
Now the registry has the updated password, but the pull secret can't reach it anymore until the overall change has rolled out. And even if we don't hit this timing, it's still generating 2 rendered MC + 2 rollouts for 1 node most likely, which is better if it can be prevented.
If I understood that correctly, it's not necessarily blocking, but would be good to see if we have an option to make that cleaner.
| _, err := ctrl.client.MachineconfigurationV1alpha1().InternalReleaseImages().Update(context.TODO(), iri, metav1.UpdateOptions{}) | ||
| return err | ||
| } | ||
|
|
There was a problem hiding this comment.
(very minor nit): random removal of newline
4 participants
- What I did
Implement credential rotation that accepts brief registry downtime.
When an admin updates iriAuthSecret.Data["password"], the controller:
rollout is accepted
Key changes:
(Distribution registry re-reads htpasswd on mtime change, no restart needed)
(tests use ExecCmdOnNode via MCD pod to reach api-int:22625 in CI)
- How to verify it
Update the password to trigger the rotation to start:
Verify the /etc/iri-registry/auth/htpasswd has been updated.
Verify iri-registry works new credentials after rollout is complete.
Verify global pull-secret contains the new credentials after rollout is complete.
- Description for the changelog
Support credential rotation in IRI registry.
Summary by CodeRabbit
Release Notes
New Features
Tests