Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fail Create() if object already exists #165

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fail Create() if object already exists #165

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
df5c866
fail Create() if object already exists
matthyx Oct 24, 2024
d52706a
add hidden option to fetch only metadata with Get()
matthyx Oct 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 19 additions & 10 deletions pkg/registry/file/storage.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,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 {
Expand All @@ -198,6 +198,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"
Expand Down Expand Up @@ -246,11 +250,7 @@ func (s *StorageImpl) Delete(ctx context.Context, key string, metaOut runtime.Ob
}()
// try to fill metaOut
decoder := json.NewDecoder(metadataFile)
err = decoder.Decode(metaOut)
if err != nil {
logger.L().Ctx(ctx).Error("Delete - json unmarshal failed", helpers.Error(err), helpers.String("key", key))
return err
}
_ = decoder.Decode(metaOut)
// delete payload and metadata files
err = s.appFs.Remove(makePayloadPath(p))
if err != nil {
Expand Down Expand Up @@ -301,7 +301,13 @@ 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)
payloadFile, err := s.appFs.OpenFile(makePayloadPath(p), syscall.O_DIRECT|os.O_RDONLY, 0)
var openPath string
if opts.ResourceVersion == "metadata" {
openPath = makeMetadataPath(p)
} else {
openPath = makePayloadPath(p)
}
openFile, err := s.appFs.OpenFile(openPath, syscall.O_DIRECT|os.O_RDONLY, 0)
if err != nil {
if errors.Is(err, afero.ErrFileNotFound) {
if opts.IgnoreNotFound {
Expand All @@ -314,10 +320,13 @@ func (s *StorageImpl) get(ctx context.Context, key string, opts storage.GetOptio
return err
}
defer func() {
_ = payloadFile.Close()
_ = openFile.Close()
}()
decoder := gob.NewDecoder(NewDirectIOReader(payloadFile))
err = decoder.Decode(objPtr)
if opts.ResourceVersion == "metadata" {
err = json.NewDecoder(NewDirectIOReader(openFile)).Decode(objPtr)
} else {
err = gob.NewDecoder(NewDirectIOReader(openFile)).Decode(objPtr)
}
if err != nil {
if errors.Is(err, io.ErrUnexpectedEOF) || errors.Is(err, io.EOF) {
// irrecoverable error, delete both files
Expand Down
54 changes: 44 additions & 10 deletions pkg/registry/file/storage_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,8 +203,7 @@ func TestStorageImpl_Delete(t *testing.T) {
key: "/spdx.softwarecomposition.kubescape.io/sbomsyfts/kubescape/toto",
out: &v1beta1.SBOMSyft{},
},
create: true,
wantErr: true,
create: true,
},
{
name: "empty object",
Expand Down Expand Up @@ -256,24 +255,37 @@ 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
opts storage.GetOptions
objPtr runtime.Object
}
tests := []struct {
name string
args args
content string
create bool
wantErr assert.ErrorAssertionFunc
want runtime.Object
name string
args args
content string
contentMeta string
create bool
wantErr assert.ErrorAssertionFunc
want runtime.Object
}{
{
name: "not found",
Expand Down Expand Up @@ -311,6 +323,28 @@ func TestStorageImpl_Get(t *testing.T) {
content: realObj.String(),
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.String(),
create: true,
wantErr: assert.NoError,
want: &v1beta1.SBOMSyft{
ObjectMeta: v1.ObjectMeta{
Name: "toto",
Expand All @@ -332,8 +366,8 @@ func TestStorageImpl_Get(t *testing.T) {
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)
_ = afero.WriteFile(fs, getStoredMetadataFilepath(DefaultStorageRoot, tt.args.key), []byte(tt.contentMeta), 0644)
_ = afero.WriteFile(fs, getStoredPayloadFilepath(DefaultStorageRoot, tt.args.key), []byte(tt.content), 0644)
}
s := NewStorageImpl(fs, DefaultStorageRoot)
if err := s.Get(context.TODO(), tt.args.key, tt.args.opts, tt.args.objPtr); !tt.wantErr(t, err) {
Expand Down
2 changes: 1 addition & 1 deletion pkg/registry/file/vulnerabilitysummarystorage_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -425,10 +425,10 @@ func TestVulnSummaryStorageImpl_Get(t *testing.T) {
createObj: true, wantErr: false,
},
}
realStorage := NewStorageImpl(afero.NewMemMapFs(), "/")

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
realStorage := NewStorageImpl(afero.NewMemMapFs(), "/")
if tt.createObj {
for i := range tt.args.keyCreatedObj {
err := realStorage.Create(context.TODO(), tt.args.keyCreatedObj[i], tt.args.createdObj[i], nil, 0)
Expand Down
Loading