Skip to content

Commit 6b9b034

Browse files
SimonRichardsonSimonRichardson
committed
feat: use tmp buffer to then store it
We require a buffer so that it's possible to read the charm after it's been written to the object store.
1 parent 0769cfc commit 6b9b034

File tree

5 files changed

+120
-74
lines changed

5 files changed

+120
-74
lines changed

‎domain/application/charm/store/store.go

Lines changed: 24 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -39,12 +39,18 @@ type Digest struct {
3939
Size int64
4040
}
4141

42+
// StoreFromReaderResult contains the unique name of the charm archive and the
43+
// object store UUID.
44+
type StoreFromReaderResult struct {
45+
UniqueName string
46+
ObjectStoreUUID objectstore.UUID
47+
}
48+
4249
// StoreResult contains the path of the stored charm archive, the unique name
4350
// of the charm archive, and the object store UUID.
4451
type StoreResult struct {
45-
ArchivePath string
46-
UniqueName string
47-
ObjectStoreUUID objectstore.UUID
52+
StoreFromReaderResult
53+
ArchivePath string
4854
}
4955

5056
// CharmStore provides an API for storing and retrieving charm blobs.
@@ -95,19 +101,21 @@ func (s *CharmStore) Store(ctx context.Context, path string, size int64, sha384
95101
return StoreResult{}, errors.Errorf("putting charm: %w", err)
96102
}
97103
return StoreResult{
98-
ArchivePath: file.Name(),
99-
UniqueName: uniqueName,
100-
ObjectStoreUUID: uuid,
104+
StoreFromReaderResult: StoreFromReaderResult{
105+
UniqueName: uniqueName,
106+
ObjectStoreUUID: uuid,
107+
},
108+
ArchivePath: file.Name(),
101109
}, nil
102110
}
103111

104112
// StoreFromReader stores the charm from the provided reader into the object
105113
// store. The caller is expected to remove the temporary file after the call.
106114
// IThis does not check the integrity of the charm hash.
107-
func (s *CharmStore) StoreFromReader(ctx context.Context, reader io.Reader, hashPrefix string) (StoreResult, Digest, error) {
115+
func (s *CharmStore) StoreFromReader(ctx context.Context, reader io.Reader, hashPrefix string) (StoreFromReaderResult, Digest, error) {
108116
file, err := os.CreateTemp("", "charm-")
109117
if err != nil {
110-
return StoreResult{}, Digest{}, errors.Errorf("creating temporary file: %w", err)
118+
return StoreFromReaderResult{}, Digest{}, errors.Errorf("creating temporary file: %w", err)
111119
}
112120

113121
// Ensure that we close any open handles to the file.
@@ -124,41 +132,41 @@ func (s *CharmStore) StoreFromReader(ctx context.Context, reader io.Reader, hash
124132
// Store the file in the object store.
125133
objectStore, err := s.objectStoreGetter.GetObjectStore(ctx)
126134
if err != nil {
127-
return StoreResult{}, Digest{}, errors.Errorf("getting object store: %w", err)
135+
return StoreFromReaderResult{}, Digest{}, errors.Errorf("getting object store: %w", err)
128136
}
129137

130138
// Generate a unique path for the file.
131139
unique, err := uuid.NewUUID()
132140
if err != nil {
133-
return StoreResult{}, Digest{}, errors.Errorf("cannot generate unique path")
141+
return StoreFromReaderResult{}, Digest{}, errors.Errorf("cannot generate unique path")
134142
}
135143
uniqueName := s.encoder.EncodeToString(unique[:])
136144

137145
// Copy the reader into the temporary file.
138146
sha256, sha384, size, err := storeAndComputeHashes(file, reader)
139147
if err != nil {
140-
return StoreResult{}, Digest{}, errors.Errorf("storing charm from reader: %w", err)
148+
return StoreFromReaderResult{}, Digest{}, errors.Errorf("storing charm from reader: %w", err)
141149
}
142150

143151
// Ensure that we sync the file to disk, as the process may crash before
144152
// the file is written to disk.
145153
if err := file.Sync(); err != nil {
146-
return StoreResult{}, Digest{}, errors.Errorf("syncing temporary file: %w", err)
154+
return StoreFromReaderResult{}, Digest{}, errors.Errorf("syncing temporary file: %w", err)
147155
}
148156
if _, err := file.Seek(0, io.SeekStart); err != nil {
149-
return StoreResult{}, Digest{}, errors.Errorf("seeking temporary file: %w", err)
157+
return StoreFromReaderResult{}, Digest{}, errors.Errorf("seeking temporary file: %w", err)
150158
}
151159

152160
if !strings.HasPrefix(sha256, hashPrefix) {
153-
return StoreResult{}, Digest{}, ErrCharmHashMismatch
161+
return StoreFromReaderResult{}, Digest{}, ErrCharmHashMismatch
154162
}
155163

156164
uuid, err := objectStore.PutAndCheckHash(ctx, uniqueName, file, size, sha384)
157165
if err != nil {
158-
return StoreResult{}, Digest{}, errors.Errorf("putting charm: %w", err)
166+
return StoreFromReaderResult{}, Digest{}, errors.Errorf("putting charm: %w", err)
159167
}
160168

161-
return StoreResult{
169+
return StoreFromReaderResult{
162170
UniqueName: uniqueName,
163171
ObjectStoreUUID: uuid,
164172
}, Digest{

‎domain/application/service/application_test.go

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1338,9 +1338,11 @@ func (s *applicationServiceSuite) TestResolveCharmDownload(c *gc.C) {
13381338

13391339
s.state.EXPECT().GetAsyncCharmDownloadInfo(gomock.Any(), appUUID).Return(info, nil)
13401340
s.charmStore.EXPECT().Store(gomock.Any(), path, int64(42), "hash-384").Return(store.StoreResult{
1341-
ArchivePath: "/tmp/somepath",
1342-
UniqueName: "somepath",
1343-
ObjectStoreUUID: objectStoreUUID,
1341+
StoreFromReaderResult: store.StoreFromReaderResult{
1342+
UniqueName: "somepath",
1343+
ObjectStoreUUID: objectStoreUUID,
1344+
},
1345+
ArchivePath: "/tmp/somepath",
13441346
}, nil)
13451347
s.state.EXPECT().ResolveCharmDownload(gomock.Any(), charmUUID, application.ResolvedCharmDownload{
13461348
Actions: actions,
@@ -1510,9 +1512,11 @@ func (s *applicationServiceSuite) TestResolveCharmDownloadAlreadyStored(c *gc.C)
15101512

15111513
s.state.EXPECT().GetAsyncCharmDownloadInfo(gomock.Any(), appUUID).Return(info, nil)
15121514
s.charmStore.EXPECT().Store(gomock.Any(), path, int64(42), "hash-384").Return(store.StoreResult{
1513-
ArchivePath: "/tmp/somepath",
1514-
UniqueName: "somepath",
1515-
ObjectStoreUUID: objectStoreUUID,
1515+
StoreFromReaderResult: store.StoreFromReaderResult{
1516+
UniqueName: "somepath",
1517+
ObjectStoreUUID: objectStoreUUID,
1518+
},
1519+
ArchivePath: "/tmp/somepath",
15161520
}, nil)
15171521
s.state.EXPECT().ResolveCharmDownload(gomock.Any(), charmUUID, application.ResolvedCharmDownload{
15181522
Actions: actions,

‎domain/application/service/charm.go

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
package service
55

66
import (
7+
"bytes"
78
"context"
89
"fmt"
910
"io"
@@ -181,7 +182,7 @@ type CharmStore interface {
181182
// call.
182183
// sha256Prefix is the prefix characters of the SHA256 hash of the charm
183184
// archive.
184-
StoreFromReader(ctx context.Context, reader io.Reader, sha256Prefix string) (store.StoreResult, store.Digest, error)
185+
StoreFromReader(ctx context.Context, reader io.Reader, sha256Prefix string) (store.StoreFromReaderResult, store.Digest, error)
185186

186187
// GetCharm retrieves a ReadCloser for the charm archive at the give path
187188
// from the underlying storage.
@@ -690,7 +691,13 @@ func (s *Service) ResolveUploadCharm(ctx context.Context, args charm.ResolveUplo
690691
}
691692

692693
func (s *Service) resolveLocalUploadedCharm(ctx context.Context, args charm.ResolveUploadCharm) (charm.CharmLocator, error) {
693-
result, digest, err := s.charmStore.StoreFromReader(ctx, args.Reader, args.SHA256Prefix)
694+
// The charm has been stored in the objectstore. Rather than read it back
695+
// out (consider S3 in a different region), we can just tee the reader
696+
// into a buffer and read the charm from the buffer.
697+
buffer := new(bytes.Buffer)
698+
699+
// Store the charm and validate it against the sha256 prefix.
700+
result, digest, err := s.charmStore.StoreFromReader(ctx, io.TeeReader(args.Reader, buffer), args.SHA256Prefix)
694701
if err != nil {
695702
return charm.CharmLocator{}, internalerrors.Errorf("resolving uploaded charm: %w", err)
696703
}
@@ -705,9 +712,9 @@ func (s *Service) resolveLocalUploadedCharm(ctx context.Context, args charm.Reso
705712
// of the SHA256 hash (that's enough to prevent collisions).
706713

707714
// Make sure it's actually a valid charm.
708-
ch, err := internalcharm.ReadCharmArchive(result.ArchivePath)
715+
ch, err := internalcharm.ReadCharmArchiveBytes(buffer.Bytes())
709716
if err != nil {
710-
return charm.CharmLocator{}, internalerrors.Errorf("reading charm archive %q: %w", result.ArchivePath, err)
717+
return charm.CharmLocator{}, internalerrors.Errorf("reading charm archive: %w", err)
711718
}
712719

713720
// This is a full blown upload, we need to set everything up.
@@ -767,7 +774,12 @@ func (s *Service) resolveMigratingUploadedCharm(ctx context.Context, args charm.
767774
return charm.CharmLocator{}, errors.Annotate(err, "locating existing charm")
768775
}
769776

770-
result, digest, err := s.charmStore.StoreFromReader(ctx, args.Reader, args.SHA256Prefix)
777+
// The charm has been stored in the objectstore. Rather than read it back
778+
// out (consider S3 in a different region), we can just tee the reader
779+
// into a buffer and read the charm from the buffer.
780+
buffer := new(bytes.Buffer)
781+
782+
result, digest, err := s.charmStore.StoreFromReader(ctx, io.TeeReader(args.Reader, buffer), args.SHA256Prefix)
771783
if err != nil {
772784
return charm.CharmLocator{}, internalerrors.Errorf("resolving uploaded charm: %w", err)
773785
}
@@ -782,8 +794,8 @@ func (s *Service) resolveMigratingUploadedCharm(ctx context.Context, args charm.
782794
// before accepting the charm. For now this is a check to ensure that the
783795
// charm is valid.
784796
// Work should be done to ensure the integrity of the charm archive.
785-
if _, err := internalcharm.ReadCharmArchive(result.ArchivePath); err != nil {
786-
return charm.CharmLocator{}, errors.Annotatef(err, "reading charm archive %q", result.ArchivePath)
797+
if _, err := internalcharm.ReadCharmArchiveBytes(buffer.Bytes()); err != nil {
798+
return charm.CharmLocator{}, errors.Annotatef(err, "reading charm archive")
787799
}
788800

789801
// This will correctly sequence the charm if it's a local charm.

‎domain/application/service/charm_test.go

Lines changed: 62 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -1363,15 +1363,21 @@ func (s *charmServiceSuite) TestResolveUploadCharmLocalCharmNotImporting(c *gc.C
13631363
Provenance: charm.ProvenanceUpload,
13641364
}
13651365

1366-
s.charmStore.EXPECT().StoreFromReader(gomock.Any(), gomock.Not(gomock.Nil()), "abc").Return(store.StoreResult{
1367-
ObjectStoreUUID: objectStoreUUID,
1368-
UniqueName: "unique-name",
1369-
ArchivePath: path,
1370-
}, store.Digest{
1371-
SHA256: "sha-256",
1372-
SHA384: "sha-384",
1373-
Size: stat.Size(),
1374-
}, nil)
1366+
s.charmStore.EXPECT().StoreFromReader(gomock.Any(), gomock.Not(gomock.Nil()), "abc").
1367+
DoAndReturn(func(ctx context.Context, r io.Reader, s string) (store.StoreFromReaderResult, store.Digest, error) {
1368+
// Force the reader, so we populate the buffer.
1369+
_, err := io.Copy(io.Discard, r)
1370+
c.Assert(err, jc.ErrorIsNil)
1371+
1372+
return store.StoreFromReaderResult{
1373+
ObjectStoreUUID: objectStoreUUID,
1374+
UniqueName: "unique-name",
1375+
}, store.Digest{
1376+
SHA256: "sha-256",
1377+
SHA384: "sha-384",
1378+
Size: stat.Size(),
1379+
}, nil
1380+
})
13751381
s.state.EXPECT().SetCharm(gomock.Any(), gomock.Any(), downloadInfo).DoAndReturn(func(_ context.Context, ch charm.Charm, _ *charm.DownloadInfo) (corecharm.ID, charm.CharmLocator, error) {
13761382
c.Check(ch.Metadata.Name, gc.Equals, "dummy")
13771383
return charmID, charm.CharmLocator{
@@ -1404,10 +1410,9 @@ func (s *charmServiceSuite) TestResolveUploadCharmLocalCharmNotImportingFailedRe
14041410

14051411
objectStoreUUID := objectstoretesting.GenObjectStoreUUID(c)
14061412

1407-
s.charmStore.EXPECT().StoreFromReader(gomock.Any(), gomock.Not(gomock.Nil()), "abc").Return(store.StoreResult{
1413+
s.charmStore.EXPECT().StoreFromReader(gomock.Any(), gomock.Not(gomock.Nil()), "abc").Return(store.StoreFromReaderResult{
14081414
ObjectStoreUUID: objectStoreUUID,
14091415
UniqueName: "unique-name",
1410-
ArchivePath: "/tmp/foo",
14111416
}, store.Digest{
14121417
SHA256: "sha-256",
14131418
SHA384: "sha-384",
@@ -1442,15 +1447,21 @@ func (s *charmServiceSuite) TestResolveUploadCharmLocalCharmNotImportingFailedSe
14421447
Provenance: charm.ProvenanceUpload,
14431448
}
14441449

1445-
s.charmStore.EXPECT().StoreFromReader(gomock.Any(), gomock.Not(gomock.Nil()), "abc").Return(store.StoreResult{
1446-
ObjectStoreUUID: objectStoreUUID,
1447-
UniqueName: "unique-name",
1448-
ArchivePath: path,
1449-
}, store.Digest{
1450-
SHA256: "sha-256",
1451-
SHA384: "sha-384",
1452-
Size: stat.Size(),
1453-
}, nil)
1450+
s.charmStore.EXPECT().StoreFromReader(gomock.Any(), gomock.Not(gomock.Nil()), "abc").
1451+
DoAndReturn(func(ctx context.Context, r io.Reader, s string) (store.StoreFromReaderResult, store.Digest, error) {
1452+
// Force the reader, so we populate the buffer.
1453+
_, err := io.Copy(io.Discard, r)
1454+
c.Assert(err, jc.ErrorIsNil)
1455+
1456+
return store.StoreFromReaderResult{
1457+
ObjectStoreUUID: objectStoreUUID,
1458+
UniqueName: "unique-name",
1459+
}, store.Digest{
1460+
SHA256: "sha-256",
1461+
SHA384: "sha-384",
1462+
Size: stat.Size(),
1463+
}, nil
1464+
})
14541465
s.state.EXPECT().SetCharm(gomock.Any(), gomock.Any(), downloadInfo).DoAndReturn(func(_ context.Context, _ charm.Charm, _ *charm.DownloadInfo) (corecharm.ID, charm.CharmLocator, error) {
14551466
return charmID, charm.CharmLocator{}, errors.NotValidf("failed to set charm")
14561467
})
@@ -1487,15 +1498,21 @@ func (s *charmServiceSuite) TestResolveUploadCharmLocalCharmImporting(c *gc.C) {
14871498
}
14881499

14891500
s.state.EXPECT().GetCharmID(gomock.Any(), "test", 1, charm.LocalSource).Return(charmID, nil)
1490-
s.charmStore.EXPECT().StoreFromReader(gomock.Any(), gomock.Not(gomock.Nil()), "abc").Return(store.StoreResult{
1491-
ObjectStoreUUID: objectStoreUUID,
1492-
UniqueName: "unique-name",
1493-
ArchivePath: path,
1494-
}, store.Digest{
1495-
SHA256: "sha-256",
1496-
SHA384: "sha-384",
1497-
Size: stat.Size(),
1498-
}, nil)
1501+
s.charmStore.EXPECT().StoreFromReader(gomock.Any(), gomock.Not(gomock.Nil()), "abc").
1502+
DoAndReturn(func(ctx context.Context, r io.Reader, s string) (store.StoreFromReaderResult, store.Digest, error) {
1503+
// Force the reader, so we populate the buffer.
1504+
_, err := io.Copy(io.Discard, r)
1505+
c.Assert(err, jc.ErrorIsNil)
1506+
1507+
return store.StoreFromReaderResult{
1508+
ObjectStoreUUID: objectStoreUUID,
1509+
UniqueName: "unique-name",
1510+
}, store.Digest{
1511+
SHA256: "sha-256",
1512+
SHA384: "sha-384",
1513+
Size: stat.Size(),
1514+
}, nil
1515+
})
14991516
s.state.EXPECT().ResolveMigratingUploadedCharm(gomock.Any(), charmID, charm.ResolvedMigratingUploadedCharm{
15001517
ObjectStoreUUID: objectStoreUUID,
15011518
Hash: "sha-256",
@@ -1564,10 +1581,9 @@ func (s *charmServiceSuite) TestResolveUploadCharmLocalCharmImportingFailedStore
15641581
objectStoreUUID := objectstoretesting.GenObjectStoreUUID(c)
15651582

15661583
s.state.EXPECT().GetCharmID(gomock.Any(), "test", 1, charm.LocalSource).Return(charmID, nil)
1567-
s.charmStore.EXPECT().StoreFromReader(gomock.Any(), gomock.Not(gomock.Nil()), "abc").Return(store.StoreResult{
1584+
s.charmStore.EXPECT().StoreFromReader(gomock.Any(), gomock.Not(gomock.Nil()), "abc").Return(store.StoreFromReaderResult{
15681585
ObjectStoreUUID: objectStoreUUID,
15691586
UniqueName: "unique-name",
1570-
ArchivePath: path,
15711587
}, store.Digest{
15721588
SHA256: "sha-256",
15731589
SHA384: "sha-384",
@@ -1604,15 +1620,21 @@ func (s *charmServiceSuite) TestResolveUploadCharmLocalCharmImportingFailedResol
16041620
}
16051621

16061622
s.state.EXPECT().GetCharmID(gomock.Any(), "test", 1, charm.LocalSource).Return(charmID, nil)
1607-
s.charmStore.EXPECT().StoreFromReader(gomock.Any(), gomock.Not(gomock.Nil()), "abc").Return(store.StoreResult{
1608-
ObjectStoreUUID: objectStoreUUID,
1609-
UniqueName: "unique-name",
1610-
ArchivePath: path,
1611-
}, store.Digest{
1612-
SHA256: "sha-256",
1613-
SHA384: "sha-384",
1614-
Size: stat.Size(),
1615-
}, nil)
1623+
s.charmStore.EXPECT().StoreFromReader(gomock.Any(), gomock.Not(gomock.Nil()), "abc").
1624+
DoAndReturn(func(ctx context.Context, r io.Reader, s string) (store.StoreFromReaderResult, store.Digest, error) {
1625+
// Force the reader, so we populate the buffer.
1626+
_, err := io.Copy(io.Discard, r)
1627+
c.Assert(err, jc.ErrorIsNil)
1628+
1629+
return store.StoreFromReaderResult{
1630+
ObjectStoreUUID: objectStoreUUID,
1631+
UniqueName: "unique-name",
1632+
}, store.Digest{
1633+
SHA256: "sha-256",
1634+
SHA384: "sha-384",
1635+
Size: stat.Size(),
1636+
}, nil
1637+
})
16161638
s.state.EXPECT().ResolveMigratingUploadedCharm(gomock.Any(), charmID, charm.ResolvedMigratingUploadedCharm{
16171639
ObjectStoreUUID: objectStoreUUID,
16181640
Hash: "sha-256",

‎domain/application/service/package_mock_test.go

Lines changed: 5 additions & 5 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)