feat: implemented artifact cleanup#554
Open
rebEllieous wants to merge 1 commit into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR introduces ChangesRenderArtifact and RenderBinding resource system
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/solar/renderartifact_rest.go`:
- Around line 49-56: Add explicit enforcement for required RenderArtifactSpec
fields: in PrepareForCreate ensure o.Spec.BaseURL, o.Spec.Repository, o.Spec.Tag
and o.Spec.RenderTaskRef are non-empty and emit/return a validation error if any
are missing; in PrepareForUpdate, if any of those fields on the incoming o are
empty, copy the values from the old.(*RenderArtifact).Spec to prevent emptying
required fields, and still call incrementGenerationIfNotEqual as before.
Reference RenderArtifact, PrepareForCreate, PrepareForUpdate, RenderArtifactSpec
and its fields baseURL/Repository/Tag/RenderTaskRef when making the changes.
In `@api/solar/renderbinding_rest.go`:
- Around line 42-49: Add explicit validation in RenderBinding.PrepareForCreate
and RenderBinding.PrepareForUpdate to ensure RenderBindingSpec required fields
are non-empty: check o.Spec.RenderArtifactRef.Name, o.Spec.OwnerKind,
o.Spec.OwnerName, and o.Spec.OwnerNamespace; on create, validate the new object
and on update compare against the old object (or := old.(*RenderBinding)) and
validate the incoming o.Spec before incrementing Generation. If any required
field is empty, surface an API validation error (use the repo's existing
validation/error helpers or return an apiserver validation error consistent with
other api/solar/*_rest.go lifecycles) so creation/update is rejected.
In `@charts/solar/templates/controller/deployment.yaml`:
- Around line 123-129: When caBundle.enabled is true the template blindly uses
caBundle.configMapName which may be empty; update the controller/deployment.yaml
fragment to guard or require caBundle.configMapName before rendering the
ConfigMap volume: either wrap the volume block with an additional condition that
checks (ne .Values.caBundle.configMapName "") or replace the direct usage with
Helm's required function (e.g. required "message"
.Values.caBundle.configMapName) so that caBundle.configMapName (and optionally
caBundle.key) are validated and you never emit name: "" for the ca-bundle
volume.
In `@pkg/controller/renderartifact_controller.go`:
- Around line 150-151: The code constructs rawRef from artifact.Spec.BaseURL
without normalizing, which can include "oci://" prefixes or trailing slashes and
cause mismatched auth host lookups; normalize BaseURL once (strip "oci://"
prefix and any trailing slashes) into a local variable (e.g., normalizedBase)
and use that when building rawRef and when performing Docker/OCI auth host
lookups, replacing usages of artifact.Spec.BaseURL with normalizedBase (ensure
rawRef becomes normalizedBase + "/" + artifact.Spec.Repository + ":" +
artifact.Spec.Tag and any auth map key lookups use normalizedBase or the
host-only form derived from it).
- Line 160: The OCI tag deletion (DeleteTag) is a network call invoked via
deleteFn and needs a local timeout to avoid hanging the reconcile worker; wrap
the call to deleteFn(ctx, rawRef, auth) with a context.WithTimeout (choose a
sensible timeout like 15–30s), use the derived ctx (and defer cancel()) when
calling deleteFn, and ensure the context and time packages are imported so the
DeleteTag/backing network call is cancelled if it exceeds the timeout.
In `@pkg/controller/target_controller.go`:
- Around line 491-498: The bootstrap ensure errors are being logged but
swallowed; change the reconcile flow so failures from
r.ensureRenderArtifact(ctx, bootstrapArtifactName, bootstrapRT,
registry.Spec.Flavor) and r.ensureRenderBinding(ctx, target,
bootstrapArtifactName, bootstrapBindingName) are returned to the caller instead
of only logged. Locate the block using
bootstrapArtifactName/bootstrapBindingName and the calls to ensureRenderArtifact
and ensureRenderBinding in target_controller.go and replace the current log-only
branches with error propagation (return err) so reconcile fails when these
ensures fail, preserving the original error context in the returned error.
- Around line 228-234: The no-bindings branch currently swallows errors from
cleanup by only logging them; change it to return the error so the reconcile is
requeued and retries cleanup. Specifically, in the section that calls
r.deleteStaleRenderTasks(ctx, target, map[string]struct{}{}) and
r.deleteStaleRenderBindings(ctx, target, map[string]struct{}{}), replace the
log-only handling with error returns (e.g., if err != nil { log.Error(err,
"..."); return ctrl.Result{}, err }) so failures in deleteStaleRenderTasks and
deleteStaleRenderBindings cause the controller to retry reconciliation.
In `@test/e2e/e2e_test.go`:
- Around line 1022-1028: The test currently treats any error from
repo.FetchReference as success, which can hide transient failures; update the
Eventually block that calls zotClient.Repository and repo.FetchReference to
assert that the returned error is specifically a not-found sentinel by using
errors.Is(err, errdef.ErrNotFound) (importing errors and errdef if not present)
instead of g.Expect(err).To(HaveOccurred()); ensure the failure message still
references artTag and releaseRepo() but only consider the check succeeded when
errors.Is indicates ErrNotFound.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fd3f4d1b-0e95-4b59-8dfa-5a4598b5904b
📒 Files selected for processing (51)
api/solar/register.goapi/solar/renderartifact_rest.goapi/solar/renderartifact_types.goapi/solar/renderbinding_rest.goapi/solar/renderbinding_types.goapi/solar/v1alpha1/register.goapi/solar/v1alpha1/renderartifact_types.goapi/solar/v1alpha1/renderbinding_types.goapi/solar/v1alpha1/zz_generated.conversion.goapi/solar/v1alpha1/zz_generated.deepcopy.goapi/solar/v1alpha1/zz_generated.model_name.goapi/solar/zz_generated.deepcopy.gocharts/solar/files/role.yamlcharts/solar/templates/controller/deployment.yamlcharts/solar/values.yamlclient-go/applyconfigurations/solar/v1alpha1/renderartifact.goclient-go/applyconfigurations/solar/v1alpha1/renderartifactspec.goclient-go/applyconfigurations/solar/v1alpha1/renderartifactstatus.goclient-go/applyconfigurations/solar/v1alpha1/renderbinding.goclient-go/applyconfigurations/solar/v1alpha1/renderbindingspec.goclient-go/applyconfigurations/utils.goclient-go/clientset/versioned/typed/solar/v1alpha1/fake/fake_renderartifact.goclient-go/clientset/versioned/typed/solar/v1alpha1/fake/fake_renderbinding.goclient-go/clientset/versioned/typed/solar/v1alpha1/fake/fake_solar_client.goclient-go/clientset/versioned/typed/solar/v1alpha1/generated_expansion.goclient-go/clientset/versioned/typed/solar/v1alpha1/renderartifact.goclient-go/clientset/versioned/typed/solar/v1alpha1/renderbinding.goclient-go/clientset/versioned/typed/solar/v1alpha1/solar_client.goclient-go/informers/externalversions/generic.goclient-go/informers/externalversions/solar/v1alpha1/interface.goclient-go/informers/externalversions/solar/v1alpha1/renderartifact.goclient-go/informers/externalversions/solar/v1alpha1/renderbinding.goclient-go/listers/solar/v1alpha1/expansion_generated.goclient-go/listers/solar/v1alpha1/renderartifact.goclient-go/listers/solar/v1alpha1/renderbinding.goclient-go/openapi/zz_generated.openapi.gocmd/solar-apiserver/main.gocmd/solar-controller-manager/main.godocs/user-guide/api-reference.mdpkg/controller/helpers.gopkg/controller/renderartifact_controller.gopkg/controller/renderartifact_controller_test.gopkg/controller/suite_test.gopkg/controller/target_controller.gopkg/controller/target_controller_test.gopkg/ociregistry/deleter.gopkg/ociregistry/ociregistry_test.gopkg/ociregistry/standard.gotest/e2e/e2e_suite_test.gotest/e2e/e2e_test.gotest/fixtures/solar.values.yaml
Comment on lines
+49
to
+56
| func (o *RenderArtifact) PrepareForCreate(ctx context.Context) { | ||
| o.Generation = 1 | ||
| } | ||
|
|
||
| func (o *RenderArtifact) PrepareForUpdate(ctx context.Context, old runtime.Object) { | ||
| or := old.(*RenderArtifact) | ||
| incrementGenerationIfNotEqual(o, o.Spec, or.Spec) | ||
| } |
Contributor
There was a problem hiding this comment.
Enforce RenderArtifactSpec invariants in lifecycle validation.
Line 49 and Line 53 only manage generation; they do not enforce non-empty required spec values (baseURL, repository, tag, renderTaskRef). That allows invalid artifacts to be stored and fail later during reconciliation/GC.
Based on learnings: field-level API validation in this repo is enforced via custom logic in lifecycle methods like PrepareForCreate/PrepareForUpdate in api/solar/*_rest.go.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/solar/renderartifact_rest.go` around lines 49 - 56, Add explicit
enforcement for required RenderArtifactSpec fields: in PrepareForCreate ensure
o.Spec.BaseURL, o.Spec.Repository, o.Spec.Tag and o.Spec.RenderTaskRef are
non-empty and emit/return a validation error if any are missing; in
PrepareForUpdate, if any of those fields on the incoming o are empty, copy the
values from the old.(*RenderArtifact).Spec to prevent emptying required fields,
and still call incrementGenerationIfNotEqual as before. Reference
RenderArtifact, PrepareForCreate, PrepareForUpdate, RenderArtifactSpec and its
fields baseURL/Repository/Tag/RenderTaskRef when making the changes.
Comment on lines
+42
to
+49
| func (o *RenderBinding) PrepareForCreate(ctx context.Context) { | ||
| o.Generation = 1 | ||
| } | ||
|
|
||
| func (o *RenderBinding) PrepareForUpdate(ctx context.Context, old runtime.Object) { | ||
| or := old.(*RenderBinding) | ||
| incrementGenerationIfNotEqual(o, o.Spec, or.Spec) | ||
| } |
Contributor
There was a problem hiding this comment.
Add create/update validation for RenderBindingSpec required fields.
Line 42 and Line 46 currently only set/increment generation. There is no enforcement for required non-empty identity fields (renderArtifactRef.name, ownerKind, ownerName, ownerNamespace), which can break binding-based reference counting.
Based on learnings: field-level API validation in this repo is enforced via custom logic in lifecycle methods like PrepareForCreate/PrepareForUpdate in api/solar/*_rest.go.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/solar/renderbinding_rest.go` around lines 42 - 49, Add explicit
validation in RenderBinding.PrepareForCreate and RenderBinding.PrepareForUpdate
to ensure RenderBindingSpec required fields are non-empty: check
o.Spec.RenderArtifactRef.Name, o.Spec.OwnerKind, o.Spec.OwnerName, and
o.Spec.OwnerNamespace; on create, validate the new object and on update compare
against the old object (or := old.(*RenderBinding)) and validate the incoming
o.Spec before incrementing Generation. If any required field is empty, surface
an API validation error (use the repo's existing validation/error helpers or
return an apiserver validation error consistent with other api/solar/*_rest.go
lifecycles) so creation/update is rejected.
Comment on lines
+123
to
+129
| {{- if .Values.caBundle.enabled }} | ||
| - name: ca-bundle | ||
| configMap: | ||
| name: {{ .Values.caBundle.configMapName }} | ||
| items: | ||
| - key: {{ .Values.caBundle.key }} | ||
| path: ca-bundle.pem |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify default is empty and template is currently unguarded.
rg -n 'caBundle:' charts/solar/values.yaml -A8 -B1
rg -n 'name:\s*\{\{\s*\.Values\.caBundle\.configMapName\s*\}\}' charts/solar/templates/controller/deployment.yaml -A3 -B2Repository: opendefensecloud/solution-arsenal
Length of output: 662
Guard caBundle inputs to prevent invalid ConfigMap volume at runtime.
caBundle.enabled only gates rendering; when it’s true, the template uses caBundle.configMapName directly. In charts/solar/values.yaml, caBundle.configMapName defaults to "", so Helm can emit name: "" for the ca-bundle ConfigMap volume, causing a runtime deploy failure. Add required guards for at least caBundle.configMapName (guarding caBundle.key is optional since it already defaults to "trust-bundle.pem").
Suggested fix
- name: ca-bundle
configMap:
- name: {{ .Values.caBundle.configMapName }}
+ name: {{ required "caBundle.configMapName must be set when caBundle.enabled=true" .Values.caBundle.configMapName }}
items:
- - key: {{ .Values.caBundle.key }}
+ - key: {{ required "caBundle.key must be set when caBundle.enabled=true" .Values.caBundle.key }}
path: ca-bundle.pem📝 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 .Values.caBundle.enabled }} | |
| - name: ca-bundle | |
| configMap: | |
| name: {{ .Values.caBundle.configMapName }} | |
| items: | |
| - key: {{ .Values.caBundle.key }} | |
| path: ca-bundle.pem | |
| {{- if .Values.caBundle.enabled }} | |
| - name: ca-bundle | |
| configMap: | |
| name: {{ required "caBundle.configMapName must be set when caBundle.enabled=true" .Values.caBundle.configMapName }} | |
| items: | |
| - key: {{ required "caBundle.key must be set when caBundle.enabled=true" .Values.caBundle.key }} | |
| path: ca-bundle.pem |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@charts/solar/templates/controller/deployment.yaml` around lines 123 - 129,
When caBundle.enabled is true the template blindly uses caBundle.configMapName
which may be empty; update the controller/deployment.yaml fragment to guard or
require caBundle.configMapName before rendering the ConfigMap volume: either
wrap the volume block with an additional condition that checks (ne
.Values.caBundle.configMapName "") or replace the direct usage with Helm's
required function (e.g. required "message" .Values.caBundle.configMapName) so
that caBundle.configMapName (and optionally caBundle.key) are validated and you
never emit name: "" for the ca-bundle volume.
Comment on lines
+150
to
+151
| rawRef := artifact.Spec.BaseURL + "/" + artifact.Spec.Repository + ":" + artifact.Spec.Tag | ||
| log.V(1).Info("Attempting OCI tag cleanup", "ref", rawRef) |
Contributor
There was a problem hiding this comment.
Normalize BaseURL before building the OCI delete ref and auth lookup host.
Using artifact.Spec.BaseURL directly can break cleanup when it carries oci:// or trailing slashes, and can also miss Docker auth map keys. Normalize once and use the normalized host consistently.
Proposed fix
import (
"context"
"encoding/json"
"slices"
+ "strings"
@@
func (r *RenderArtifactReconciler) cleanupOCIArtifact(ctx context.Context, artifact *solarv1alpha1.RenderArtifact) error {
log := ctrl.LoggerFrom(ctx)
- rawRef := artifact.Spec.BaseURL + "/" + artifact.Spec.Repository + ":" + artifact.Spec.Tag
+ registryHost := strings.TrimPrefix(strings.TrimSuffix(artifact.Spec.BaseURL, "/"), "oci://")
+ rawRef := registryHost + "/" + strings.TrimPrefix(artifact.Spec.Repository, "/") + ":" + artifact.Spec.Tag
log.V(1).Info("Attempting OCI tag cleanup", "ref", rawRef)
@@
- auth := r.resolveAuth(ctx, artifact)
+ auth := r.resolveAuth(ctx, artifact, registryHost)
@@
-func (r *RenderArtifactReconciler) resolveAuth(ctx context.Context, artifact *solarv1alpha1.RenderArtifact) authn.Authenticator {
+func (r *RenderArtifactReconciler) resolveAuth(ctx context.Context, artifact *solarv1alpha1.RenderArtifact, registryHost string) authn.Authenticator {
@@
- return ociAuthFromSecret(secret, artifact.Spec.BaseURL)
+ return ociAuthFromSecret(secret, registryHost)
}Also applies to: 211-212
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/controller/renderartifact_controller.go` around lines 150 - 151, The code
constructs rawRef from artifact.Spec.BaseURL without normalizing, which can
include "oci://" prefixes or trailing slashes and cause mismatched auth host
lookups; normalize BaseURL once (strip "oci://" prefix and any trailing slashes)
into a local variable (e.g., normalizedBase) and use that when building rawRef
and when performing Docker/OCI auth host lookups, replacing usages of
artifact.Spec.BaseURL with normalizedBase (ensure rawRef becomes normalizedBase
+ "/" + artifact.Spec.Repository + ":" + artifact.Spec.Tag and any auth map key
lookups use normalizedBase or the host-only form derived from it).
|
|
||
| auth := r.resolveAuth(ctx, artifact) | ||
|
|
||
| if err := deleteFn(ctx, rawRef, auth); err != nil { |
Contributor
There was a problem hiding this comment.
Add a timeout around OCI tag deletion calls.
DeleteTag is an external network call on the reconcile path; without a local timeout, a hung registry can stall workers.
Proposed fix
import (
"context"
"encoding/json"
"slices"
+ "time"
@@
- if err := deleteFn(ctx, rawRef, auth); err != nil {
+ deleteCtx, cancel := context.WithTimeout(ctx, 30*time.Second)
+ defer cancel()
+ if err := deleteFn(deleteCtx, rawRef, auth); err != 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
| if err := deleteFn(ctx, rawRef, auth); err != nil { | |
| deleteCtx, cancel := context.WithTimeout(ctx, 30*time.Second) | |
| defer cancel() | |
| if err := deleteFn(deleteCtx, rawRef, auth); err != nil { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/controller/renderartifact_controller.go` at line 160, The OCI tag
deletion (DeleteTag) is a network call invoked via deleteFn and needs a local
timeout to avoid hanging the reconcile worker; wrap the call to deleteFn(ctx,
rawRef, auth) with a context.WithTimeout (choose a sensible timeout like
15–30s), use the derived ctx (and defer cancel()) when calling deleteFn, and
ensure the context and time packages are imported so the DeleteTag/backing
network call is cancelled if it exceeds the timeout.
Comment on lines
+228
to
+234
| // Clean up any stale RenderTasks and RenderBindings left from prior reconciles. | ||
| if err := r.deleteStaleRenderTasks(ctx, target, map[string]struct{}{}); err != nil { | ||
| log.Error(err, "failed to clean up stale RenderTasks after all bindings removed") | ||
| } | ||
| if err := r.deleteStaleRenderBindings(ctx, target, map[string]struct{}{}); err != nil { | ||
| log.Error(err, "failed to clean up stale RenderBindings after all bindings removed") | ||
| } |
Contributor
There was a problem hiding this comment.
Retry when stale cleanup fails in the no-bindings branch.
Only logging cleanup failures here can leave stale RenderTasks/RenderBindings indefinitely if no further events requeue the Target.
Proposed fix
- if err := r.deleteStaleRenderTasks(ctx, target, map[string]struct{}{}); err != nil {
- log.Error(err, "failed to clean up stale RenderTasks after all bindings removed")
- }
- if err := r.deleteStaleRenderBindings(ctx, target, map[string]struct{}{}); err != nil {
- log.Error(err, "failed to clean up stale RenderBindings after all bindings removed")
- }
+ if err := r.deleteStaleRenderTasks(ctx, target, map[string]struct{}{}); err != nil {
+ return ctrl.Result{}, errLogAndWrap(log, err, "failed to clean up stale RenderTasks after all bindings removed")
+ }
+ if err := r.deleteStaleRenderBindings(ctx, target, map[string]struct{}{}); err != nil {
+ return ctrl.Result{}, errLogAndWrap(log, err, "failed to clean up stale RenderBindings after all bindings removed")
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/controller/target_controller.go` around lines 228 - 234, The no-bindings
branch currently swallows errors from cleanup by only logging them; change it to
return the error so the reconcile is requeued and retries cleanup. Specifically,
in the section that calls r.deleteStaleRenderTasks(ctx, target,
map[string]struct{}{}) and r.deleteStaleRenderBindings(ctx, target,
map[string]struct{}{}), replace the log-only handling with error returns (e.g.,
if err != nil { log.Error(err, "..."); return ctrl.Result{}, err }) so failures
in deleteStaleRenderTasks and deleteStaleRenderBindings cause the controller to
retry reconciliation.
Comment on lines
+491
to
+498
| // Ensure RenderArtifact + RenderBinding exist for the bootstrap chart. | ||
| bootstrapArtifactName := renderArtifactName(target.Namespace, bootstrapRT.Spec.BaseURL, bootstrapRT.Spec.Repository, bootstrapRT.Spec.Tag) | ||
| bootstrapBindingName := renderBindingName(bootstrapArtifactName, target.Name) | ||
| if err := r.ensureRenderArtifact(ctx, bootstrapArtifactName, bootstrapRT, registry.Spec.Flavor); err != nil { | ||
| log.Error(err, "failed to ensure RenderArtifact for bootstrap") | ||
| } else if err := r.ensureRenderBinding(ctx, target, bootstrapArtifactName, bootstrapBindingName); err != nil { | ||
| log.Error(err, "failed to ensure RenderBinding for bootstrap") | ||
| } |
Contributor
There was a problem hiding this comment.
Do not swallow bootstrap artifact/binding ensure errors.
If these calls fail and reconcile still returns success, bootstrap tracking can drift and GC/reference-count behavior becomes nondeterministic.
Proposed fix
if err := r.ensureRenderArtifact(ctx, bootstrapArtifactName, bootstrapRT, registry.Spec.Flavor); err != nil {
- log.Error(err, "failed to ensure RenderArtifact for bootstrap")
- } else if err := r.ensureRenderBinding(ctx, target, bootstrapArtifactName, bootstrapBindingName); err != nil {
- log.Error(err, "failed to ensure RenderBinding for bootstrap")
+ return ctrl.Result{}, errLogAndWrap(log, err, "failed to ensure RenderArtifact for bootstrap")
+ }
+ if err := r.ensureRenderBinding(ctx, target, bootstrapArtifactName, bootstrapBindingName); err != nil {
+ return ctrl.Result{}, errLogAndWrap(log, err, "failed to ensure RenderBinding for bootstrap")
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/controller/target_controller.go` around lines 491 - 498, The bootstrap
ensure errors are being logged but swallowed; change the reconcile flow so
failures from r.ensureRenderArtifact(ctx, bootstrapArtifactName, bootstrapRT,
registry.Spec.Flavor) and r.ensureRenderBinding(ctx, target,
bootstrapArtifactName, bootstrapBindingName) are returned to the caller instead
of only logged. Locate the block using
bootstrapArtifactName/bootstrapBindingName and the calls to ensureRenderArtifact
and ensureRenderBinding in target_controller.go and replace the current log-only
branches with error propagation (return err) so reconcile fails when these
ensures fail, preserving the original error context in the returned error.
Comment on lines
+1022
to
+1028
| Eventually(func(g Gomega) { | ||
| repo, err := zotClient.Repository(ociCtx, releaseRepo()) | ||
| g.Expect(err).NotTo(HaveOccurred(), "should connect to zot-deploy") | ||
| _, _, err = repo.FetchReference(ociCtx, artTag) | ||
| g.Expect(err).To(HaveOccurred(), | ||
| "OCI tag %s in %s should no longer exist after GC", artTag, releaseRepo()) | ||
| }).Should(Succeed()) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file
ls -la test/e2e/e2e_test.go
# Show relevant line ranges around 1020-1040
sed -n '1000,1060p' test/e2e/e2e_test.go | nl -ba | sed -n '1,120p'
# Show relevant line ranges around 1125-1155
sed -n '1100,1160p' test/e2e/e2e_test.go | nl -ba | sed -n '1,140p'
# Also show imports section to see current imports around errors/errdef
# Print top of file through import block
sed -n '1,120p' test/e2e/e2e_test.go | nl -ba | sed -n '1,140p'Repository: opendefensecloud/solution-arsenal
Length of output: 209
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file
ls -la test/e2e/e2e_test.go
# Show relevant line ranges around 1020-1040
sed -n '1000,1060p' test/e2e/e2e_test.go | nl -ba | sed -n '1,120p'
# Show relevant line ranges around 1125-1155
sed -n '1100,1160p' test/e2e/e2e_test.go | nl -ba | sed -n '1,140p'
# Also show imports section to see current imports around errors/errdef
sed -n '1,140p' test/e2e/e2e_test.go | nl -ba | sed -n '1,200p'Repository: opendefensecloud/solution-arsenal
Length of output: 209
🌐 Web query:
oras.land oras-go v2 errdef ErrNotFound FetchReference
💡 Result:
In oras.land/oras-go/v2, a FetchReference call returns the package-level sentinel error errdef.ErrNotFound when the registry responds with HTTP 404 (status not found). That sentinel error is defined as errors.New("missing reference"). [1][2] How FetchReference fits into ORAS v2: - oras-go v2 has an internal content.Fetch path that checks whether the target implements registry.ReferenceFetcher; if so, it calls refFetcher.FetchReference(ctx, reference). [3] - The oras-go documentation/API describes Targets: Resolve resolves a tag string to a descriptor, while Fetch retrieves content identified by a descriptor from the CAS; ReferenceFetcher.FetchReference is the optimized path that fetches by reference directly when supported. [4][3] Therefore, encountering "oras-go v2 ErrNotFound FetchReference" typically means ORAS attempted to fetch a manifest/artifact by reference (tag/digest string) and the reference was missing on the registry, resulting in ErrNotFound/"missing reference". [1][2]
Citations:
- 1: https://github.com/oras-project/oras-go/blob/main/registry/remote/registry.go
- 2: https://pkg.go.dev/oras.land/oras-go/v2/errdef
- 3: https://github.com/oras-project/oras-go/blob/42c1a3adcfba8ae373e3fe53308867f0c7357dc2/content.go
- 4: https://github.com/oras-project/oras-go/blob/main/docs/Targets.md
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat go.mod | rg -n "oras\.land/oras-go/v2" || trueRepository: opendefensecloud/solution-arsenal
Length of output: 109
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "FetchReference\\(" -S test/e2e/e2e_test.goRepository: opendefensecloud/solution-arsenal
Length of output: 366
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "errdef|ErrNotFound|not found" test/e2e/e2e_test.goRepository: opendefensecloud/solution-arsenal
Length of output: 59
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "not[- ]found|ErrNotFound|Err.*NotFound|errors\\.Is\\(err" test/e2e/e2e_test.goRepository: opendefensecloud/solution-arsenal
Length of output: 645
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("test/e2e/e2e_test.go")
lines = p.read_text().splitlines()
start, end = 980, 1060
for i in range(start, end+1):
if i < len(lines):
print(f"{i+1:5d}:\t{lines[i]}")
PYRepository: opendefensecloud/solution-arsenal
Length of output: 4355
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("test/e2e/e2e_test.go")
lines = p.read_text().splitlines()
start, end = 1090, 1170
for i in range(start, end+1):
if i < len(lines):
print(f"{i+1:5d}:\t{lines[i]}")
PYRepository: opendefensecloud/solution-arsenal
Length of output: 4387
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("test/e2e/e2e_test.go")
lines = p.read_text().splitlines()
# print import block area
for i in range(0, 180):
if i < len(lines):
print(f"{i+1:5d}:\t{lines[i]}")
PYRepository: opendefensecloud/solution-arsenal
Length of output: 8028
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "errdef|ErrNotFound|errors\\.Is\\(err" test/e2e/e2e_test.go || trueRepository: opendefensecloud/solution-arsenal
Length of output: 59
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("test/e2e/e2e_test.go")
lines = p.read_text().splitlines()
start, end = 980, 1060
for i in range(start, end+1):
if i < len(lines):
print(f"{i+1:5d}:\t{lines[i]}")
PYRepository: opendefensecloud/solution-arsenal
Length of output: 4355
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("test/e2e/e2e_test.go")
lines = p.read_text().splitlines()
start, end = 1090, 1170
for i in range(start, end+1):
if i < len(lines):
print(f"{i+1:5d}:\t{lines[i]}")
PYRepository: opendefensecloud/solution-arsenal
Length of output: 4387
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("test/e2e/e2e_test.go")
lines = p.read_text().splitlines()
for i in range(1, 220):
if i <= len(lines):
print(f"{i:5d}:\t{lines[i-1]}")
PYRepository: opendefensecloud/solution-arsenal
Length of output: 9729
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "errdef|ErrNotFound|errors\\.Is\\(err" test/e2e/e2e_test.go || trueRepository: opendefensecloud/solution-arsenal
Length of output: 59
Assert OCI tag “not found” specifically for GC checks.
test/e2e/e2e_test.go lines 1026 and 1137 (1133-1139) currently treat any repo.FetchReference error as success, which can mask transient/auth/network issues. Match the ORAS sentinel not-found error (errdef.ErrNotFound) via errors.Is.
Suggested fix
import (
"context"
+ "errors"
"fmt"
"os"
"os/exec"
"path/filepath"
"strings"
"time"
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
+ "oras.land/oras-go/v2/errdef"
)
@@
- _, _, err = repo.FetchReference(ociCtx, artTag)
- g.Expect(err).To(HaveOccurred(),
+ _, _, err = repo.FetchReference(ociCtx, artTag)
+ g.Expect(errors.Is(err, errdef.ErrNotFound)).To(BeTrue(),
"OCI tag %s in %s should no longer exist after GC", artTag, releaseRepo())
@@
- _, _, err = repo.FetchReference(ociCtx, artTag3)
- g.Expect(err).To(HaveOccurred(),
+ _, _, err = repo.FetchReference(ociCtx, artTag3)
+ g.Expect(errors.Is(err, errdef.ErrNotFound)).To(BeTrue(),
"OCI tag %s in %s should no longer exist after GC via stale-binding path", artTag3, releaseRepo())🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e/e2e_test.go` around lines 1022 - 1028, The test currently treats any
error from repo.FetchReference as success, which can hide transient failures;
update the Eventually block that calls zotClient.Repository and
repo.FetchReference to assert that the returned error is specifically a
not-found sentinel by using errors.Is(err, errdef.ErrNotFound) (importing errors
and errdef if not present) instead of g.Expect(err).To(HaveOccurred()); ensure
the failure message still references artTag and releaseRepo() but only consider
the check succeeded when errors.Is indicates ErrNotFound.
Coverage Report for CI Build 26530885563Warning No base build found for commit Coverage: 71.664%Details
Uncovered Changes
Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
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
3 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.
What
Clean up rendered OCI artifacts and
RenderTasks when SolAr API resources are deleted.Closes #358
Why
Deleting a
ReleaseBindingorTargetpreviously left orphanedRenderTaskobjects and OCI images in the render registry. This implements the full deletion lifecycle.Testing
RenderArtifactReconcilercovered with fake client + injectedDeleteTagstub (pkg/controller)TargetReconcilerextended forensureRenderArtifact,ensureRenderBinding, anddeleteStaleRenderBindingspkg/ociregistry.DeleteTagtested for success and error pathsNotes for reviewers
New API resources (CRD-equivalent, served via extension apiserver):
RenderArtifactrecords OCI coordinates of a pushed chart artifact; carries a finalizer; reconciled byRenderArtifactReconcilerRenderBindingref-count record linking a Target to aRenderArtifact; drives GC when the last binding is removedHelm chart: top-level
caBundleblock added to mount a CA bundleConfigMapinto the controller-manager pod (SSL_CERT_FILE), required for TLS to self-signed registries in cluster.Checklist
Summary by CodeRabbit
RenderArtifactandRenderBindingresources for managing and referencing rendered OCI artifacts.