Introduce seal/reseal-related operations#91
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:
📝 WalkthroughWalkthroughAdds ImageSealed API, CRDs, controller, Tekton sealed tasks and embedded script, Build API endpoints and client methods for sealed workflows, CLI sealed subcommands (local/container and API modes), deepcopy code, tests, RBAC, kustomization, and operator CSV registration. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Sealed CLI
participant Workspace as Workspace FS
participant Container as Container Runtime
participant AIB as AIB
rect rgba(200,200,255,0.5)
CLI->>Workspace: Resolve and validate workspace, ensure inputs/outputs
CLI->>Container: Mount workspace and execute AIB with operation
Container->>AIB: Run sealed operation (prepare/reseal/extract/inject)
AIB-->>Container: Produce artifacts / exit status
Container-->>CLI: Return artifacts/status
end
sequenceDiagram
participant CLI as Sealed CLI
participant APIServer as Build API Server
participant K8s as Kubernetes API
participant Ctrl as ImageSealed Controller
participant Tekton as Tekton (TaskRun/PipelineRun)
rect rgba(200,255,200,0.5)
CLI->>APIServer: POST /v1/sealed (SealedRequest)
APIServer->>K8s: Create transient Secrets (if needed)
APIServer->>K8s: Create ImageSealed resource
K8s->>Ctrl: Notify reconciler (ImageSealed Pending)
Ctrl->>K8s: Create TaskRun or PipelineRun
Tekton->>Tekton: Execute sealed steps
Tekton-->>K8s: Update TaskRun/PipelineRun status
Ctrl->>K8s: Update ImageSealed status (Running -> Completed/Failed)
CLI->>APIServer: GET /v1/sealed/{name} / logs
APIServer-->>CLI: SealedResponse / log stream
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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: 8
🤖 Fix all issues with AI agents
In `@api/v1alpha1/imagesealed_types.go`:
- Around line 105-111: The default AIB image tag is inconsistent:
ImageSealedSpec.GetAIBImage currently returns
quay.io/centos-sig-automotive/automotive-image-builder:latest while
tasks.AutomotiveImageBuilder uses :1.0.0; to fix, keep the API method as-is
(GetAIBImage) and change tasks.AutomotiveImageBuilder to use the :latest tag (or
better, introduce a neutral shared constant like DefaultAIBImage in a non-api,
non-internal package and reference that from tasks.AutomotiveImageBuilder and
other callers) so there’s no API→internal import; after updating any
api/v1alpha1 defaults run make generate manifests.
In `@cmd/caib/sealed/sealed.go`:
- Around line 202-226: The function streamSealedLogs currently ignores the error
from http.NewRequestWithContext which can produce a nil req and cause a panic
when passed to client.Do; change the code to capture and check the error
returned by http.NewRequestWithContext (returning/logging and exiting the
function if non-nil) before calling client.Do, and also check scanner.Err()
after the scan loop and log or print any non-nil error so stream read failures
aren’t silently dropped; references: streamSealedLogs,
http.NewRequestWithContext, client.Do, scanner.Err().
- Around line 116-132: The relative-path branch in ensurePathInWorkspace
currently does filepath.Join(absWork, path) without validating that the
resulting absPath stays inside absWork; after joining, compute absPath =
filepath.Clean(filepath.Join(absWork, path)) and then use filepath.Rel(absWork,
absPath) (or equivalent) to ensure the resulting rel does not start with ".."
and error if it does; mirror the containment check used in the absolute-path
branch so absPath cannot escape the workspace.
In `@internal/buildapi/server.go`:
- Around line 2886-2914: The current sealed secret creation block checks only
RegistryURL/Username/Password and silently ignores other auth types; replace
this logic with a call to the existing createRegistrySecret(ctx, clientset,
namespace, req.Name, req.RegistryCredentials) (or equivalent helper) to handle
all supported AuthType values, capture and return any error to the client (HTTP
400/500 as appropriate), and assign the returned secret name to secretRef; if
you prefer not to reuse createRegistrySecret, validate
req.RegistryCredentials.Enabled and switch on AuthType (e.g.,
"username-password", "token", "docker-config") and return a clear error via
c.JSON when an unsupported or incomplete auth type is provided.
- Around line 2834-2855: The createSealed handler doesn't validate req.OutputRef
for operations that produce an output; update createSealed to require non-empty
req.OutputRef when req.Operation is SealedPrepareReseal, SealedReseal, or
SealedInjectSigned (in addition to the existing SignedRef check for
SealedInjectSigned). Locate the validOps map and the subsequent checks in
function createSealed and add a conditional that trims and checks req.OutputRef,
returning c.JSON(http.StatusBadRequest, gin.H{"error": "outputRef is required
for prepare-reseal/reseal/inject-signed"}) when missing so TaskRun push
destinations are always populated.
- Around line 2957-2968: The Get and Update calls that set the OwnerReference on
the secret currently discard errors; update the block that uses secretRef and
clientset.CoreV1().Secrets(namespace).Get/Update to capture both returned errors
and log a warning on failure (matching the pattern used in createBuild), e.g.,
check error from Get and log a WARNING with context (secretRef, namespace,
imageSealed.Name/UID) if non-nil, and similarly check the Update error and log a
WARNING if it fails; keep the OwnerReference population (APIVersion, Kind
"ImageSealed", Name imageSealed.Name, UID imageSealed.UID) but ensure failures
are not silently swallowed.
In `@internal/common/tasks/tasks.go`:
- Around line 1438-1457: The sealed-op step is missing the device and
container-storage mounts and the embedded script file, which causes
build/runtime failures; add VolumeMounts to the tektonv1.Step named "sealed-op"
with entries for Name "dev" mountPath "/dev" and Name "container-storage"
mountPath "/var/lib/containers/storage", and add corresponding corev1.Volumes to
the Task spec (names "dev" and "container-storage") matching how other
privileged steps (e.g., "build-image" / "prepare-builder") declare them; also
create the missing script file referenced by SealedOperationScript (add
internal/common/tasks/scripts/sealed_operation.sh and update the //go:embed in
scripts.go to include it so SealedOperationScript points to the new file).
In `@internal/controller/imagesealed/controller.go`:
- Around line 125-181: The createSealedTaskRun function currently ignores
sealed.Spec.StorageClass and sealed.Spec.AIBExtraArgs: update
createSealedTaskRun to create a PVC-backed workspace when
sealed.Spec.StorageClass (or a boolean flag in spec) is set instead of always
using EmptyDir — create a PersistentVolumeClaim object (or a WorkspaceBinding
referencing an existing PVC) and attach it as the "shared" workspace; reference
createSealedTaskRun, sealed.Spec.StorageClass, and the workspaces slice to
locate where to change. Also pass extra AIB args into the TaskRun by adding a
param (e.g., "aib-extra-args") to the TaskRun params when
sealed.Spec.AIBExtraArgs is non-empty and ensure tasks.GenerateSealedTask
declares the corresponding param in the Task spec (update GenerateSealedTask to
include a Param named "aib-extra-args"); finally, handle cleanup/owner refs for
the PVC and update/extend unit tests for createSealedTaskRun and
GenerateSealedTask to cover both EmptyDir and PVC cases and the extra-args
param.
🧹 Nitpick comments (10)
cmd/caib/sealed/prepare_reseal.go (1)
29-29: Package-level vars for workspace flags across all sealed subcommands could be consolidated.Each subcommand (
prepare_reseal.go,reseal.go,inject_signed.go) declares its own package-levelvar *Workspace stringand repeats the same workspace resolution + path validation + container-path-translation logic. Consider extracting the common local execution flow into a shared helper to reduce duplication.cmd/caib/sealed/extract_for_signing.go (2)
46-51:handleSealedErrorcallsos.Exit(1), makingreturn nilunreachable — consider returning the error instead.Since
RunEexpects error-based control flow, returning the error directly would be more idiomatic and testable. The current pattern silently exits, bypassing cobra's error handling.Proposed fix
if strings.TrimSpace(sealedServerURL) != "" { - if err := runSealedViaAPI(buildapitypes.SealedExtractForSigning, args[0], args[1], ""); err != nil { - handleSealedError(err) - } - return nil + return runSealedViaAPI(buildapitypes.SealedExtractForSigning, args[0], args[1], "") }
79-80: Silently discarded errors fromfilepath.Rel.While practically safe (ensurePathInWorkspace guarantees containment), consider at least a debug log or brief comment explaining why the error is safe to ignore.
cmd/caib/sealed/sealed.go (2)
192-195: Log streaming attempted during "Pending" phase may be premature.The condition
st.Phase == "Running" || st.Phase == "Pending"triggers log streaming even before the task has started. During "Pending", the log endpoint will likely return an error or empty response. Consider restricting to"Running"only, consistent with how the main build'swaitForBuildCompletionhandles this (it skips streaming for "Pending" incmd/caib/main.golines 1430–1437).
163-200:waitForSealedCompletionandhandleSealedErrorcallos.Exitdirectly, making the code untestable and inconsistent withRunEconvention.Functions called from
RunEhandlers should propagate errors rather than callingos.Exit. This is consistent with the pattern concern raised inextract_for_signing.go.api/v1alpha1/imagesealed_types.go (1)
24-52: Phase constants and cross-field validation forSignedRef.Two observations on the spec:
The
Phasefield values ("Pending","Running", etc.) are used as raw strings across the controller and server code. Consider defining typed constants (e.g.,PhasePending,PhaseRunning, etc.) in this file and referencing them, to avoid typo-induced bugs as the codebase grows.
SignedRefis documented as "required when operation is inject-signed" but has no kubebuilder CEL/webhook validation enforcing this. The server validates it, but the CRD itself would accept aninject-signedoperation withoutsignedRefif created directly via kubectl. A CEL rule or validating webhook would strengthen the contract.Both can be deferred to a follow-up.
internal/controller/imagesealed/controller.go (3)
154-161: OwnerReference is missingBlockOwnerDeletion.The OwnerReference sets
Controller: ptr(true)but does not setBlockOwnerDeletion. Without it, the TaskRun could be deleted before the ImageSealed resource's finalizer runs (if one is ever added). Consider usingmetav1.NewControllerRef()which sets both fields, consistent with the pattern used inserver.go(Line 1945).Proposed fix
OwnerReferences: []metav1.OwnerReference{ - { - APIVersion: automotivev1alpha1.GroupVersion.String(), - Kind: "ImageSealed", - Name: sealed.Name, - UID: sealed.UID, - Controller: ptr(true), - }, + *metav1.NewControllerRef(sealed, automotivev1alpha1.GroupVersion.WithKind("ImageSealed")), },
86-94: Potential stale-write race betweenhandlePendingandupdateStatus.
handlePendingsets multiple status fields (Lines 86-89) and then callsr.Status().Update()(Line 91). If the resource was modified between the initialGet(Line 57) and thisUpdate, the write will fail with a conflict error. This is mostly fine since the controller will re-reconcile, but there's a subtlety: ifcreateSealedTaskRunsucceeds (TaskRun is created) but the status update fails, the next reconciliation will re-enterhandlePendingand hit the idempotency check increateSealedTaskRun(Line 128-129), returning the existing TaskRun. So this is safe.However, note that
StartTimeis set totime.Now()each timehandlePendingruns. If the status update fails and retries,StartTimewill be slightly different. Consider usingmetav1.Now()and only setting it if it's not already set.
192-194: Preferk8s.io/utils/ptr.Toover a localptrhelper.The
tasks.gofile in this same repo already usesptr.To(true)fromk8s.io/utils/ptr(seeGenerateSealedTasksnippet). Defining a localptrfunction shadows that well-known utility and creates an inconsistency.internal/buildapi/server.go (1)
3046-3139: Log streaming code is largely duplicated fromstreamFlashLogs.
streamSealedLogsandstreamFlashLogs(Lines 2729-2832) follow an almost identical pattern: get resource → resolve TaskRun → resolve pod → setup headers → stream from container → scanner loop. Consider extracting a shared helper (e.g.,streamTaskRunLogs(c, namespace, podName, containerName, sinceTime, streamDuration, headerLabel)) to reduce duplication.
bkhizgiy
force-pushed
the
sealed
branch
2 times, most recently
from
9fd6345 to
d144fb7
Compare
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Fix all issues with AI agents
In `@api/v1alpha1/imagesealed_types.go`:
- Around line 29-33: The Stages field lacks item-level enum validation causing
invalid stage names to pass admission; add validation so items must match the
same set as Operation (prepare-reseal;reseal;extract-for-signing;inject-signed)
— either add a kubebuilder item-level enum tag (e.g.
+kubebuilder:validation:Items/Enum or equivalent for list items) or a CEL rule
on Stages to validate every(*) element against that enum, and also add a
defensive runtime check in the controller's handlePending (or the reconciliation
path that reads imagesealed.Spec.Stages) that returns a clear validation error
if any stage is not in the allowed set (use the same allowed set constant/shared
slice to avoid drift).
In `@bundle/manifests/automotive.sdv.cloud.redhat.com_imagesealeds.yaml`:
- Around line 51-113: The bundle CRD for ImageSealed is missing the
spec.properties fields "architecture" and "builderImage" that exist in the base
CRD; update the bundle by regenerating it so spec.properties for the ImageSealed
CRD includes architecture and builderImage (under spec -> properties) to match
the base, then rebuild the bundle (e.g., run the repository's bundle generation
task such as "make bundle") and commit the regenerated bundle manifest so
ImageSealed's spec.properties contains the architecture and builderImage
entries.
In `@cmd/caib/sealed/extract_for_signing.go`:
- Around line 79-81: The code silently ignores errors from filepath.Rel when
computing relIn/relOut before calling runAIB; change the logic in the function
that computes relIn/relOut (the lines using filepath.Rel with absWork, inPath,
outPath) to capture and handle the returned errors (e.g., return the error up
the call chain or log and fail) instead of discarding them, and only call
runAIB("extract-for-signing", absWork, toContainerPath(relIn),
toContainerPath(relOut)) when both Rel calls succeed so you never pass
empty/invalid relIn or relOut to toContainerPath/runAIB.
In `@cmd/main.go`:
- Around line 250-258: The ImageSealed reconciler (imagesealed.Reconciler
instantiated as imageSealedReconciler and registered via
imageSealedReconciler.SetupWithManager) is being started unconditionally; wrap
its creation and SetupWithManager call in the same mode check used by other
build controllers (check mode == modeBuild || mode == modeAll) so the reconciler
only registers in build modes—mirror the gating used for
ImageBuild/Image/CatalogImage to keep ImageSealed from running in platform-only
mode.
In `@internal/buildapi/client/client.go`:
- Around line 391-419: Replace the manual HTTP get/decode logic inside
ListSealed with a single call to the existing helper c.listJSON so it follows
the same pattern as ListBuilds and ListFlash: create the result slice var out
[]buildapi.SealedListItem, call c.listJSON(ctx, "/v1/sealed", &out), and return
out and the error from that call; remove the duplicated request creation, header
handling, response close and JSON decode code but keep the same return types and
error propagation.
In `@internal/buildapi/server.go`:
- Around line 3274-3284: The code in streamSealedLogs currently picks only
trList.Items[0] when sealed.Status.PipelineRunName is set, so multi-stage
pipelines show just one TaskRun; modify streamSealedLogs to handle
tektonv1.TaskRunList results by sorting the trList.Items by creationTimestamp
(or startTime) and then iterate through each TaskRun in order, streaming logs
for each taskRun sequentially (rather than using trList.Items[0]); ensure you
use the same log-streaming logic used by streamLogs for pods (reusing any helper
functions) so all stages like prepare-reseal, extract-for-signing,
inject-signed, reseal are streamed in sequence.
- Around line 3298-3330: The response is writing JSON errors after
setupLogStreamHeaders has already sent 200 and flushed; instead of calling
c.JSON in get-log-stream logic, write plain-text error messages to the open
stream (c.Writer) consistent with the existing pattern (use
fmt.Fprintf(c.Writer, "[Error ...]\n", ...) and c.Writer.Flush()), and then
return; update the two places that call c.JSON (the gateway timeout branch using
streamCtx.Done() and the final stream==nil branch) to write plain-text error
lines to the stream and flush so clients reading the log stream receive a clear
plain-text error rather than a JSON payload.
In `@internal/common/tasks/scripts/sealed_operation.sh`:
- Around line 403-411: The find invocation in sealed_operation.sh that sets
TARBALL is suffering from operator-precedence: the expression "-type f -name
'*.tar.gz' -o -name '*.tgz'" lets the second -name match non-files. Fix TARBALL
by grouping the name tests so -type f applies to both names (e.g. use
parentheses around the -name alternatives) or by repeating -type f for each
alternative; update the find command used to set the TARBALL variable so it only
returns regular files matching *.tar.gz or *.tgz.
In `@internal/common/tasks/tasks.go`:
- Around line 1488-1503: The two memory-backed EmptyDir volumes named
"container-storage" and "run-osbuild" in the sealed task spec are missing
SizeLimit and can OOM the node; update their corev1.EmptyDirVolumeSource entries
to set SizeLimit (use BuildConfig.MemoryVolumeSize when available, falling back
to a sensible default like "512Mi" or similar) so the sealed task matches the
other build tasks (e.g., GenerateBuildAutomotiveImageTask) that conditionally
apply BuildConfig.MemoryVolumeSize; ensure the code references the same config
variable and applies it to both VolumeSource.EmptyDir.SizeLimit fields.
In `@internal/controller/imagesealed/controller.go`:
- Around line 410-421: cleanupTransientSecrets currently deletes any secret
named in sealed.Spec.SecretRef, KeySecretRef, and KeyPasswordSecretRef which can
remove user-managed secrets; change it to only delete secrets that were created
transiently by the controller. Update cleanupTransientSecrets to check the
referenced Secret's metadata for a controller marker (e.g. a specific annotation
like "imagesealed.example.com/transient" or an ownerReference) before calling
deleteSecretWithRetry, and ensure the code path that creates secrets from
sealed.Spec.KeyContent and sealed.Spec.KeyPassword (the transient secret
creation logic) sets that marker when creating them (so use the same marker name
in both the creator and cleanup). Reference: cleanupTransientSecrets,
deleteSecretWithRetry, sealed.Spec.SecretRef, sealed.Spec.KeySecretRef,
sealed.Spec.KeyPasswordSecretRef, and the transient-creation code that consumes
sealed.Spec.KeyContent and sealed.Spec.KeyPassword.
In `@internal/controller/test/imagesealed_controller_test.go`:
- Around line 73-92: The test currently returns silently when
Reconciler.Reconcile returns an error and never asserts status on success;
update the test to assert the error content in the failure path and assert
sealed.Status.Phase == automotivev1alpha1.PhaseRunning in the success path:
after calling r.Reconcile(ctx, reconcile.Request{NamespacedName:
typeNamespacedName}) if err != nil assert the error message contains a
Tekton-related substring (e.g. "Tekton" or "TaskRun") so the failure is
intentional, otherwise (err == nil) fetch the resource into sealed and
Expect(sealed.Status.Phase).To(Equal(automotivev1alpha1.PhaseRunning)) to verify
the primary path; use the existing Reconciler r, Reconcile call, and sealed
variable names to locate and modify the assertions.
🧹 Nitpick comments (9)
internal/controller/operatorconfig/controller.go (1)
709-712: Cleanup task names are hardcoded; consider deriving them fromtasks.SealedOperationNames.The deploy path (line 532) dynamically generates sealed task names via
tasks.GenerateSealedTasks, but cleanup hardcodes them. IfSealedOperationNameschanges, this list will silently drift. Consider building the cleanup list from the same source of truth:Suggested approach
taskNames := []string{ "build-automotive-image", "push-artifact-registry", "prepare-builder", "flash-image", - "sealed-prepare-reseal", "sealed-reseal", "sealed-extract-for-signing", "sealed-inject-signed", } + for _, op := range tasks.SealedOperationNames { + taskNames = append(taskNames, tasks.SealedTaskName(op)) + }cmd/caib/sealed/prepare_reseal.go (1)
42-48: Code pattern is confusing but functionally correct—consider returning the error directly instead.The function calls
handleSealedError(err)which prints the error and callsos.Exit(1), so the CLI does exit with a non-zero status on failure. However, thereturn nilon line 47 is unreachable dead code (the program terminates atos.Exit(1)), making the pattern confusing for future maintainers. The clearer approach is to return the error directly rather than rely onhandleSealedErrorto terminate:Suggested refactor
if strings.TrimSpace(sealedServerURL) != "" { if err := runSealedViaAPI(buildapitypes.SealedPrepareReseal, args[0], args[1], ""); err != nil { - handleSealedError(err) + return err } return nil }internal/controller/imagesealed/controller.go (1)
424-452:time.Sleepinside the reconciler blocks a worker goroutine.While the total sleep (up to ~700ms across 3 retries) is short, blocking a controller-runtime worker is an anti-pattern. If you want retry semantics, consider requeueing with a delay or using a finalizer-based cleanup pattern instead. Low urgency given the short duration, but worth noting for future scaling.
cmd/caib/sealed/sealed.go (1)
295-321:os.Exit(1)calls bypass Cobra's error handling and defer cleanup.
waitForSealedCompletion(Lines 297, 321) andhandleSealedError(Line 359) callos.Exit(1)directly. This preventsdeferstatements from running and prevents Cobra from printing usage help or performing graceful shutdown. Consider returning errors and letting theRunEhandler propagate them.internal/common/tasks/scripts/sealed_operation.sh (4)
1-11:validate_argdoes not block newlines or carriage returns.The regex on line 7 checks for common shell metacharacters but misses
\nand\r, which can enable argument injection or log forgery when values originate from user input. Although Tekton environment variables are unlikely to contain newlines, this is a defense-in-depth gap.Proposed fix
validate_arg() { local arg="$1" local name="$2" - if [[ "$arg" =~ [\;\|\&\$\`\(\)\{\}\<\>\!\\] ]]; then + if [[ "$arg" =~ [\;\|\&\$\`\(\)\{\}\<\>\!\\] ]] || [[ "$arg" == *$'\n'* ]] || [[ "$arg" == *$'\r'* ]]; then echo "ERROR: Invalid characters in $name: $arg" exit 1 fi }
56-79: Installing packages at runtime (dnf install/yum install) is fragile and slow.If the task image doesn't ship
fuse-overlayfs, every run pays the install cost and risks network/repo failures. Consider baking it into the builder image instead.
286-330:extract-for-signingsilently succeeds even whenoutput_diris empty.After
aib extract-for-signingcompletes, there is no check thatoutput_diractually contains files. If the command produces nothing (unexpected input image, etc.), the script pushes an empty tarball to the registry without any error, which makes downstreaminject-signedfail with a confusing error.Consider adding a guard:
if [ -z "$(ls -A output_dir 2>/dev/null)" ]; then echo "ERROR: extract-for-signing produced no output files" >&2 exit 1 fi
183-196: Hardcoded oras version1.2.0will become stale.Consider making the version configurable via an environment variable (e.g.,
ORAS_VERSION="${ORAS_VERSION:-1.2.0}"), which allows operators to override without modifying the script.internal/buildapi/types.go (1)
286-312:Stagesis[]stringwhileOperationis typedSealedOperation— inconsistent.
Operationbenefits from theSealedOperationtype alias for clarity and type safety, butStagesis[]string. Using[]SealedOperation(or at minimum documenting that entries must be validSealedOperationvalues) would make the API self-documenting and allow compile-time checks where possible.
bkhizgiy
force-pushed
the
sealed
branch
2 times, most recently
from
dd39771 to
3d9b954
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In `@cmd/caib/main.go`:
- Line 679: The AddCommand call registers loginCmd twice which will panic at
runtime; remove the duplicate reference to loginCmd in the rootCmd.AddCommand
invocation (the call that currently lists buildCmd, diskCmd, buildDevCmd,
listCmd, showCmd, downloadCmd, flashCmd, loginCmd, loginCmd,
catalog.NewCatalogCmd(), sealed.NewSealedCmd()) so each Cobra command is unique,
and if a different command was intended in the second slot replace the duplicate
loginCmd with the correct command symbol (or omit it) to avoid duplicate command
names.
In `@internal/buildapi/server.go`:
- Around line 3141-3149: The current IsAlreadyExists(err) branch in the
createSealedSecrets call incorrectly reports "sealed job %s already exists" when
the collision is actually a Kubernetes Secret (e.g.,
"<req.Name>-registry-auth"); update the handler in the createSealedSecrets error
branch to detect/report a secret name collision by including the actual secret
name (fmt.Sprintf("%s-registry-auth", req.Name)) in the 409 response message
(or, if you prefer, remove the IsAlreadyExists check so the error falls through
to the 500 path); ensure you modify the error handling immediately after the
createSealedSecrets(...) call and reference req.Name and createSealedSecrets to
locate the code.
- Around line 3108-3119: The createSealed handler does not validate OCI
reference fields before embedding them in the ImageSealed spec; call the
existing validateContainerRef function for req.InputRef, req.OutputRef and
req.SignedRef (same checks used by createBuild) after validateSealedRequest
returns and reject the request with HTTP 400 on invalid refs, ensuring the
ImageSealed spec only receives validated container references to prevent
injection when those values are later passed to Tekton tasks.
- Around line 3013-3093: The createSealedSecrets function can leak the registry
secret if a later secret creation (e.g., keySecret or keyPwSecret) fails because
it returns nil, error and the caller cannot run cleanupSealedSecrets; update
createSealedSecrets so that either (A) it always returns the partial refs struct
(refs with refs.secretRef/keySecretRef/keyPasswordSecretRef set) alongside the
error so the caller can call cleanupSealedSecrets, or (B) perform immediate
cleanup on failure by deleting any already-created secrets (use
clientset.CoreV1().Secrets(namespace).Delete for refs.secretRef, keySecretName,
keyPwSecretName) before returning the error; pick one approach and apply it
consistently in createSealedSecrets and any error-return paths so no secret
created by createSealedSecrets can be orphaned (ensure refs is set when
returning an error if you choose option A).
- Around line 3217-3246: The listSealed handler returns items in
non-deterministic order; make it consistent with listBuilds and listFlash by
sorting the results by creation time (newest first) before building the
response. After you populate resp (or before appending), sort using the
CreationTimestamp/CreatedAt value (or sort list.Items by
s.CreationTimestamp.Time descending) so SealedListItem entries are ordered
newest-to-oldest; update listSealed to perform that sorting step prior to
writeJSON.
In `@internal/common/tasks/scripts/sealed_operation.sh`:
- Around line 179-181: Add validation for BUILDER_IMAGE using the existing
validate_arg helper before any use (same pattern as
INPUT_REF/OUTPUT_REF/SIGNED_REF) so the variable is sanitized prior to skopeo
copy calls; specifically, call validate_arg with BUILDER_IMAGE (e.g.,
validate_arg "${BUILDER_IMAGE:-}" "builder-image") in sealed_operation.sh before
the sections that invoke skopeo copy and any other places that reference
BUILDER_IMAGE to ensure it cannot inject unsafe content.
- Around line 184-196: The install_oras function currently streams the tarball
directly into tar without verifying integrity; change it to download the release
tarball to a temp file (e.g., /tmp/oras_${ORAS_VERSION}.tar.gz), fetch or embed
a pinned SHA-256 digest (add ORAS_SHA256 for the pinned value or download the
.sha256 from GitHub), verify the temp file with sha256sum (or shasum -a 256) and
fail if the checksum does not match, then extract the verified archive to a temp
dir, move the oras binary to /usr/local/bin/oras, set +x and cleanup temp files;
ensure any failure returns non-zero and include ORAS_VERSION and ORAS_ARCH
references so the download URL remains identical to the original logic in
install_oras.
In `@internal/controller/imagesealed/controller.go`:
- Around line 326-358: The pipeline builds per-stage PipelineTask entries with
params "input-ref"/"output-ref" but leaves intermediate stages empty, causing
the script to fail; update the PipelineTask param wiring in the loop that
populates pipelineTasks so that each stage's "input-ref" uses the previous
stage's result when i>0 (e.g., set "input-ref" to Tekton result substitution
from the prior task like $(tasks.stage-(i-1).results.<resultName>) instead of an
empty string), and ensure the previous stage writes that result (or
alternatively switch to passing image refs via the shared workspace); adjust
code around stages, pt.Params, and tasks.SealedTaskName(op) to reference the
correct task result name for chaining.
In `@internal/controller/test/imagesealed_controller_test.go`:
- Around line 250-262: The test Context uses an undeclared ctx from outer scope;
update the "When reconciling a non-existent resource" block to declare a local
context variable (e.g., ctx := context.Background()) at the start of the It
block so the call to imagesealed.Reconciler{}.Reconcile (reconcile.Request with
NamespacedName) uses a locally scoped ctx consistent with the other contexts;
ensure the variable name is ctx to match the Reconcile invocation.
🧹 Nitpick comments (9)
internal/common/tasks/scripts/sealed_operation.sh (1)
56-59: Runtimednf installmakes the task fragile and non-reproducible.Installing
fuse-overlayfsat runtime viadnf/yumdepends on network access and package repo availability during execution, and adds latency on every run. If the AIB container image is under your control, consider bakingfuse-overlayfsinto the image instead.internal/common/tasks/tasks.go (1)
1425-1426: Shared mutable pointer fordefaultSealedMemoryVolumeSize— low risk but worth noting.
&defaultSealedMemoryVolumeSizeis shared across all generated tasks. If any downstream code (e.g., an admission webhook or defaulting) mutates theQuantityin-place, it would silently affect all tasks. Consider making a copy per task if this ever becomes an issue, but for current usage this is fine.cmd/caib/sealed/inject_signed.go (1)
45-51: Argument reordering between CLI and API is correct but could use a clarifying comment.The CLI takes
<input> <signed> <output>butrunSealedViaAPIexpects(op, input, output, signed). The swap at Line 47 (args[0], args[2], args[1]) is correct, but a brief inline comment would prevent future confusion.cmd/caib/sealed/extract_for_signing.go (2)
45-87: Local path duplicatesrunLocalTwoArgOpfromsealed.go.The local execution branch (lines 52-87) closely mirrors
runLocalTwoArgOpinsealed.go(lines 149-180), differing only in that it callsos.MkdirAll(outPath, …)(treating output as a directory) instead ofos.MkdirAll(filepath.Dir(outPath), …). Consider refactoring to share the common workspace/path logic and parameterize the output-path preparation, reducing duplication and drift risk.
46-51:return nilon Line 50 is dead code afterhandleSealedError.
handleSealedErrorcallsos.Exit(1), so Line 50 is only reached whenerr == nil. Theif err != nilguard +handleSealedError+ fall-through pattern is confusing. A clearer pattern would be to return the error directly:Suggested improvement
if strings.TrimSpace(sealedServerURL) != "" { - if err := runSealedViaAPI(buildapitypes.SealedExtractForSigning, args[0], args[1], ""); err != nil { - handleSealedError(err) - } - return nil + return runSealedViaAPI(buildapitypes.SealedExtractForSigning, args[0], args[1], "") }This keeps RunE's contract (return an error to Cobra) instead of hard-exiting, which also helps testability.
internal/controller/test/imagesealed_controller_test.go (1)
73-95: Test assertions improved — but the success path is still weakly constrained.The updated assertions (line 88-95) are a significant improvement over the previous review. When
err == nilandPhase == "Running", the test passes without verifyingTaskRunNamewas set. If the intent is to verify the full happy path when Tekton CRDs happen to be available, consider addingExpect(sealed.Status.TaskRunName).NotTo(BeEmpty())inside theRunningbranch to strengthen coverage.internal/controller/imagesealed/controller.go (2)
459-488:time.SleepindeleteSecretWithRetryblocks the reconciler goroutine.The retry loop with exponential backoff (up to ~700ms total for 3 retries) blocks the controller's worker goroutine. This is a minor concern for a cleanup path, but if multiple ImageSealed resources complete simultaneously, it could add up. Consider using
RequeueAfterinstead of synchronous retries for cleanup, or accepting that best-effort single-attempt deletion is sufficient here since the secrets are transient.
409-416:isPipelineRunSuccessfuluses raw string comparison for Tekton condition status.Comparing
c.Status == "True"works but is fragile. Tekton/Knative usescorev1.ConditionTrue("True") — referencing the constant is safer against upstream changes.Suggested fix
+import corev1 "k8s.io/api/core/v1" + func isPipelineRunSuccessful(pr *tektonv1.PipelineRun) bool { for _, c := range pr.Status.Conditions { if c.Type == "Succeeded" { - return c.Status == "True" + return c.Status == corev1.ConditionTrue } } return false }cmd/caib/sealed/sealed.go (1)
273-328:waitForSealedCompletioncallsos.Exitdirectly, bypassing Cobra error handling.Lines 303 and 327 call
os.Exit(1)on failure/timeout. This prevents Cobra'sRunEfrom returning an error, skips any deferred cleanup, and makes the function untestable. Consider returning an error and letting the caller (or Cobra) handle exit.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/buildapi/server.go`:
- Around line 3026-3058: The condition in createSealedSecrets currently only
creates a registry secret when RegistryCredentials.RegistryURL, Username, and
Password are non-empty, so AuthType values like "token" or "docker-config" are
ignored; update createSealedSecrets to delegate to the existing
createRegistrySecret function (or replicate its switch on
RegistryCredentials.AuthType) whenever req.RegistryCredentials != nil and
req.RegistryCredentials.Enabled is true, passing ctx, clientset, namespace and
req to createRegistrySecret, then set refs.secretRef from its result; ensure
createRegistrySecret's validation for Username/Password, Token, and DockerConfig
is used and return errors for unsupported/invalid AuthType instead of silently
doing nothing.
In `@internal/common/tasks/tasks.go`:
- Around line 1496-1511: The sealed-op step is missing the Tekton result path
env var for the declared sealed-container result; update the sealed-op step's
Env list to include a RESULT_PATH env var whose value is
$(results.sealed-container.path) so the SealedOperationScript can write the
result file to the declared Tekton result (i.e., add {Name: "RESULT_PATH",
Value: "$(results.sealed-container.path)"} to the Env slice for the sealed-op
step).
🧹 Nitpick comments (7)
internal/common/tasks/tasks.go (2)
1428-1429: Exported mutable sliceSealedOperationNamescan be accidentally modified by callers.Since this is an exported package-level
varholding a slice, any importing package can append to or overwrite elements. Consider either making it unexported or exposing it via a function that returns a copy.♻️ Option: return a copy from a function
-// SealedOperationNames is the list of sealed operation names (used for task names and validation). -var SealedOperationNames = []string{"prepare-reseal", "reseal", "extract-for-signing", "inject-signed"} +// sealedOperationNames is the canonical list of sealed operation names. +var sealedOperationNames = []string{"prepare-reseal", "reseal", "extract-for-signing", "inject-signed"} + +// SealedOperationNames returns a copy of the sealed operation names list. +func SealedOperationNames() []string { + out := make([]string, len(sealedOperationNames)) + copy(out, sealedOperationNames) + return out +}Note: callers using
SealedOperationNameswould need to change toSealedOperationNames().
1559-1573: No validation ofoperationparameter.
GenerateSealedTaskForOperationpassesoperationstraight through toSealedTaskNameandsealedTaskSpecwithout validating it againstSealedOperationNames. An invalid operation string would silently produce a task with an unrecognized operation name. Consider adding a guard, especially since this is a public API.cmd/caib/sealed/inject_signed.go (1)
45-51: API-mode error path relies onos.ExitinsidehandleSealedError.If
handleSealedErroris ever refactored to not callos.Exit(1), this function will swallow the error and returnnil. This pattern is consistent across all sealed subcommands, but it's fragile. Consider returning the error directly:Proposed fix
if strings.TrimSpace(sealedServerURL) != "" { - if err := runSealedViaAPI(buildapitypes.SealedInjectSigned, args[0], args[2], args[1]); err != nil { - handleSealedError(err) - } - return nil + return runSealedViaAPI(buildapitypes.SealedInjectSigned, args[0], args[2], args[1]) }cmd/caib/sealed/extract_for_signing.go (1)
45-51: SamehandleSealedError/return nilpattern as other subcommands.Same fragile error handling pattern noted in
inject_signed.go. The error is swallowed ifhandleSealedErrorever stops callingos.Exit.internal/controller/imagesealed/controller.go (2)
460-488:time.Sleepinside reconcile loop blocks the controller goroutine.
deleteSecretWithRetrysleeps up to ~700ms total (100ms + 200ms + 400ms). While tolerable since it only runs during terminal-phase cleanup, blocking the reconcile goroutine is generally discouraged in controller-runtime. Consider logging and requeuing instead if this becomes an issue under load.
409-416: Prefer Tekton API constants for condition checks.
isPipelineRunSuccessfulcomparesc.Type == "Succeeded"andc.Status == "True"using raw strings. Consider using Tekton'sapis.ConditionSucceededandcorev1.ConditionTrueconstants for resilience against upstream changes.cmd/caib/sealed/sealed.go (1)
273-328:waitForSealedCompletioncallsos.Exit(1)directly, making the function untestable.Both the failure case (line 303) and timeout case (line 327) call
os.Exit(1), which prevents the caller from handling the error and makes unit testing impossible. Consider returning an error instead and letting the caller decide how to exit.
|
I don't think we need the |
bkhizgiy
force-pushed
the
sealed
branch
2 times, most recently
from
2a6ab54 to
181b8d9
Compare
bkhizgiy
force-pushed
the
sealed
branch
4 times, most recently
from
8adbbc0 to
e865d02
Compare
bkhizgiy
force-pushed
the
sealed
branch
2 times, most recently
from
1fe4dcc to
b5d903a
Compare
Introduce the ImageReseal custom resource definition for managing sealed/reseal operations on automotive OS images. Includes API types, generated deepcopy methods, CRD manifests, RBAC role updates, and bundle manifests. Signed-off-by: Bella Khizgiyaev <bkhizgiy@redhat.com> Assisted-by: Claude (Anthropic) Made-with: Cursor
Add reseal and prepare-reseal subcommands to the caib CLI for triggering sealed image operations from the command line. Signed-off-by: Bella Khizgiyaev <bkhizgiy@redhat.com> Assisted-by: Claude (Anthropic) Made-with: Cursor
Implement Tekton task definitions and the sealed_operation.sh script for reseal, prepare-reseal, extract-for-signing, and inject-signed operations. Includes builder image auto-detection, architecture matching, storage volume configuration, and proper credential handling with chmod-based file protection. Signed-off-by: Bella Khizgiyaev <bkhizgiy@redhat.com> Assisted-by: Claude (Anthropic) Made-with: Cursor
Implement the ImageReseal reconciler that creates Tekton TaskRuns for sealed operations and manages the build lifecycle. Add corresponding build API server routes, request/response types, and client methods for reseal, prepare-reseal, extract-for-signing, and inject-signed operations. Signed-off-by: Bella Khizgiyaev <bkhizgiy@redhat.com> Assisted-by: Claude (Anthropic) Made-with: Cursor
Add tests for ImageReseal API types, sealed Tekton task spec generation, and the ImageReseal controller reconciliation logic. Signed-off-by: Bella Khizgiyaev <bkhizgiy@redhat.com> Assisted-by: Claude (Anthropic) Made-with: Cursor
Register the ImageReseal controller in the manager and update the OperatorConfig controller to deploy sealed Tekton tasks alongside existing build tasks. Signed-off-by: Bella Khizgiyaev <bkhizgiy@redhat.com> Assisted-by: Claude (Anthropic) Made-with: Cursor
Replace the outdated hardcoded AutomotiveImageBuilder constant (1.0.0) with DefaultAutomotiveImageBuilderImage (1.1.11) from the operator config types, so sealed tasks use the same default as all other tasks. Signed-off-by: Bella Khizgiyaev <bkhizgiy@redhat.com> Assisted-by: Claude (Anthropic) Made-with: Cursor
2 participants
Add sealed image operations (prepare-reseal, reseal, extract-for-signing, inject-signed)
Implements the full AIB sealed-image workflow as a new ImageSealed Custom Resource with dedicated Tekton Tasks, Build API endpoints, and CLI commands. This enables users to perform secure boot sealing operations on bootc container images through the operator.
Adds ImageSealed CRD with support for single-operation TaskRuns and multi-stage PipelineRuns, including sealing key management, registry credentials, architecture-aware builder image defaults, and transient secret lifecycle
Adds caib sealed CLI subcommands (prepare-reseal, reseal, extract-for-signing, inject-signed) with both local (container) and remote (Build API) execution modes, --key-file for automatic secret creation, and robust log streaming with retry logic.
CLI (cmd/caib/sealed/)
caib prepare-reseal --input <input-ref> --output <output-ref> --key <key-file>- Prepare container for resealingcaib reseal --input <input-ref> --output <output-ref>- Reseal with ephemeral or provided keycaib extract-for-signing --input <input-ref> --output <output-ref> - Extract components for external signingcaib inject-signed --input --output- Inject signed components back Flags:--server, --builder-image, --input, --output, --signed, --arch, --key, --key-password, --key-secret, --follow, --wait`Log streaming with client-side retry (up to 24 attempts) for container initialization delays
Based on #53
Summary by CodeRabbit
New Features
Tasks & Scripts
RBAC
Tests