Imagemetadata: Pass in a simplestreams fetcher

The following forces the simplestreams fetcher into the imagemetadata so we can ensure we pass the right http client through. The imagemetadata and tools packages are in desperate need of some refactoring to locate all the package level functions onto a type.
sdg0919 · Jun 4, 2021 · b3dd521 · b3dd521
1 parent 23c1fa0
commit b3dd521
Show file tree

Hide file tree

Showing 34 changed files with 275 additions and 165 deletions.
diff --git a/apiserver/common/tools.go b/apiserver/common/tools.go
@@ -372,6 +372,7 @@ func (f *ToolsFinder) findMatchingTools(args params.FindToolsParams) (result cor
 	if args.AgentStream != "" {
 		requestedStream = args.AgentStream
 	}
+
 	streams := envtools.PreferredStreams(&args.Number, cfg.Development(), requestedStream)
 	ss := simplestreams.NewSimpleStreams(simplestreams.DefaultDataSourceFactory())
 	simplestreamsList, err := envtoolsFindTools(ss,

diff --git a/apiserver/facades/agent/provisioner/provisioninginfo.go b/apiserver/facades/agent/provisioner/provisioninginfo.go
@@ -751,8 +751,8 @@ func (api *ProvisionerAPI) imageMetadataFromState(constraint *imagemetadata.Imag
 
 // imageMetadataFromDataSources finds image metadata that match specified criteria in existing data sources.
 func (api *ProvisionerAPI) imageMetadataFromDataSources(env environs.Environ, constraint *imagemetadata.ImageConstraint) ([]params.CloudImageMetadata, error) {
-	factory := simplestreams.DefaultDataSourceFactory()
-	sources, err := environs.ImageMetadataSources(env, factory)
+	fetcher := simplestreams.NewSimpleStreams(simplestreams.DefaultDataSourceFactory())
+	sources, err := environs.ImageMetadataSources(env, fetcher)
 	if err != nil {
 		return nil, errors.Trace(err)
 	}
@@ -788,7 +788,7 @@ func (api *ProvisionerAPI) imageMetadataFromDataSources(env environs.Environ, co
 	var metadataState []cloudimagemetadata.Metadata
 	for _, source := range sources {
 		logger.Debugf("looking in data source %v", source.Description())
-		found, info, err := imagemetadata.Fetch([]simplestreams.DataSource{source}, constraint)
+		found, info, err := imagemetadata.Fetch(fetcher, []simplestreams.DataSource{source}, constraint)
 		if err != nil {
 			// Do not stop looking in other data sources if there is an issue here.
 			logger.Warningf("encountered %v while getting published images metadata from %v", err, source.Description())

diff --git a/apiserver/tools.go b/apiserver/tools.go
@@ -247,6 +247,7 @@ func (h *toolsDownloadHandler) fetchAndCacheTools(
 	if err != nil {
 		return md, nil, err
 	}
+
 	ss := simplestreams.NewSimpleStreams(simplestreams.DefaultDataSourceFactory())
 	exactTools, err := envtools.FindExactTools(ss, env, v.Number, v.Release, v.Arch)
 	if err != nil {

diff --git a/cmd/juju/commands/bootstrap_test.go b/cmd/juju/commands/bootstrap_test.go
@@ -1250,7 +1250,8 @@ func createImageMetadata(c *gc.C) (string, []*imagemetadata.ImageMetadata) {
 	sourceDir := c.MkDir()
 	sourceStor, err := filestorage.NewFileStorageWriter(sourceDir)
 	c.Assert(err, jc.ErrorIsNil)
-	err = imagemetadata.MergeAndWriteMetadata("xenial", im, cloudSpec, sourceStor)
+	ss := simplestreams.NewSimpleStreams(sstesting.TestDataSourceFactory())
+	err = imagemetadata.MergeAndWriteMetadata(ss, "xenial", im, cloudSpec, sourceStor)
 	c.Assert(err, jc.ErrorIsNil)
 	return sourceDir, im
 }

diff --git a/cmd/plugins/juju-metadata/imagemetadata.go b/cmd/plugins/juju-metadata/imagemetadata.go
@@ -219,7 +219,8 @@ func (c *imageMetadataCommand) Run(context *cmd.Context) error {
 	if err != nil {
 		return err
 	}
-	err = imagemetadata.MergeAndWriteMetadata(c.Series, []*imagemetadata.ImageMetadata{im}, &cloudSpec, targetStorage)
+	fetcher := simplestreams.NewSimpleStreams(simplestreams.DefaultDataSourceFactory())
+	err = imagemetadata.MergeAndWriteMetadata(fetcher, c.Series, []*imagemetadata.ImageMetadata{im}, &cloudSpec, targetStorage)
 	if err != nil {
 		return errors.Errorf("image metadata files could not be created: %v", err)
 	}

diff --git a/cmd/plugins/juju-metadata/validateimagemetadata.go b/cmd/plugins/juju-metadata/validateimagemetadata.go
@@ -142,7 +142,8 @@ func (c *validateImageMetadataCommand) Run(context *cmd.Context) error {
 		return err
 	}
 
-	images, resolveInfo, err := imagemetadata.ValidateImageMetadata(params)
+	fetcher := simplestreams.NewSimpleStreams(simplestreams.DefaultDataSourceFactory())
+	images, resolveInfo, err := imagemetadata.ValidateImageMetadata(fetcher, params)
 	if err != nil {
 		if resolveInfo != nil {
 			metadata := map[string]interface{}{

diff --git a/cmd/plugins/juju-metadata/validateimagemetadata_test.go b/cmd/plugins/juju-metadata/validateimagemetadata_test.go
@@ -89,7 +89,8 @@ func (s *ValidateImageMetadataSuite) makeLocalMetadata(id, region, series, endpo
 	if err != nil {
 		return err
 	}
-	err = imagemetadata.MergeAndWriteMetadata(series, []*imagemetadata.ImageMetadata{im}, &cloudSpec, targetStorage)
+	ss := simplestreams.NewSimpleStreams(sstestings.TestDataSourceFactory())
+	err = imagemetadata.MergeAndWriteMetadata(ss, series, []*imagemetadata.ImageMetadata{im}, &cloudSpec, targetStorage)
 	if err != nil {
 		return err
 	}

diff --git a/container/kvm/container.go b/container/kvm/container.go
@@ -12,13 +12,14 @@ import (
 	"github.com/juju/juju/container"
 	"github.com/juju/juju/container/kvm/libvirt"
 	"github.com/juju/juju/core/network"
-	corenetwork "github.com/juju/juju/core/network"
 	"github.com/juju/juju/core/status"
 	"github.com/juju/juju/environs/imagedownloads"
+	"github.com/juju/juju/environs/imagemetadata"
 	"github.com/juju/juju/environs/simplestreams"
 )
 
 type kvmContainer struct {
+	fetcher imagemetadata.SimplestreamsFetcher
 	factory *containerFactory
 	name    string
 	// started is a three state boolean, true, false, or unknown
@@ -42,7 +43,7 @@ func (c *kvmContainer) EnsureCachedImage(params StartParams) error {
 	var srcFunc func() simplestreams.DataSource
 	if params.ImageDownloadURL != "" {
 		srcFunc = func() simplestreams.DataSource {
-			return imagedownloads.NewDataSource(params.ImageDownloadURL)
+			return imagedownloads.NewDataSource(c.fetcher, params.ImageDownloadURL)
 		}
 	}
 	var fType = BIOSFType
@@ -51,6 +52,7 @@ func (c *kvmContainer) EnsureCachedImage(params StartParams) error {
 	}
 
 	sp := syncParams{
+		fetcher: c.fetcher,
 		arch:    params.Arch,
 		series:  params.Series,
 		stream:  params.Stream,
@@ -154,7 +156,7 @@ func (c *kvmContainer) String() string {
 }
 
 type interfaceInfo struct {
-	config                corenetwork.InterfaceInfo
+	config                network.InterfaceInfo
 	parentVirtualPortType network.VirtualPortType
 }
 

diff --git a/container/kvm/containerfactory.go b/container/kvm/containerfactory.go
@@ -3,15 +3,37 @@
 
 package kvm
 
+import "github.com/juju/juju/environs/imagemetadata"
+
 type containerFactory struct {
+	fetcher imagemetadata.SimplestreamsFetcher
 }
 
 var _ ContainerFactory = (*containerFactory)(nil)
 
 func (factory *containerFactory) New(name string) Container {
+	alwaysFalse := false
+	return factory.new(name, &alwaysFalse)
+}
+
+func (factory *containerFactory) List() (result []Container, err error) {
+	machines, err := ListMachines(run)
+	if err != nil {
+		return nil, err
+	}
+	for hostname, status := range machines {
+		result = append(result, factory.new(hostname, isRunning(status)))
+
+	}
+	return result, nil
+}
+
+func (factory *containerFactory) new(name string, started *bool) *kvmContainer {
 	return &kvmContainer{
+		fetcher: factory.fetcher,
 		factory: factory,
 		name:    name,
+		started: started,
 	}
 }
 
@@ -22,19 +44,3 @@ func isRunning(value string) *bool {
 	}
 	return result
 }
-
-func (factory *containerFactory) List() (result []Container, err error) {
-	machines, err := ListMachines(run)
-	if err != nil {
-		return nil, err
-	}
-	for hostname, status := range machines {
-		result = append(result, &kvmContainer{
-			factory: factory,
-			name:    hostname,
-			started: isRunning(status),
-		})
-
-	}
-	return result, nil
-}
diff --git a/container/kvm/kvm.go b/container/kvm/kvm.go
@@ -26,14 +26,19 @@ import (
 	"github.com/juju/juju/environs/config"
 	"github.com/juju/juju/environs/imagemetadata"
 	"github.com/juju/juju/environs/instances"
+	"github.com/juju/juju/environs/simplestreams"
 )
 
 var (
 	logger = loggo.GetLogger("juju.container.kvm")
 
-	// KvmObjectFactory implements the container factory interface for kvm
+	// KVMObjectFactory implements the container factory interface for kvm
 	// containers.
-	KvmObjectFactory ContainerFactory = &containerFactory{}
+	// TODO (stickupkid): This _only_ exists here because we can patch it in
+	// tests. This is horrid!
+	KVMObjectFactory ContainerFactory = &containerFactory{
+		fetcher: simplestreams.NewSimpleStreams(simplestreams.DefaultDataSourceFactory()),
+	}
 
 	// In order for Juju to be able to create the hardware characteristics of
 	// the kvm machines it creates, we need to be explicit in our definition
@@ -166,7 +171,7 @@ func (manager *containerManager) CreateContainer(
 	// Note here that the kvmObjectFactory only returns a valid container
 	// object, and doesn't actually construct the underlying kvm container on
 	// disk.
-	kvmContainer := KvmObjectFactory.New(name)
+	kvmContainer := KVMObjectFactory.New(name)
 
 	hc = &instance.HardwareCharacteristics{AvailabilityZone: &manager.availabilityZone}
 
@@ -250,7 +255,7 @@ func (manager *containerManager) IsInitialized() bool {
 
 func (manager *containerManager) DestroyContainer(id instance.Id) error {
 	name := string(id)
-	kvmContainer := KvmObjectFactory.New(name)
+	kvmContainer := KVMObjectFactory.New(name)
 	if err := kvmContainer.Stop(); err != nil {
 		logger.Errorf("failed to stop kvm container: %v", err)
 		return err
@@ -259,7 +264,7 @@ func (manager *containerManager) DestroyContainer(id instance.Id) error {
 }
 
 func (manager *containerManager) ListContainers() (result []instances.Instance, err error) {
-	containers, err := KvmObjectFactory.List()
+	containers, err := KVMObjectFactory.List()
 	if err != nil {
 		logger.Errorf("failed getting all instances: %v", err)
 		return

diff --git a/container/kvm/sync.go b/container/kvm/sync.go
@@ -19,6 +19,7 @@ import (
 	"github.com/juju/errors"
 	"github.com/juju/juju/core/paths"
 	"github.com/juju/juju/environs/imagedownloads"
+	"github.com/juju/juju/environs/imagemetadata"
 	"github.com/juju/juju/environs/simplestreams"
 )
 
@@ -37,6 +38,7 @@ type Oner interface {
 
 // syncParams conveys the information necessary for calling imagedownloads.One.
 type syncParams struct {
+	fetcher                     imagemetadata.SimplestreamsFetcher
 	arch, series, stream, fType string
 	srcFunc                     func() simplestreams.DataSource
 }
@@ -46,7 +48,7 @@ func (p syncParams) One() (*imagedownloads.Metadata, error) {
 	if err := p.exists(); err != nil {
 		return nil, errors.Trace(err)
 	}
-	return imagedownloads.One(p.arch, p.series, p.stream, p.fType, p.srcFunc)
+	return imagedownloads.One(p.fetcher, p.arch, p.series, p.stream, p.fType, p.srcFunc)
 }
 
 func (p syncParams) exists() error {

diff --git a/container/kvm/testing/test.go b/container/kvm/testing/test.go
@@ -32,5 +32,5 @@ func (s *TestSuite) SetUpTest(c *gc.C) {
 	s.RemovedDir = c.MkDir()
 	s.PatchValue(&container.RemovedContainerDir, s.RemovedDir)
 	s.ContainerFactory = mock.MockFactory()
-	s.PatchValue(&kvm.KvmObjectFactory, s.ContainerFactory)
+	s.PatchValue(&kvm.KVMObjectFactory, s.ContainerFactory)
 }
diff --git a/container/kvm/wrappedcmds_test.go b/container/kvm/wrappedcmds_test.go
@@ -17,6 +17,7 @@ import (
 	. "github.com/juju/juju/container/kvm"
 	"github.com/juju/juju/environs/imagedownloads"
 	"github.com/juju/juju/environs/simplestreams"
+	sstesting "github.com/juju/juju/environs/simplestreams/testing"
 	coretesting "github.com/juju/juju/testing"
 )
 
@@ -62,9 +63,12 @@ func (s *LibVertSuite) TestSyncImagesUtilizesSimpleStreamsSource(c *gc.C) {
 		source = "mocked-url"
 	)
 	p := testSyncParams{
-		arch:    arch,
-		series:  series,
-		srcFunc: func() simplestreams.DataSource { return imagedownloads.NewDataSource(source) },
+		arch:   arch,
+		series: series,
+		srcFunc: func() simplestreams.DataSource {
+			ss := simplestreams.NewSimpleStreams(sstesting.TestDataSourceFactory())
+			return imagedownloads.NewDataSource(ss, source)
+		},
 		success: true,
 	}
 	err := Sync(p, fakeFetcher{}, source, nil)

diff --git a/environs/bootstrap/bootstrap.go b/environs/bootstrap/bootstrap.go
@@ -854,7 +854,7 @@ func userPublicSigningKey() (string, error) {
 // state database will have the synthesised image metadata added to it.
 func bootstrapImageMetadata(
 	environ environs.BootstrapEnviron,
-	dataSourceFactory simplestreams.DataSourceFactory,
+	fetcher imagemetadata.SimplestreamsFetcher,
 	bootstrapSeries *string,
 	bootstrapArch string,
 	bootstrapImageId string,
@@ -905,7 +905,7 @@ func bootstrapImageMetadata(
 	// For providers that support making use of simplestreams
 	// image metadata, search public image metadata. We need
 	// to pass this onto Bootstrap for selecting images.
-	sources, err := environs.ImageMetadataSources(environ, dataSourceFactory)
+	sources, err := environs.ImageMetadataSources(environ, fetcher)
 	if err != nil {
 		return nil, errors.Trace(err)
 	}
@@ -923,7 +923,7 @@ func bootstrapImageMetadata(
 	// Since order of data source matters, order of image metadata matters too. Append is important here.
 	var publicImageMetadata []*imagemetadata.ImageMetadata
 	for _, source := range sources {
-		sourceMetadata, _, err := imagemetadata.Fetch([]simplestreams.DataSource{source}, imageConstraint)
+		sourceMetadata, _, err := imagemetadata.Fetch(fetcher, []simplestreams.DataSource{source}, imageConstraint)
 		if err != nil {
 			logger.Debugf("ignoring image metadata in %s: %v", source.Description(), err)
 			// Just keep looking...
@@ -1006,7 +1006,7 @@ func isCompatibleVersion(v1, v2 version.Number) bool {
 // and adds an image metadata source after verifying the contents. If the
 // directory ends in tools, only the default tools metadata source will be
 // set. Same for images.
-func setPrivateMetadataSources(dataSourceFactory simplestreams.DataSourceFactory, metadataDir string) ([]*imagemetadata.ImageMetadata, error) {
+func setPrivateMetadataSources(fetcher imagemetadata.SimplestreamsFetcher, metadataDir string) ([]*imagemetadata.ImageMetadata, error) {
 	if _, err := os.Stat(metadataDir); err != nil {
 		if !os.IsNotExist(err) {
 			return nil, errors.Annotate(err, "cannot access simplestreams metadata directory")
@@ -1069,14 +1069,14 @@ func setPrivateMetadataSources(dataSourceFactory simplestreams.DataSourceFactory
 	if err := dataSourceConfig.Validate(); err != nil {
 		return nil, errors.Annotate(err, "simplestreams config validation failed")
 	}
-	dataSource := dataSourceFactory.NewDataSource(dataSourceConfig)
+	dataSource := fetcher.NewDataSource(dataSourceConfig)
 
 	// Read the image metadata, as we'll want to upload it to the environment.
 	imageConstraint, err := imagemetadata.NewImageConstraint(simplestreams.LookupParams{})
 	if err != nil {
 		return nil, errors.Trace(err)
 	}
-	existingMetadata, _, err := imagemetadata.Fetch([]simplestreams.DataSource{dataSource}, imageConstraint)
+	existingMetadata, _, err := imagemetadata.Fetch(fetcher, []simplestreams.DataSource{dataSource}, imageConstraint)
 	if err != nil && !errors.IsNotFound(err) {
 		return nil, errors.Annotate(err, "cannot read image metadata")
 	}

diff --git a/environs/bootstrap/bootstrap_test.go b/environs/bootstrap/bootstrap_test.go
@@ -1124,7 +1124,8 @@ func createImageMetadataForArch(c *gc.C, arch string) (dir string, _ []*imagemet
 	sourceDir := c.MkDir()
 	sourceStor, err := filestorage.NewFileStorageWriter(sourceDir)
 	c.Assert(err, jc.ErrorIsNil)
-	err = imagemetadata.MergeAndWriteMetadata("xenial", im, cloudSpec, sourceStor)
+	ss := simplestreams.NewSimpleStreams(sstesting.TestDataSourceFactory())
+	err = imagemetadata.MergeAndWriteMetadata(ss, "xenial", im, cloudSpec, sourceStor)
 	c.Assert(err, jc.ErrorIsNil)
 	return sourceDir, im
 }