Skip to content

Commit

Permalink
feat: remove temp file after storing
Browse files Browse the repository at this point in the history 
Once we've done with the file remove it, cleaning up the file system
where possible.
  • Loading branch information
@SimonRichardson
SimonRichardson committed Jan 6, 2025
1 parent 3f8e436 commit 0769cfc
Show file tree
Hide file tree
Showing 4 changed files with 134 additions and 40 deletions.
4 changes: 2 additions & 2 deletions core/objectstore/objectstore.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,8 @@ type WriteObjectStore interface {
Put(ctx context.Context, path string, r io.Reader, size int64) (UUID, error)

// PutAndCheckHash stores data from reader at path, namespaced to the model.
// It also ensures the stored data has the correct hash.
PutAndCheckHash(ctx context.Context, path string, r io.Reader, size int64, hash string) (UUID, error)
// It also ensures the stored data has the correct sha384.
PutAndCheckHash(ctx context.Context, path string, r io.Reader, size int64, sha384 string) (UUID, error)

// Remove removes data at path, namespaced to the model.
Remove(ctx context.Context, path string) error
Expand Down
20 changes: 15 additions & 5 deletions domain/application/charm/store/store.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"os"
"strings"

"github.com/juju/juju/core/logger"
"github.com/juju/juju/core/objectstore"
"github.com/juju/juju/internal/errors"
objectstoreerrors "github.com/juju/juju/internal/objectstore/errors"
Expand Down Expand Up @@ -50,20 +51,22 @@ type StoreResult struct {
type CharmStore struct {
objectStoreGetter objectstore.ModelObjectStoreGetter
encoder *base64.Encoding
logger logger.Logger
}

// NewCharmStore returns a new charm store instance.
func NewCharmStore(objectStoreGetter objectstore.ModelObjectStoreGetter) *CharmStore {
func NewCharmStore(objectStoreGetter objectstore.ModelObjectStoreGetter, logger logger.Logger) *CharmStore {
return &CharmStore{
objectStoreGetter: objectStoreGetter,
encoder: base64.StdEncoding.WithPadding(base64.NoPadding),
logger: logger,
}
}

// Store the charm at the specified path into the object store. It is expected
// that the archive already exists at the specified path. If the file isn't
// found, a [ErrNotFound] is returned.
func (s *CharmStore) Store(ctx context.Context, path string, size int64, hash string) (StoreResult, error) {
func (s *CharmStore) Store(ctx context.Context, path string, size int64, sha384 string) (StoreResult, error) {
file, err := os.Open(path)
if errors.Is(err, os.ErrNotExist) {
return StoreResult{}, errors.Errorf("%q: %w", path, ErrNotFound)
Expand All @@ -87,7 +90,7 @@ func (s *CharmStore) Store(ctx context.Context, path string, size int64, hash st
return StoreResult{}, errors.Errorf("getting object store: %w", err)
}

uuid, err := objectStore.PutAndCheckHash(ctx, uniqueName, file, size, hash)
uuid, err := objectStore.PutAndCheckHash(ctx, uniqueName, file, size, sha384)
if err != nil {
return StoreResult{}, errors.Errorf("putting charm: %w", err)
}
Expand All @@ -108,7 +111,15 @@ func (s *CharmStore) StoreFromReader(ctx context.Context, reader io.Reader, hash
}

// Ensure that we close any open handles to the file.
defer file.Close()
defer func() {
_ = file.Close()
if err := os.Remove(file.Name()); err != nil {
// We don't need to log this as error, as the file will be removed
// when the process exits or by the OS. It's not a direct action
// by the user, hence Info.
s.logger.Infof("removing temporary file: %v", err)
}
}()

// Store the file in the object store.
objectStore, err := s.objectStoreGetter.GetObjectStore(ctx)
Expand Down Expand Up @@ -148,7 +159,6 @@ func (s *CharmStore) StoreFromReader(ctx context.Context, reader io.Reader, hash
}

return StoreResult{
ArchivePath: file.Name(),
UniqueName: uniqueName,
ObjectStoreUUID: uuid,
}, Digest{
Expand Down
148 changes: 116 additions & 32 deletions domain/application/charm/store/store_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package store
import (
"context"
"crypto/sha256"
"crypto/sha512"
"encoding/hex"
"io"
"os"
Expand All @@ -20,6 +21,7 @@ import (
"github.com/juju/juju/core/objectstore"
objectstoretesting "github.com/juju/juju/core/objectstore/testing"
"github.com/juju/juju/internal/errors"
loggertesting "github.com/juju/juju/internal/logger/testing"
objectstoreerrors "github.com/juju/juju/internal/objectstore/errors"
)

Expand All @@ -36,26 +38,28 @@ func (s *storeSuite) TestStore(c *gc.C) {
defer s.setupMocks(c).Finish()

dir := c.MkDir()
path, size, hash := s.createTempFile(c, dir, "hello world")
path, contentDigest := s.createTempFile(c, dir, "hello world")

uuid := objectstoretesting.GenObjectStoreUUID(c)

var (
uniqueName string
contents string
)
s.objectStore.EXPECT().PutAndCheckHash(gomock.Any(), gomock.Any(), gomock.Any(), size, hash).DoAndReturn(func(_ context.Context, name string, reader io.Reader, _ int64, _ string) (objectstore.UUID, error) {
uniqueName = name
s.objectStore.EXPECT().
PutAndCheckHash(gomock.Any(), gomock.Any(), gomock.Any(), contentDigest.Size, contentDigest.SHA384).
DoAndReturn(func(_ context.Context, name string, reader io.Reader, _ int64, _ string) (objectstore.UUID, error) {
uniqueName = name

data, err := io.ReadAll(reader)
c.Assert(err, jc.ErrorIsNil)
contents = string(data)
data, err := io.ReadAll(reader)
c.Assert(err, jc.ErrorIsNil)
contents = string(data)

return uuid, nil
})
return uuid, nil
})

storage := NewCharmStore(s.objectStoreGetter)
storeResult, err := storage.Store(context.Background(), path, size, hash)
storage := NewCharmStore(s.objectStoreGetter, loggertesting.WrapCheckLog(c))
storeResult, err := storage.Store(context.Background(), path, contentDigest.Size, contentDigest.SHA384)
c.Assert(err, jc.ErrorIsNil)

c.Check(storeResult.ObjectStoreUUID, gc.DeepEquals, uuid)
Expand All @@ -69,19 +73,21 @@ func (s *storeSuite) TestStoreFileClosed(c *gc.C) {
defer s.setupMocks(c).Finish()

dir := c.MkDir()
path, size, hash := s.createTempFile(c, dir, "hello world")
path, contentDigest := s.createTempFile(c, dir, "hello world")

uuid := objectstoretesting.GenObjectStoreUUID(c)

var reader io.Reader
s.objectStore.EXPECT().PutAndCheckHash(gomock.Any(), gomock.Any(), gomock.Any(), size, hash).DoAndReturn(func(_ context.Context, _ string, r io.Reader, _ int64, _ string) (objectstore.UUID, error) {
reader = r
s.objectStore.EXPECT().
PutAndCheckHash(gomock.Any(), gomock.Any(), gomock.Any(), contentDigest.Size, contentDigest.SHA384).
DoAndReturn(func(_ context.Context, _ string, r io.Reader, _ int64, _ string) (objectstore.UUID, error) {
reader = r

return uuid, nil
})
return uuid, nil
})

storage := NewCharmStore(s.objectStoreGetter)
_, err := storage.Store(context.Background(), path, size, hash)
storage := NewCharmStore(s.objectStoreGetter, loggertesting.WrapCheckLog(c))
_, err := storage.Store(context.Background(), path, contentDigest.Size, contentDigest.SHA384)
c.Assert(err, jc.ErrorIsNil)

// Attempt to read the contents of the read after it's been closed.
Expand All @@ -95,7 +101,7 @@ func (s *storeSuite) TestStoreFileNotFound(c *gc.C) {

dir := c.MkDir()

storage := NewCharmStore(s.objectStoreGetter)
storage := NewCharmStore(s.objectStoreGetter, loggertesting.WrapCheckLog(c))
_, err := storage.Store(context.Background(), filepath.Join(dir, "foo"), 12, "hash")
c.Assert(err, jc.ErrorIs, ErrNotFound)
}
Expand All @@ -104,22 +110,88 @@ func (s *storeSuite) TestStoreFailed(c *gc.C) {
defer s.setupMocks(c).Finish()

dir := c.MkDir()
path, size, hash := s.createTempFile(c, dir, "hello world")
path, contentDigest := s.createTempFile(c, dir, "hello world")

s.objectStore.EXPECT().PutAndCheckHash(gomock.Any(), gomock.Any(), gomock.Any(), size, hash).Return("", errors.Errorf("boom"))
s.objectStore.EXPECT().
PutAndCheckHash(gomock.Any(), gomock.Any(), gomock.Any(), contentDigest.Size, contentDigest.SHA384).
Return("", errors.Errorf("boom"))

storage := NewCharmStore(s.objectStoreGetter)
_, err := storage.Store(context.Background(), path, size, hash)
storage := NewCharmStore(s.objectStoreGetter, loggertesting.WrapCheckLog(c))
_, err := storage.Store(context.Background(), path, contentDigest.Size, contentDigest.SHA384)
c.Assert(err, gc.ErrorMatches, ".*boom")
}

func (s *storeSuite) TestStoreFromReader(c *gc.C) {
defer s.setupMocks(c).Finish()

dir := c.MkDir()
path, contentDigest := s.createTempFile(c, dir, "hello world")
reader, err := os.Open(path)
c.Assert(err, jc.ErrorIsNil)

uuid := objectstoretesting.GenObjectStoreUUID(c)

var (
uniqueName string
contents string
)
s.objectStore.EXPECT().
PutAndCheckHash(gomock.Any(), gomock.Any(), gomock.Any(), contentDigest.Size, contentDigest.SHA384).
DoAndReturn(func(_ context.Context, name string, reader io.Reader, _ int64, _ string) (objectstore.UUID, error) {
uniqueName = name

data, err := io.ReadAll(reader)
c.Assert(err, jc.ErrorIsNil)
contents = string(data)

return uuid, nil
})

storage := NewCharmStore(s.objectStoreGetter, loggertesting.WrapCheckLog(c))
storeResult, digest, err := storage.StoreFromReader(context.Background(), reader, contentDigest.SHA256[:7])
c.Assert(err, jc.ErrorIsNil)

c.Check(storeResult.ObjectStoreUUID, gc.DeepEquals, uuid)
c.Check(storeResult.UniqueName, gc.Equals, uniqueName)

c.Check(digest, gc.DeepEquals, contentDigest)

// Make sure the contents are the same and it's not been tampered with.
c.Check(contents, gc.Equals, "hello world")
}

func (s *storeSuite) TestStoreFromReaderEmptyReader(c *gc.C) {
defer s.setupMocks(c).Finish()

dir := c.MkDir()
_, contentDigest := s.createTempFile(c, dir, "hello world")
reader := io.NopCloser(strings.NewReader(""))

storage := NewCharmStore(s.objectStoreGetter, loggertesting.WrapCheckLog(c))
_, _, err := storage.StoreFromReader(context.Background(), reader, contentDigest.SHA256[:7])
c.Assert(err, jc.ErrorIs, ErrCharmHashMismatch)
}

func (s *storeSuite) TestStoreFromReaderInvalidHash(c *gc.C) {
defer s.setupMocks(c).Finish()

dir := c.MkDir()
path, _ := s.createTempFile(c, dir, "hello world")
reader, err := os.Open(path)
c.Assert(err, jc.ErrorIsNil)

storage := NewCharmStore(s.objectStoreGetter, loggertesting.WrapCheckLog(c))
_, _, err = storage.StoreFromReader(context.Background(), reader, "blah")
c.Assert(err, jc.ErrorIs, ErrCharmHashMismatch)
}

func (s *storeSuite) TestGet(c *gc.C) {
defer s.setupMocks(c).Finish()

archive := io.NopCloser(strings.NewReader("archive-content"))
s.objectStore.EXPECT().Get(gomock.Any(), "foo").Return(archive, 0, nil)

storage := NewCharmStore(s.objectStoreGetter)
storage := NewCharmStore(s.objectStoreGetter, loggertesting.WrapCheckLog(c))
reader, err := storage.Get(context.Background(), "foo")
c.Assert(err, jc.ErrorIsNil)

Expand All @@ -133,7 +205,7 @@ func (s *storeSuite) TestGetFailed(c *gc.C) {

s.objectStore.EXPECT().Get(gomock.Any(), "foo").Return(nil, 0, errors.Errorf("boom"))

storage := NewCharmStore(s.objectStoreGetter)
storage := NewCharmStore(s.objectStoreGetter, loggertesting.WrapCheckLog(c))

_, err := storage.Get(context.Background(), "foo")
c.Assert(err, gc.ErrorMatches, ".*boom")
Expand All @@ -144,7 +216,7 @@ func (s *storeSuite) TestGetNotFound(c *gc.C) {

s.objectStore.EXPECT().Get(gomock.Any(), "foo").Return(nil, 0, objectstoreerrors.ObjectNotFound)

storage := NewCharmStore(s.objectStoreGetter)
storage := NewCharmStore(s.objectStoreGetter, loggertesting.WrapCheckLog(c))
_, err := storage.Get(context.Background(), "foo")
c.Assert(err, jc.ErrorIs, ErrNotFound)
}
Expand All @@ -155,7 +227,7 @@ func (s *storeSuite) TestGetBySHA256Prefix(c *gc.C) {
archive := io.NopCloser(strings.NewReader("archive-content"))
s.objectStore.EXPECT().GetBySHA256Prefix(gomock.Any(), "02638299").Return(archive, 0, nil)

storage := NewCharmStore(s.objectStoreGetter)
storage := NewCharmStore(s.objectStoreGetter, loggertesting.WrapCheckLog(c))
reader, err := storage.GetBySHA256Prefix(context.Background(), "02638299")
c.Assert(err, jc.ErrorIsNil)
content, err := io.ReadAll(reader)
Expand All @@ -168,7 +240,7 @@ func (s *storeSuite) TestGetBySHA256PrefixFailed(c *gc.C) {

s.objectStore.EXPECT().GetBySHA256Prefix(gomock.Any(), "02638299").Return(nil, 0, errors.Errorf("boom"))

storage := NewCharmStore(s.objectStoreGetter)
storage := NewCharmStore(s.objectStoreGetter, loggertesting.WrapCheckLog(c))
_, err := storage.GetBySHA256Prefix(context.Background(), "02638299")
c.Assert(err, gc.ErrorMatches, ".*boom")
}
Expand All @@ -178,7 +250,7 @@ func (s *storeSuite) TestGetBySHA256NotFound(c *gc.C) {

s.objectStore.EXPECT().GetBySHA256Prefix(gomock.Any(), "02638299").Return(nil, 0, objectstoreerrors.ObjectNotFound)

storage := NewCharmStore(s.objectStoreGetter)
storage := NewCharmStore(s.objectStoreGetter, loggertesting.WrapCheckLog(c))
_, err := storage.GetBySHA256Prefix(context.Background(), "02638299")
c.Assert(err, jc.ErrorIs, ErrNotFound)
}
Expand All @@ -194,19 +266,31 @@ func (s *storeSuite) setupMocks(c *gc.C) *gomock.Controller {
return ctrl
}

func (s *storeSuite) createTempFile(c *gc.C, dir, content string) (string, int64, string) {
func (s *storeSuite) createTempFile(c *gc.C, dir, content string) (string, Digest) {
path := filepath.Join(dir, "test")
err := os.WriteFile(path, []byte(content), 0644)
c.Assert(err, jc.ErrorIsNil)

info, err := os.Stat(path)
c.Assert(err, jc.ErrorIsNil)

return path, info.Size(), calculateHash(content)
return path, Digest{
SHA256: calculateSHA256(c, content),
SHA384: calculateSHA384(c, content),
Size: info.Size(),
}
}

func calculateHash(content string) string {
func calculateSHA384(c *gc.C, content string) string {
hash := sha512.New384()
_, err := hash.Write([]byte(content))
c.Assert(err, jc.ErrorIsNil)
return hex.EncodeToString(hash.Sum(nil))
}

func calculateSHA256(c *gc.C, content string) string {
hash := sha256.New()
hash.Write([]byte(content))
_, err := hash.Write([]byte(content))
c.Assert(err, jc.ErrorIsNil)
return hex.EncodeToString(hash.Sum(nil))
}
2 changes: 1 addition & 1 deletion domain/services/model.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ func (s *ModelServices) Application() *applicationservice.WatchableService {
s.modelWatcherFactory("application"),
modelagentstate.NewState(changestream.NewTxnRunnerFactory(s.controllerDB)),
providertracker.ProviderRunner[applicationservice.Provider](s.providerFactory, s.modelUUID.String()),
charmstore.NewCharmStore(s.objectstore),
charmstore.NewCharmStore(s.objectstore, log.Child("charmstore")),
s.clock,
log,
)
Expand Down

0 comments on commit 0769cfc

Please sign in to comment.