Unify how store and hub fetch resources

addyess · May 23, 2022 · 1cf425f · 1cf425f
1 parent 41ba6e9
commit 1cf425f
Show file tree

Hide file tree

Showing 14 changed files with 90 additions and 128 deletions.
diff --git a/charmstore/client.go b/charmstore/client.go
@@ -191,49 +191,18 @@ type ResourceRequest struct {
 	Revision int
 }
 
-// ResourceData represents the response from the charmstore about a request for
-// resource bytes.
-type ResourceData struct {
-	// ReadCloser holds the bytes for the resource.
-	io.ReadCloser
-
-	// Resource holds the metadata for the resource.
-	Resource resource.Resource
-}
-
-// GetResource returns the data (bytes) and metadata for a resource from the charmstore.
-func (c Client) GetResource(req ResourceRequest) (data ResourceData, err error) {
+// DownloadResource returns the data (bytes) for a resource from the charmstore.
+func (c Client) DownloadResource(req ResourceRequest) (io.ReadCloser, string, error) {
 	if err := c.jar.Activate(req.Charm); err != nil {
-		return ResourceData{}, errors.Trace(err)
+		return nil, "", errors.Trace(err)
 	}
 	defer func() { _ = c.jar.Deactivate() }()
-	meta, err := c.csWrapper.ResourceMeta(req.Channel, req.Charm, req.Name, req.Revision)
 
-	if err != nil {
-		return ResourceData{}, errors.Trace(err)
-	}
-	data.Resource, err = csparams.API2Resource(meta)
-	if err != nil {
-		return ResourceData{}, errors.Trace(err)
-	}
 	resData, err := c.csWrapper.GetResource(req.Channel, req.Charm, req.Name, req.Revision)
 	if err != nil {
-		return ResourceData{}, errors.Trace(err)
-	}
-	defer func() {
-		if err != nil {
-			resData.Close()
-		}
-	}()
-	data.ReadCloser = resData.ReadCloser
-	if data.Resource.Type == resource.TypeFile {
-		fpHash := data.Resource.Fingerprint.String()
-		if resData.Hash != fpHash {
-			return ResourceData{},
-				errors.Errorf("fingerprint for data (%s) does not match fingerprint in metadata (%s)", resData.Hash, fpHash)
-		}
+		return nil, "", errors.Trace(err)
 	}
-	return data, nil
+	return resData.ReadCloser, resData.Hash, nil
 }
 
 // ResourceInfo returns the metadata for the given resource from the charmstore.

diff --git a/charmstore/client_test.go b/charmstore/client_test.go
@@ -187,24 +187,14 @@ func (s *ClientSuite) TestListResourcesAPI2ResourcesFailure(c *gc.C) {
 	c.Assert(ret, gc.IsNil)
 }
 
-func (s *ClientSuite) TestGetResource(c *gc.C) {
+func (s *ClientSuite) TestDownloadResource(c *gc.C) {
 	fp, err := resource.GenerateFingerprint(strings.NewReader("data"))
 	c.Assert(err, jc.ErrorIsNil)
 	rc := ioutil.NopCloser(strings.NewReader("data"))
 	s.wrapper.ReturnGetResource = csclient.ResourceData{
 		ReadCloser: rc,
 		Hash:       fp.String(),
 	}
-	apiRes := params.Resource{
-		Name:        "name",
-		Type:        "file",
-		Path:        "foo.zip",
-		Description: "something",
-		Revision:    5,
-		Fingerprint: fp.Bytes(),
-		Size:        4,
-	}
-	s.wrapper.ReturnResourceMeta = apiRes
 
 	client, err := newCachingClient(s.cache, "", s.wrapper.makeWrapper)
 	c.Assert(err, jc.ErrorIsNil)
@@ -215,15 +205,13 @@ func (s *ClientSuite) TestGetResource(c *gc.C) {
 		Name:     "name",
 		Revision: 5,
 	}
-	data, err := client.GetResource(req)
+	rdr, hash, err := client.DownloadResource(req)
 	c.Assert(err, jc.ErrorIsNil)
-	expected, err := params.API2Resource(apiRes)
 	c.Assert(err, jc.ErrorIsNil)
-	c.Check(data.Resource, gc.DeepEquals, expected)
-	c.Check(data.ReadCloser, gc.DeepEquals, rc)
+	c.Check(hash, gc.Equals, fp.String())
+	c.Check(rdr, gc.DeepEquals, rc)
 	// call #0 is a call to makeWrapper
-	s.wrapper.stub.CheckCall(c, 1, "ResourceMeta", params.EdgeChannel, req.Charm, req.Name, req.Revision)
-	s.wrapper.stub.CheckCall(c, 2, "GetResource", params.EdgeChannel, req.Charm, req.Name, req.Revision)
+	s.wrapper.stub.CheckCall(c, 1, "GetResource", params.EdgeChannel, req.Charm, req.Name, req.Revision)
 }
 
 func (s *ClientSuite) TestGetResourceDockerType(c *gc.C) {
@@ -234,16 +222,6 @@ func (s *ClientSuite) TestGetResourceDockerType(c *gc.C) {
 		ReadCloser: rc,
 		Hash:       fp.String(),
 	}
-	apiRes := params.Resource{
-		Name:        "mysql_image",
-		Type:        "oci-image",
-		Description: "something",
-		Revision:    2,
-		Fingerprint: resource.Fingerprint{}.Bytes(),
-		Size:        4,
-	}
-	s.wrapper.ReturnResourceMeta = apiRes
-
 	client, err := newCachingClient(s.cache, "", s.wrapper.makeWrapper)
 	c.Assert(err, jc.ErrorIsNil)
 
@@ -253,15 +231,13 @@ func (s *ClientSuite) TestGetResourceDockerType(c *gc.C) {
 		Name:     "mysql_image",
 		Revision: 5,
 	}
-	data, err := client.GetResource(req)
+	rdr, hash, err := client.DownloadResource(req)
 	c.Assert(err, jc.ErrorIsNil)
-	expected, err := params.API2Resource(apiRes)
 	c.Assert(err, jc.ErrorIsNil)
-	c.Check(data.Resource, gc.DeepEquals, expected)
-	c.Check(data.ReadCloser, gc.DeepEquals, rc)
+	c.Check(hash, gc.Equals, fp.String())
+	c.Check(rdr, gc.DeepEquals, rc)
 	// call #0 is a call to makeWrapper
-	s.wrapper.stub.CheckCall(c, 1, "ResourceMeta", params.EdgeChannel, req.Charm, req.Name, req.Revision)
-	s.wrapper.stub.CheckCall(c, 2, "GetResource", params.EdgeChannel, req.Charm, req.Name, req.Revision)
+	s.wrapper.stub.CheckCall(c, 1, "GetResource", params.EdgeChannel, req.Charm, req.Name, req.Revision)
 }
 
 func (s *ClientSuite) TestResourceInfo(c *gc.C) {

diff --git a/resource/charmhub.go b/resource/charmhub.go
@@ -15,7 +15,6 @@ import (
 
 	"github.com/juju/juju/charmhub"
 	"github.com/juju/juju/charmhub/transport"
-	"github.com/juju/juju/charmstore"
 	corelogger "github.com/juju/juju/core/logger"
 )
 
@@ -41,7 +40,7 @@ type chClientState interface {
 	Model() (Model, error)
 }
 
-func newCharmHubClient(st chClientState) (ResourceClient, error) {
+func newCharmHubClient(st chClientState) (ResourceGetter, error) {
 	m, err := st.Model()
 	if err != nil {
 		return &CharmHubClient{}, errors.Trace(err)
@@ -75,9 +74,9 @@ type CharmHubClient struct {
 
 // GetResource returns data about the resource including an io.ReadCloser
 // to download the resource.  The caller is responsible for closing it.
-func (ch *CharmHubClient) GetResource(req ResourceRequest) (charmstore.ResourceData, error) {
+func (ch *CharmHubClient) GetResource(req ResourceRequest) (ResourceData, error) {
 	ch.logger.Tracef("GetResource(%s)", pretty.Sprint(req))
-	var data charmstore.ResourceData
+	var data ResourceData
 
 	origin := req.CharmID.Origin
 

diff --git a/resource/charmstore.go b/resource/charmstore.go
@@ -5,6 +5,7 @@ package resource
 
 import (
 	"github.com/juju/charm/v8"
+	charmresource "github.com/juju/charm/v8/resource"
 	csparams "github.com/juju/charmrepo/v6/csclient/params"
 	"github.com/juju/errors"
 
@@ -40,7 +41,7 @@ type csClient struct {
 	client charmstore.Client
 }
 
-func (cs *csClient) GetResource(req ResourceRequest) (charmstore.ResourceData, error) {
+func (cs *csClient) GetResource(req ResourceRequest) (ResourceData, error) {
 	csReq := charmstore.ResourceRequest{
 		Charm:    req.CharmID.URL,
 		Name:     req.Name,
@@ -51,16 +52,35 @@ func (cs *csClient) GetResource(req ResourceRequest) (charmstore.ResourceData, e
 	// an empty string is valid for the request channel.  It
 	// will be handled by the charmstore client.
 	stChannel := req.CharmID.Origin.Channel
-	if stChannel == nil {
-		return cs.client.GetResource(csReq)
+	if stChannel != nil {
+		channel, err := charm.MakeChannel(stChannel.Track, stChannel.Risk, stChannel.Branch)
+		if err != nil {
+			return ResourceData{}, errors.Trace(err)
+		}
+		csReq.Channel = csparams.Channel(channel.String())
 	}
 
-	channel, err := charm.MakeChannel(stChannel.Track, stChannel.Risk, stChannel.Branch)
+	var data ResourceData
+	meta, err := cs.client.ResourceInfo(csReq)
 	if err != nil {
-		return charmstore.ResourceData{}, errors.Trace(err)
+		return ResourceData{}, errors.Trace(err)
 	}
-	csReq.Channel = csparams.Channel(channel.String())
-	return cs.client.GetResource(csReq)
+	data.Resource = meta
+
+	rdr, hash, err := cs.client.DownloadResource(csReq)
+	if err != nil {
+		return ResourceData{}, errors.Trace(err)
+	}
+
+	if data.Resource.Type == charmresource.TypeFile {
+		fpHash := meta.Fingerprint.String()
+		if hash != fpHash {
+			return ResourceData{},
+				errors.Errorf("fingerprint for data (%s) does not match fingerprint in metadata (%s)", hash, fpHash)
+		}
+	}
+	data.ReadCloser = rdr
+	return data, nil
 }
 
 // NewClient opens a new charm store client.

diff --git a/resource/charmstore_test.go b/resource/charmstore_test.go
@@ -12,7 +12,6 @@ import (
 	jc "github.com/juju/testing/checkers"
 	gc "gopkg.in/check.v1"
 
-	"github.com/juju/juju/charmstore"
 	"github.com/juju/juju/resource"
 	"github.com/juju/juju/state"
 )
@@ -43,9 +42,9 @@ func (s *CharmStoreSuite) TestGetResourceTerminatesNoChannel(c *gc.C) {
 func (s *CharmStoreSuite) testGetResourceTerminates(c *gc.C, channel bool) {
 	msg := "trust"
 	attempts := int32(0)
-	s.resourceClient.getResourceF = func(req resource.ResourceRequest) (data charmstore.ResourceData, err error) {
+	s.resourceClient.getResourceF = func(req resource.ResourceRequest) (data resource.ResourceData, err error) {
 		atomic.AddInt32(&attempts, 1)
-		return charmstore.ResourceData{}, errors.New(msg)
+		return resource.ResourceData{}, errors.New(msg)
 	}
 	csRes := resource.NewCSRetryClientForTest(s.resourceClient)
 
@@ -85,8 +84,8 @@ func (s *CharmStoreSuite) TestGetResourceAbortedOnNotValid(c *gc.C) {
 }
 
 func (s *CharmStoreSuite) assertAbortedGetResourceOnError(c *gc.C, csRes *resource.ResourceRetryClient, expectedError error, expectedMessage string) {
-	s.resourceClient.getResourceF = func(req resource.ResourceRequest) (data charmstore.ResourceData, err error) {
-		return charmstore.ResourceData{}, expectedError
+	s.resourceClient.getResourceF = func(req resource.ResourceRequest) (data resource.ResourceData, err error) {
+		return resource.ResourceData{}, expectedError
 	}
 	_, err := csRes.GetResource(resource.ResourceRequest{
 		CharmID: resource.CharmID{
@@ -105,10 +104,10 @@ func (s *CharmStoreSuite) assertAbortedGetResourceOnError(c *gc.C, csRes *resour
 type testResourceClient struct {
 	stub *testing.Stub
 
-	getResourceF func(req resource.ResourceRequest) (data charmstore.ResourceData, err error)
+	getResourceF func(req resource.ResourceRequest) (data resource.ResourceData, err error)
 }
 
-func (f *testResourceClient) GetResource(req resource.ResourceRequest) (data charmstore.ResourceData, err error) {
+func (f *testResourceClient) GetResource(req resource.ResourceRequest) (data resource.ResourceData, err error) {
 	f.stub.AddCall("GetResource", req)
 	return f.getResourceF(req)
 }
diff --git a/resource/export_test.go b/resource/export_test.go
@@ -9,7 +9,7 @@ import (
 	"github.com/juju/names/v4"
 )
 
-func NewCSRetryClientForTest(client ResourceClient) *ResourceRetryClient {
+func NewCSRetryClientForTest(client ResourceGetter) *ResourceRetryClient {
 	retryClient := newRetryClient(client)
 	// Reduce retry delay for test.
 	retryClient.retryArgs.Delay = 1 * time.Millisecond

diff --git a/resource/interfaces.go b/resource/interfaces.go
@@ -14,7 +14,6 @@ import (
 
 	"github.com/juju/juju/charmhub"
 	"github.com/juju/juju/charmhub/transport"
-	"github.com/juju/juju/charmstore"
 	"github.com/juju/juju/controller"
 	"github.com/juju/juju/core/resource"
 	"github.com/juju/juju/environs/config"
@@ -81,10 +80,17 @@ type ResourceRetryClientGetter interface {
 	NewClient() (*ResourceRetryClient, error)
 }
 
-// ResourceClient defines a set of functionality that a client
-// needs to define to support resources.
-type ResourceClient interface {
-	GetResource(req ResourceRequest) (data charmstore.ResourceData, err error)
+// ResourceGetter provides the functionality for getting a resource file.
+type ResourceGetter interface {
+	// GetResource returns a reader for the resource's data. That data
+	// is streamed from the charm store. The charm's revision, if any,
+	// is ignored. If the identified resource is not in the charm store
+	// then errors.NotFound is returned.
+	//
+	// But if you write any code that assumes a NotFound error returned
+	// from this method means that the resource was not found, you fail
+	// basic logic.
+	GetResource(ResourceRequest) (ResourceData, error)
 }
 
 // CharmHub represents methods required from a charmhub client talking to the

diff --git a/resource/mocks/repositories_mock.go b/resource/mocks/repositories_mock.go
diff --git a/resource/opener.go b/resource/opener.go
@@ -14,7 +14,6 @@ import (
 	"github.com/juju/errors"
 	"github.com/juju/names/v4"
 
-	"github.com/juju/juju/charmstore"
 	"github.com/juju/juju/core/resource"
 	corestate "github.com/juju/juju/state"
 )
@@ -250,11 +249,11 @@ func (o *nopOpener) NewClient() (*ResourceRetryClient, error) {
 // scenarios covering local charms.
 type nopClient struct{}
 
-// GetResource is a no-op client implementation of a ResourceClient. The
+// GetResource is a no-op client implementation of a ResourceGetter. The
 // implementation expects to never call the underlying client and instead
 // returns a not-found error straight away.
-func (nopClient) GetResource(req ResourceRequest) (charmstore.ResourceData, error) {
-	return charmstore.ResourceData{}, errors.NotFoundf("resource %q", req.Name)
+func (nopClient) GetResource(req ResourceRequest) (ResourceData, error) {
+	return ResourceData{}, errors.NotFoundf("resource %q", req.Name)
 }
 
 type stateShim struct {

diff --git a/resource/opener_test.go b/resource/opener_test.go
@@ -18,7 +18,6 @@ import (
 	jc "github.com/juju/testing/checkers"
 	gc "gopkg.in/check.v1"
 
-	"github.com/juju/juju/charmstore"
 	coreresource "github.com/juju/juju/core/resource"
 	"github.com/juju/juju/resource"
 	"github.com/juju/juju/resource/mocks"
@@ -56,7 +55,7 @@ func (s *OpenerSuite) TestOpenResource(c *gc.C) {
 	}
 	s.expectCharmOrigin(1)
 	s.expectCacheMethods(res, 1)
-	s.resourceGetter.EXPECT().GetResource(gomock.Any()).Return(charmstore.ResourceData{
+	s.resourceGetter.EXPECT().GetResource(gomock.Any()).Return(resource.ResourceData{
 		ReadCloser: nil,
 		Resource:   res.Resource,
 	}, nil)
@@ -89,7 +88,7 @@ func (s *OpenerSuite) TestOpenResourceThrottle(c *gc.C) {
 	)
 	s.expectCharmOrigin(numConcurrentRequests)
 	s.expectCacheMethods(res, numConcurrentRequests)
-	s.resourceGetter.EXPECT().GetResource(gomock.Any()).Return(charmstore.ResourceData{
+	s.resourceGetter.EXPECT().GetResource(gomock.Any()).Return(resource.ResourceData{
 		ReadCloser: nil,
 		Resource:   res.Resource,
 	}, nil)
@@ -143,7 +142,7 @@ func (s *OpenerSuite) TestOpenResourceApplication(c *gc.C) {
 	}
 	s.expectCharmOrigin(1)
 	s.expectCacheMethods(res, 1)
-	s.resourceGetter.EXPECT().GetResource(gomock.Any()).Return(charmstore.ResourceData{
+	s.resourceGetter.EXPECT().GetResource(gomock.Any()).Return(resource.ResourceData{
 		ReadCloser: nil,
 		Resource:   res.Resource,
 	}, nil)