diff --git a/pkg/sbommanager/v1/sbom_manager.go b/pkg/sbommanager/v1/sbom_manager.go index 1d0e426826..652fa7acb7 100644 --- a/pkg/sbommanager/v1/sbom_manager.go +++ b/pkg/sbommanager/v1/sbom_manager.go @@ -5,6 +5,7 @@ import ( "encoding/json" "errors" "fmt" + "maps" "net" "os" "path/filepath" @@ -27,6 +28,7 @@ import ( mapset "github.com/deckarep/golang-set/v2" "github.com/distribution/distribution/reference" "github.com/google/go-containerregistry/pkg/name" + "github.com/hashicorp/golang-lru/v2/expirable" containercollection "github.com/inspektor-gadget/inspektor-gadget/pkg/container-collection" "github.com/kubescape/go-logger" "github.com/kubescape/go-logger/helpers" @@ -62,6 +64,8 @@ const ( maxScanRetries = 3 scannerReadinessCheckInterval = 5 * time.Second maxPendingScans = 1000 + maxFailureRetryEntries = 1000 + failureRetryTTL = 30 * time.Minute ) // pendingScan holds the data needed to retry a container scan after the sidecar becomes ready. @@ -88,11 +92,18 @@ type SbomManager struct { scannerClient sbomscanner.SBOMScannerClient scannerMemLimit int64 scanRetries map[string]int // safe without mutex: only accessed from pool workers (pool size 1) - pendingScans map[string]pendingScan - pendingOrder []string - pendingMu sync.Mutex - failureReporter sbommanager.SbomFailureReporter - metrics metricsmanager.MetricsManager + // failureRetries tracks consecutive generic SBOM-generation failures per sbomName. Bounded + // + TTL'd so short-lived images don't leak entries; a side effect is that the count resets + // if two failures for the same image are spaced more than failureRetryTTL apart, so it + // bounds retries for a tight failure cadence (e.g. a crash loop) rather than every possible + // one -- a slow-cadence permanent failure (long-lived pod, infrequent CronJob) can still + // reprocess indefinitely without ever accumulating enough consecutive failures to pin. + failureRetries *expirable.LRU[string, int] + pendingScans map[string]pendingScan + pendingOrder []string + pendingMu sync.Mutex + failureReporter sbommanager.SbomFailureReporter + metrics metricsmanager.MetricsManager } var _ sbommanager.SbomManagerClient = (*SbomManager)(nil) @@ -141,6 +152,7 @@ func CreateSbomManager(ctx context.Context, cfg config.Config, socketPath string scannerClient: scannerClient, scannerMemLimit: scannerMemLimit, scanRetries: make(map[string]int), + failureRetries: expirable.NewLRU[string, int](maxFailureRetryEntries, nil, failureRetryTTL), pendingScans: make(map[string]pendingScan), failureReporter: failureReporter, metrics: metrics, @@ -283,6 +295,15 @@ func (s *SbomManager) processContainerWithMetadata(notif containercollection.Pub }, } wipSbom, err = s.storageClient.CreateSBOM(wipSbom) + // wipSbomHadContent is true only when we're about to reprocess an SBOM that previously + // completed successfully (the Learning case below). It exists solely to keep a + // content-bearing SBOM from ever being marked TooLarge on the reprocess path: unlike + // Incomplete, TooLarge is a one-way door in the storage layer -- GuaranteedUpdate + // silently drops every future write once status=too-large is set, so persisting it here + // (with the real Spec still attached, since PatchSBOMAnnotations never clears it) would + // leave the SBOM permanently frozen with its old content, unfixable by any later version. + // Incomplete has no such short-circuit and stays safely retryable, so it's used instead. + var wipSbomHadContent bool switch { case k8serrors.IsAlreadyExists(err): // get the existing SBOM metadata and check if it is ready or being processed by another node @@ -316,24 +337,19 @@ func (s *SbomManager) processContainerWithMetadata(notif containercollection.Pub helpers.String("nodeName", wipSbom.Annotations[NodeNameMetadataKey])) return case wipSbom.Annotations[helpersv1.StatusMetadataKey] == helpersv1.Learning: - // only skip if the SBOM was created with the same version of tool - if wipSbom.Annotations[helpersv1.ToolVersionMetadataKey] == s.version { - logger.L().Debug("SbomManager - SBOM is already created, skipping", - helpers.String("namespace", notif.Container.K8s.Namespace), - helpers.String("pod", notif.Container.K8s.PodName), - helpers.String("container", notif.Container.K8s.ContainerName), - helpers.String("sbomName", sbomName)) + if !s.shouldRetryAtCurrentVersion(wipSbom, sbomName, notif, + "SBOM is already created, skipping", + "SBOM was created with an different version of tool, recreating it") { + return + } + wipSbomHadContent = true + // continue to create SBOM + case wipSbom.Annotations[helpersv1.StatusMetadataKey] == helpersv1.Incomplete: + if !s.shouldRetryAtCurrentVersion(wipSbom, sbomName, notif, + "SBOM generation previously failed with this tool version, skipping", + "SBOM generation previously failed with a different tool version, retrying") { return } - logger.L().Debug("SbomManager - SBOM was created with an different version of tool, recreating it", - helpers.String("namespace", notif.Container.K8s.Namespace), - helpers.String("pod", notif.Container.K8s.PodName), - helpers.String("container", notif.Container.K8s.ContainerName), - helpers.String("sbomName", sbomName), - helpers.String("got version", wipSbom.Annotations[helpersv1.ToolVersionMetadataKey]), - helpers.String("expected version", s.version)) - // update the version of the tool - wipSbom.Annotations[helpersv1.ToolVersionMetadataKey] = s.version // continue to create SBOM case wipSbom.Annotations[NodeNameMetadataKey] != s.cfg.NodeName: logger.L().Debug("SbomManager - SBOM is already being processed by another node, skipping", @@ -410,7 +426,7 @@ func (s *SbomManager) processContainerWithMetadata(notif containercollection.Pub s.metrics.ObserveSBOMScanDuration("oom_killed", scanDuration) s.metrics.ReportSBOMScannerRestart() s.metrics.SetSBOMScannerReady(false) - s.handleScannerCrash(sbomName, wipSbom, notif, scanErr, imageTag, imageID) + s.handleScannerCrash(sbomName, notif, scanErr, imageTag, imageID, wipSbomHadContent) return } s.metrics.ReportSBOMScan("error") @@ -421,6 +437,7 @@ func (s *SbomManager) processContainerWithMetadata(notif containercollection.Pub helpers.String("pod", notif.Container.K8s.PodName), helpers.String("container", notif.Container.K8s.ContainerName), helpers.String("sbomName", sbomName)) + s.handleGenericFailure(sbomName) s.reportFailure(notif, imageTag, imageID, scanfailure.ReasonSBOMGenerationFailed, scanErr) return } @@ -464,15 +481,16 @@ func (s *SbomManager) processContainerWithMetadata(notif containercollection.Pub helpers.String("container", notif.Container.K8s.ContainerName), helpers.String("sbomName", sbomName)) if errors.Is(srcErr, syftutil.ErrImageTooLarge) { - delete(wipSbom.Annotations, NodeNameMetadataKey) - wipSbom.Annotations[helpersv1.StatusMetadataKey] = helpersv1.TooLarge - if _, replaceErr := s.storageClient.ReplaceSBOM(wipSbom); replaceErr != nil { - logger.L().Ctx(s.ctx).Error("SbomManager - failed to persist TooLarge SBOM", - helpers.Error(replaceErr), - helpers.String("sbomName", sbomName)) + if wipSbomHadContent { + // don't let a content-bearing SBOM reach the TooLarge one-way door; treat + // it as a generic (retryable, eventually Incomplete) failure instead. + s.handleGenericFailure(sbomName) + } else { + s.markSBOMStatus(sbomName, helpersv1.TooLarge, nil) } s.reportFailure(notif, imageTag, imageID, scanfailure.ReasonImageTooLarge, srcErr) } else { + s.handleGenericFailure(sbomName) s.reportFailure(notif, imageTag, imageID, scanfailure.ReasonSBOMGenerationFailed, srcErr) } return @@ -498,6 +516,7 @@ func (s *SbomManager) processContainerWithMetadata(notif containercollection.Pub helpers.String("pod", notif.Container.K8s.PodName), helpers.String("container", notif.Container.K8s.ContainerName), helpers.String("sbomName", sbomName)) + s.handleGenericFailure(sbomName) s.reportFailure(notif, imageTag, imageID, scanfailure.ReasonSBOMGenerationFailed, syftErr) return } @@ -506,6 +525,7 @@ func (s *SbomManager) processContainerWithMetadata(notif containercollection.Pub } // prepare the SBOM + s.failureRetries.Remove(sbomName) delete(wipSbom.Annotations, NodeNameMetadataKey) wipSbom.Spec.Metadata.Report.CreatedAt = wipSbom.CreationTimestamp wipSbom.Spec.Metadata.Tool.Name = "syft" @@ -557,7 +577,12 @@ func (s *SbomManager) waitForSharedContainerData(containerID string) (*objectcac }, backoff.WithBackOff(backoff.NewExponentialBackOff())) } -func (s *SbomManager) handleScannerCrash(sbomName string, wipSbom *v1beta1.SBOMSyft, notif containercollection.PubSubEvent, scanErr error, imageTag, imageID string) { +// handleScannerCrash responds to repeated sidecar OOM crashes while scanning the same image. +// hadContent must be true when the SBOM being reprocessed previously completed successfully +// (see the wipSbomHadContent doc comment in processContainerWithMetadata) -- in that case the +// terminal status is Incomplete rather than TooLarge, since TooLarge is a one-way door in the +// storage layer that would permanently freeze the SBOM's existing content. +func (s *SbomManager) handleScannerCrash(sbomName string, notif containercollection.PubSubEvent, scanErr error, imageTag, imageID string, hadContent bool) { s.scanRetries[sbomName]++ retryCount := s.scanRetries[sbomName] @@ -571,14 +596,12 @@ func (s *SbomManager) handleScannerCrash(sbomName string, wipSbom *v1beta1.SBOMS helpers.Int("maxRetries", maxScanRetries)) if retryCount >= maxScanRetries { - delete(wipSbom.Annotations, NodeNameMetadataKey) - wipSbom.Annotations[helpersv1.StatusMetadataKey] = helpersv1.TooLarge - wipSbom.Annotations[ScannerMemoryLimitAnnotation] = fmt.Sprintf("%d", s.scannerMemLimit) - wipSbom.Spec = v1beta1.SBOMSyftSpec{} - if _, replaceErr := s.storageClient.ReplaceSBOM(wipSbom); replaceErr != nil { - logger.L().Error("SbomManager - failed to mark SBOM as TooLarge after scanner crashes", - helpers.Error(replaceErr), - helpers.String("sbomName", sbomName)) + if hadContent { + s.markSBOMStatus(sbomName, helpersv1.Incomplete, nil) + } else { + s.markSBOMStatus(sbomName, helpersv1.TooLarge, map[string]any{ + ScannerMemoryLimitAnnotation: fmt.Sprintf("%d", s.scannerMemLimit), + }) } // Report OOM regardless of persist success — the user should know the scan failed s.reportFailure(notif, imageTag, imageID, scanfailure.ReasonScannerOOMKilled, scanErr) @@ -630,6 +653,66 @@ func (s *SbomManager) drainPendingScans() { } } +// markSBOMStatus persists the SBOM's terminal status (e.g. TooLarge, Incomplete) so a later +// container start for the same image is handled by the matching case in +// processContainerWithMetadata instead of retrying and failing indefinitely. It also records +// the currently-running tool version alongside the status, since that's what determined the +// outcome -- the Learning/Incomplete cases' version check relies on this being accurate. +func (s *SbomManager) markSBOMStatus(sbomName, status string, extraAnnotations map[string]any) { + annotations := map[string]any{ + NodeNameMetadataKey: nil, // no longer owned by this node + helpersv1.StatusMetadataKey: status, + helpersv1.ToolVersionMetadataKey: s.version, + } + maps.Copy(annotations, extraAnnotations) + if _, err := s.storageClient.PatchSBOMAnnotations(sbomName, annotations); err != nil { + logger.L().Ctx(s.ctx).Error("SbomManager - failed to persist SBOM status", + helpers.Error(err), + helpers.String("sbomName", sbomName), + helpers.String("status", status)) + } +} + +// shouldRetryAtCurrentVersion checks a status-gated SBOM's recorded tool version against the +// running version. If they match, it logs skipMsg and returns false (the caller should skip +// reprocessing). Otherwise it logs retryMsg, updates the tool-version annotation, and returns +// true (the caller should continue to reprocess). +func (s *SbomManager) shouldRetryAtCurrentVersion(wipSbom *v1beta1.SBOMSyft, sbomName string, notif containercollection.PubSubEvent, skipMsg, retryMsg string) bool { + if wipSbom.Annotations[helpersv1.ToolVersionMetadataKey] == s.version { + logger.L().Debug(skipMsg, + helpers.String("namespace", notif.Container.K8s.Namespace), + helpers.String("pod", notif.Container.K8s.PodName), + helpers.String("container", notif.Container.K8s.ContainerName), + helpers.String("sbomName", sbomName)) + return false + } + logger.L().Debug(retryMsg, + helpers.String("namespace", notif.Container.K8s.Namespace), + helpers.String("pod", notif.Container.K8s.PodName), + helpers.String("container", notif.Container.K8s.ContainerName), + helpers.String("sbomName", sbomName), + helpers.String("got version", wipSbom.Annotations[helpersv1.ToolVersionMetadataKey]), + helpers.String("expected version", s.version)) + wipSbom.Annotations[helpersv1.ToolVersionMetadataKey] = s.version + return true +} + +// handleGenericFailure responds to a non-deterministic SBOM-generation failure (source +// construction, syft cataloging, or sidecar scan error). markSBOMStatus only ever patches +// annotations, never Spec, so it's always safe to call regardless of whether the SBOM +// previously had real content -- but the image is only pinned Incomplete after +// maxScanRetries consecutive failures, so a single transient error doesn't lose coverage. +func (s *SbomManager) handleGenericFailure(sbomName string) { + count, _ := s.failureRetries.Get(sbomName) + count++ + if count < maxScanRetries { + s.failureRetries.Add(sbomName, count) + return + } + s.failureRetries.Remove(sbomName) + s.markSBOMStatus(sbomName, helpersv1.Incomplete, nil) +} + // reportFailure sends a scan failure report to the backend via the failure reporter. // Fire-and-forget: errors are logged, never propagated. Safe to call with nil reporter. func (s *SbomManager) reportFailure(notif containercollection.PubSubEvent, imageTag, imageID, reason string, scanErr error) { diff --git a/pkg/sbommanager/v1/sbom_manager_reprocessing_test.go b/pkg/sbommanager/v1/sbom_manager_reprocessing_test.go new file mode 100644 index 0000000000..9a31fbb3e4 --- /dev/null +++ b/pkg/sbommanager/v1/sbom_manager_reprocessing_test.go @@ -0,0 +1,391 @@ +package v1 + +import ( + "context" + "errors" + "fmt" + "os" + "path/filepath" + "sync" + "testing" + + mapset "github.com/deckarep/golang-set/v2" + "github.com/hashicorp/golang-lru/v2/expirable" + containercollection "github.com/inspektor-gadget/inspektor-gadget/pkg/container-collection" + "github.com/inspektor-gadget/inspektor-gadget/pkg/types" + helpersv1 "github.com/kubescape/k8s-interface/instanceidhandler/v1/helpers" + "github.com/kubescape/k8s-interface/names" + "github.com/kubescape/node-agent/pkg/config" + "github.com/kubescape/node-agent/pkg/metricsmanager" + sbomscanner "github.com/kubescape/node-agent/pkg/sbomscanner/v1" + "github.com/kubescape/storage/pkg/apis/softwarecomposition/v1beta1" + "github.com/stretchr/testify/assert" + k8serrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime/schema" + runtime "k8s.io/cri-api/pkg/apis/runtime/v1" +) + +// fakeSbomClient is an in-memory storage.SbomClient used to observe how +// processContainerWithMetadata persists SBOM state across repeated calls. +// +// GetSBOMMeta mirrors the real Storage.GetSBOMMeta contract (pkg/storage/v1/storage.go), +// which fetches with metav1.GetOptions{ResourceVersion: softwarecomposition.ResourceVersionMetadata} +// and therefore returns the object's metadata WITHOUT its Spec. Tests that rely on Spec +// being present on a GetSBOMMeta result would hide the exact bug this fake is meant to catch. +// +// PatchSBOMAnnotations mirrors the real Storage.PatchSBOMAnnotations contract: it only ever +// modifies the stored object's Annotations map (nil value deletes the key), never its Spec. +type fakeSbomClient struct { + mu sync.Mutex + sboms map[string]*v1beta1.SBOMSyft + replaceCalls int + patchCalls int +} + +func newFakeSbomClient() *fakeSbomClient { + return &fakeSbomClient{sboms: map[string]*v1beta1.SBOMSyft{}} +} + +func (f *fakeSbomClient) CreateSBOM(sbom *v1beta1.SBOMSyft) (*v1beta1.SBOMSyft, error) { + f.mu.Lock() + defer f.mu.Unlock() + if _, ok := f.sboms[sbom.Name]; ok { + return nil, k8serrors.NewAlreadyExists(schema.GroupResource{Resource: "sbomsyfts"}, sbom.Name) + } + f.sboms[sbom.Name] = sbom.DeepCopy() + return sbom, nil +} + +func (f *fakeSbomClient) GetSBOMMeta(name string) (*v1beta1.SBOMSyft, error) { + f.mu.Lock() + defer f.mu.Unlock() + s, ok := f.sboms[name] + if !ok { + return nil, k8serrors.NewNotFound(schema.GroupResource{Resource: "sbomsyfts"}, name) + } + meta := s.DeepCopy() + meta.Spec = v1beta1.SBOMSyftSpec{} // metadata-only fetch: mirrors the real API server + return meta, nil +} + +func (f *fakeSbomClient) ReplaceSBOM(sbom *v1beta1.SBOMSyft) (*v1beta1.SBOMSyft, error) { + f.mu.Lock() + defer f.mu.Unlock() + f.replaceCalls++ + f.sboms[sbom.Name] = sbom.DeepCopy() + return sbom, nil +} + +func (f *fakeSbomClient) PatchSBOMAnnotations(name string, annotations map[string]any) (*v1beta1.SBOMSyft, error) { + f.mu.Lock() + defer f.mu.Unlock() + f.patchCalls++ + s, ok := f.sboms[name] + if !ok { + return nil, k8serrors.NewNotFound(schema.GroupResource{Resource: "sbomsyfts"}, name) + } + if s.Annotations == nil { + s.Annotations = map[string]string{} + } + for k, v := range annotations { + if v == nil { + delete(s.Annotations, k) + continue + } + s.Annotations[k] = fmt.Sprintf("%v", v) + } + return s.DeepCopy(), nil +} + +func (f *fakeSbomClient) get(name string) *v1beta1.SBOMSyft { + f.mu.Lock() + defer f.mu.Unlock() + return f.sboms[name].DeepCopy() +} + +// fakeScannerClient is a sbomscanner.SBOMScannerClient that always fails with a fixed error, +// used to deterministically drive processContainerWithMetadata into a specific failure branch +// without depending on real image/digest parsing. +type fakeScannerClient struct{ err error } + +func (f *fakeScannerClient) CreateSBOM(_ context.Context, _ sbomscanner.ScanRequest) (*sbomscanner.ScanResult, error) { + return nil, f.err +} +func (f *fakeScannerClient) Ready() bool { return true } +func (f *fakeScannerClient) Close() error { return nil } + +func newFailureRetries() *expirable.LRU[string, int] { + return expirable.NewLRU[string, int](maxFailureRetryEntries, nil, failureRetryTTL) +} + +func newTestManager(fake *fakeSbomClient, version string) *SbomManager { + return newTestManagerWithScannerErr(fake, version, errors.New("scan failed")) +} + +func newTestManagerWithScannerErr(fake *fakeSbomClient, version string, scannerErr error) *SbomManager { + return &SbomManager{ + cfg: config.Config{NodeName: "node-1"}, + ctx: context.Background(), + processing: mapset.NewSet[string](), + storageClient: fake, + scannerClient: &fakeScannerClient{err: scannerErr}, + metrics: metricsmanager.NewMetricsNoop(), + version: version, + scanRetries: make(map[string]int), + failureRetries: newFailureRetries(), + } +} + +// newTestManagerInProcess builds a manager with no scanner sidecar configured, so +// processContainerWithMetadata takes the in-process (syftutil.NewSource) fallback path. +func newTestManagerInProcess(fake *fakeSbomClient, version string, maxImageSize int64) *SbomManager { + return &SbomManager{ + cfg: config.Config{NodeName: "node-1", MaxImageSize: maxImageSize}, + ctx: context.Background(), + processing: mapset.NewSet[string](), + storageClient: fake, + version: version, + scanRetries: make(map[string]int), + failureRetries: newFailureRetries(), + } +} + +// testImageStatusWithLayer builds an ImageStatusResponse with one valid diff-id and a mount +// pointing to a real temp directory containing a file, so syftutil.toLayers computes a +// non-zero totalSize and NewSource can be driven into ErrImageTooLarge via a small MaxImageSize. +func testImageStatusWithLayer(t *testing.T, imageTag string) (*runtime.ImageStatusResponse, []string) { + t.Helper() + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "layer.tar"), make([]byte, 1024), 0o644); err != nil { + t.Fatalf("failed to write test layer file: %v", err) + } + imageStatus := &runtime.ImageStatusResponse{ + Image: &runtime.Image{ + Id: "img-id", + RepoTags: []string{imageTag}, + }, + Info: map[string]string{"info": `{"imageSpec":{"rootfs":{"type":"layers","diff_ids":["sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"]}}}`}, + } + return imageStatus, []string{dir} +} + +func testNotifAndImageStatus() (containercollection.PubSubEvent, *runtime.ImageStatusResponse, string, string) { + imageTag := "quay.io/kubescape/kubevuln:v0.3.2" + imageID := "sha256:94cbbb94f8d6bdf2529d5f9c5279ac4c7411182f4e8e5a3d0b5e8f10a465f73a" + notif := containercollection.PubSubEvent{ + Container: &containercollection.Container{ + Runtime: containercollection.RuntimeMetadata{ + BasicRuntimeMetadata: types.BasicRuntimeMetadata{ + ContainerID: "container-1", + ContainerImageName: imageTag, + }, + }, + K8s: containercollection.K8sMetadata{ + BasicK8sMetadata: types.BasicK8sMetadata{ + Namespace: "default", + PodName: "pod-1", + ContainerName: "container-1", + }, + }, + }, + } + imageStatus := &runtime.ImageStatusResponse{ + Image: &runtime.Image{ + Id: "img-id", + RepoTags: []string{imageTag}, + }, + Info: map[string]string{"info": `{"imageSpec":{}}`}, + } + return notif, imageStatus, imageTag, imageID +} + +// Test_processContainerWithMetadata_IncompleteReprocessing guards against the +// SBOM-generation-failure reprocessing loop: a fresh reservation that keeps failing must +// only be pinned Incomplete after maxScanRetries consecutive failures (so a single transient +// error doesn't permanently lose SBOM coverage), after which a later container start at the +// same tool version must skip reprocessing instead of retrying and re-failing indefinitely. +// Marking must go through PatchSBOMAnnotations (annotations only), never a full ReplaceSBOM. +func Test_processContainerWithMetadata_IncompleteReprocessing(t *testing.T) { + fake := newFakeSbomClient() + mgr := newTestManager(fake, "v1.0.0") + notif, imageStatus, imageTag, imageID := testNotifAndImageStatus() + + sbomName, err := names.ImageInfoToSlug(imageTag, imageID) + assert.NoError(t, err) + + // Failures below the retry threshold must not touch storage -- this is what makes + // transient errors (a brief sidecar blip, a registry hiccup) self-healing. + for range maxScanRetries - 1 { + mgr.processContainerWithMetadata(notif, nil, imageStatus, imageTag, imageID) + } + assert.Equal(t, 0, fake.patchCalls, "failures below the retry threshold must not mark Incomplete") + assert.Equal(t, helpersv1.Initializing, fake.get(sbomName).Annotations[helpersv1.StatusMetadataKey]) + + // The maxScanRetries-th consecutive failure must persist Incomplete via a patch. + mgr.processContainerWithMetadata(notif, nil, imageStatus, imageTag, imageID) + stored := fake.get(sbomName) + assert.Equal(t, helpersv1.Incomplete, stored.Annotations[helpersv1.StatusMetadataKey]) + assert.Equal(t, "v1.0.0", stored.Annotations[helpersv1.ToolVersionMetadataKey]) + assert.Equal(t, 1, fake.patchCalls) + assert.Equal(t, 0, fake.replaceCalls, "failure marking must never use a full ReplaceSBOM") + + // A later attempt at the same tool version must be skipped without touching storage + // again -- this is the exact bug being fixed: previously the SBOM stayed dangling with + // no terminal status and was reprocessed on every container start forever. + mgr.processContainerWithMetadata(notif, nil, imageStatus, imageTag, imageID) + assert.Equal(t, 1, fake.patchCalls, "the same tool version must not reprocess") + + // A different tool version must retry -- a fixed/updated node-agent build gets a fresh + // maxScanRetries budget instead of being pinned to Incomplete forever. + mgr.version = "v2.0.0" + for range maxScanRetries - 1 { + mgr.processContainerWithMetadata(notif, nil, imageStatus, imageTag, imageID) + } + assert.Equal(t, 1, fake.patchCalls, "a version bump must not immediately re-pin Incomplete") + mgr.processContainerWithMetadata(notif, nil, imageStatus, imageTag, imageID) + assert.Equal(t, 2, fake.patchCalls) + + stored = fake.get(sbomName) + assert.Equal(t, helpersv1.Incomplete, stored.Annotations[helpersv1.StatusMetadataKey]) + assert.Equal(t, "v2.0.0", stored.Annotations[helpersv1.ToolVersionMetadataKey]) +} + +// Test_processContainerWithMetadata_PreservesContentOnReprocessFailure guards against +// silently wiping a previously-successful SBOM. GetSBOMMeta (used to fetch the SBOM on the +// reprocessing path, e.g. after a tool-version bump) returns metadata only, with no Spec. +// Marking a repeatedly-failing content-bearing SBOM Incomplete must go through +// PatchSBOMAnnotations, which never sends Spec, so the existing artifacts survive even though +// the status/tool-version annotations do get updated once the retry budget is exhausted. +func Test_processContainerWithMetadata_PreservesContentOnReprocessFailure(t *testing.T) { + fake := newFakeSbomClient() + mgr := newTestManager(fake, "v2.0.0") + notif, imageStatus, imageTag, imageID := testNotifAndImageStatus() + + sbomName, err := names.ImageInfoToSlug(imageTag, imageID) + assert.NoError(t, err) + + // Seed a previously-successful SBOM, created by an older tool version, with real content. + good := &v1beta1.SBOMSyft{} + good.Name = sbomName + good.Annotations = map[string]string{ + helpersv1.StatusMetadataKey: helpersv1.Learning, + helpersv1.ToolVersionMetadataKey: "v1.0.0", + } + good.Spec.Syft.Artifacts = make([]v1beta1.SyftPackage, 2) + fake.sboms[sbomName] = good + + // A container start at the new version triggers reprocessing (version mismatch); the scan + // then fails (mgr's scanner always errors) on every subsequent attempt. Once the retry + // budget is exhausted the SBOM is pinned Incomplete, and further attempts at that recorded + // version are skipped -- but the artifacts must survive throughout, since marking never + // uses a full ReplaceSBOM. + for range maxScanRetries + 2 { + mgr.processContainerWithMetadata(notif, nil, imageStatus, imageTag, imageID) + } + + raw := fake.get(sbomName) + assert.Len(t, raw.Spec.Syft.Artifacts, 2, "existing SBOM content must survive an annotation-only status update") + assert.Equal(t, helpersv1.Incomplete, raw.Annotations[helpersv1.StatusMetadataKey]) + assert.Equal(t, "v2.0.0", raw.Annotations[helpersv1.ToolVersionMetadataKey]) + assert.Equal(t, 1, fake.patchCalls, "only the maxScanRetries-th consecutive failure marks Incomplete; later attempts must skip") + assert.Equal(t, 0, fake.replaceCalls, "failure marking must never use a full ReplaceSBOM") +} + +// Test_processContainerWithMetadata_PreservesContentOnScannerCrash is the handleScannerCrash +// counterpart to Test_processContainerWithMetadata_PreservesContentOnReprocessFailure. TooLarge +// is a one-way door in the storage layer (all future writes to a TooLarge object are silently +// dropped server-side), so repeated sidecar OOM crashes while reprocessing a content-bearing +// SBOM must pin it to Incomplete (safely retryable) instead of TooLarge, via an annotation-only +// patch that never touches its Spec. +func Test_processContainerWithMetadata_PreservesContentOnScannerCrash(t *testing.T) { + fake := newFakeSbomClient() + mgr := newTestManagerWithScannerErr(fake, "v2.0.0", sbomscanner.ErrScannerCrashed) + notif, imageStatus, imageTag, imageID := testNotifAndImageStatus() + + sbomName, err := names.ImageInfoToSlug(imageTag, imageID) + assert.NoError(t, err) + + good := &v1beta1.SBOMSyft{} + good.Name = sbomName + good.Annotations = map[string]string{ + helpersv1.StatusMetadataKey: helpersv1.Learning, + helpersv1.ToolVersionMetadataKey: "v1.0.0", + } + good.Spec.Syft.Artifacts = make([]v1beta1.SyftPackage, 2) + fake.sboms[sbomName] = good + + // Each container start triggers reprocessing (version mismatch) and the sidecar + // "crashes" (ErrScannerCrashed); handleScannerCrash's own maxScanRetries threshold pins + // the SBOM to Incomplete once exhausted, after which further attempts are skipped. + for range maxScanRetries + 2 { + mgr.processContainerWithMetadata(notif, nil, imageStatus, imageTag, imageID) + } + + raw := fake.get(sbomName) + assert.Len(t, raw.Spec.Syft.Artifacts, 2, "existing SBOM content must survive an annotation-only status update") + assert.Equal(t, helpersv1.Incomplete, raw.Annotations[helpersv1.StatusMetadataKey], "content-bearing SBOMs must never be pinned TooLarge, a storage-layer one-way door") + assert.Equal(t, 1, fake.patchCalls, "only the maxScanRetries-th consecutive crash marks Incomplete; later attempts must skip") + assert.Equal(t, 0, fake.replaceCalls, "scanner-crash marking must never use a full ReplaceSBOM") +} + +// Test_processContainerWithMetadata_PreservesContentOnTooLarge is the ErrImageTooLarge +// counterpart to the other content-preservation tests: totalSize in syftutil.toLayers is +// computed from the currently-mounted layer paths, not a fixed property of the image, so a +// content-bearing SBOM being reprocessed can also hit ErrImageTooLarge. TooLarge is a one-way +// door in the storage layer, so a content-bearing SBOM must never be pinned to it -- instead +// this is treated as a generic (retryable, eventually Incomplete) failure, same as any other +// generic error, and always through PatchSBOMAnnotations so existing content is never wiped. +func Test_processContainerWithMetadata_PreservesContentOnTooLarge(t *testing.T) { + fake := newFakeSbomClient() + imageTag := "quay.io/kubescape/kubevuln:v0.3.2" + imageID := "sha256:94cbbb94f8d6bdf2529d5f9c5279ac4c7411182f4e8e5a3d0b5e8f10a465f73a" + imageStatus, mounts := testImageStatusWithLayer(t, imageTag) + // MaxImageSize (1 byte) smaller than the seeded layer file guarantees NewSource returns + // ErrImageTooLarge on every attempt. + mgr := newTestManagerInProcess(fake, "v2.0.0", 1) + notif, _, _, _ := testNotifAndImageStatus() + + sbomName, err := names.ImageInfoToSlug(imageTag, imageID) + assert.NoError(t, err) + + good := &v1beta1.SBOMSyft{} + good.Name = sbomName + good.Annotations = map[string]string{ + helpersv1.StatusMetadataKey: helpersv1.Learning, + helpersv1.ToolVersionMetadataKey: "v1.0.0", + } + good.Spec.Syft.Artifacts = make([]v1beta1.SyftPackage, 2) + fake.sboms[sbomName] = good + + for range maxScanRetries + 2 { + mgr.processContainerWithMetadata(notif, mounts, imageStatus, imageTag, imageID) + } + + raw := fake.get(sbomName) + assert.Len(t, raw.Spec.Syft.Artifacts, 2, "existing SBOM content must survive an annotation-only status update") + assert.Equal(t, helpersv1.Incomplete, raw.Annotations[helpersv1.StatusMetadataKey], "content-bearing SBOMs must never be pinned TooLarge, a storage-layer one-way door") + assert.Equal(t, 1, fake.patchCalls, "only the maxScanRetries-th consecutive failure marks Incomplete; later attempts must skip") + assert.Equal(t, 0, fake.replaceCalls, "ErrImageTooLarge marking must never use a full ReplaceSBOM") +} + +// Test_processContainerWithMetadata_MarksFreshImageTooLargeImmediately guards the unchanged +// half of the ErrImageTooLarge behavior: an image with no prior content (a fresh reservation) +// is still marked TooLarge immediately, with no retry budget -- TooLarge's storage-layer +// one-way door is only a problem when it freezes real content, which a content-free SBOM +// never had to begin with. +func Test_processContainerWithMetadata_MarksFreshImageTooLargeImmediately(t *testing.T) { + fake := newFakeSbomClient() + imageTag := "quay.io/kubescape/kubevuln:v0.3.2" + imageStatus, mounts := testImageStatusWithLayer(t, imageTag) + mgr := newTestManagerInProcess(fake, "v2.0.0", 1) + notif, _, _, imageID := testNotifAndImageStatus() + + sbomName, err := names.ImageInfoToSlug(imageTag, imageID) + assert.NoError(t, err) + + mgr.processContainerWithMetadata(notif, mounts, imageStatus, imageTag, imageID) + + stored := fake.get(sbomName) + assert.Equal(t, helpersv1.TooLarge, stored.Annotations[helpersv1.StatusMetadataKey]) + assert.Equal(t, 1, fake.patchCalls, "a fresh reservation must mark TooLarge on the first occurrence, with no retry budget") +} diff --git a/pkg/storage/storage_interface.go b/pkg/storage/storage_interface.go index 9a1c8125f1..e8f3e80dc4 100644 --- a/pkg/storage/storage_interface.go +++ b/pkg/storage/storage_interface.go @@ -26,6 +26,11 @@ type SbomClient interface { CreateSBOM(SBOM *v1beta1.SBOMSyft) (*v1beta1.SBOMSyft, error) GetSBOMMeta(name string) (*v1beta1.SBOMSyft, error) ReplaceSBOM(SBOM *v1beta1.SBOMSyft) (*v1beta1.SBOMSyft, error) + // PatchSBOMAnnotations updates only metadata.annotations via a merge patch, never sending + // spec. A nil value for a key deletes that annotation. Safe to call regardless of whether + // the caller holds the SBOM's real spec (e.g. after a metadata-only GetSBOMMeta fetch), + // since spec is never part of the patch payload. + PatchSBOMAnnotations(name string, annotations map[string]any) (*v1beta1.SBOMSyft, error) } type StorageClient interface { diff --git a/pkg/storage/storage_mock.go b/pkg/storage/storage_mock.go index 13e96f3aaf..955431d281 100644 --- a/pkg/storage/storage_mock.go +++ b/pkg/storage/storage_mock.go @@ -2,6 +2,7 @@ package storage import ( "context" + "fmt" "github.com/kubescape/storage/pkg/apis/softwarecomposition/v1beta1" spdxv1beta1 "github.com/kubescape/storage/pkg/apis/softwarecomposition/v1beta1" @@ -78,14 +79,31 @@ func (sc *StorageHttpClientMock) ReplaceSBOM(SBOM *v1beta1.SBOMSyft) (*v1beta1.S return SBOM, nil } +func (sc *StorageHttpClientMock) PatchSBOMAnnotations(_ string, annotations map[string]any) (*v1beta1.SBOMSyft, error) { + if sc.mockSBOM == nil { + return nil, nil + } + if sc.mockSBOM.Annotations == nil { + sc.mockSBOM.Annotations = map[string]string{} + } + for k, v := range annotations { + if v == nil { + delete(sc.mockSBOM.Annotations, k) + continue + } + sc.mockSBOM.Annotations[k] = fmt.Sprintf("%v", v) + } + return sc.mockSBOM, nil +} + // SeccompProfileClientMock is a mock implementation of SeccompProfileClient for testing type SeccompProfileClientMock struct { - Profiles []*v1beta1.SeccompProfile - WatchEvents chan watch.Event - WatchStopped bool - GetError error - ListError error - WatchError error + Profiles []*v1beta1.SeccompProfile + WatchEvents chan watch.Event + WatchStopped bool + GetError error + ListError error + WatchError error } var _ SeccompProfileClient = (*SeccompProfileClientMock)(nil) diff --git a/pkg/storage/v1/storage.go b/pkg/storage/v1/storage.go index 811c1f1252..e9828df2f3 100644 --- a/pkg/storage/v1/storage.go +++ b/pkg/storage/v1/storage.go @@ -2,6 +2,7 @@ package storage import ( "context" + "encoding/json" "fmt" "os" "strconv" @@ -19,6 +20,7 @@ import ( "github.com/kubescape/storage/pkg/generated/clientset/versioned/fake" spdxv1beta1 "github.com/kubescape/storage/pkg/generated/clientset/versioned/typed/softwarecomposition/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/dynamic" "k8s.io/client-go/rest" "k8s.io/client-go/tools/clientcmd" @@ -102,6 +104,18 @@ func (sc *Storage) ReplaceSBOM(SBOM *v1beta1.SBOMSyft) (*v1beta1.SBOMSyft, error return sc.storageClient.SBOMSyfts(sc.namespace).Update(context.Background(), SBOM, metav1.UpdateOptions{}) } +func (sc *Storage) PatchSBOMAnnotations(name string, annotations map[string]any) (*v1beta1.SBOMSyft, error) { + patch, err := json.Marshal(map[string]any{ + "metadata": map[string]any{ + "annotations": annotations, + }, + }) + if err != nil { + return nil, err + } + return sc.storageClient.SBOMSyfts(sc.namespace).Patch(context.Background(), name, types.MergePatchType, patch, metav1.PatchOptions{}) +} + func (sc *Storage) modifyName(n string) string { if sc.multiplier != nil { return fmt.Sprintf("%s-%d", n, *sc.multiplier)