introduce seal/reseal operations#53
Closed
bennyz wants to merge 1 commit into
Closed
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
📝 WalkthroughWalkthroughThis pull request introduces ImageReseal, a new Kubernetes custom resource and controller for orchestrating container image resealing operations. It includes CRD definitions, API handlers, a CLI command, controller reconciliation logic, Tekton task workflows, and supporting scripts to manage the end-to-end reseal workflow. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as CLI<br/>(caib reseal)
participant APIServer as Build API<br/>Server
participant K8s as Kubernetes API
participant Ctrl as ImageReseal<br/>Controller
participant Tekton as Tekton<br/>TaskRun
participant Registry as Container<br/>Registry
User->>CLI: caib reseal --from-build build1 ...
CLI->>APIServer: POST /v1/reseals (ResealRequest)
APIServer->>APIServer: Validate request & credentials
APIServer->>K8s: Create registry secrets
APIServer->>K8s: Create seal key secrets
APIServer->>K8s: Create ImageReseal CR (status: Pending)
APIServer-->>CLI: ResealResponse (status: Pending)
CLI->>APIServer: GET /v1/reseals/{name} (polling)
Note over CLI,APIServer: Poll loop (if --wait)
K8s->>Ctrl: ImageReseal created (Pending)
Ctrl->>Ctrl: handlePending()
Ctrl->>K8s: Create Tekton TaskRun
Ctrl->>K8s: Update CR (status: Running)
Tekton->>Tekton: Execute reseal_image.sh
Tekton->>Registry: Pull source container
Tekton->>Registry: Pull builder image (aib)
Tekton->>Registry: Execute aib reseal
Tekton->>Registry: Push resealed image
Ctrl->>K8s: Poll TaskRun completion
Ctrl->>K8s: Update CR (status: Completed)<br/>+ SealedContainer ref
Ctrl->>K8s: Cleanup transient secrets
CLI->>APIServer: GET /v1/reseals/{name}
APIServer-->>CLI: ResealResponse (Completed)<br/>+ sealed image reference
CLI-->>User: Display results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 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)
Comment |
Contributor
Author
|
@coderabbitai please review |
|
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@api/v1alpha1/imagereseal_types.go`:
- Around line 46-48: Update the BuilderImage field to be required in the CRD
schema by adding the kubebuilder marker +kubebuilder:validation:Required above
the BuilderImage declaration and remove ",omitempty" from its json tag (change
`json:"builderImage,omitempty"` to `json:"builderImage"`), matching the
SourceContainer/TargetContainer pattern; after editing the BuilderImage field,
run `make generate manifests` to regenerate CRD manifests and DeepCopy code.
In `@cmd/caib/main.go`:
- Around line 1751-1763: The GetBuild call is using context.Background() instead
of the existing ctx; change the api.GetBuild invocation to use the function's
ctx variable so cancellation and deadlines propagate (replace
api.GetBuild(context.Background(), fromBuild) with api.GetBuild(ctx,
fromBuild)), keeping the surrounding error handling logic that checks
build.BuilderImageUsed and assigns builderImage unchanged.
In `@internal/buildapi/server.go`:
- Around line 2206-2293: The created secrets (registry secret via
createRegistrySecret, sealKeySecret/passwordSecret created before ImageReseal)
can become orphaned if k8sClient.Create(ctx, imageReseal) fails; ensure secrets
are either created after the ImageReseal exists or are cleaned up on failure by:
create the ImageReseal CR first (using empty/placeholder secret refs if needed)
or set OwnerReferences on each secret to the ImageReseal before creating them,
and if you choose to create secrets first implement a deferred cleanup that
deletes createRegistrySecret's secret and the local sealKeySecret/passwordSecret
when k8sClient.Create(ctx, imageReseal) returns an error; update the logic
around createRegistrySecret, the sealKeySecret/passwordSecret creation blocks
and the final k8sClient.Create(imageReseal) call to perform one of these two
approaches.
In `@internal/common/tasks/scripts/reseal_image.sh`:
- Around line 272-282: The script currently writes the Tekton result file
(/tekton/results/sealed-container) only inside the CONTAINER_PUSH branch; ensure
the result is always written. After the conditional push block that uses
push_cmd, log_command and the skopeo invocation (and also in the branch where
CONTAINER_PUSH is empty), write the value that represents the sealed image (use
the same variable used in the push branch, e.g., $CONTAINER_PUSH or
$OUTPUT_CONTAINER as appropriate) to /tekton/results/sealed-container so
downstream tasks always have the result; update reseal_image.sh to perform that
write in both branches.
In `@internal/controller/imagereseal/controller.go`:
- Around line 161-169: The code currently checks only for existence of the
"app.kubernetes.io/managed-by" label on existingTask before overwriting; change
the condition to verify the label value equals this controller's identifier
(compare existingTask.Labels["app.kubernetes.io/managed-by"] ==
"<controller-name>") instead of just presence, and if it doesn't match, do not
update and return or skip with a clear error/permission message; adjust the
block that assigns existingTask.Spec = resealTask.Spec and calls r.Update(ctx,
existingTask) to run only when the label value matches, and handle the
non-matching case (e.g., return nil with a descriptive fmt.Errorf or skip
update) so you never overwrite tasks managed by other controllers.
- Around line 288-295: The comment in cleanupTransientSecrets (function
cleanupTransientSecrets) contradicts the fact that seal key secrets have
ownerReferences set in server.go (the code that creates the seal key secrets) —
decide which behavior you want: if seal keys should be reusable, remove the
ownerReferences where seal key secrets are created in server.go so they are not
GC'd and keep/delete logic as-is (or add explicit deletion via
deleteSecretWithRetry); otherwise, keep the ownerReferences and update
cleanupTransientSecrets' comment and logic to reflect that seal key secrets are
lifecycle-managed by ownerReferences (i.e., they will be garbage collected and
should not be manually deleted). Ensure the chosen change touches either the
owner reference assignment in the seal-key creation code or the comment/cleanup
logic in cleanupTransientSecrets and reference deleteSecretWithRetry if adding
explicit deletion.
🧹 Nitpick comments (4)
api/v1alpha1/imagereseal_types.go (1)
42-44: Align the AIB default/tag across API helpers and task defaults.
This file defaults to...:latest, whileinternal/common/tasks/tasks.gouses...:1.0.0. That can lead to different images depending on the entrypoint, and the mutablelatesttag hurts reproducibility. Consider pinning and sharing a single canonical value.🔧 Suggested alignment (pick a single canonical tag)
-// +kubebuilder:default="quay.io/centos-sig-automotive/automotive-image-builder:latest" +// +kubebuilder:default="quay.io/centos-sig-automotive/automotive-image-builder:1.0.0" @@ -return "quay.io/centos-sig-automotive/automotive-image-builder:latest" +return "quay.io/centos-sig-automotive/automotive-image-builder:1.0.0"Also applies to: 112-118
config/crd/bases/automotive.sdv.cloud.redhat.com_imagereseals.yaml (1)
57-61: Consider using a versioned tag instead of:latestfor the default AIB image.The default value uses
:latestwhich can lead to non-reproducible builds. Looking atinternal/common/tasks/tasks.go:24, the codebase uses a versioned tag1.0.0for the same image elsewhere.Suggested fix
automotiveImageBuilder: - default: quay.io/centos-sig-automotive/automotive-image-builder:latest + default: quay.io/centos-sig-automotive/automotive-image-builder:1.0.0 description: AutomotiveImageBuilder is the AIB container image to use for the reseal operation type: stringcmd/caib/main.go (1)
1828-1871: Consider adding log streaming support for reseal operations.The
waitForResealCompletionfunction only polls for status updates but doesn't support log streaming, unlikewaitForBuildCompletionandwaitForFlashCompletion. This may make it harder for users to debug failed reseal operations.The
--followflag is defined for the reseal command but isn't utilized in this function.internal/controller/imagereseal/controller.go (1)
222-230: Consider usingptr.To(true)for cleaner pointer creation.The inline function for creating a bool pointer can be simplified using
k8s.io/utils/ptr.To(true)which is more idiomatic in Kubernetes codebases.Suggested fix
+import "k8s.io/utils/ptr" + OwnerReferences: []metav1.OwnerReference{ { APIVersion: automotivev1alpha1.GroupVersion.String(), Kind: "ImageReseal", Name: reseal.Name, UID: reseal.UID, - Controller: func(b bool) *bool { return &b }(true), + Controller: ptr.To(true), }, },
Comment on lines
+46
to
+48
| // BuilderImage is the osbuild builder container image to use | ||
| // Required for reseal operations - the builder runs rpm-ostree compose | ||
| BuilderImage string `json:"builderImage,omitempty"` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Read the imagereseal_types.go file to see full context
cat -n api/v1alpha1/imagereseal_types.goRepository: centos-automotive-suite/automotive-dev-operator
Length of output: 5846
🏁 Script executed:
# Search for reseal task implementation to verify empty-check behavior
rg -A 5 -B 2 "BuilderImage" --type goRepository: centos-automotive-suite/automotive-dev-operator
Length of output: 10345
🏁 Script executed:
# Check for kubebuilder validation patterns in the api/v1alpha1 directory
rg "kubebuilder:validation" api/v1alpha1/ --type go | head -20Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 1986
🏁 Script executed:
# Check Go version requirements
cat -n go.mod | head -5Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 222
Add +kubebuilder:validation:Required and remove omitempty from BuilderImage.
The field is documented as required and the API handler explicitly rejects empty values, but lacks CRD-level schema validation. This allows invalid CRs to pass admission and fail at runtime. Align it with SourceContainer and TargetContainer, which follow the required field pattern.
After making this change, run make generate manifests to regenerate manifests and DeepCopy methods.
🛠️ Suggested schema fix
// BuilderImage is the osbuild builder container image to use
// Required for reseal operations - the builder runs rpm-ostree compose
-BuilderImage string `json:"builderImage,omitempty"`
+// +kubebuilder:validation:Required
+// +kubebuilder:validation:MinLength=1
+BuilderImage string `json:"builderImage"`📝 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
| // BuilderImage is the osbuild builder container image to use | |
| // Required for reseal operations - the builder runs rpm-ostree compose | |
| BuilderImage string `json:"builderImage,omitempty"` | |
| // BuilderImage is the osbuild builder container image to use | |
| // Required for reseal operations - the builder runs rpm-ostree compose | |
| // +kubebuilder:validation:Required | |
| // +kubebuilder:validation:MinLength=1 | |
| BuilderImage string `json:"builderImage"` |
🤖 Prompt for AI Agents
In `@api/v1alpha1/imagereseal_types.go` around lines 46 - 48, Update the
BuilderImage field to be required in the CRD schema by adding the kubebuilder
marker +kubebuilder:validation:Required above the BuilderImage declaration and
remove ",omitempty" from its json tag (change `json:"builderImage,omitempty"` to
`json:"builderImage"`), matching the SourceContainer/TargetContainer pattern;
after editing the BuilderImage field, run `make generate manifests` to
regenerate CRD manifests and DeepCopy code.
Comment on lines
+1751
to
+1763
| if builderImage == "" && fromBuild != "" { | ||
| // Fetch the ImageBuild to get its builder image | ||
| fmt.Printf("Looking up builder image from build: %s\n", fromBuild) | ||
| build, err := api.GetBuild(context.Background(), fromBuild) | ||
| if err != nil { | ||
| handleError(fmt.Errorf("failed to get build %s: %w", fromBuild, err)) | ||
| } | ||
| if build.BuilderImageUsed == "" { | ||
| handleError(fmt.Errorf("build %s does not have a recorded builder image", fromBuild)) | ||
| } | ||
| builderImage = build.BuilderImageUsed | ||
| fmt.Printf("Using builder image from build: %s\n", builderImage) | ||
| } |
There was a problem hiding this comment.
Use the existing ctx instead of context.Background().
Line 1754 creates a new context.Background() for the GetBuild call, but the function already has a valid ctx that should be used for consistency and proper cancellation propagation.
Suggested fix
if builderImage == "" && fromBuild != "" {
// Fetch the ImageBuild to get its builder image
fmt.Printf("Looking up builder image from build: %s\n", fromBuild)
- build, err := api.GetBuild(context.Background(), fromBuild)
+ build, err := api.GetBuild(ctx, fromBuild)
if err != nil {
handleError(fmt.Errorf("failed to get build %s: %w", fromBuild, err))
}📝 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
| if builderImage == "" && fromBuild != "" { | |
| // Fetch the ImageBuild to get its builder image | |
| fmt.Printf("Looking up builder image from build: %s\n", fromBuild) | |
| build, err := api.GetBuild(context.Background(), fromBuild) | |
| if err != nil { | |
| handleError(fmt.Errorf("failed to get build %s: %w", fromBuild, err)) | |
| } | |
| if build.BuilderImageUsed == "" { | |
| handleError(fmt.Errorf("build %s does not have a recorded builder image", fromBuild)) | |
| } | |
| builderImage = build.BuilderImageUsed | |
| fmt.Printf("Using builder image from build: %s\n", builderImage) | |
| } | |
| if builderImage == "" && fromBuild != "" { | |
| // Fetch the ImageBuild to get its builder image | |
| fmt.Printf("Looking up builder image from build: %s\n", fromBuild) | |
| build, err := api.GetBuild(ctx, fromBuild) | |
| if err != nil { | |
| handleError(fmt.Errorf("failed to get build %s: %w", fromBuild, err)) | |
| } | |
| if build.BuilderImageUsed == "" { | |
| handleError(fmt.Errorf("build %s does not have a recorded builder image", fromBuild)) | |
| } | |
| builderImage = build.BuilderImageUsed | |
| fmt.Printf("Using builder image from build: %s\n", builderImage) | |
| } |
🤖 Prompt for AI Agents
In `@cmd/caib/main.go` around lines 1751 - 1763, The GetBuild call is using
context.Background() instead of the existing ctx; change the api.GetBuild
invocation to use the function's ctx variable so cancellation and deadlines
propagate (replace api.GetBuild(context.Background(), fromBuild) with
api.GetBuild(ctx, fromBuild)), keeping the surrounding error handling logic that
checks build.BuilderImageUsed and assigns builderImage unchanged.
Comment on lines
+2206
to
+2293
| // Create registry secret if credentials provided | ||
| var envSecretRef string | ||
| if req.RegistryCredentials != nil && req.RegistryCredentials.Enabled { | ||
| envSecretRef, err = createRegistrySecret(ctx, k8sClient, namespace, req.Name+"-reseal", req.RegistryCredentials) | ||
| if err != nil { | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": fmt.Sprintf("error creating registry secret: %v", err)}) | ||
| return | ||
| } | ||
| } | ||
|
|
||
| // Create seal key secret if key provided directly | ||
| var sealKeySecretRef string | ||
| if req.SealKey != "" { | ||
| sealKeySecretRef = req.Name + "-seal-key" | ||
| sealKeySecret := &corev1.Secret{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: sealKeySecretRef, | ||
| Namespace: namespace, | ||
| Labels: map[string]string{ | ||
| "app.kubernetes.io/managed-by": "build-api", | ||
| "app.kubernetes.io/part-of": "automotive-dev", | ||
| "automotive.sdv.cloud.redhat.com/resource-type": "seal-key", | ||
| "automotive.sdv.cloud.redhat.com/reseal-name": req.Name, | ||
| }, | ||
| }, | ||
| Type: corev1.SecretTypeOpaque, | ||
| Data: map[string][]byte{ | ||
| "private-key": []byte(req.SealKey), | ||
| }, | ||
| } | ||
| if err := k8sClient.Create(ctx, sealKeySecret); err != nil { | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": fmt.Sprintf("error creating seal key secret: %v", err)}) | ||
| return | ||
| } | ||
| } | ||
|
|
||
| // Create seal key password secret if password provided | ||
| var sealKeyPasswordSecretRef string | ||
| if req.SealKeyPassword != "" { | ||
| sealKeyPasswordSecretRef = req.Name + "-seal-key-password" | ||
| passwordSecret := &corev1.Secret{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: sealKeyPasswordSecretRef, | ||
| Namespace: namespace, | ||
| Labels: map[string]string{ | ||
| "app.kubernetes.io/managed-by": "build-api", | ||
| "app.kubernetes.io/part-of": "automotive-dev", | ||
| "automotive.sdv.cloud.redhat.com/resource-type": "seal-key-password", | ||
| "automotive.sdv.cloud.redhat.com/reseal-name": req.Name, | ||
| }, | ||
| }, | ||
| Type: corev1.SecretTypeOpaque, | ||
| Data: map[string][]byte{ | ||
| "password": []byte(req.SealKeyPassword), | ||
| }, | ||
| } | ||
| if err := k8sClient.Create(ctx, passwordSecret); err != nil { | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": fmt.Sprintf("error creating seal key password secret: %v", err)}) | ||
| return | ||
| } | ||
| } | ||
|
|
||
| // Create the ImageReseal CR | ||
| imageReseal := &automotivev1alpha1.ImageReseal{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: req.Name, | ||
| Namespace: namespace, | ||
| Labels: map[string]string{ | ||
| "app.kubernetes.io/managed-by": "build-api", | ||
| "app.kubernetes.io/part-of": "automotive-dev", | ||
| }, | ||
| }, | ||
| Spec: automotivev1alpha1.ImageResealSpec{ | ||
| SourceContainer: req.SourceContainer, | ||
| TargetContainer: req.TargetContainer, | ||
| SealKeySecretRef: sealKeySecretRef, | ||
| SealKeyPasswordSecretRef: sealKeyPasswordSecretRef, | ||
| AutomotiveImageBuilder: req.AutomotiveImageBuilder, | ||
| BuilderImage: req.BuilderImage, | ||
| EnvSecretRef: envSecretRef, | ||
| StorageClass: req.StorageClass, | ||
| }, | ||
| } | ||
|
|
||
| if err := k8sClient.Create(ctx, imageReseal); err != nil { | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": fmt.Sprintf("error creating ImageReseal: %v", err)}) | ||
| return | ||
| } |
There was a problem hiding this comment.
Secrets may be orphaned if ImageReseal CR creation fails.
If createRegistrySecret, sealKeySecret, or passwordSecret succeeds but the subsequent k8sClient.Create(ctx, imageReseal) fails, the created secrets will be orphaned since owner references are only set after the CR is created.
Consider wrapping the secret creation in a cleanup handler or creating secrets after the CR exists.
Suggested approach
+ // Track created secrets for cleanup on failure
+ var createdSecrets []string
+ defer func() {
+ if err != nil {
+ // Cleanup orphaned secrets
+ for _, secretName := range createdSecrets {
+ _ = k8sClient.Delete(ctx, &corev1.Secret{
+ ObjectMeta: metav1.ObjectMeta{Name: secretName, Namespace: namespace},
+ })
+ }
+ }
+ }()
+
// Create registry secret if credentials provided
var envSecretRef string
if req.RegistryCredentials != nil && req.RegistryCredentials.Enabled {
envSecretRef, err = createRegistrySecret(ctx, k8sClient, namespace, req.Name+"-reseal", req.RegistryCredentials)
if err != nil {
c.JSON(http.StatusInternalServerError, gin.H{"error": fmt.Sprintf("error creating registry secret: %v", err)})
return
}
+ createdSecrets = append(createdSecrets, envSecretRef)
}🤖 Prompt for AI Agents
In `@internal/buildapi/server.go` around lines 2206 - 2293, The created secrets
(registry secret via createRegistrySecret, sealKeySecret/passwordSecret created
before ImageReseal) can become orphaned if k8sClient.Create(ctx, imageReseal)
fails; ensure secrets are either created after the ImageReseal exists or are
cleaned up on failure by: create the ImageReseal CR first (using
empty/placeholder secret refs if needed) or set OwnerReferences on each secret
to the ImageReseal before creating them, and if you choose to create secrets
first implement a deferred cleanup that deletes createRegistrySecret's secret
and the local sealKeySecret/passwordSecret when k8sClient.Create(ctx,
imageReseal) returns an error; update the logic around createRegistrySecret, the
sealKeySecret/passwordSecret creation blocks and the final
k8sClient.Create(imageReseal) call to perform one of these two approaches.
Comment on lines
+272
to
+282
| # Push resealed container to registry if target is specified | ||
| if [ -n "$CONTAINER_PUSH" ]; then | ||
| echo "Pushing resealed container to registry: $CONTAINER_PUSH" | ||
| push_cmd=(skopeo copy --authfile="$REGISTRY_AUTH_FILE" "containers-storage:$OUTPUT_CONTAINER" "docker://$CONTAINER_PUSH") | ||
| log_command "${push_cmd[@]}" | ||
| "${push_cmd[@]}" | ||
| echo "Resealed container pushed successfully to $CONTAINER_PUSH" | ||
|
|
||
| # Write result for Tekton | ||
| echo "$CONTAINER_PUSH" > /tekton/results/sealed-container | ||
| fi |
There was a problem hiding this comment.
Always write the sealed-container Tekton result.
The result file is only written when container-push is set; for local-only reseals the result is missing and downstream consumers can fail. Write the result in both branches.
🛠️ Suggested fix
if [ -n "$CONTAINER_PUSH" ]; then
echo "Pushing resealed container to registry: $CONTAINER_PUSH"
push_cmd=(skopeo copy --authfile="$REGISTRY_AUTH_FILE" "containers-storage:$OUTPUT_CONTAINER" "docker://$CONTAINER_PUSH")
log_command "${push_cmd[@]}"
"${push_cmd[@]}"
echo "Resealed container pushed successfully to $CONTAINER_PUSH"
# Write result for Tekton
echo "$CONTAINER_PUSH" > /tekton/results/sealed-container
+else
+ # Write result for Tekton (local-only reseal)
+ echo "$OUTPUT_CONTAINER" > /tekton/results/sealed-container
fi📝 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
| # Push resealed container to registry if target is specified | |
| if [ -n "$CONTAINER_PUSH" ]; then | |
| echo "Pushing resealed container to registry: $CONTAINER_PUSH" | |
| push_cmd=(skopeo copy --authfile="$REGISTRY_AUTH_FILE" "containers-storage:$OUTPUT_CONTAINER" "docker://$CONTAINER_PUSH") | |
| log_command "${push_cmd[@]}" | |
| "${push_cmd[@]}" | |
| echo "Resealed container pushed successfully to $CONTAINER_PUSH" | |
| # Write result for Tekton | |
| echo "$CONTAINER_PUSH" > /tekton/results/sealed-container | |
| fi | |
| # Push resealed container to registry if target is specified | |
| if [ -n "$CONTAINER_PUSH" ]; then | |
| echo "Pushing resealed container to registry: $CONTAINER_PUSH" | |
| push_cmd=(skopeo copy --authfile="$REGISTRY_AUTH_FILE" "containers-storage:$OUTPUT_CONTAINER" "docker://$CONTAINER_PUSH") | |
| log_command "${push_cmd[@]}" | |
| "${push_cmd[@]}" | |
| echo "Resealed container pushed successfully to $CONTAINER_PUSH" | |
| # Write result for Tekton | |
| echo "$CONTAINER_PUSH" > /tekton/results/sealed-container | |
| else | |
| # Write result for Tekton (local-only reseal) | |
| echo "$OUTPUT_CONTAINER" > /tekton/results/sealed-container | |
| fi |
🤖 Prompt for AI Agents
In `@internal/common/tasks/scripts/reseal_image.sh` around lines 272 - 282, The
script currently writes the Tekton result file
(/tekton/results/sealed-container) only inside the CONTAINER_PUSH branch; ensure
the result is always written. After the conditional push block that uses
push_cmd, log_command and the skopeo invocation (and also in the branch where
CONTAINER_PUSH is empty), write the value that represents the sealed image (use
the same variable used in the push branch, e.g., $CONTAINER_PUSH or
$OUTPUT_CONTAINER as appropriate) to /tekton/results/sealed-container so
downstream tasks always have the result; update reseal_image.sh to perform that
write in both branches.
Comment on lines
+161
to
+169
| } else { | ||
| // Task exists - check if it's managed by us before updating | ||
| if _, managed := existingTask.Labels["app.kubernetes.io/managed-by"]; managed { | ||
| existingTask.Spec = resealTask.Spec | ||
| if err := r.Update(ctx, existingTask); err != nil { | ||
| return nil, fmt.Errorf("failed to update reseal task: %w", err) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Task update check should verify the label value, not just presence.
The condition if _, managed := existingTask.Labels["app.kubernetes.io/managed-by"]; managed only checks if the label exists, but doesn't verify it was set by this controller. This could unintentionally overwrite a Task managed by another component.
Suggested fix
} else {
// Task exists - check if it's managed by us before updating
- if _, managed := existingTask.Labels["app.kubernetes.io/managed-by"]; managed {
+ if existingTask.Labels["app.kubernetes.io/managed-by"] == "automotive-dev-operator" {
existingTask.Spec = resealTask.Spec
if err := r.Update(ctx, existingTask); err != nil {
return nil, fmt.Errorf("failed to update reseal task: %w", err)
}
}
}📝 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
| } else { | |
| // Task exists - check if it's managed by us before updating | |
| if _, managed := existingTask.Labels["app.kubernetes.io/managed-by"]; managed { | |
| existingTask.Spec = resealTask.Spec | |
| if err := r.Update(ctx, existingTask); err != nil { | |
| return nil, fmt.Errorf("failed to update reseal task: %w", err) | |
| } | |
| } | |
| } | |
| } else { | |
| // Task exists - check if it's managed by us before updating | |
| if existingTask.Labels["app.kubernetes.io/managed-by"] == "automotive-dev-operator" { | |
| existingTask.Spec = resealTask.Spec | |
| if err := r.Update(ctx, existingTask); err != nil { | |
| return nil, fmt.Errorf("failed to update reseal task: %w", err) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@internal/controller/imagereseal/controller.go` around lines 161 - 169, The
code currently checks only for existence of the "app.kubernetes.io/managed-by"
label on existingTask before overwriting; change the condition to verify the
label value equals this controller's identifier (compare
existingTask.Labels["app.kubernetes.io/managed-by"] == "<controller-name>")
instead of just presence, and if it doesn't match, do not update and return or
skip with a clear error/permission message; adjust the block that assigns
existingTask.Spec = resealTask.Spec and calls r.Update(ctx, existingTask) to run
only when the label value matches, and handle the non-matching case (e.g.,
return nil with a descriptive fmt.Errorf or skip update) so you never overwrite
tasks managed by other controllers.
Comment on lines
+288
to
+295
| // cleanupTransientSecrets deletes any transient secrets created for this reseal | ||
| func (r *Reconciler) cleanupTransientSecrets(ctx context.Context, reseal *automotivev1alpha1.ImageReseal, log logr.Logger) { | ||
| // Cleanup env secret if it was created transiently | ||
| if reseal.Spec.EnvSecretRef != "" { | ||
| r.deleteSecretWithRetry(ctx, reseal.Namespace, reseal.Spec.EnvSecretRef, "registry auth", log) | ||
| } | ||
| // Note: We don't delete seal key secrets as they may be reused | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check for owner reference setup related to seal secrets
rg -n 'setResealSecretOwnerRef.*seal|OwnerReferences.*seal' internal/buildapi/server.goRepository: centos-automotive-suite/automotive-dev-operator
Length of output: 433
🏁 Script executed:
# Also search more broadly for owner reference setups in the buildapi package
rg -n 'OwnerReferences' internal/buildapi/server.go | head -20Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 317
🏁 Script executed:
# Let's check the actual lines mentioned in the review (2296-2310)
sed -n '2296,2310p' internal/buildapi/server.goRepository: centos-automotive-suite/automotive-dev-operator
Length of output: 858
🏁 Script executed:
# Check the controller file to understand the context better
sed -n '280,300p' internal/controller/imagereseal/controller.goRepository: centos-automotive-suite/automotive-dev-operator
Length of output: 934
🏁 Script executed:
# Search for where seal key secrets are created or handled
rg -n 'seal.*secret|sealKeySecret' internal/buildapi/server.go -A 3 -B 3 | head -50Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 2016
Resolve the inconsistency between seal key secret owner references and the cleanup comment.
Owner references are set on seal key secrets (lines 2297, 2302 in server.go), which means they will be garbage collected when the ImageReseal is deleted. This contradicts the comment stating "they may be reused." Either:
- Remove owner references from seal key secrets if they're intended for reuse across multiple ImageReseals, or
- Update the comment and cleanup logic to reflect that all transient secrets will be garbage collected, or
- If selective cleanup is necessary, clarify the distinction in the comment (e.g., "sealed key secrets are lifecycle-managed via owner references" vs "env secrets are transiently created and must be manually deleted").
🤖 Prompt for AI Agents
In `@internal/controller/imagereseal/controller.go` around lines 288 - 295, The
comment in cleanupTransientSecrets (function cleanupTransientSecrets)
contradicts the fact that seal key secrets have ownerReferences set in server.go
(the code that creates the seal key secrets) — decide which behavior you want:
if seal keys should be reusable, remove the ownerReferences where seal key
secrets are created in server.go so they are not GC'd and keep/delete logic
as-is (or add explicit deletion via deleteSecretWithRetry); otherwise, keep the
ownerReferences and update cleanupTransientSecrets' comment and logic to reflect
that seal key secrets are lifecycle-managed by ownerReferences (i.e., they will
be garbage collected and should not be manually deleted). Ensure the chosen
change touches either the owner reference assignment in the seal-key creation
code or the comment/cleanup logic in cleanupTransientSecrets and reference
deleteSecretWithRetry if adding explicit deletion.
bennyz
force-pushed
the
seal-reseal-v2
branch
from
4604308 to
c9124ba
Compare
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
bennyz
force-pushed
the
seal-reseal-v2
branch
from
c9124ba to
46c25fc
Compare
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
1 participant
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.
Summary by CodeRabbit
resealCLI command with configurable options for builder image, storage class, and timeout settings✏️ Tip: You can customize this high-level summary in your review settings.