Encapsulate s3 implementation details in specific clients

Now the api itself no longer needs to know about implementation details of how we store charms in s3.
addyess · Jan 8, 2024 · b73bc85 · b73bc85
1 parent 87d5b00
commit b73bc85
Show file tree

Hide file tree

Showing 7 changed files with 90 additions and 13 deletions.
diff --git a/api/client/charms/downloader_s3.go b/api/client/charms/downloader_s3.go
@@ -5,6 +5,7 @@ package charms
 
 import (
 	"context"
+	"fmt"
 	"io"
 
 	"github.com/juju/charm/v12"
@@ -18,7 +19,7 @@ import (
 type CharmGetter interface {
 	// GetCharm returns an io.ReadCloser for the specified object within the
 	// specified bucket.
-	GetCharm(ctx context.Context, bucketName, objectName string) (io.ReadCloser, error)
+	GetCharm(ctx context.Context, modelUUID, charmName string) (io.ReadCloser, error)
 }
 
 // NewS3CharmDownloader returns a new charm downloader that wraps a s3Caller
@@ -63,10 +64,9 @@ func (s *s3charmOpener) OpenCharm(req downloader.Request) (io.ReadCloser, error)
 	// We can ignore the second return bool from ModelTag() because if it's
 	// a controller model, then it will fail later when calling GetCharm.
 	modelTag, _ := s.apiCaller.ModelTag()
-	bucket := "model-" + modelTag.Id()
 
-	object := "charms/" + curl.Name + "-" + shortSha256
-	return s.charmGetter.GetCharm(s.ctx, bucket, object)
+	charmRef := fmt.Sprintf("%s-%s", curl.Name, shortSha256)
+	return s.charmGetter.GetCharm(s.ctx, modelTag.Id(), charmRef)
 }
 
 // NewS3CharmOpener returns a charm opener for the specified s3Caller.

diff --git a/api/client/charms/downloader_s3_test.go b/api/client/charms/downloader_s3_test.go
@@ -59,7 +59,7 @@ func (s *charmS3DownloaderSuite) TestCharmOpener(c *gc.C) {
 
 				modelTag := names.NewModelTag("testmodel")
 				mockCaller.EXPECT().ModelTag().Return(modelTag, true)
-				mockGetter.EXPECT().GetCharm(gomock.Any(), "model-testmodel", "charms/mycharm-abcd0123").MinTimes(1).Return(nil, nil)
+				mockGetter.EXPECT().GetCharm(gomock.Any(), "testmodel", "mycharm-abcd0123").MinTimes(1).Return(nil, nil)
 			},
 		},
 	}

diff --git a/apiserver/httpcontext/model_test.go b/apiserver/httpcontext/model_test.go
@@ -42,12 +42,12 @@ func (s *ModelHandlersSuite) SetUpTest(c *gc.C) {
 	}
 	s.bucketHandler = &httpcontext.BucketModelHandler{
 		Handler: h,
-		Query:   ":bucket",
+		Query:   ":modeluuid",
 	}
 	mux := apiserverhttp.NewMux()
 	mux.AddHandler("GET", "/query", s.queryHandler)
 	mux.AddHandler("GET", "/implied", s.impliedHandler)
-	mux.AddHandler("GET", "/:bucket/charms/:object", s.bucketHandler)
+	mux.AddHandler("GET", "/model-:modeluuid/charms/:object", s.bucketHandler)
 	s.server = httptest.NewServer(mux)
 }
 
@@ -103,12 +103,12 @@ func (s *ModelHandlersSuite) TestBucket(c *gc.C) {
 func (s *ModelHandlersSuite) TestInvalidBucket(c *gc.C) {
 	resp, err := s.server.Client().Get(s.server.URL + "/modelwrongbucket/charms/somecharm-abcd0123")
 	c.Assert(err, jc.ErrorIsNil)
-	c.Assert(resp.StatusCode, gc.Equals, http.StatusBadRequest)
+	c.Assert(resp.StatusCode, gc.Equals, http.StatusNotFound)
 	defer resp.Body.Close()
 
 	out, err := io.ReadAll(resp.Body)
 	c.Assert(err, jc.ErrorIsNil)
-	c.Assert(string(out), gc.Equals, `invalid bucket format "modelwrongbucket"`+"\n")
+	c.Assert(string(out), gc.Equals, "404 page not found\n")
 }
 
 func (s *ModelHandlersSuite) TestBucketInvalidModelUUID(c *gc.C) {

diff --git a/internal/s3client/charmclient.go b/internal/s3client/charmclient.go
@@ -5,6 +5,7 @@ package s3client
 
 import (
 	"context"
+	"fmt"
 	"io"
 )
 
@@ -16,7 +17,9 @@ type Charms struct {
 
 // GetCharm retrieves a charm from the S3-compatible object store hosted
 // by the apiserver. Returns an archived charm as a stream of bytes
-func (c *Charms) GetCharm(ctx context.Context, bucketName, objectName string) (io.ReadCloser, error) {
+func (c *Charms) GetCharm(ctx context.Context, modelUUID, charmRef string) (io.ReadCloser, error) {
+	bucketName := fmt.Sprintf("model-%s", modelUUID)
+	objectName := fmt.Sprintf("charms/%s", charmRef)
 	return c.session.GetObject(ctx, bucketName, objectName)
 }
 

diff --git a/internal/s3client/charmclient_test.go b/internal/s3client/charmclient_test.go
@@ -0,0 +1,35 @@
+// Copyright 2024 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package s3client
+
+import (
+	"context"
+
+	"go.uber.org/mock/gomock"
+	gc "gopkg.in/check.v1"
+
+	coretesting "github.com/juju/juju/testing"
+)
+
+type charmsS3ClientSuite struct {
+	session *MockSession
+}
+
+var _ = gc.Suite(&charmsS3ClientSuite{})
+
+func (s *charmsS3ClientSuite) setupMocks(c *gc.C) *gomock.Controller {
+	ctrl := gomock.NewController(c)
+	s.session = NewMockSession(ctrl)
+
+	return ctrl
+}
+
+func (s *charmsS3ClientSuite) TestGetCharm(c *gc.C) {
+	defer s.setupMocks(c).Finish()
+
+	s.session.EXPECT().GetObject(gomock.Any(), "model-"+coretesting.ModelTag.Id(), "charms/somecharm-abcd0123")
+
+	cli := NewCharmsS3Client(s.session)
+	cli.GetCharm(context.Background(), coretesting.ModelTag.Id(), "somecharm-abcd0123")
+}
diff --git a/internal/s3client/client_mock_test.go b/internal/s3client/client_mock_test.go
diff --git a/internal/s3client/package_test.go b/internal/s3client/package_test.go
@@ -9,7 +9,7 @@ import (
 	gc "gopkg.in/check.v1"
 )
 
-//go:generate go run go.uber.org/mock/mockgen -package s3client -destination client_mock_test.go github.com/juju/juju/internal/s3client S3Client
+//go:generate go run go.uber.org/mock/mockgen -package s3client -destination client_mock_test.go github.com/juju/juju/internal/s3client S3Client,Session
 
 func TestSuite(t *testing.T) {
 	gc.TestingT(t)