refactor(charm): drop dangerous ReadCharm and ReadBundle

These generic functions look at a path, determin if the resource is an archive or dir, and then run the associated parser. However, ever since we dropped support for deploying charms from directories, we have always known ahead of time the format of what we are about to read. In fact, we never need to parse a full charm as a directory. At most, we need to read one particular charm file. The only exception are in tests. So move ReadCharmDir into a testing package. The rest of the PR is made up of refactors/simplifications which were made possible by this change.
sinanawad · Dec 19, 2024 · ed0ff3e · ed0ff3e
1 parent bba5ec3
commit ed0ff3e
Show file tree

Hide file tree

Showing 62 changed files with 1,016 additions and 899 deletions.
diff --git a/api/client/charms/client_test.go b/api/client/charms/client_test.go
@@ -7,6 +7,7 @@ import (
 	"archive/zip"
 	context "context"
 	"os"
+	"path/filepath"
 
 	"github.com/juju/errors"
 	jc "github.com/juju/testing/checkers"
@@ -300,42 +301,33 @@ func (s *charmsMockSuite) TestListCharmResources(c *gc.C) {
 
 func (s *charmsMockSuite) TestZipHasHooksOnly(c *gc.C) {
 	ch := testcharms.Repo.CharmDir("storage-filesystem-subordinate") // has hooks only
-	tempFile, err := os.CreateTemp(c.MkDir(), "charm")
-	c.Assert(err, jc.ErrorIsNil)
-	defer tempFile.Close()
-	defer os.Remove(tempFile.Name())
-	err = ch.ArchiveTo(tempFile)
+	charmPath := filepath.Join(c.MkDir(), "charm")
+	err := ch.ArchiveToPath(charmPath)
 	c.Assert(err, jc.ErrorIsNil)
 	f := *charms.HasHooksOrDispatch
-	hasHooks, err := f(tempFile.Name())
+	hasHooks, err := f(charmPath)
 	c.Assert(err, jc.ErrorIsNil)
 	c.Assert(hasHooks, jc.IsTrue)
 }
 
 func (s *charmsMockSuite) TestZipHasDispatchFileOnly(c *gc.C) {
 	ch := testcharms.Repo.CharmDir("category-dispatch") // has dispatch file only
-	tempFile, err := os.CreateTemp(c.MkDir(), "charm")
-	c.Assert(err, jc.ErrorIsNil)
-	defer tempFile.Close()
-	defer os.Remove(tempFile.Name())
-	err = ch.ArchiveTo(tempFile)
+	charmPath := filepath.Join(c.MkDir(), "charm")
+	err := ch.ArchiveToPath(charmPath)
 	c.Assert(err, jc.ErrorIsNil)
 	f := *charms.HasHooksOrDispatch
-	hasDispatch, err := f(tempFile.Name())
+	hasDispatch, err := f(charmPath)
 	c.Assert(err, jc.ErrorIsNil)
 	c.Assert(hasDispatch, jc.IsTrue)
 }
 
 func (s *charmsMockSuite) TestZipHasNoHooksNorDispatch(c *gc.C) {
 	ch := testcharms.Repo.CharmDir("category") // has no hooks nor dispatch file
-	tempFile, err := os.CreateTemp(c.MkDir(), "charm")
-	c.Assert(err, jc.ErrorIsNil)
-	defer tempFile.Close()
-	defer os.Remove(tempFile.Name())
-	err = ch.ArchiveTo(tempFile)
+	charmPath := filepath.Join(c.MkDir(), "charm")
+	err := ch.ArchiveToPath(charmPath)
 	c.Assert(err, jc.ErrorIsNil)
 	f := *charms.HasHooksOrDispatch
-	hasHooks, err := f(tempFile.Name())
+	hasHooks, err := f(charmPath)
 	c.Assert(err, jc.ErrorIsNil)
 	c.Assert(hasHooks, jc.IsFalse)
 }

diff --git a/cmd/juju/application/bundle_test.go b/cmd/juju/application/bundle_test.go
@@ -24,6 +24,7 @@ import (
 	corecharm "github.com/juju/juju/core/charm"
 	"github.com/juju/juju/core/crossmodel"
 	"github.com/juju/juju/internal/charm"
+	charmtesting "github.com/juju/juju/internal/charm/testing"
 	"github.com/juju/juju/internal/cmd/cmdtesting"
 	coretesting "github.com/juju/juju/internal/testing"
 	jujutesting "github.com/juju/juju/juju/testing"
@@ -129,7 +130,7 @@ func (s *BundleDeploySuite) setupCharmMaybeForce(c *gc.C, url, name string, abas
 	}
 
 	var chDir charm.Charm
-	chDir, err := charm.ReadCharmDir(testcharms.RepoWithSeries("bionic").CharmDirPath(name))
+	chDir, err := charmtesting.ReadCharmDir(testcharms.RepoWithSeries("bionic").CharmDirPath(name))
 	if err != nil {
 		if !os.IsNotExist(errors.Cause(err)) {
 			c.Fatal(err)
@@ -140,12 +141,11 @@ func (s *BundleDeploySuite) setupCharmMaybeForce(c *gc.C, url, name string, abas
 	return chDir
 }
 
-func (s *BundleDeploySuite) setupBundle(c *gc.C, url, name string, allBase ...base.Base) {
+func (s *BundleDeploySuite) setupFakeBundle(c *gc.C, url string, allBase ...base.Base) {
 	bundleResolveURL := charm.MustParseURL(url)
 	baseURL := *bundleResolveURL
 	baseURL.Revision = -1
 	withCharmRepoResolvable(s.fakeAPI, &baseURL, base.Base{})
-	bundleDir := testcharms.RepoWithSeries("bionic").BundleArchive(c.MkDir(), name)
 
 	// Resolve a bundle with no revision and return a url with a version.  Ensure
 	// GetBundle expects the url with revision.
@@ -159,7 +159,7 @@ func (s *BundleDeploySuite) setupBundle(c *gc.C, url, name string, allBase ...ba
 			origin,
 			error(nil),
 		)
-		s.fakeAPI.Call("GetBundle", bundleResolveURL).Returns(bundleDir, error(nil))
+		s.fakeAPI.Call("GetBundle", bundleResolveURL).Returns(nil, error(nil))
 	}
 }
 
@@ -172,7 +172,7 @@ func (s *BundleDeploySuite) runDeploy(c *gc.C, args ...string) error {
 func (s *BundleDeploySuite) TestDeployBundleInvalidFlags(c *gc.C) {
 	s.setupCharm(c, "ch:mysql-42", "mysql", base.MustParseBaseFromString("[email protected]"))
 	s.setupCharm(c, "ch:wordpress-47", "wordpress", base.MustParseBaseFromString("[email protected]"))
-	s.setupBundle(c, "ch:wordpress-simple-1", "wordpress-simple", base.Base{}, base.MustParseBaseFromString("[email protected]"), base.MustParseBaseFromString("[email protected]"))
+	s.setupFakeBundle(c, "ch:wordpress-simple-1", base.Base{}, base.MustParseBaseFromString("[email protected]"), base.MustParseBaseFromString("[email protected]"))
 
 	err := s.runDeploy(c, "ch:wordpress-simple", "--config", "config.yaml")
 	c.Assert(err, gc.ErrorMatches, "options provided but not supported when deploying a bundle: --config")

diff --git a/cmd/juju/application/deployer/bundlehandler_test.go b/cmd/juju/application/deployer/bundlehandler_test.go
@@ -7,6 +7,7 @@ import (
 	"bytes"
 	"context"
 	"fmt"
+	"path/filepath"
 	"strings"
 
 	"github.com/juju/collections/set"
@@ -1847,14 +1848,21 @@ applications:
 relations:
     - ["wordpress:db", "mysql:server"]
 `
-	charmsPath := c.MkDir()
-	mysql := testcharms.RepoWithSeries("focal").ClonedDir(charmsPath, "mysql")
-	s.expectLocalCharm(mysql, mysqlCurl, nil)
+	tmpDir := c.MkDir()
 
-	wordpress := testcharms.RepoWithSeries("focal").ClonedDir(charmsPath, "wordpress")
-	s.expectLocalCharm(wordpress, wordpressCurl, nil)
+	mysql := testcharms.RepoWithSeries("focal").CharmDir("mysql")
+	mysqlPath := filepath.Join(tmpDir, "mysql.charm")
+	err := mysql.ArchiveToPath(mysqlPath)
+	c.Assert(err, jc.ErrorIsNil)
+	s.expectLocalCharm(mysqlPath, mysql, mysqlCurl, nil)
+
+	wordpress := testcharms.RepoWithSeries("focal").CharmDir("wordpress")
+	wordpressPath := filepath.Join(tmpDir, "wordpress.charm")
+	err = wordpress.ArchiveToPath(wordpressPath)
+	c.Assert(err, jc.ErrorIsNil)
+	s.expectLocalCharm(wordpressPath, wordpress, wordpressCurl, nil)
 
-	bundle := fmt.Sprintf(content, wordpress.Path, mysql.Path)
+	bundle := fmt.Sprintf(content, wordpressPath, mysqlPath)
 	bundleData, err := charm.ReadBundleData(strings.NewReader(bundle))
 	c.Assert(err, jc.ErrorIsNil)
 
@@ -1874,7 +1882,7 @@ relations:
 		"- add unit wordpress/0 to new machine 2\n" +
 		"Deploy of bundle completed.\n"
 
-	c.Check(s.output.String(), gc.Equals, fmt.Sprintf(expectedOutput, mysql.Path, wordpress.Path))
+	c.Check(s.output.String(), gc.Equals, fmt.Sprintf(expectedOutput, mysqlPath, wordpressPath))
 }
 
 func (s *BundleDeployRepositorySuite) TestDeployBundleLocalPathInvalidSeriesWithForce(c *gc.C) {
@@ -1918,14 +1926,22 @@ applications:
 relations:
     - ["wordpress:db", "mysql:server"]
 `
-	charmsPath := c.MkDir()
-	mysql := testcharms.RepoWithSeries("focal").ClonedDir(charmsPath, "mysql")
-	s.expectLocalCharm(mysql, mysqlCurl, nil)
 
-	wordpress := testcharms.RepoWithSeries("focal").ClonedDir(charmsPath, "wordpress")
-	s.expectLocalCharm(wordpress, wordpressCurl, nil)
+	tmpdir := c.MkDir()
+
+	mysql := testcharms.RepoWithSeries("focal").CharmDir("mysql")
+	mysqlPath := filepath.Join(tmpdir, "mysql.charm")
+	err := mysql.ArchiveToPath(mysqlPath)
+	c.Assert(err, jc.ErrorIsNil)
+	s.expectLocalCharm(mysqlPath, mysql, mysqlCurl, nil)
+
+	wordpress := testcharms.RepoWithSeries("focal").CharmDir("wordpress")
+	wordpressPath := filepath.Join(tmpdir, "wordpress.charm")
+	err = wordpress.ArchiveToPath(wordpressPath)
+	c.Assert(err, jc.ErrorIsNil)
+	s.expectLocalCharm(wordpressPath, wordpress, wordpressCurl, nil)
 
-	bundle := fmt.Sprintf(content, wordpress.Path, mysql.Path)
+	bundle := fmt.Sprintf(content, wordpressPath, mysqlPath)
 	bundleData, err := charm.ReadBundleData(strings.NewReader(bundle))
 	c.Assert(err, jc.ErrorIsNil)
 
@@ -1945,7 +1961,7 @@ relations:
 		"- add unit wordpress/0 to new machine 2\n" +
 		"Deploy of bundle completed.\n"
 
-	c.Check(s.output.String(), gc.Equals, fmt.Sprintf(expectedOutput, mysql.Path, wordpress.Path))
+	c.Check(s.output.String(), gc.Equals, fmt.Sprintf(expectedOutput, mysqlPath, wordpressPath))
 
 }
 
@@ -2434,8 +2450,8 @@ func (s *BundleDeployRepositorySuite) expectAddRelation(endpoints []string) {
 	s.deployerAPI.EXPECT().AddRelation(gomock.Any(), endpoints, nil).Return(nil, nil)
 }
 
-func (s *BundleDeployRepositorySuite) expectLocalCharm(ch *charm.CharmDir, curl *charm.URL, err error) {
-	s.charmReader.EXPECT().NewCharmAtPath(ch.Path).Return(ch, curl, err)
+func (s *BundleDeployRepositorySuite) expectLocalCharm(path string, ch charm.Charm, curl *charm.URL, err error) {
+	s.charmReader.EXPECT().NewCharmAtPath(path).Return(ch, curl, err)
 }
 
 func (s *BundleDeployRepositorySuite) expectAddOneUnit(name, directive, unit string) {

diff --git a/cmd/juju/application/deployer/deployer_test.go b/cmd/juju/application/deployer/deployer_test.go
@@ -89,11 +89,8 @@ func (s *deployerSuite) TestGetDeployerLocalCharm(c *gc.C) {
 	chDir := testcharms.RepoWithSeries("bionic").ClonedDir(dir, "multi-series")
 
 	archivePath := filepath.Join(dir, "archive.charm")
-	file, err := os.Create(archivePath)
-	c.Assert(err, jc.ErrorIsNil)
-	defer file.Close()
 
-	err = chDir.ArchiveTo(file)
+	err := chDir.ArchiveToPath(archivePath)
 	c.Assert(err, jc.ErrorIsNil)
 
 	s.expectStat(archivePath, nil)
@@ -116,11 +113,8 @@ func (s *deployerSuite) TestGetDeployerLocalCharmPathWithSchema(c *gc.C) {
 	chDir := testcharms.RepoWithSeries("bionic").ClonedDir(dir, "multi-series")
 
 	archivePath := filepath.Join(dir, "archive.charm")
-	file, err := os.Create(archivePath)
-	c.Assert(err, jc.ErrorIsNil)
-	defer file.Close()
 
-	err = chDir.ArchiveTo(file)
+	err := chDir.ArchiveToPath(archivePath)
 	c.Assert(err, jc.ErrorIsNil)
 
 	archivePath = "local:" + archivePath

diff --git a/cmd/juju/application/refresh_test.go b/cmd/juju/application/refresh_test.go
@@ -38,6 +38,7 @@ import (
 	coreresouces "github.com/juju/juju/core/resource"
 	"github.com/juju/juju/internal/charm"
 	charmresource "github.com/juju/juju/internal/charm/resource"
+	charmtesting "github.com/juju/juju/internal/charm/testing"
 	"github.com/juju/juju/internal/charmhub"
 	"github.com/juju/juju/internal/cmd"
 	"github.com/juju/juju/internal/cmd/cmdtesting"
@@ -1011,14 +1012,11 @@ func (s *RefreshSuite) TestUpgradeSameVersionWithResourceUpload(c *gc.C) {
 }
 
 func (s *RefreshSuite) archivePath(c *gc.C, path string) string {
-	charm, err := charm.ReadCharmDir(path)
+	charm, err := charmtesting.ReadCharmDir(path)
 	c.Assert(err, jc.ErrorIsNil)
 
 	archivePath := filepath.Join(c.MkDir(), "myriak.charm")
-	archiveFile, err := os.Create(archivePath)
-	c.Assert(err, jc.ErrorIsNil)
-	defer func() { _ = archiveFile.Close() }()
-	err = charm.ArchiveTo(archiveFile)
+	err = charm.ArchiveToPath(archivePath)
 	c.Assert(err, jc.ErrorIsNil)
 
 	return archivePath

diff --git a/cmd/juju/commands/bootstrap.go b/cmd/juju/commands/bootstrap.go
@@ -427,7 +427,7 @@ func (c *bootstrapCommand) Init(args []string) (err error) {
 			if err != nil {
 				return errors.Annotatef(err, "problem with --controller-charm-path")
 			}
-			ch, err := charm.ReadCharm(c.ControllerCharmPath)
+			ch, err := charm.ReadCharmArchive(c.ControllerCharmPath)
 			if err != nil {
 				return errors.Annotatef(err, "--controller-charm-path %q is not a valid charm", c.ControllerCharmPath)
 			}

diff --git a/cmd/juju/commands/bootstrap_test.go b/cmd/juju/commands/bootstrap_test.go
@@ -2019,43 +2019,69 @@ func (s *BootstrapSuite) TestBootstrapTestingOptions(c *gc.C) {
 }
 
 func (s *BootstrapSuite) TestBootstrapWithLocalControllerCharm(c *gc.C) {
-	for _, test := range []struct {
-		charmPath string
-		err       string
-	}{
-		{
-			charmPath: testcharms.Repo.CharmDir("juju-controller").Path,
-		}, {
-			charmPath: testcharms.Repo.CharmDir("mysql").Path,
-			err:       `--controller-charm-path ".*mysql" is not a "juju-controller" charm`,
-		}, {
-			charmPath: c.MkDir(),
-			err:       `--controller-charm-path ".*" is not a valid charm: .*`,
-		}, {
-			charmPath: "/invalid/path",
-			err:       `problem with --controller-charm-path: .* /invalid/path: .*`,
+	tmpDir := c.MkDir()
+	controllerCharmDir := testcharms.Repo.CharmDir("juju-controller")
+	controllerCharmPath := filepath.Join(tmpDir, "juju-controller.charm")
+	err := controllerCharmDir.ArchiveToPath(controllerCharmPath)
+	c.Assert(err, jc.ErrorIsNil)
+
+	var gotArgs bootstrap.BootstrapParams
+	bootstrapFuncs := &fakeBootstrapFuncs{
+		bootstrapF: func(_ environs.BootstrapContext, _ environs.BootstrapEnviron, callCtx envcontext.ProviderCallContext, args bootstrap.BootstrapParams) error {
+			gotArgs = args
+			return errors.New("test error")
 		},
-	} {
-		var gotArgs bootstrap.BootstrapParams
-		bootstrapFuncs := &fakeBootstrapFuncs{
-			bootstrapF: func(_ environs.BootstrapContext, _ environs.BootstrapEnviron, callCtx envcontext.ProviderCallContext, args bootstrap.BootstrapParams) error {
-				gotArgs = args
-				return errors.New("test error")
-			},
-		}
-		s.PatchValue(&getBootstrapFuncs, func() BootstrapInterface {
-			return bootstrapFuncs
-		})
-		_, err := cmdtesting.RunCommand(c, s.newBootstrapCommand(),
-			"dummy", "devcontroller", "--controller-charm-path", test.charmPath,
-		)
-		if test.err == "" {
-			c.Assert(err, gc.Equals, cmd.ErrSilent)
-			c.Assert(gotArgs.ControllerCharmPath, gc.DeepEquals, test.charmPath)
-		} else {
-			c.Assert(err, gc.ErrorMatches, test.err)
-		}
 	}
+	s.PatchValue(&getBootstrapFuncs, func() BootstrapInterface {
+		return bootstrapFuncs
+	})
+
+	_, err = cmdtesting.RunCommand(c, s.newBootstrapCommand(),
+		"dummy", "devcontroller", "--controller-charm-path", controllerCharmPath,
+	)
+	c.Assert(err, gc.Equals, cmd.ErrSilent)
+	c.Assert(gotArgs.ControllerCharmPath, gc.DeepEquals, controllerCharmPath)
+}
+
+func (s *BootstrapSuite) TestBootstrapWithLocalControllerCharmFailsWithWrongCharm(c *gc.C) {
+	tmpDir := c.MkDir()
+	controllerCharmDir := testcharms.Repo.CharmDir("mysql")
+	controllerCharmPath := filepath.Join(tmpDir, "juju-controller.charm")
+	err := controllerCharmDir.ArchiveToPath(controllerCharmPath)
+	c.Assert(err, jc.ErrorIsNil)
+
+	_, err = cmdtesting.RunCommand(c, s.newBootstrapCommand(),
+		"dummy", "devcontroller", "--controller-charm-path", controllerCharmPath,
+	)
+	c.Assert(err, gc.ErrorMatches, `--controller-charm-path ".*" is not a "juju-controller" charm`)
+}
+
+func (s *BootstrapSuite) TestBootstrapWithLocalControllerCharmFailsWithInvalidCharm(c *gc.C) {
+	tmpDir := c.MkDir()
+	controllerCharmPath := filepath.Join(tmpDir, "juju-controller.charm")
+	err := os.WriteFile(controllerCharmPath, []byte("invalid"), 0644)
+	c.Assert(err, jc.ErrorIsNil)
+
+	_, err = cmdtesting.RunCommand(c, s.newBootstrapCommand(),
+		"dummy", "devcontroller", "--controller-charm-path", controllerCharmPath,
+	)
+	c.Assert(err, gc.ErrorMatches, `--controller-charm-path .* is not a valid charm: .*`)
+}
+
+func (s *BootstrapSuite) TestBootstrapWithLocalControllerCharmFailsWithDir(c *gc.C) {
+	controllerDirPath := testcharms.Repo.CharmDir("juju-controller").Path
+
+	_, err := cmdtesting.RunCommand(c, s.newBootstrapCommand(),
+		"dummy", "devcontroller", "--controller-charm-path", controllerDirPath,
+	)
+	c.Assert(err, gc.ErrorMatches, `.* is a directory`)
+}
+
+func (s *BootstrapSuite) TestBootstrapWithLocalControllerCharmFailsWithNonexistant(c *gc.C) {
+	_, err := cmdtesting.RunCommand(c, s.newBootstrapCommand(),
+		"dummy", "devcontroller", "--controller-charm-path", "/invalid/path.charm",
+	)
+	c.Assert(err, gc.ErrorMatches, `problem with --controller-charm-path: .* /invalid/path.charm: .*`)
 }
 
 func (s *BootstrapSuite) TestBootstrapInvalidControllerCharmChannel(c *gc.C) {

diff --git a/core/base/package_test.go b/core/base/package_test.go
@@ -24,11 +24,10 @@ func (*ImportTest) TestImports(c *gc.C) {
 	found := coretesting.FindJujuCoreImports(c, "github.com/juju/juju/core/base")
 	c.Assert(found, jc.SameContents, []string{
 		"core/arch",
-		"core/logger",
 		"internal/charm",
 		"internal/charm/assumes",
 		"internal/charm/hooks",
 		"internal/charm/resource",
-		"internal/logger",
+		"internal/errors",
 	})
 }