extract OCI secret from auth.json for flashing#150
Merged
Conversation
📝 WalkthroughWalkthroughResolves OCI registry credentials for flash image pulls (via --registry-auth-file), attaches resolved RegistryCredentials to FlashRequest, creates Jumpstarter client and optional OCI auth secrets, wires them into Tekton TaskRuns with owner references, and adds registry host normalization utilities plus unit tests. Changes
Sequence DiagramsequenceDiagram
participant User as User/CLI
participant Flash as Flash Command
participant RegAuth as Registry Auth Resolver
participant API as Build API Server
participant K8s as Kubernetes API
participant Tekton as Tekton
User->>Flash: flash --registry-auth-file=path
Flash->>RegAuth: ResolveRegistryCredentials(imageRef, authFile)
RegAuth-->>Flash: RegistryCredentials (optional)
Flash->>API: CreateFlash(FlashRequest + RegistryCredentials)
API->>K8s: createFlashClientConfigSecret()
K8s-->>API: jumpstarter-client secret name
API->>K8s: createFlashOCIAuthSecret(RegistryCredentials) [optional]
K8s-->>API: flash-oci-auth secret name
API->>K8s: Build Tekton workspace bindings (jumpstarter-client + optional flash-oci-auth)
API->>Tekton: Create TaskRun with workspaces
Tekton->>K8s: Tekton mounts secrets as volumes
Note over K8s: Secrets receive OwnerReferences to TaskRun for cascading deletion
API-->>Flash: FlashResponse
Flash-->>User: Task initiated
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/buildapi/flash_helpers.go`:
- Around line 76-87: When RegistryCredentials.Enabled is true but
extractOCICredentials(creds) yields empty ociUsername/ociPassword, treat that as
an error instead of silently returning no secret: in createFlashOCIAuthSecret
detect the case where creds.Enabled && (ociUsername=="" || ociPassword=="") and
return an httpError (e.g., BadRequest with a message like "invalid registry
credentials: missing username or password") so the caller fails fast; apply the
same change to the other similar helper that handles image pull secrets (the
second block around lines 122-136) to ensure both functions validate that
Enabled implies usable username/password and return a request error when they
cannot be resolved.
- Around line 151-160: The current loop in the registry lookup uses substring
matching (strings.Contains) against cfg.Auths keys with registryURL, which can
produce false matches and nondeterministic results; replace this by normalizing
both the auth key and registryURL (e.g., implement
normalizeDockerConfigRegistry) to strip "http(s)://" and trailing "/v1/" or "/"
and then compare for equality, and only when equal use
decodeAuthField(entry.Auth) to return user, pass; update the logic in the block
that references registryURL, cfg.Auths, and decodeAuthField to use the new
normalize function for deterministic exact-match credential selection.
In `@internal/controller/imagebuild/controller.go`:
- Around line 653-657: The helper extractFlashCredentials currently returns the
first decodable entry from cfg.Auths which can pick the wrong registry when
multiple credentials exist; change extractFlashCredentials to accept the flash
registry host (pass flashImageRef's registry host from the caller) and iterate
cfg.Auths preferring entries where normalizeDockerConfigRegistry(key) ==
normalizeDockerConfigRegistry(registryURL) before decoding/returning
username/password (optionally fall back to the first valid entry only if no
exact match is found); update callers that invoke extractFlashCredentials (e.g.,
where registrySecret is used) to pass the flash registry host.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f3a6537a-5131-439a-b3d3-9ff55898b4c2
📒 Files selected for processing (7)
cmd/caib/flashcmd/flash.gocmd/caib/image/image.gocmd/caib/runtime_wiring.gointernal/buildapi/flash_helpers.gointernal/buildapi/server.gointernal/buildapi/types.gointernal/controller/imagebuild/controller.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/buildapi/flash_helpers.go`:
- Around line 109-118: The Create call swallows IsAlreadyExists and returns a
nil `created`, which leads to downstream nil derefs; update the block in the OCI
secret creation path (the `clientset.CoreV1().Secrets(...).Create` handling
around `created, createErr`) so that when `k8serrors.IsAlreadyExists(createErr)`
you perform a `Get` for `secretName` and return that existing Secret (or
propagate a Get error as an httpError) instead of returning nil; ensure the
function ultimately returns `secretName` and a non-nil Secret object to callers
that set owner references.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f13789fa-7e41-4923-b3a0-0e897374fe0f
📒 Files selected for processing (5)
cmd/caib/registryauth/loader.gointernal/buildapi/flash_helpers.gointernal/common/registryutil/registryutil.gointernal/common/registryutil/registryutil_test.gointernal/controller/imagebuild/controller.go
Comment on lines
+109
to
+118
| created, createErr := clientset.CoreV1().Secrets(namespace).Create(ctx, ociSecret, metav1.CreateOptions{}) | ||
| if createErr != nil { | ||
| if !k8serrors.IsAlreadyExists(createErr) { | ||
| return "", nil, &httpError{ | ||
| code: http.StatusInternalServerError, | ||
| message: fmt.Sprintf("failed to create flash OCI auth secret: %v", createErr), | ||
| } | ||
| } | ||
| } | ||
| return secretName, created, nil |
There was a problem hiding this comment.
Return nil secret when AlreadyExists causes silent handling.
When Create returns IsAlreadyExists, the function swallows the error but returns created which will be nil. The caller in server.go uses the returned secret for owner reference setup, which could cause a nil pointer dereference or skip setting ownership.
🐛 Proposed fix
created, createErr := clientset.CoreV1().Secrets(namespace).Create(ctx, ociSecret, metav1.CreateOptions{})
if createErr != nil {
if !k8serrors.IsAlreadyExists(createErr) {
return "", nil, &httpError{
code: http.StatusInternalServerError,
message: fmt.Sprintf("failed to create flash OCI auth secret: %v", createErr),
}
}
+ // Secret already exists - fetch it for owner reference setup
+ existing, getErr := clientset.CoreV1().Secrets(namespace).Get(ctx, secretName, metav1.GetOptions{})
+ if getErr != nil {
+ return "", nil, &httpError{
+ code: http.StatusInternalServerError,
+ message: fmt.Sprintf("failed to get existing flash OCI auth secret: %v", getErr),
+ }
+ }
+ return secretName, existing, nil
}
return secretName, created, nil📝 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.
Suggested change
| created, createErr := clientset.CoreV1().Secrets(namespace).Create(ctx, ociSecret, metav1.CreateOptions{}) | |
| if createErr != nil { | |
| if !k8serrors.IsAlreadyExists(createErr) { | |
| return "", nil, &httpError{ | |
| code: http.StatusInternalServerError, | |
| message: fmt.Sprintf("failed to create flash OCI auth secret: %v", createErr), | |
| } | |
| } | |
| } | |
| return secretName, created, nil | |
| created, createErr := clientset.CoreV1().Secrets(namespace).Create(ctx, ociSecret, metav1.CreateOptions{}) | |
| if createErr != nil { | |
| if !k8serrors.IsAlreadyExists(createErr) { | |
| return "", nil, &httpError{ | |
| code: http.StatusInternalServerError, | |
| message: fmt.Sprintf("failed to create flash OCI auth secret: %v", createErr), | |
| } | |
| } | |
| // Secret already exists - fetch it for owner reference setup | |
| existing, getErr := clientset.CoreV1().Secrets(namespace).Get(ctx, secretName, metav1.GetOptions{}) | |
| if getErr != nil { | |
| return "", nil, &httpError{ | |
| code: http.StatusInternalServerError, | |
| message: fmt.Sprintf("failed to get existing flash OCI auth secret: %v", getErr), | |
| } | |
| } | |
| return secretName, existing, nil | |
| } | |
| return secretName, created, nil |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/buildapi/flash_helpers.go` around lines 109 - 118, The Create call
swallows IsAlreadyExists and returns a nil `created`, which leads to downstream
nil derefs; update the block in the OCI secret creation path (the
`clientset.CoreV1().Secrets(...).Create` handling around `created, createErr`)
so that when `k8serrors.IsAlreadyExists(createErr)` you perform a `Get` for
`secretName` and return that existing Secret (or propagate a Get error as an
httpError) instead of returning nil; ensure the function ultimately returns
`secretName` and a non-nil Secret object to callers that set owner references.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/buildapi/flash_helpers.go (1)
168-173: Nondeterministic fallback when multiple auth entries exist.Go's map iteration order is randomized, so if the docker config contains multiple registry entries and no
registryURLis provided (or none match), different credentials could be returned on different runs. If this is intentional, consider documenting it; otherwise, consider a deterministic selection strategy (e.g., alphabetically first key).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/buildapi/flash_helpers.go` around lines 168 - 173, The fallback loop over cfg.Auths uses Go map iteration which is nondeterministic; update the logic in the code that calls decodeAuthField (the loop iterating cfg.Auths) to pick credentials deterministically by gathering the map keys, sorting them (e.g., sort.Strings(keys)), then iterating keys in order and returning the first successful decodeAuthField(entry.Auth) result; reference cfg.Auths and decodeAuthField to locate the change and ensure behavior remains the same when only one entry exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/buildapi/flash_helpers.go`:
- Around line 168-173: The fallback loop over cfg.Auths uses Go map iteration
which is nondeterministic; update the logic in the code that calls
decodeAuthField (the loop iterating cfg.Auths) to pick credentials
deterministically by gathering the map keys, sorting them (e.g.,
sort.Strings(keys)), then iterating keys in order and returning the first
successful decodeAuthField(entry.Auth) result; reference cfg.Auths and
decodeAuthField to locate the change and ensure behavior remains the same when
only one entry exists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 826bcc6e-d759-45db-b2a7-38404399afa9
📒 Files selected for processing (1)
internal/buildapi/flash_helpers.go
When creds are not explictly set. Since Jumpstarter does not support auth.json (yet), we need to extract the username/password for the falshing task Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: claude-sonnet-4.6
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: claude-sonnet-4.6
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
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)
internal/controller/imagebuild/controller.go (1)
659-683:⚠️ Potential issue | 🟠 MajorRefresh the copied
flash-oci-authsecret on retry.Line 681 ignores
AlreadyExists, but this copied secret is not cleaned up bycleanupTransientSecrets. If the source registry secret rotates or the first extraction was wrong, later reconciles will keep mounting stale flash credentials.Possible fix
- if err := r.Create(ctx, ociSecret); err != nil && !errors.IsAlreadyExists(err) { - return fmt.Errorf("failed to create flash OCI auth secret from registry credentials: %w", err) - } + if err := r.Create(ctx, ociSecret); err != nil { + if !errors.IsAlreadyExists(err) { + return fmt.Errorf("failed to create flash OCI auth secret from registry credentials: %w", err) + } + existing := &corev1.Secret{} + if getErr := r.Get(ctx, client.ObjectKey{ + Namespace: imageBuild.Namespace, + Name: flashOCIAuthSecretName, + }, existing); getErr != nil { + return fmt.Errorf("failed to get existing flash OCI auth secret: %w", getErr) + } + existing.Labels = ociSecret.Labels + existing.OwnerReferences = ociSecret.OwnerReferences + existing.Type = ociSecret.Type + existing.Data = ociSecret.Data + if updateErr := r.Update(ctx, existing); updateErr != nil { + return fmt.Errorf("failed to refresh flash OCI auth secret: %w", updateErr) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/imagebuild/controller.go` around lines 659 - 683, The code currently ignores AlreadyExists when creating the transient flash OCI auth secret (flashOCIAuthSecretName) so stale credentials persist; instead, on errors.IsAlreadyExists(err) fetch the existing Secret (r.Get) and refresh its Data (username/password), OwnerReferences and Labels as needed, then call r.Update (or r.Patch) to replace the secret contents so retries overwrite rotated or corrected credentials; ensure cleanupTransientSecrets still recognizes the secret labels/owner refs.
♻️ Duplicate comments (1)
internal/buildapi/flash_helpers.go (1)
156-174:⚠️ Potential issue | 🟠 MajorDo not fall back to an arbitrary auth.json entry when the target registry is known.
If
registryURLis set and no host match is found, this code still picks the first decodable entry fromcfg.Auths. With multi-registry auth files that choice is map-order dependent, and the flash pull can end up sending unrelated credentials to the target registry.Possible fix
// Try matching the target registry first if registryURL != "" { for key, entry := range cfg.Auths { if !registryutil.RegistryHostMatches(key, registryURL) { continue } if user, pass, ok := decodeAuthField(entry.Auth); ok { return user, pass, nil } } + return "", "", fmt.Errorf("no credentials found for registry %q", registryURL) } // Fall back to first valid entry for _, entry := range cfg.Auths { if user, pass, ok := decodeAuthField(entry.Auth); ok {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/buildapi/flash_helpers.go` around lines 156 - 174, When registryURL is provided, do not fall back to an arbitrary entry from cfg.Auths; instead return an error if no matching host is found. In the code around the registryURL check that iterates cfg.Auths and calls registryutil.RegistryHostMatches (and uses decodeAuthField), change the logic so that if registryURL != "" and the loop completes without returning credentials you return an explicit error (e.g., "no credentials found for registry <registryURL>") rather than proceeding to the "Fall back to first valid entry" loop; keep the fallback loop only for the case where registryURL == "".
🧹 Nitpick comments (1)
internal/controller/imagebuild/controller.go (1)
1952-2014: Share the docker-config parsing withinternal/buildapi/flash_helpers.go.This helper now duplicates the host matching and auth decoding logic from
decodeDockerConfigAuth/decodeAuthFieldininternal/buildapi/flash_helpers.go. Keeping both copies in sync is brittle, especially around registry matching and fallback behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/imagebuild/controller.go` around lines 1952 - 2014, The docker-config parsing and auth-decoding in extractFlashCredentials and decodeAuthEntry duplicate logic present in internal/buildapi/flash_helpers.go (decodeDockerConfigAuth / decodeAuthField); replace the local parsing/decoding with calls to those shared helpers: remove decodeAuthEntry and the internal cfg struct/unmarshal/matching code and instead invoke the flash_helpers decodeDockerConfigAuth (or equivalent) to parse dockerConfig and get the preferred credentials (with host matching to registryURL) and fallback behavior, update imports to reference the buildapi package, and preserve the current nil-on-error semantics so extractFlashCredentials returns the same []byte results when no valid credentials are found.
🤖 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 `@internal/controller/imagebuild/controller.go`:
- Around line 659-683: The code currently ignores AlreadyExists when creating
the transient flash OCI auth secret (flashOCIAuthSecretName) so stale
credentials persist; instead, on errors.IsAlreadyExists(err) fetch the existing
Secret (r.Get) and refresh its Data (username/password), OwnerReferences and
Labels as needed, then call r.Update (or r.Patch) to replace the secret contents
so retries overwrite rotated or corrected credentials; ensure
cleanupTransientSecrets still recognizes the secret labels/owner refs.
---
Duplicate comments:
In `@internal/buildapi/flash_helpers.go`:
- Around line 156-174: When registryURL is provided, do not fall back to an
arbitrary entry from cfg.Auths; instead return an error if no matching host is
found. In the code around the registryURL check that iterates cfg.Auths and
calls registryutil.RegistryHostMatches (and uses decodeAuthField), change the
logic so that if registryURL != "" and the loop completes without returning
credentials you return an explicit error (e.g., "no credentials found for
registry <registryURL>") rather than proceeding to the "Fall back to first valid
entry" loop; keep the fallback loop only for the case where registryURL == "".
---
Nitpick comments:
In `@internal/controller/imagebuild/controller.go`:
- Around line 1952-2014: The docker-config parsing and auth-decoding in
extractFlashCredentials and decodeAuthEntry duplicate logic present in
internal/buildapi/flash_helpers.go (decodeDockerConfigAuth / decodeAuthField);
replace the local parsing/decoding with calls to those shared helpers: remove
decodeAuthEntry and the internal cfg struct/unmarshal/matching code and instead
invoke the flash_helpers decodeDockerConfigAuth (or equivalent) to parse
dockerConfig and get the preferred credentials (with host matching to
registryURL) and fallback behavior, update imports to reference the buildapi
package, and preserve the current nil-on-error semantics so
extractFlashCredentials returns the same []byte results when no valid
credentials are found.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1b2bccf8-4ed1-4d68-9455-ecdb4ee1f707
📒 Files selected for processing (10)
cmd/caib/flashcmd/flash.gocmd/caib/image/image.gocmd/caib/registryauth/loader.gocmd/caib/runtime_wiring.gointernal/buildapi/flash_helpers.gointernal/buildapi/server.gointernal/buildapi/types.gointernal/common/registryutil/registryutil.gointernal/common/registryutil/registryutil_test.gointernal/controller/imagebuild/controller.go
🚧 Files skipped from review as they are similar to previous changes (6)
- cmd/caib/registryauth/loader.go
- cmd/caib/image/image.go
- internal/buildapi/types.go
- internal/common/registryutil/registryutil.go
- internal/common/registryutil/registryutil_test.go
- cmd/caib/runtime_wiring.go
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: claude-sonnet-4.6
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 `@internal/buildapi/flash_helpers.go`:
- Around line 28-34: The code currently skips target mapping when
exporterSelector is non-empty, which prevents inheriting mapping.FlashCmd;
change the guard to always perform the TargetMappings lookup when req.Target !=
"" and operatorConfig.Spec.Jumpstarter != nil, then conditionally set
exporterSelector only if exporterSelector == "" and always set flashCmd when
flashCmd == "" using mapping.FlashCmd (ensure you handle missing map entries by
checking the ok from
operatorConfig.Spec.Jumpstarter.TargetMappings[req.Target]). Use the symbols
req.Target, exporterSelector, flashCmd,
operatorConfig.Spec.Jumpstarter.TargetMappings, and mapping.FlashCmd to locate
and update the logic.
- Around line 77-88: createFlashOCIAuthSecret currently only matches
docker-config auth entries against creds.RegistryURL, so if creds.RegistryURL is
empty but req.ImageRef contains the registry host the lookup fails; modify
createFlashOCIAuthSecret to accept (or compute locally) the imageRef and, when
creds.RegistryURL is empty, parse the registry host from the image reference
(use the same parsing logic you have elsewhere) and use that host to match
auth.json entries returned by extractOCICredentials; update callers (notably the
invocation in internal/buildapi/server.go) to pass req.ImageRef into
createFlashOCIAuthSecret so the function can derive the registry host when
RegistryCredentials.RegistryURL is not provided.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 88dbbd5a-72e1-4d77-8dff-fad0a35852bd
📒 Files selected for processing (2)
internal/buildapi/flash_helpers.gointernal/controller/imagebuild/controller.go
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: claude-sonnet-4.6
bkhizgiy
reviewed
Mar 12, 2026
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
bkhizgiy
approved these changes
Mar 12, 2026
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/controller/imagebuild/controller.go (1)
1979-2035: Share docker-config parsing with the build-API helper.This logic now overlaps with
internal/buildapi/flash_helpers.go, and the two paths already differ in how they surface parse/match failures. Pulling the auth.json decoding into one shared helper will keep standalone flash and build-after-flash behavior in sync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/imagebuild/controller.go` around lines 1979 - 2035, Duplicate docker-config parsing/auth-decoding exists between extractFlashCredentials and internal/buildapi/flash_helpers.go; refactor by extracting the JSON parsing and auth decoding into a single shared helper (e.g., New function in a common package or move decodeAuthEntry + the cfg struct/unmarshal logic into a new function like ParseDockerConfigAuth) and replace extractFlashCredentials's local parsing: have extractFlashCredentials call the shared ParseDockerConfigAuth(registryURL, dockerConfig, log) which returns username/password or error; update internal/buildapi/flash_helpers.go to call the same shared helper and remove its duplicate parsing code, keeping callers' logging/return behavior consistent by surfacing parse/match failures via returned error values rather than duplicated logging.internal/buildapi/server.go (1)
2981-2999: Consider adding a nil check forcreatedSecret.The code correctly handles the optional
createdOCIAuthSecretwith a nil check (line 2994), butcreatedSecreton line 2990 is accessed directly. IfcreateFlashClientConfigSecretcan return(secretName, nil, nil)on success, this would panic.🛡️ Suggested defensive check
ownerRef := []metav1.OwnerReference{ { APIVersion: "tekton.dev/v1", Kind: "TaskRun", Name: taskRun.Name, UID: taskRun.UID, }, } - createdSecret.OwnerReferences = ownerRef - if _, err := clientset.CoreV1().Secrets(namespace).Update(ctx, createdSecret, metav1.UpdateOptions{}); err != nil { - log.Printf("WARNING: failed to set owner reference on secret %s: %v", secretName, err) - } + if createdSecret != nil { + createdSecret.OwnerReferences = ownerRef + if _, err := clientset.CoreV1().Secrets(namespace).Update(ctx, createdSecret, metav1.UpdateOptions{}); err != nil { + log.Printf("WARNING: failed to set owner reference on secret %s: %v", secretName, err) + } + }Based on learnings: "registry secrets managed by
setContainerBuildSecretOwnerRefare newly created and don't have pre-existing owner references, so directly setting the OwnerReferences slice is safe." The same principle applies here for the direct assignment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/buildapi/server.go` around lines 2981 - 2999, The code assigns OwnerReferences to createdSecret without verifying it's non-nil, which can panic if createFlashClientConfigSecret returns nil; update setContainerBuildSecretOwnerRef to check createdSecret != nil before setting createdSecret.OwnerReferences and calling clientset.CoreV1().Secrets(...).Update, mirroring the existing nil-check pattern used for createdOCIAuthSecret (and use flashOCIAuthSecretName/secretName in the warning log when skipping or on update failure).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/v1alpha1/imagebuild_types.go`:
- Around line 74-77: You added the new ExporterSelector string field on the
ImageBuild type but didn’t regenerate the deepcopy methods and CRD manifests;
run the project generation to produce updated artifacts (e.g., regenerate
zz_generated.deepcopy.go and updated CRD YAMLs) by running the repository’s
generation command (make generate manifests) so the new field is present in
DeepCopy code and the flashed CRD, then include and commit the generated files
to the PR so the API server won’t prune flash.exporterSelector.
In `@internal/controller/imagebuild/controller.go`:
- Around line 645-656: The current logic unconditionally creates/overwrites and
later deletes a secret named from imageBuild.Name (ociSecret) which can clobber
unrelated secrets; instead ensure the secret is uniquely tied to this ImageBuild
and only update/delete if the controller owns it: when creating, include a
build-unique identifier (e.g., imageBuild.UID or a generated suffix) in the
secret name or set a controller-owned label/ownerReference on ociSecret, and
when errors.IsAlreadyExists(err) inspect the existing secret (the variable
existing) for that ownerReference/label before modifying it — if not owned,
return an error/refuse to update or generate a new unique secret name; apply the
same ownership check before deletion in the cleanup path that references the
same secret.
---
Nitpick comments:
In `@internal/buildapi/server.go`:
- Around line 2981-2999: The code assigns OwnerReferences to createdSecret
without verifying it's non-nil, which can panic if createFlashClientConfigSecret
returns nil; update setContainerBuildSecretOwnerRef to check createdSecret !=
nil before setting createdSecret.OwnerReferences and calling
clientset.CoreV1().Secrets(...).Update, mirroring the existing nil-check pattern
used for createdOCIAuthSecret (and use flashOCIAuthSecretName/secretName in the
warning log when skipping or on update failure).
In `@internal/controller/imagebuild/controller.go`:
- Around line 1979-2035: Duplicate docker-config parsing/auth-decoding exists
between extractFlashCredentials and internal/buildapi/flash_helpers.go; refactor
by extracting the JSON parsing and auth decoding into a single shared helper
(e.g., New function in a common package or move decodeAuthEntry + the cfg
struct/unmarshal logic into a new function like ParseDockerConfigAuth) and
replace extractFlashCredentials's local parsing: have extractFlashCredentials
call the shared ParseDockerConfigAuth(registryURL, dockerConfig, log) which
returns username/password or error; update internal/buildapi/flash_helpers.go to
call the same shared helper and remove its duplicate parsing code, keeping
callers' logging/return behavior consistent by surfacing parse/match failures
via returned error values rather than duplicated logging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e0ba1764-8613-4b1c-bb06-ab91b417054c
⛔ Files ignored due to path filters (1)
config/crd/bases/automotive.sdv.cloud.redhat.com_imagebuilds.yamlis excluded by!config/crd/bases/**
📒 Files selected for processing (8)
api/v1alpha1/imagebuild_types.gocmd/caib/buildcmd/build.gocmd/caib/image/image.gocmd/caib/runtime_wiring.gointernal/buildapi/flash_helpers.gointernal/buildapi/server.gointernal/buildapi/types.gointernal/controller/imagebuild/controller.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/buildapi/types.go
- cmd/caib/runtime_wiring.go
Comment on lines
+645
to
656
| _, err = clientset.CoreV1().Secrets(imageBuild.Namespace).Create(ctx, ociSecret, metav1.CreateOptions{}) | ||
| if errors.IsAlreadyExists(err) { | ||
| existing, getErr := clientset.CoreV1().Secrets(imageBuild.Namespace).Get(ctx, ociSecret.Name, metav1.GetOptions{}) | ||
| if getErr != nil { | ||
| return fmt.Errorf("failed to get existing flash OCI auth secret: %w", getErr) | ||
| } | ||
| existing.Data = ociSecret.Data | ||
| _, err = clientset.CoreV1().Secrets(imageBuild.Namespace).Update(ctx, existing, metav1.UpdateOptions{}) | ||
| } | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create/update flash OCI auth secret: %w", err) | ||
| } |
There was a problem hiding this comment.
Do not reuse or delete a pre-existing *-flash-oci-auth secret by name alone.
Both AlreadyExists branches overwrite whatever secret already has that deterministic name, and cleanup later deletes the same name unconditionally. Since the name is derived only from imageBuild.Name, this can clobber or remove a secret that is not owned by the current ImageBuild. Please either make the name build-unique or refuse to update/delete an existing secret unless it is controller-owned by this object.
Also applies to: 668-708, 1430-1431
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/controller/imagebuild/controller.go` around lines 645 - 656, The
current logic unconditionally creates/overwrites and later deletes a secret
named from imageBuild.Name (ociSecret) which can clobber unrelated secrets;
instead ensure the secret is uniquely tied to this ImageBuild and only
update/delete if the controller owns it: when creating, include a build-unique
identifier (e.g., imageBuild.UID or a generated suffix) in the secret name or
set a controller-owned label/ownerReference on ociSecret, and when
errors.IsAlreadyExists(err) inspect the existing secret (the variable existing)
for that ownerReference/label before modifying it — if not owned, return an
error/refuse to update or generate a new unique secret name; apply the same
ownership check before deletion in the cleanup path that references the same
secret.
Merged
10 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When creds are not explictly set.
Since Jumpstarter does not support auth.json (yet), we need to extract the username/password for the falshing task
Summary by CodeRabbit
New Features
Refactor
Tests