AGENT-1448: Add IRI registry authentication support#10389
AGENT-1448: Add IRI registry authentication support#10389rwsu wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@rwsu: This pull request references AGENT-1448 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 the "4.22.0" version, but no target version was set. 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. |
WalkthroughAdds an in-memory IRI registry credential asset, a Bootkube secret template asset, integration to merge IRI credentials into the bootstrap pull secret, and registration of the new assets in manifests and targets; includes unit tests and test updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Generator as IRIRegistryAuth (in-memory)
participant TemplateAsset as InternalReleaseImageRegistryAuthSecret (template)
participant Bootstrap as Ignition Bootstrap
participant Manifests as Operators Manifests
participant Registry as IRI Registry (api-int.<cluster>.domain:22625)
Generator->>Generator: generate username, password, htpasswd
Generator-->>TemplateAsset: expose credentials (Username, Password, HtpasswdContent)
TemplateAsset->>Manifests: emit secret template file (template vars for htpasswd/password)
Generator->>Bootstrap: provide credentials (username/password) via asset dependency
Bootstrap->>Bootstrap: merge base64(username:password) into pullSecret for Registry
Bootstrap->>Registry: bootstrap flow uses merged pullSecret to authenticate
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.3)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
|
@rwsu: This pull request references AGENT-1448 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/asset/targets/targets.go`:
- Line 63: ManifestTemplates is missing the new bootkube asset for registry
auth; add the bootkube.InternalReleaseImageRegistryAuthSecret asset to the
ManifestTemplates slice (alongside &tls.IRIRegistryAuth{}) so the registry-auth
template is emitted by "openshift-install create manifest-templates"; locate the
ManifestTemplates declaration in targets.go and append
&bootkube.InternalReleaseImageRegistryAuthSecret{} to that slice.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 81c52f52-cdb5-4293-b8d8-6844615db77a
📒 Files selected for processing (7)
data/data/manifests/bootkube/internal-release-image-registry-auth-secret.yaml.templatepkg/asset/ignition/bootstrap/common.gopkg/asset/manifests/operators.gopkg/asset/targets/targets.gopkg/asset/templates/content/bootkube/internal-release-image-registry-auth-secret.gopkg/asset/tls/iriregistryauth.gopkg/asset/tls/iriregistryauth_test.go
|
@rwsu: This pull request references AGENT-1448 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. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/asset/tls/iriregistryauth.go (1)
50-82:⚠️ Potential issue | 🔴 CriticalThese credentials still rotate between installer invocations.
Generate()always creates a fresh password andLoad()never restores the previous one. In the standardcreate manifests→create ignition-configsflow,pkg/asset/manifests/operators.gopersists one password intomanifests/internal-release-image-registry-auth-secret.yaml, whilepkg/asset/ignition/bootstrap/common.golater merges a different password into the pull secret. That leaves the Secret and pull secret out of sync and breaks registry auth. Please reload the existing credential here, or derive it from the persisted manifest, and add a regression test for that two-step workflow.Also applies to: 90-97
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/tls/iriregistryauth.go` around lines 50 - 82, The Generate method in IRIRegistryAuth (Generate) always creates a new password, causing creds to rotate because Load never restores persisted values; change Generate to first attempt to load an existing credential (via the asset Load method or by reading the persisted InternalReleaseImage registry auth secret manifest used by manifests/internal-release-image-registry-auth-secret.yaml) and only generate a new password if none exists, or alternatively derive the password from the persisted manifest, and ensure Load restores a.Password/HtpasswdContent when present; also add a regression test that runs the two-step flow (create manifests -> create ignition-configs) to confirm the same credential is used across both steps.
🤖 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/asset/tls/iriregistryauth.go`:
- Around line 50-82: The Generate method in IRIRegistryAuth (Generate) always
creates a new password, causing creds to rotate because Load never restores
persisted values; change Generate to first attempt to load an existing
credential (via the asset Load method or by reading the persisted
InternalReleaseImage registry auth secret manifest used by
manifests/internal-release-image-registry-auth-secret.yaml) and only generate a
new password if none exists, or alternatively derive the password from the
persisted manifest, and ensure Load restores a.Password/HtpasswdContent when
present; also add a regression test that runs the two-step flow (create
manifests -> create ignition-configs) to confirm the same credential is used
across both steps.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 451bd7d6-2968-41a3-b1a0-3c2b26a37d52
📒 Files selected for processing (7)
data/data/manifests/bootkube/internal-release-image-registry-auth-secret.yaml.templatepkg/asset/ignition/bootstrap/common.gopkg/asset/manifests/operators.gopkg/asset/targets/targets.gopkg/asset/templates/content/bootkube/internal-release-image-registry-auth-secret.gopkg/asset/tls/iriregistryauth.gopkg/asset/tls/iriregistryauth_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/asset/tls/iriregistryauth_test.go
- data/data/manifests/bootkube/internal-release-image-registry-auth-secret.yaml.template
|
@rwsu: This pull request references AGENT-1448 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. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/asset/manifests/operators.go (1)
234-241:⚠️ Potential issue | 🟠 MajorPropagate the merged pull secret into the bootkube pull-secret manifest too.
This branch adds the registry-auth Secret, but the bootkube template data is still built from the unmodified
install-configpull secret. The generatedopenshift-config-secret-pull-secretmanifest therefore goes out without theapi-int.<clusterDomain>:22625auth entry, so the IRI credentials never reach the cluster-wide pull secret. Please merge the IRI auth before populatingPullSecretBase64as well, ideally via a shared helper instead of keeping the merge logic only inpkg/asset/ignition/bootstrap/common.go.Also applies to: 270-291
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/manifests/operators.go` around lines 234 - 241, The bootkube pull-secret manifest is still built from the original install-config pull secret so IRI credentials never make it into the cluster pull secret; update operators.go to merge the InternalReleaseImage registry auth into the install-config pull secret before creating the bootkube template data (the value used for PullSecretBase64) by reusing the same merge helper used in pkg/asset/ignition/bootstrap/common.go (or extract that merge into a shared helper), and ensure this merged secret is used when producing the bootkube/openshift-config-secret-pull-secret manifest (references: InternalReleaseImage, appendIRIRegistryAuth, appendIRIcerts, PullSecretBase64).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/asset/manifests/operators.go`:
- Around line 234-241: The bootkube pull-secret manifest is still built from the
original install-config pull secret so IRI credentials never make it into the
cluster pull secret; update operators.go to merge the InternalReleaseImage
registry auth into the install-config pull secret before creating the bootkube
template data (the value used for PullSecretBase64) by reusing the same merge
helper used in pkg/asset/ignition/bootstrap/common.go (or extract that merge
into a shared helper), and ensure this merged secret is used when producing the
bootkube/openshift-config-secret-pull-secret manifest (references:
InternalReleaseImage, appendIRIRegistryAuth, appendIRIcerts, PullSecretBase64).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 88a78d10-6306-4e0b-80aa-ebdb801f3997
📒 Files selected for processing (8)
data/data/manifests/bootkube/internal-release-image-registry-auth-secret.yaml.templatepkg/asset/ignition/bootstrap/common.gopkg/asset/manifests/operators.gopkg/asset/store/assetcreate_test.gopkg/asset/targets/targets.gopkg/asset/templates/content/bootkube/internal-release-image-registry-auth-secret.gopkg/asset/tls/iriregistryauth.gopkg/asset/tls/iriregistryauth_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/asset/tls/iriregistryauth_test.go
- data/data/manifests/bootkube/internal-release-image-registry-auth-secret.yaml.template
Add authentication credentials for the Internal Release Image (IRI) registry. New assets: - IRIRegistryAuth: In-memory-only asset that generates a random password and bcrypt htpasswd for the IRI registry. Not written to disk to avoid interfering with assisted-service's auth/ directory cleanup. - InternalReleaseImageRegistryAuthSecret: Bootkube manifest template that creates the openshift-machine-config-operator/internal-release- image-registry-auth Secret containing the htpasswd and password. Pull secret integration: - Merge IRI registry credentials (openshift:<password>) into the global pull secret during bootstrap ignition generation so that kubelet/CRI-O can authenticate to the IRI registry on master nodes at api-int.<baseDomain>:22625. Assisted-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@rwsu: This pull request references AGENT-1448 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. |
There was a problem hiding this comment.
🧹 Nitpick comments (4)
pkg/asset/ignition/bootstrap/common.go (2)
719-719: Remove unnecessary entry -IRIRegistryAuth.Files()returns empty.Since
IRIRegistryAuthis an in-memory-only asset that returns an emptyFiles()slice, including it inaddParentFileshas no effect. The asset is already fetched ingetTemplateData(line 389) where its credentials are used. This entry adds confusion about what files are being embedded.♻️ Suggested removal
&tls.MCSCertKey{}, &tls.IRICertKey{}, - &tls.IRIRegistryAuth{}, &tls.ServiceAccountKeyPair{},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/ignition/bootstrap/common.go` at line 719, Remove the unnecessary &tls.IRIRegistryAuth{} entry from the addParentFiles call: IRIRegistryAuth.Files() returns an empty slice and the asset is already accessed in getTemplateData for credentials, so deleting this entry clarifies intent and avoids embedding a no-op asset; update any nearby comments if present that imply this asset is being embedded and ensure references to IRIRegistryAuth remain only where credentials are read (getTemplateData).
426-449: Consider handling missingauthsfield more gracefully.If the pull secret has a null or missing
authsfield (edge case with malformed input), the function returns an error. Consider initializing an emptyauthsmap instead, which would make the merge more robust.♻️ Suggested defensive handling
auths, ok := pullSecretMap["auths"].(map[string]interface{}) if !ok { - return "", fmt.Errorf("pull secret missing 'auths' field") + auths = make(map[string]interface{}) + pullSecretMap["auths"] = auths }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/ignition/bootstrap/common.go` around lines 426 - 449, The function mergeIRIAuthIntoPullSecret currently errors if pullSecretMap["auths"] is missing or not a map; instead, detect when "auths" is absent or nil and initialize auths as an empty map[string]interface{} before inserting the registry entry (operate on the local variable auths and assign it back to pullSecretMap["auths"] if you create it), so the function accepts pull secrets without an existing auths block while preserving existing auths and still returning the merged JSON or errors on marshal/unmarshal failures.pkg/asset/tls/iriregistryauth_test.go (1)
159-162: Consider usingk8s.io/utils/ptr.Tofor consistency.The codebase already imports
k8s.io/utils/ptrin other files (e.g.,common.gousesptr.To). Using the standard utility would align with the rest of the codebase.♻️ Suggested refactor
+import "k8s.io/utils/ptr" + // In test setup: -Replicas: pointer(int64(3)), +Replicas: ptr.To(int64(3)), - -// pointer returns a pointer to the given value -func pointer(i int64) *int64 { - return &i -}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/tls/iriregistryauth_test.go` around lines 159 - 162, The test defines a local helper function pointer(i int64) *int64; replace usages of this helper with the standard k8s utility ptr.To to be consistent with the codebase: remove the pointer function, add/import "k8s.io/utils/ptr" if not already imported in pkg/asset/tls/iriregistryauth_test.go, and change calls like pointer(x) to ptr.To(x) (refers to the pointer helper function name "pointer" and test code usages).pkg/asset/manifests/operators.go (1)
239-242: Implicit coupling between IRI assets - consider adding a defensive check.The guard
len(iri.FileList) > 0ensuresInternalReleaseImageexists, butappendIRIRegistryAuthassumesiriAuthSecret.Files()is non-empty (line 276). While this holds becauseInternalReleaseImageRegistryAuthSecret.Generateuses the same guard, the coupling is implicit.Consider adding a defensive check for robustness:
♻️ Optional defensive check
if len(iri.FileList) > 0 { files = append(files, appendIRIcerts(dependencies)) - files = append(files, appendIRIRegistryAuth(dependencies)) + if authFile := appendIRIRegistryAuth(dependencies); authFile != nil { + files = append(files, authFile) + } }And in
appendIRIRegistryAuth:func appendIRIRegistryAuth(dependencies asset.Parents) *asset.File { iriAuth := &tls.IRIRegistryAuth{} iriAuthSecret := &bootkube.InternalReleaseImageRegistryAuthSecret{} dependencies.Get(iriAuth, iriAuthSecret) + if len(iriAuthSecret.Files()) == 0 { + return nil + } f := iriAuthSecret.Files()[0]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/manifests/operators.go` around lines 239 - 242, The code implicitly assumes that when len(iri.FileList) > 0 (i.e. an InternalReleaseImage exists) then the registry auth secret also has files, creating coupling between the caller and appendIRIRegistryAuth; update the caller and appendIRIRegistryAuth to be defensive: in the caller (where appendIRIcerts and appendIRIRegistryAuth are invoked) only call appendIRIRegistryAuth if iriAuthSecret.Files() (or the equivalent InternalReleaseImageRegistryAuthSecret) is non-empty, and inside appendIRIRegistryAuth add a guard that returns early if the provided secret/files list is empty; reference appendIRIRegistryAuth, appendIRIcerts, InternalReleaseImage, iriAuthSecret.Files(), and InternalReleaseImageRegistryAuthSecret.Generate to locate the relevant code paths to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/asset/ignition/bootstrap/common.go`:
- Line 719: Remove the unnecessary &tls.IRIRegistryAuth{} entry from the
addParentFiles call: IRIRegistryAuth.Files() returns an empty slice and the
asset is already accessed in getTemplateData for credentials, so deleting this
entry clarifies intent and avoids embedding a no-op asset; update any nearby
comments if present that imply this asset is being embedded and ensure
references to IRIRegistryAuth remain only where credentials are read
(getTemplateData).
- Around line 426-449: The function mergeIRIAuthIntoPullSecret currently errors
if pullSecretMap["auths"] is missing or not a map; instead, detect when "auths"
is absent or nil and initialize auths as an empty map[string]interface{} before
inserting the registry entry (operate on the local variable auths and assign it
back to pullSecretMap["auths"] if you create it), so the function accepts pull
secrets without an existing auths block while preserving existing auths and
still returning the merged JSON or errors on marshal/unmarshal failures.
In `@pkg/asset/manifests/operators.go`:
- Around line 239-242: The code implicitly assumes that when len(iri.FileList) >
0 (i.e. an InternalReleaseImage exists) then the registry auth secret also has
files, creating coupling between the caller and appendIRIRegistryAuth; update
the caller and appendIRIRegistryAuth to be defensive: in the caller (where
appendIRIcerts and appendIRIRegistryAuth are invoked) only call
appendIRIRegistryAuth if iriAuthSecret.Files() (or the equivalent
InternalReleaseImageRegistryAuthSecret) is non-empty, and inside
appendIRIRegistryAuth add a guard that returns early if the provided
secret/files list is empty; reference appendIRIRegistryAuth, appendIRIcerts,
InternalReleaseImage, iriAuthSecret.Files(), and
InternalReleaseImageRegistryAuthSecret.Generate to locate the relevant code
paths to change.
In `@pkg/asset/tls/iriregistryauth_test.go`:
- Around line 159-162: The test defines a local helper function pointer(i int64)
*int64; replace usages of this helper with the standard k8s utility ptr.To to be
consistent with the codebase: remove the pointer function, add/import
"k8s.io/utils/ptr" if not already imported in
pkg/asset/tls/iriregistryauth_test.go, and change calls like pointer(x) to
ptr.To(x) (refers to the pointer helper function name "pointer" and test code
usages).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 744b58fe-0a6a-4ce5-80dc-431e5fefe435
📒 Files selected for processing (8)
data/data/manifests/bootkube/internal-release-image-registry-auth-secret.yaml.templatepkg/asset/ignition/bootstrap/common.gopkg/asset/manifests/operators.gopkg/asset/store/assetcreate_test.gopkg/asset/targets/targets.gopkg/asset/templates/content/bootkube/internal-release-image-registry-auth-secret.gopkg/asset/tls/iriregistryauth.gopkg/asset/tls/iriregistryauth_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/asset/store/assetcreate_test.go
|
@rwsu: The following tests failed, say
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
Add authentication credentials for the Internal Release Image (IRI) registry.
New assets:
Pull secret integration:
Assisted-By: Claude Opus 4.6 noreply@anthropic.com
Summary by CodeRabbit
New Features
Tests