From 8b43fa46c8214be66749a414e22180b6fb93cdd2 Mon Sep 17 00:00:00 2001 From: Matthias Bertschy Date: Thu, 24 Oct 2024 11:54:15 +0200 Subject: [PATCH 1/2] fail Create() if object already exists Signed-off-by: Matthias Bertschy --- pkg/registry/file/storage.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/pkg/registry/file/storage.go b/pkg/registry/file/storage.go index c3bd34b3c..328c504f7 100644 --- a/pkg/registry/file/storage.go +++ b/pkg/registry/file/storage.go @@ -193,7 +193,7 @@ func (s *StorageImpl) writeFiles(key string, obj runtime.Object, metaOut runtime return nil } -// Create adds a new object at a key even when it already exists. 'ttl' is time-to-live +// Create adds a new object at a key unless it already exists. 'ttl' is time-to-live // in seconds (and is ignored). If no error is returned and out is not nil, out will be // set to the read value from database. func (s *StorageImpl) Create(ctx context.Context, key string, obj, metaOut runtime.Object, _ uint64) error { @@ -204,6 +204,10 @@ func (s *StorageImpl) Create(ctx context.Context, key string, obj, metaOut runti s.locks.Lock(key) defer s.locks.Unlock(key) spanLock.End() + // check if object already exists + if _, err := s.appFs.Stat(makePayloadPath(filepath.Join(s.root, key))); err == nil { + return storage.NewKeyExistsError(key, 0) + } // resourceVersion should not be set on create if version, err := s.versioner.ObjectResourceVersion(obj); err == nil && version != 0 { msg := "resourceVersion should not be set on objects to be created" From d7aed142029749547e780302e7ef078d0c6ede84 Mon Sep 17 00:00:00 2001 From: Matthias Bertschy Date: Thu, 24 Oct 2024 13:06:19 +0200 Subject: [PATCH 2/2] add hidden option to fetch only metadata with Get() Signed-off-by: Matthias Bertschy --- pkg/registry/file/storage.go | 13 ++++ pkg/registry/file/storage_test.go | 65 +++++++++++++++---- .../file/vulnerabilitysummarystorage_test.go | 2 +- 3 files changed, 66 insertions(+), 14 deletions(-) diff --git a/pkg/registry/file/storage.go b/pkg/registry/file/storage.go index 328c504f7..70a816a92 100644 --- a/pkg/registry/file/storage.go +++ b/pkg/registry/file/storage.go @@ -296,6 +296,19 @@ func (s *StorageImpl) Get(ctx context.Context, key string, opts storage.GetOptio // get is a helper function for Get to allow calls without locks from other methods that already have them func (s *StorageImpl) get(ctx context.Context, key string, opts storage.GetOptions, objPtr runtime.Object) error { p := filepath.Join(s.root, key) + if opts.ResourceVersion == "metadata" { + // get metadata from SQLite + conn, err := s.pool.Take(context.Background()) + if err != nil { + return fmt.Errorf("take connection: %w", err) + } + defer s.pool.Put(conn) + metadata, err := ReadMetadata(conn, key) + if err != nil { + return fmt.Errorf("read metadata: %w", err) + } + return json.Unmarshal(metadata, objPtr) + } payloadFile, err := s.appFs.OpenFile(makePayloadPath(p), syscall.O_DIRECT|os.O_RDONLY, 0) if err != nil { if errors.Is(err, afero.ErrFileNotFound) { diff --git a/pkg/registry/file/storage_test.go b/pkg/registry/file/storage_test.go index 1b19625db..523fe90b2 100644 --- a/pkg/registry/file/storage_test.go +++ b/pkg/registry/file/storage_test.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "encoding/gob" + "encoding/json" "fmt" "testing" @@ -264,11 +265,23 @@ func isNotFoundError(_ assert.TestingT, err error, _ ...any) bool { func TestStorageImpl_Get(t *testing.T) { var emptyObj bytes.Buffer _ = gob.NewEncoder(&emptyObj).Encode(v1beta1.SBOMSyft{}) + var realMeta bytes.Buffer + _ = json.NewEncoder(&realMeta).Encode(v1beta1.SBOMSyft{ + ObjectMeta: v1.ObjectMeta{ + Name: "toto", + }, + }) var realObj bytes.Buffer _ = gob.NewEncoder(&realObj).Encode(v1beta1.SBOMSyft{ ObjectMeta: v1.ObjectMeta{ Name: "toto", }, + Spec: v1beta1.SBOMSyftSpec{ + Metadata: v1beta1.SPDXMeta{ + Tool: v1beta1.ToolMeta{ + Name: "syft"}, + }, + }, }) type args struct { key string @@ -276,12 +289,13 @@ func TestStorageImpl_Get(t *testing.T) { objPtr runtime.Object } tests := []struct { - name string - args args - content string - create bool - wantErr assert.ErrorAssertionFunc - want runtime.Object + name string + args args + content []byte + contentMeta []byte + create bool + wantErr assert.ErrorAssertionFunc + want runtime.Object }{ { name: "not found", @@ -305,7 +319,7 @@ func TestStorageImpl_Get(t *testing.T) { key: "/spdx.softwarecomposition.kubescape.io/sbomsyfts/kubescape/toto", objPtr: &v1beta1.SBOMSyft{}, }, - content: emptyObj.String(), + content: emptyObj.Bytes(), create: true, wantErr: assert.NoError, want: &v1beta1.SBOMSyft{}, @@ -316,9 +330,31 @@ func TestStorageImpl_Get(t *testing.T) { key: "/spdx.softwarecomposition.kubescape.io/sbomsyfts/kubescape/toto", objPtr: &v1beta1.SBOMSyft{}, }, - content: realObj.String(), + content: realObj.Bytes(), create: true, wantErr: assert.NoError, + want: &v1beta1.SBOMSyft{ + ObjectMeta: v1.ObjectMeta{ + Name: "toto", + }, + Spec: v1beta1.SBOMSyftSpec{ + Metadata: v1beta1.SPDXMeta{ + Tool: v1beta1.ToolMeta{ + Name: "syft"}, + }, + }, + }, + }, + { + name: "real object - metadata only", + args: args{ + key: "/spdx.softwarecomposition.kubescape.io/sbomsyfts/kubescape/toto", + objPtr: &v1beta1.SBOMSyft{}, + opts: storage.GetOptions{ResourceVersion: "metadata"}, + }, + contentMeta: realMeta.Bytes(), + create: true, + wantErr: assert.NoError, want: &v1beta1.SBOMSyft{ ObjectMeta: v1.ObjectMeta{ Name: "toto", @@ -331,7 +367,7 @@ func TestStorageImpl_Get(t *testing.T) { key: "/spdx.softwarecomposition.kubescape.io/sbomsyfts/kubescape/toto", objPtr: &v1beta1.SBOMSyft{}, }, - content: string(realObj.Bytes()[10]), + content: realObj.Bytes()[10:10], create: true, wantErr: isNotFoundError, }, @@ -339,16 +375,19 @@ func TestStorageImpl_Get(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { fs := afero.NewMemMapFs() - if tt.create { - path := getStoredPayloadFilepath(DefaultStorageRoot, tt.args.key) - _ = afero.WriteFile(fs, path, []byte(tt.content), 0644) - } pool := NewTestPool(t.TempDir()) require.NotNil(t, pool) defer func(pool *sqlitemigration.Pool) { _ = pool.Close() }(pool) s := NewStorageImpl(fs, DefaultStorageRoot, pool, nil) + if tt.create { + conn, err := pool.Take(context.Background()) + require.NoError(t, err) + require.NoError(t, WriteJSON(conn, tt.args.key, tt.contentMeta)) + require.NoError(t, afero.WriteFile(fs, getStoredPayloadFilepath(DefaultStorageRoot, tt.args.key), tt.content, 0644)) + pool.Put(conn) + } if err := s.Get(context.TODO(), tt.args.key, tt.args.opts, tt.args.objPtr); !tt.wantErr(t, err) { t.Errorf("Get() error = %v, wantErr %v", err, tt.wantErr(t, err)) } diff --git a/pkg/registry/file/vulnerabilitysummarystorage_test.go b/pkg/registry/file/vulnerabilitysummarystorage_test.go index 338824d7b..25667b91c 100644 --- a/pkg/registry/file/vulnerabilitysummarystorage_test.go +++ b/pkg/registry/file/vulnerabilitysummarystorage_test.go @@ -443,9 +443,9 @@ func TestVulnSummaryStorageImpl_Get(t *testing.T) { }(pool) sch := scheme.Scheme require.NoError(t, softwarecomposition.AddToScheme(sch)) - realStorage := NewStorageImpl(afero.NewMemMapFs(), "/", pool, sch) for _, tt := range tests { + realStorage := NewStorageImpl(afero.NewMemMapFs(), "/", pool, sch) t.Run(tt.name, func(t *testing.T) { if tt.createObj { for i := range tt.args.keyCreatedObj {