Merge pull request juju#18816 from Aflynn50/fix-object-store-errors

juju#18816

#### Fix store resource object store errors
Handle errors where the store has already got a resource with that UUID and move to using the correct object store errors.

The errors used previously were from the object store domain, they should have been the errors from the internal object store package, as this is what is actually emitted. Because of this they were not being caught.

Also correctly catch the error where the object already exists in the store, and return an appropriate error.

#### Add revision and origin to StoreResource in resource opener.

These commits was previously in juju#18669 which got closed. 

They also fixes some of the bash tests.
## Checklist

<!-- If an item is not applicable, use `~strikethrough~`. -->

- [x] Code style: imports ordered, good names, simple structure, etc
- [x] Comments saying why design decisions were made
- [x] Go unit tests, with comments saying what you're testing
- [ ] [Integration tests](https://github.com/juju/juju/tree/main/tests), with comments saying what you're testing
- [ ] [doc.go](https://discourse.charmhub.io/t/readme-in-packages/451) added or updated in changed packages

## QA steps
```
> juju bootstrap lxd resource-gating
> ./main.sh -v -l resource-gating resources test_basic_resources
...
==> Test result: success
...
```


## Links

<!-- Link to all relevant specification, documentation, bug, issue or JIRA card. -->


**Jira card:** [JUJU-7066](https://warthogs.atlassian.net/browse/JUJU-7066)



[JUJU-7066]: https://warthogs.atlassian.net/browse/JUJU-7066?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ

domain/resource/errors/errors.go

@@ -62,6 +62,10 @@ const (
 	// is not valid.
 	ResourceRevisionNotValid = errors.ConstError("resource revision not valid")
 
+	// StoredResourceAlreadyExists describes an error where the resource being
+	// stored already exists in the store.
+	StoredResourceAlreadyExists = errors.ConstError("stored resource already exists")
+
 	// ResourceAlreadyStored describes an errors where the resource has already
 	// been stored.
 	ResourceAlreadyStored = errors.ConstError("resource already found in storage")

domain/resource/service/resource.go

@@ -14,11 +14,11 @@ import (
 	coreresourcestore "github.com/juju/juju/core/resource/store"
 	coreunit "github.com/juju/juju/core/unit"
 	containerimageresourcestoreerrors "github.com/juju/juju/domain/containerimageresourcestore/errors"
-	objectstoreerrors "github.com/juju/juju/domain/objectstore/errors"
 	"github.com/juju/juju/domain/resource"
 	resourceerrors "github.com/juju/juju/domain/resource/errors"
 	charmresource "github.com/juju/juju/internal/charm/resource"
 	"github.com/juju/juju/internal/errors"
+	objectstoreerrors "github.com/juju/juju/internal/objectstore/errors"
 )
 
 // State describes retrieval and persistence methods for resource.
@@ -365,12 +365,6 @@ func (s *Service) storeResource(
 		return errors.Errorf("getting resource: %w", err)
 	}
 
-	if args.Fingerprint.String() == res.Fingerprint.String() {
-		// This resource blob has already been stored, no need to store it
-		// again.
-		return nil
-	}
-
 	store, err := s.resourceStoreGetter.GetResourceStore(ctx, res.Type)
 	if err != nil {
 		return errors.Errorf("getting resource store for %s: %w", res.Type.String(), err)
@@ -384,7 +378,10 @@ func (s *Service) storeResource(
 		args.Size,
 		coreresourcestore.NewFingerprint(args.Fingerprint.Fingerprint),
 	)
-	if err != nil {
+	if errors.Is(err, objectstoreerrors.ObjectAlreadyExists) ||
+		errors.Is(err, containerimageresourcestoreerrors.ContainerImageMetadataAlreadyStored) {
+		return resourceerrors.StoredResourceAlreadyExists
+	} else if err != nil {
 		return errors.Errorf("putting resource %q in store: %w", res.Name, err)
 	}
 	defer func() {
@@ -456,7 +453,7 @@ func (s *Service) OpenResource(
 	// resources storageID, however the object store does not currently have a
 	// method for this.
 	reader, _, err := store.Get(ctx, blobPath(resourceUUID, res.Fingerprint.String()))
-	if errors.Is(err, objectstoreerrors.ErrNotFound) ||
+	if errors.Is(err, objectstoreerrors.ObjectNotFound) ||
 		errors.Is(err, containerimageresourcestoreerrors.ContainerImageMetadataNotFound) {
 		return coreresource.Resource{}, nil, resourceerrors.StoredResourceNotFound
 	} else if err != nil {

domain/resource/service/resource_test.go

@@ -24,11 +24,11 @@ import (
 	resourcetesting "github.com/juju/juju/core/resource/testing"
 	unittesting "github.com/juju/juju/core/unit/testing"
 	containerimageresourcestoreerrors "github.com/juju/juju/domain/containerimageresourcestore/errors"
-	objectstoreerrors "github.com/juju/juju/domain/objectstore/errors"
 	"github.com/juju/juju/domain/resource"
 	resourceerrors "github.com/juju/juju/domain/resource/errors"
 	charmresource "github.com/juju/juju/internal/charm/resource"
 	loggertesting "github.com/juju/juju/internal/logger/testing"
+	objectstoreerrors "github.com/juju/juju/internal/objectstore/errors"
 )
 
 type resourceServiceSuite struct {
@@ -495,11 +495,10 @@ func (s *resourceServiceSuite) TestStoreResourceRemovedOldResourceBlob(c *gc.C)
 	c.Assert(err, jc.ErrorIsNil)
 }
 
-func (s *resourceServiceSuite) TestStoreResourceDoesNotStoreIdenticalBlob(c *gc.C) {
+func (s *resourceServiceSuite) TestStoreResourceDoesNotStoreIdenticalBlobContainer(c *gc.C) {
 	defer s.setupMocks(c).Finish()
 	// Arrange: We only expect a call to GetResource, not to
 	// RecordStoredResource since the blob is identical.
-
 	resourceUUID := resourcetesting.GenResourceUUID(c)
 
 	reader := bytes.NewBufferString("spamspamspam")
@@ -512,13 +511,24 @@ func (s *resourceServiceSuite) TestStoreResourceDoesNotStoreIdenticalBlob(c *gc.
 	s.state.EXPECT().GetResource(gomock.Any(), resourceUUID).Return(
 		coreresource.Resource{
 			Resource: charmresource.Resource{
+				Meta: charmresource.Meta{
+					Type: charmresource.TypeContainerImage,
+				},
 				Fingerprint: fp,
 			},
 		}, nil,
 	)
 
-	// Act:
+	s.resourceStoreGetter.EXPECT().GetResourceStore(gomock.Any(), charmresource.TypeContainerImage).Return(s.resourceStore, nil)
+	s.resourceStore.EXPECT().Put(
+		gomock.Any(),
+		blobPath(resourceUUID, fp.String()),
+		reader,
+		int64(0),
+		coreresourcestore.NewFingerprint(fp.Fingerprint),
+	).Return(coreresourcestore.ID{}, containerimageresourcestoreerrors.ContainerImageMetadataAlreadyStored)
 
+	// Act:
 	err = s.service.StoreResource(
 		context.Background(),
 		resource.StoreResourceArgs{
@@ -531,7 +541,55 @@ func (s *resourceServiceSuite) TestStoreResourceDoesNotStoreIdenticalBlob(c *gc.
 	)
 
 	// Assert:
+	c.Assert(err, jc.ErrorIs, resourceerrors.StoredResourceAlreadyExists)
+}
+
+func (s *resourceServiceSuite) TestStoreResourceDoesNotStoreIdenticalBlobFile(c *gc.C) {
+	defer s.setupMocks(c).Finish()
+	// Arrange:
+	resourceUUID := resourcetesting.GenResourceUUID(c)
+
+	reader := bytes.NewBufferString("spamspamspam")
+	fp, err := charmresource.NewFingerprint(fingerprint)
 	c.Assert(err, jc.ErrorIsNil)
+
+	origin := charmresource.OriginStore
+	revision := 17
+
+	s.state.EXPECT().GetResource(gomock.Any(), resourceUUID).Return(
+		coreresource.Resource{
+			Resource: charmresource.Resource{
+				Meta: charmresource.Meta{
+					Type: charmresource.TypeFile,
+				},
+				Fingerprint: fp,
+			},
+		}, nil,
+	)
+
+	s.resourceStoreGetter.EXPECT().GetResourceStore(gomock.Any(), charmresource.TypeFile).Return(s.resourceStore, nil)
+	s.resourceStore.EXPECT().Put(
+		gomock.Any(),
+		blobPath(resourceUUID, fp.String()),
+		reader,
+		int64(0),
+		coreresourcestore.NewFingerprint(fp.Fingerprint),
+	).Return(coreresourcestore.ID{}, objectstoreerrors.ObjectAlreadyExists)
+
+	// Act:
+	err = s.service.StoreResource(
+		context.Background(),
+		resource.StoreResourceArgs{
+			ResourceUUID: resourceUUID,
+			Reader:       reader,
+			Fingerprint:  fp,
+			Origin:       origin,
+			Revision:     revision,
+		},
+	)
+
+	// Assert:
+	c.Assert(err, jc.ErrorIs, resourceerrors.StoredResourceAlreadyExists)
 }
 
 func (s *resourceServiceSuite) TestStoreResourceBadUUID(c *gc.C) {
@@ -857,7 +915,7 @@ func (s *resourceServiceSuite) TestOpenResourceFileNotFound(c *gc.C) {
 	s.resourceStore.EXPECT().Get(
 		gomock.Any(),
 		blobPath(id, fp.String()),
-	).Return(nil, 0, objectstoreerrors.ErrNotFound)
+	).Return(nil, 0, objectstoreerrors.ObjectNotFound)
 
 	_, _, err = s.service.OpenResource(context.Background(), id)
 	c.Assert(err, jc.ErrorIs, resourceerrors.StoredResourceNotFound)

internal/resource/opener.go

@@ -311,6 +311,8 @@ func (ro ResourceOpener) getResource(
 		data.ReadCloser,
 		data.Resource.Size,
 		data.Resource.Fingerprint,
+		data.Resource.Origin,
+		data.Resource.Revision,
 	)
 	if err != nil {
 		return coreresource.Opened{}, errors.Capture(err)
@@ -336,6 +338,8 @@ func (ro ResourceOpener) store(
 	reader io.Reader,
 	size int64,
 	fingerprint charmresource.Fingerprint,
+	origin charmresource.Origin,
+	revision int,
 ) (_ coreresource.Resource, _ io.ReadCloser, err error) {
 	err = ro.resourceService.StoreResource(
 		ctx, resource.StoreResourceArgs{
@@ -345,6 +349,8 @@ func (ro ResourceOpener) store(
 			Fingerprint:     fingerprint,
 			RetrievedBy:     ro.retrievedBy,
 			RetrievedByType: ro.retrievedByType,
+			Origin:          origin,
+			Revision:        revision,
 		},
 	)
 	if err != nil {

internal/resource/opener_test.go

@@ -42,6 +42,7 @@ type OpenerSuite struct {
 	resourceFingerprint  charmresource.Fingerprint
 	resourceSize         int64
 	resourceReader       io.ReadCloser
+	resourceRevision     int
 	charmURL             *charm.URL
 	charmOrigin          state.CharmOrigin
 	resourceClient       *MockResourceClient
@@ -66,8 +67,8 @@ func (s *OpenerSuite) TestOpenResource(c *gc.C) {
 				Name: "wal-e",
 				Type: 1,
 			},
-			Origin:      2,
-			Revision:    0,
+			Origin:      charmresource.OriginStore,
+			Revision:    s.resourceRevision,
 			Fingerprint: s.resourceFingerprint,
 			Size:        s.resourceSize,
 		},
@@ -106,8 +107,8 @@ func (s *OpenerSuite) TestOpenResourceThrottle(c *gc.C) {
 				Name: "wal-e",
 				Type: 1,
 			},
-			Origin:      2,
-			Revision:    0,
+			Origin:      charmresource.OriginStore,
+			Revision:    s.resourceRevision,
 			Fingerprint: s.resourceFingerprint,
 			Size:        s.resourceSize,
 		},
@@ -179,8 +180,8 @@ func (s *OpenerSuite) TestOpenResourceApplication(c *gc.C) {
 				Name: "wal-e",
 				Type: 1,
 			},
-			Origin:      2,
-			Revision:    0,
+			Origin:      charmresource.OriginStore,
+			Revision:    s.resourceRevision,
 			Fingerprint: s.resourceFingerprint,
 			Size:        s.resourceSize,
 		},
@@ -236,6 +237,7 @@ func (s *OpenerSuite) setupMocks(c *gc.C, includeUnit bool) *gomock.Controller {
 
 	s.resourceContent = "the resource content"
 	s.resourceSize = int64(len(s.resourceContent))
+	s.resourceRevision = 3
 	var err error
 	s.resourceFingerprint, err = charmresource.GenerateFingerprint(strings.NewReader(s.resourceContent))
 	c.Assert(err, jc.ErrorIsNil)
@@ -309,6 +311,8 @@ func (s *OpenerSuite) expectServiceMethods(
 			RetrievedByType: retrevedByType,
 			Size:            s.resourceSize,
 			Fingerprint:     s.resourceFingerprint,
+			Origin:          charmresource.OriginStore,
+			Revision:        s.resourceRevision,
 		},
 	)