cloud/amazon: renaming cloud.Config to cloud.HTTPClientConfig.

Renaming cloud.Config to cloud.HTTPClientConfig to be more specific.
Added InjectTestingRetryMaxAttempts to limit number of request retries
during testing.

Release note: None

Author: sravotto

pkg/cloud/amazon/aws_kms.go

@@ -145,7 +145,7 @@ func MakeAWSKMS(ctx context.Context, uri string, env cloud.KMSEnv) (cloud.KMS, e
 				"custom endpoints disallowed for aws kms due to --aws-kms-disable-http flag")
 		}
 		client, err := cloud.MakeHTTPClient(env.ClusterSettings(), cloud.NilMetrics,
-			cloud.Config{
+			cloud.HTTPClientConfig{
 				Bucket: "KMS",
 				Cloud:  "aws",
 			})

pkg/cloud/amazon/s3_storage.go

@@ -89,6 +89,9 @@ const (
 	scheme = "s3"
 
 	checksumAlgorithm = types.ChecksumAlgorithmSha256
+
+	// TODO(yevgeniy): Revisit retry logic.  Retrying 10 times seems arbitrary.
+	defaultRetryMaxAttempts = 10
 )
 
 // NightlyEnvVarS3Params maps param keys that get added to an S3
@@ -122,6 +125,16 @@ type s3Storage struct {
 	cached *s3Client
 }
 
+// retryMaxAttempts defines how many times we will retry a
+// S3 request on a retriable error.
+var retryMaxAttempts = defaultRetryMaxAttempts
+
+// InjectTestingRetryMaxAttempts is used to change the
+// default retries for tests that need quick fail.
+func InjectTestingRetryMaxAttempts(maxAttempts int) {
+	retryMaxAttempts = maxAttempts
+}
+
 // customRetryer implements the `request.Retryer` interface and allows for
 // customization of the retry behaviour of an AWS client.
 type customRetryer struct{}
@@ -587,7 +600,7 @@ func (s *s3Storage) newClient(ctx context.Context) (s3Client, string, error) {
 	}
 
 	client, err := cloud.MakeHTTPClient(s.settings, s.metrics,
-		cloud.Config{
+		cloud.HTTPClientConfig{
 			Bucket:             s.opts.bucket,
 			Client:             s.storageOptions.ClientName,
 			Cloud:              "aws",
@@ -598,15 +611,12 @@ func (s *s3Storage) newClient(ctx context.Context) (s3Client, string, error) {
 	}
 	addLoadOption(config.WithHTTPClient(client))
 
-	// TODO(yevgeniy): Revisit retry logic.  Retrying 10 times seems arbitrary.
-	retryMaxAttempts := 10
 	addLoadOption(config.WithRetryMaxAttempts(retryMaxAttempts))
 
 	addLoadOption(config.WithLogger(newLogAdapter(ctx)))
 	if s.opts.verbose {
 		addLoadOption(config.WithClientLogMode(awsVerboseLogging))
 	}
-
 	config.WithRetryer(func() aws.Retryer {
 		return retry.NewStandard(func(opts *retry.StandardOptions) {
 			opts.MaxAttempts = retryMaxAttempts

pkg/cloud/amazon/s3_storage_test.go

@@ -455,9 +455,8 @@ func TestPutS3Endpoint(t *testing.T) {
 		q.Add(AWSSecretParam, "secret")
 		q.Add(S3RegionParam, "region")
 		q.Add(AWSUsePathStyle, "true")
-		// Set AWSSkipTLSVerify to use with HTTPS Server with self signed certificates.
-		q.Add(AWSSkipTLSVerify, "true")
 
+		// Verify that it fails without the flag.
 		u := url.URL{
 			Scheme:   "s3",
 			Host:     "bucket",
@@ -476,15 +475,33 @@ func TestPutS3Endpoint(t *testing.T) {
 		// Setup a sink for the given args.
 		testSettings := cluster.MakeTestingClusterSettings()
 		clientFactory := blobs.TestBlobServiceClient("")
-
+		// Force to fail quickly.
+		InjectTestingRetryMaxAttempts(1)
 		storage, err := cloud.MakeExternalStorage(ctx, conf, ioConf, testSettings, clientFactory,
 			nil, nil, cloud.NilMetrics)
 		if err != nil {
 			t.Fatal(err)
 		}
 		defer storage.Close()
-		require.True(t, storage.Conf().S3Config.SkipTLSVerify)
 		_, _, err = storage.ReadFile(ctx, "test file", cloud.ReadOptions{NoFileSize: true})
+		require.Error(t, err)
+
+		// Set AWSSkipTLSVerify to use with HTTPS Server with self signed certificates.
+		q.Add(AWSSkipTLSVerify, "true")
+		u.RawQuery = q.Encode()
+
+		confWithSkipVerify, err := cloud.ExternalStorageConfFromURI(u.String(), user)
+		if err != nil {
+			t.Fatal(err)
+		}
+		storageWithSkipVerify, err := cloud.MakeExternalStorage(ctx, confWithSkipVerify, ioConf, testSettings, clientFactory,
+			nil, nil, cloud.NilMetrics)
+		if err != nil {
+			t.Fatal(err)
+		}
+		defer storageWithSkipVerify.Close()
+		require.True(t, storageWithSkipVerify.Conf().S3Config.SkipTLSVerify)
+		_, _, err = storageWithSkipVerify.ReadFile(ctx, "another test file", cloud.ReadOptions{NoFileSize: true})
 		if err != nil {
 			t.Fatal(err)
 		}

pkg/cloud/azure/azure_storage.go

@@ -222,7 +222,7 @@ func makeAzureStorage(
 
 	options := args.ExternalStorageOptions()
 	t, err := cloud.MakeHTTPClient(args.Settings, args.MetricsRecorder,
-		cloud.Config{
+		cloud.HTTPClientConfig{
 			Bucket: dest.AzureConfig.Container,
 			Client: options.ClientName,
 			Cloud:  "azure",

pkg/cloud/cloud_io.go

@@ -28,8 +28,8 @@ import (
 	"github.com/cockroachdb/errors"
 )
 
-// Config defines the settings for building the underlying transport.
-type Config struct {
+// HTTPClientConfig defines the settings for building the underlying transport.
+type HTTPClientConfig struct {
 	// Bucket is name of the bucket. Used to label metrics.
 	Bucket string
 	// Client type (e.g. CDC vs backup). Used to label metrics.
@@ -92,7 +92,7 @@ var httpMetrics = settings.RegisterBoolSetting(
 // MakeHTTPClient makes an http client configured with the common settings used
 // for interacting with cloud storage (timeouts, retries, CA certs, etc).
 func MakeHTTPClient(
-	settings *cluster.Settings, metrics *Metrics, config Config,
+	settings *cluster.Settings, metrics *Metrics, config HTTPClientConfig,
 ) (*http.Client, error) {
 	t, err := MakeTransport(settings, metrics, config)
 	if err != nil {
@@ -114,7 +114,7 @@ func MakeHTTPClientForTransport(t http.RoundTripper) (*http.Client, error) {
 // used for interacting with cloud storage (timeouts, retries, CA certs, etc).
 // Prefer MakeHTTPClient where possible.
 func MakeTransport(
-	settings *cluster.Settings, metrics *Metrics, config Config,
+	settings *cluster.Settings, metrics *Metrics, config HTTPClientConfig,
 ) (*http.Transport, error) {
 	var tlsConf *tls.Config
 	if config.InsecureSkipVerify {

pkg/cloud/gcp/gcs_storage.go

@@ -186,7 +186,7 @@ func makeGCSStorage(
 	}
 
 	clientName := args.ExternalStorageOptions().ClientName
-	baseTransport, err := cloud.MakeTransport(args.Settings, args.MetricsRecorder, cloud.Config{
+	baseTransport, err := cloud.MakeTransport(args.Settings, args.MetricsRecorder, cloud.HTTPClientConfig{
 		Bucket: conf.Bucket,
 		Client: clientName,
 		Cloud:  "gcs",

pkg/cloud/httpsink/http_storage.go

@@ -68,7 +68,7 @@ func MakeHTTPStorage(
 
 	clientName := args.ExternalStorageOptions().ClientName
 	client, err := cloud.MakeHTTPClient(args.Settings, args.MetricsRecorder,
-		cloud.Config{
+		cloud.HTTPClientConfig{
 			Cloud:  "http",
 			Bucket: "base",
 			Client: clientName,

pkg/cmd/roachtest/tests/s3_clone_backup_restore.go

@@ -27,21 +27,21 @@ import (
 type s3CloneSecureOption int
 
 const (
-	s3ClonePlain      s3CloneSecureOption = iota // Use HTTP
-	s3CloneRequireTLS                            // Use HTTPS, but skip certification verification.
-	s3CloneVerifyTLS                             // Use HTTPS and verify certificates.
+	s3ClonePlain             s3CloneSecureOption = iota // Use HTTP
+	s3CloneTLSWithSkipVerify                            // Use HTTPS, but skip certification verification.
+	s3CloneTLS                                          // Use HTTPS and verify certificates.
 )
 
-var s3CloneSecureOptions = []s3CloneSecureOption{s3ClonePlain, s3CloneRequireTLS, s3CloneVerifyTLS}
+var s3CloneSecureOptions = []s3CloneSecureOption{s3ClonePlain, s3CloneTLSWithSkipVerify, s3CloneTLS}
 
 func (o s3CloneSecureOption) String() string {
 	switch o {
 	case s3ClonePlain:
 		return "plain"
-	case s3CloneRequireTLS:
-		return "require"
-	case s3CloneVerifyTLS:
-		return "verify"
+	case s3CloneTLSWithSkipVerify:
+		return "tlsSkipVerify"
+	case s3CloneTLS:
+		return "tls"
 	default:
 		panic("invalid option")
 	}

pkg/cmd/roachtest/tests/s3_microceph.go

@@ -87,7 +87,7 @@ func (m cephManager) getBackupURI(ctx context.Context, dest string) (string, err
 	// Region is required in the URL, but not used in Ceph.
 	q.Add(amazon.S3RegionParam, "dummy")
 	q.Add(amazon.AWSEndpointParam, endpointURL)
-	if m.secure == s3CloneRequireTLS {
+	if m.secure == s3CloneTLSWithSkipVerify {
 		q.Add(amazon.AWSSkipTLSVerify, "true")
 	}
 	uri := fmt.Sprintf("s3://%s/%s?%s", m.bucket, dest, q.Encode())
@@ -145,7 +145,7 @@ func (m cephManager) install(ctx context.Context) {
 
 // maybeInstallCa adds a custom ca in the CockroachDB cluster.
 func (m cephManager) maybeInstallCa(ctx context.Context) error {
-	if m.secure != s3CloneVerifyTLS {
+	if m.secure != s3CloneTLS {
 		return nil
 	}
 	return installCa(ctx, m.t, m.c)