Improve simplestreams fetching

apiserver/facades/agent/provisioner/imagemetadata_test.go

@@ -4,6 +4,9 @@
 package provisioner_test
 
 import (
+	"net/http"
+	"net/http/httptest"
+
 	jc "github.com/juju/testing/checkers"
 	gc "gopkg.in/check.v1"
 
@@ -35,9 +38,16 @@ var _ = gc.Suite(&ImageMetadataSuite{})
 func (s *ImageMetadataSuite) SetUpSuite(c *gc.C) {
 	s.provisionerSuite.SetUpSuite(c)
 
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(404)
+	}))
+	s.AddCleanup(func(c *gc.C) {
+		server.Close()
+	})
+
 	// Make sure that there is nothing in data sources.
 	// Each individual tests will decide if it needs metadata there.
-	imagetesting.PatchOfficialDataSources(&s.CleanupSuite, "test:/daily")
+	imagetesting.PatchOfficialDataSources(&s.CleanupSuite, server.URL)
 	s.PatchValue(&imagemetadata.SimplestreamsImagesPublicKey, sstesting.SignedMetadataPublicKey)
 	s.PatchValue(&keys.JujuPublicKey, sstesting.SignedMetadataPublicKey)
 	useTestImageData(c, nil)
@@ -153,6 +163,7 @@ func (s *ImageMetadataSuite) assertImageMetadataResults(
 ) {
 	c.Assert(obtained.Results, gc.HasLen, len(expected))
 	for i, one := range obtained.Results {
+		c.Assert(one.Error, gc.IsNil)
 		// We are only concerned with images here
 		c.Assert(one.Result.ImageMetadata, gc.DeepEquals, expected[i])
 	}

apiserver/facades/agent/provisioner/provisioninginfo.go

@@ -796,11 +796,15 @@ func (api *ProvisionerAPI) imageMetadataFromDataSources(env environs.Environ, co
 	for _, source := range sources {
 		logger.Debugf("looking in data source %v", source.Description())
 		found, info, err := imagemetadata.Fetch(fetcher, []simplestreams.DataSource{source}, constraint)
-		if err != nil {
+		if errors.Is(err, errors.NotFound) || errors.Is(err, errors.Unauthorized) {
 			// 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())
 			continue
+		} else if err != nil {
+			// When we get an actual protocol/unexpected error, we need to stop.
+			return nil, errors.Annotatef(err, "failed getting published images metadata from %s", source.Description())
 		}
+
 		for _, m := range found {
 			mSeries, err := series.VersionSeries(m.Version)
 			if err != nil {

cmd/juju/commands/main_test.go

@@ -220,6 +220,11 @@ func (s *MainSuite) TestNoWarn2xFirstRun(c *gc.C) {
 	path := c.MkDir()
 	s.PatchEnvironment("JUJU_HOME", path)
 
+	s.PatchValue(&cloud.FetchAndMaybeUpdatePublicClouds,
+		func(access cloud.PublicCloudsAccessDetails, updateClient bool) (map[string]jujucloud.Cloud, string, error) {
+			return nil, "", nil
+		})
+
 	var code int
 	stdout, stderr := jujutesting.CaptureOutput(c, func() {
 		code = jujuMain{

environs/bootstrap/bootstrap.go

@@ -970,10 +970,13 @@ func bootstrapImageMetadata(
 	var publicImageMetadata []*imagemetadata.ImageMetadata
 	for _, source := range sources {
 		sourceMetadata, _, err := imagemetadata.Fetch(fetcher, []simplestreams.DataSource{source}, imageConstraint)
-		if err != nil {
+		if errors.Is(err, errors.NotFound) || errors.Is(err, errors.Unauthorized) {
 			logger.Debugf("ignoring image metadata in %s: %v", source.Description(), err)
 			// Just keep looking...
 			continue
+		} else if err != nil {
+			// When we get an actual protocol/unexpected error, we need to stop.
+			return nil, errors.Annotatef(err, "failed looking for image metadata in %s", source.Description())
 		}
 		logger.Debugf("found %d image metadata in %s", len(sourceMetadata), source.Description())
 		publicImageMetadata = append(publicImageMetadata, sourceMetadata...)
@@ -1124,7 +1127,7 @@ func setPrivateMetadataSources(fetcher imagemetadata.SimplestreamsFetcher, metad
 	}
 	existingMetadata, _, err := imagemetadata.Fetch(fetcher, []simplestreams.DataSource{dataSource}, imageConstraint)
 	if err != nil && !errors.IsNotFound(err) {
-		return nil, errors.Annotate(err, "cannot read image metadata")
+		return nil, errors.Annotatef(err, "cannot read image metadata in %s", dataSource.Description())
 	}
 
 	// Add an image metadata datasource for constraint validation, etc.

environs/bootstrap/bootstrap_test.go

@@ -9,6 +9,8 @@ import (
 	"fmt"
 	"io"
 	"io/ioutil"
+	"net/http"
+	"net/http/httptest"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -557,6 +559,12 @@ func (s *bootstrapSuite) setupProviderWithNoSupportedArches(c *gc.C) bootstrapEn
 // despite image metadata in other data sources compatible with the same configuration as well.
 // Related to bug#1560625.
 func (s *bootstrapSuite) TestBootstrapImageMetadataFromAllSources(c *gc.C) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(404)
+	}))
+	defer server.Close()
+
+	s.PatchValue(&imagemetadata.DefaultUbuntuBaseURL, server.URL)
 	s.PatchValue(&series.HostSeries, func() (string, error) { return "raring", nil })
 	s.PatchValue(&arch.HostArch, func() string { return arch.AMD64 })
 

environs/simplestreams/datasource.go

@@ -10,9 +10,12 @@ import (
 	"net/http"
 	"net/url"
 	"strings"
+	"time"
 
+	"github.com/juju/clock"
 	"github.com/juju/errors"
 	jujuhttp "github.com/juju/http/v2"
+	"github.com/juju/retry"
 	"github.com/juju/utils/v3"
 
 	corelogger "github.com/juju/juju/core/logger"
@@ -36,10 +39,6 @@ type DataSource interface {
 	// PublicSigningKey returns the public key used to validate signed metadata.
 	PublicSigningKey() string
 
-	// SetAllowRetry sets the flag which determines if the datasource will retry fetching the metadata
-	// if it is not immediately available.
-	SetAllowRetry(allow bool)
-
 	// Priority is an importance factor for Data Source. Higher number means higher priority.
 	// This is will allow to sort data sources in order of importance.
 	Priority() int
@@ -78,6 +77,7 @@ type urlDataSource struct {
 	priority         int
 	requireSigned    bool
 	httpClient       *jujuhttp.Client
+	clock            clock.Clock
 }
 
 // Config has values to be used in constructing a datasource.
@@ -108,6 +108,9 @@ type Config struct {
 	// of cloud infrastructure components
 	// The contents are Base64 encoded x.509 certs.
 	CACertificates []string
+
+	// Clock is used for retry. Will use clock.WallClock if nil.
+	Clock clock.Clock
 }
 
 // Validate checks that the baseURL is valid and the description is set.
@@ -139,13 +142,18 @@ func NewDataSource(cfg Config) DataSource {
 // NewDataSourceWithClient returns a new DataSource as defines by the given
 // Config, but with the addition of a http.Client.
 func NewDataSourceWithClient(cfg Config, client *jujuhttp.Client) DataSource {
+	clk := cfg.Clock
+	if clk == nil {
+		clk = clock.WallClock
+	}
 	return &urlDataSource{
 		description:      cfg.Description,
 		baseURL:          cfg.BaseURL,
 		publicSigningKey: cfg.PublicSigningKey,
 		priority:         cfg.Priority,
 		requireSigned:    cfg.RequireSigned,
 		httpClient:       client,
+		clock:            clk,
 	}
 }
 
@@ -171,31 +179,50 @@ func urlJoin(baseURL, relpath string) string {
 
 // Fetch is defined in simplestreams.DataSource.
 func (h *urlDataSource) Fetch(path string) (io.ReadCloser, string, error) {
+	var readCloser io.ReadCloser
 	dataURL := urlJoin(h.baseURL, path)
 	// dataURL can be http:// or file://
 	// MakeFileURL will only modify the URL if it's a file URL
 	dataURL = utils.MakeFileURL(dataURL)
-	resp, err := h.httpClient.Get(context.TODO(), dataURL)
+
+	err := retry.Call(retry.CallArgs{
+		Func: func() error {
+			var err error
+			readCloser, err = h.fetch(dataURL)
+			return err
+		},
+		IsFatalError: func(err error) bool {
+			return errors.Is(err, errors.NotFound) || errors.Is(err, errors.Unauthorized)
+		},
+		Attempts:    3,
+		Delay:       time.Second,
+		MaxDelay:    time.Second * 5,
+		BackoffFunc: retry.DoubleDelay,
+		Clock:       h.clock,
+	})
+	return readCloser, dataURL, err
+}
+
+func (h *urlDataSource) fetch(path string) (io.ReadCloser, error) {
+	resp, err := h.httpClient.Get(context.TODO(), path)
 	if err != nil {
-		if !errors.IsNotFound(err) {
-			// Callers of this mask the actual error.  Therefore warn here.
-			// This is called multiple times when a machine is created, we
-			// only need one success for images and one for tools.
-			logger.Warningf("Got error requesting %q: %v", dataURL, err)
-		}
-		return nil, dataURL, errors.NewNotFound(err, fmt.Sprintf("%q", dataURL))
+		// Callers of this mask the actual error.  Therefore warn here.
+		// This is called multiple times when a machine is created, we
+		// only need one success for images and one for tools.
+		logger.Warningf("Got error requesting %q: %v", path, err)
+		return nil, fmt.Errorf("cannot access URL %q: %w", path, err)
 	}
 	if resp.StatusCode != http.StatusOK {
 		_ = resp.Body.Close()
 		switch resp.StatusCode {
 		case http.StatusNotFound:
-			return nil, dataURL, errors.NotFoundf("%q", dataURL)
+			return nil, errors.NotFoundf("%q", path)
 		case http.StatusUnauthorized:
-			return nil, dataURL, errors.Unauthorizedf("unauthorised access to URL %q", dataURL)
+			return nil, errors.Unauthorizedf("unauthorised access to URL %q", path)
 		}
-		return nil, dataURL, fmt.Errorf("cannot access URL %q, %q", dataURL, resp.Status)
+		return nil, fmt.Errorf("cannot access URL %q, %q", path, resp.Status)
 	}
-	return resp.Body, dataURL, nil
+	return resp.Body, nil
 }
 
 // URL is defined in simplestreams.DataSource.
@@ -208,11 +235,6 @@ func (u *urlDataSource) PublicSigningKey() string {
 	return u.publicSigningKey
 }
 
-// SetAllowRetry is defined in simplestreams.DataSource.
-func (h *urlDataSource) SetAllowRetry(allow bool) {
-	// This is a NOOP for url datasources.
-}
-
 // Priority is defined in simplestreams.DataSource.
 func (h *urlDataSource) Priority() int {
 	return h.priority

environs/simplestreams/datasource_test.go

@@ -10,8 +10,10 @@ import (
 	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
+	"sync/atomic"
+	"time"
 
-	"github.com/juju/errors"
+	"github.com/juju/clock/testclock"
 	jc "github.com/juju/testing/checkers"
 	gc "gopkg.in/check.v1"
 
@@ -56,11 +58,28 @@ func (s *datasourceSuite) TestURL(c *gc.C) {
 	c.Assert(url, gc.Equals, "foo/bar")
 }
 
+func (s *datasourceSuite) TestRetry(c *gc.C) {
+	handler := &testDataHandler{}
+	server := httptest.NewServer(handler)
+	defer server.Close()
+	ds := simplestreams.NewDataSource(simplestreams.Config{
+		Description: "test",
+		BaseURL:     server.URL,
+		Priority:    simplestreams.DEFAULT_CLOUD_DATA,
+		Clock:       testclock.NewDilatedWallClock(10 * time.Millisecond),
+	})
+	_, _, err := ds.Fetch("500")
+	c.Assert(err, gc.NotNil)
+	c.Assert(handler.numReq.Load(), gc.Equals, int64(3))
+}
+
 type testDataHandler struct {
 	supportsGzip bool
+	numReq       atomic.Int64
 }
 
 func (h *testDataHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
+	h.numReq.Add(1)
 	var out io.Writer = w
 	switch r.URL.Path {
 	case "/unauth":
@@ -88,6 +107,9 @@ func (h *testDataHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		}
 		w.WriteHeader(http.StatusOK)
 		return
+	case "/500":
+		http.Error(w, r.URL.Path, 500)
+		return
 	default:
 		http.Error(w, r.URL.Path, 404)
 		return
@@ -123,51 +145,50 @@ var unsignedProduct = `
 `
 
 type datasourceHTTPSSuite struct {
-	Server *httptest.Server
+	server *httptest.Server
+	clock  testclock.AdvanceableClock
 }
 
 func (s *datasourceHTTPSSuite) SetUpTest(c *gc.C) {
+	s.clock = testclock.NewDilatedWallClock(10 * time.Millisecond)
 	mux := http.NewServeMux()
 	mux.HandleFunc("/", func(resp http.ResponseWriter, req *http.Request) {
 		_ = req.Body.Close()
 		resp.WriteHeader(200)
 		_, _ = resp.Write([]byte("Greetings!\n"))
 	})
-	s.Server = httptest.NewTLSServer(mux)
+	s.server = httptest.NewTLSServer(mux)
 }
 
 func (s *datasourceHTTPSSuite) TearDownTest(c *gc.C) {
-	if s.Server != nil {
-		s.Server.Close()
-		s.Server = nil
+	if s.server != nil {
+		s.server.Close()
+		s.server = nil
 	}
 }
 
 func (s *datasourceHTTPSSuite) TestNormalClientFails(c *gc.C) {
-	ds := testing.VerifyDefaultCloudDataSource("test", s.Server.URL)
+	ds := testing.VerifyDefaultCloudDataSource("test", s.server.URL)
 	url, err := ds.URL("bar")
 	c.Assert(err, jc.ErrorIsNil)
-	c.Check(url, gc.Equals, s.Server.URL+"/bar")
+	c.Check(url, gc.Equals, s.server.URL+"/bar")
 	reader, _, err := ds.Fetch("bar")
-	// The underlying failure is a x509: certificate signed by unknown authority
-	// However, the urlDataSource abstraction hides that as a simple NotFound
-	c.Assert(err, jc.Satisfies, errors.IsNotFound)
+	c.Assert(err, gc.ErrorMatches, `.*x509: certificate signed by unknown authority`)
 	c.Check(reader, gc.IsNil)
 }
 
 func (s *datasourceHTTPSSuite) TestNonVerifyingClientSucceeds(c *gc.C) {
 	ds := simplestreams.NewDataSource(simplestreams.Config{
 		Description:          "test",
-		BaseURL:              s.Server.URL,
+		BaseURL:              s.server.URL,
 		HostnameVerification: false,
 		Priority:             simplestreams.DEFAULT_CLOUD_DATA,
+		Clock:                s.clock,
 	})
 	url, err := ds.URL("bar")
 	c.Assert(err, jc.ErrorIsNil)
-	c.Check(url, gc.Equals, s.Server.URL+"/bar")
+	c.Check(url, gc.Equals, s.server.URL+"/bar")
 	reader, _, err := ds.Fetch("bar")
-	// The underlying failure is a x509: certificate signed by unknown authority
-	// However, the urlDataSource abstraction hides that as a simple NotFound
 	c.Assert(err, jc.ErrorIsNil)
 	defer func() { _ = reader.Close() }()
 	byteContent, err := ioutil.ReadAll(reader)
@@ -178,9 +199,10 @@ func (s *datasourceHTTPSSuite) TestNonVerifyingClientSucceeds(c *gc.C) {
 func (s *datasourceHTTPSSuite) TestClientTransportCompression(c *gc.C) {
 	ds := simplestreams.NewDataSource(simplestreams.Config{
 		Description:          "test",
-		BaseURL:              s.Server.URL,
+		BaseURL:              s.server.URL,
 		HostnameVerification: false,
 		Priority:             simplestreams.DEFAULT_CLOUD_DATA,
+		Clock:                s.clock,
 	})
 	httpClient := simplestreams.HttpClient(ds)
 	c.Assert(httpClient, gc.NotNil)