Merge pull request #784 from dmage/iso-region-ipv6

Bug 2083466: Disable dualstack when it is not available

Author: openshift-merge-robot

pkg/storage/s3/s3.go

@@ -83,6 +83,22 @@ func (er *endpointsResolver) EndpointFor(service, region string, opts ...func(*e
 	return endpoints.DefaultResolver().EndpointFor(service, region, opts...)
 }
 
+func isUnknownEndpointError(err error) bool {
+	_, ok := err.(endpoints.UnknownEndpointError)
+	return ok
+}
+
+func regionHasDualStackS3(region string) (bool, error) {
+	_, err := endpoints.DefaultResolver().EndpointFor("s3", region, endpoints.UseDualStackEndpointOption)
+	if isUnknownEndpointError(err) {
+		return false, nil
+	}
+	if err != nil {
+		return false, err
+	}
+	return true, nil
+}
+
 type driver struct {
 	Context context.Context
 	Config  *imageregistryv1.ImageRegistryConfigStorageS3
@@ -237,6 +253,18 @@ func (d *driver) CABundle() (string, bool, error) {
 	}
 }
 
+// useDualStack returns true if the driver should use dual-stack endpoints
+func (d *driver) useDualStack() (bool, error) {
+	if d.Config.RegionEndpoint != "" {
+		return true, nil
+	}
+	ok, err := regionHasDualStackS3(d.Config.Region)
+	if err != nil {
+		return false, fmt.Errorf("failed to determine if region %s has dual stack S3: %w", d.Config.Region, err)
+	}
+	return ok, nil
+}
+
 // getS3Service returns a client that allows us to interact
 // with the aws S3 service
 func (d *driver) getS3Service() (*s3.S3, error) {
@@ -309,7 +337,14 @@ func (d *driver) getS3Service() (*s3.S3, error) {
 		awsOptions.Config.HTTPClient.Transport = d.roundTripper
 	}
 
-	awsOptions.Config.WithUseDualStack(true)
+	useDualStack, err := d.useDualStack()
+	if err != nil {
+		return nil, err
+	}
+	if useDualStack {
+		awsOptions.Config.WithUseDualStack(true)
+	}
+
 	if d.Config.RegionEndpoint != "" {
 		if !d.Config.VirtualHostedStyle {
 			awsOptions.Config.WithS3ForcePathStyle(true)
@@ -377,10 +412,17 @@ func (d *driver) ConfigEnv() (envs envvar.List, err error) {
 		envvar.EnvVar{Name: "REGISTRY_STORAGE_S3_REGION", Value: d.Config.Region},
 		envvar.EnvVar{Name: "REGISTRY_STORAGE_S3_ENCRYPT", Value: d.Config.Encrypt},
 		envvar.EnvVar{Name: "REGISTRY_STORAGE_S3_VIRTUALHOSTEDSTYLE", Value: d.Config.VirtualHostedStyle},
-		envvar.EnvVar{Name: "REGISTRY_STORAGE_S3_USEDUALSTACK", Value: true},
 		envvar.EnvVar{Name: "REGISTRY_STORAGE_S3_CREDENTIALSCONFIGPATH", Value: filepath.Join(imageRegistrySecretMountpoint, imageRegistrySecretDataKey)},
 	)
 
+	useDualStack, err := d.useDualStack()
+	if err != nil {
+		return nil, err
+	}
+	if useDualStack {
+		envs = append(envs, envvar.EnvVar{Name: "REGISTRY_STORAGE_S3_USEDUALSTACK", Value: true})
+	}
+
 	if d.Config.CloudFront != nil {
 		// Use structs to make ordering deterministic
 		type cloudFrontOptions struct {

pkg/storage/s3/s3_test.go

@@ -12,6 +12,7 @@ import (
 	"testing"
 
 	"github.com/aws/aws-sdk-go/aws"
+	"github.com/aws/aws-sdk-go/aws/endpoints"
 	"github.com/aws/aws-sdk-go/private/protocol/xml/xmlutil"
 	"github.com/aws/aws-sdk-go/service/s3"
 	"github.com/google/go-cmp/cmp"
@@ -27,6 +28,92 @@ import (
 	"github.com/openshift/cluster-image-registry-operator/pkg/envvar"
 )
 
+func TestEndpointsResolver(t *testing.T) {
+	testCases := []struct {
+		region       string
+		useDualStack bool
+		endpoint     string
+	}{
+		{
+			region:   "us-east-1",
+			endpoint: "https://s3.amazonaws.com",
+		},
+		{
+			region:       "us-east-1",
+			useDualStack: true,
+			endpoint:     "https://s3.dualstack.us-east-1.amazonaws.com",
+		},
+		{
+			region:   "us-gov-east-1",
+			endpoint: "https://s3.us-gov-east-1.amazonaws.com",
+		},
+		{
+			region:       "us-gov-east-1",
+			useDualStack: true,
+			endpoint:     "https://s3.dualstack.us-gov-east-1.amazonaws.com",
+		},
+		{
+			region:   "us-iso-east-1",
+			endpoint: "https://s3.us-iso-east-1.c2s.ic.gov",
+		},
+		{
+			region:       "us-iso-east-1",
+			useDualStack: true,
+			endpoint:     "",
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.region, func(t *testing.T) {
+			er := newEndpointsResolver(tc.region, "", nil)
+			var opts []func(*endpoints.Options)
+			if tc.useDualStack {
+				opts = append(opts, endpoints.UseDualStackEndpointOption)
+			}
+			ep, err := er.EndpointFor("s3", tc.region, opts...)
+			if isUnknownEndpointError(err) && tc.endpoint == "" {
+				return // the error is expected
+			}
+			if err != nil {
+				t.Fatal(err)
+			}
+			if ep.URL != tc.endpoint {
+				t.Errorf("got %s, want %s", ep.URL, tc.endpoint)
+			}
+		})
+	}
+}
+
+func TestRegionS3DualStack(t *testing.T) {
+	testCases := []struct {
+		region string
+		want   bool
+	}{
+		{
+			region: "us-east-1",
+			want:   true,
+		},
+		{
+			region: "us-gov-east-1",
+			want:   true,
+		},
+		{
+			region: "us-iso-east-1",
+			want:   false,
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.region, func(t *testing.T) {
+			got, err := regionHasDualStackS3(tc.region)
+			if err != nil {
+				t.Fatal(err)
+			}
+			if got != tc.want {
+				t.Errorf("got %t, want %t", got, tc.want)
+			}
+		})
+	}
+}
+
 func TestGetConfig(t *testing.T) {
 	testBuilder := cirofake.NewFixturesBuilder()
 	testBuilder.AddInfraConfig(&configv1.Infrastructure{