Merge pull request #17195 from rifelpet/automated-cherry-pick-of-#171…

…83-origin-release-1.31 Automated cherry pick of #17183: Use SDK's built-in resolver for S3Path.GetHTTPsUrl
kubernetes · Jan 10, 2025 · fbf1ac0 · fbf1ac0
2 parents 4930afb + dffc224
commit fbf1ac0
Show file tree

Hide file tree

Showing 2 changed files with 85 additions and 6 deletions.
diff --git a/util/pkg/vfs/s3fs.go b/util/pkg/vfs/s3fs.go
@@ -561,13 +561,18 @@ func (p *S3Path) GetHTTPsUrl(dualstack bool) (string, error) {
 		return "", fmt.Errorf("failed to get bucket details for %q: %w", p.String(), err)
 	}
 
-	var url string
-	if dualstack {
-		url = fmt.Sprintf("https://s3.dualstack.%s.amazonaws.com/%s/%s", bucketDetails.region, bucketDetails.name, p.Key())
-	} else {
-		url = fmt.Sprintf("https://%s.s3.%s.amazonaws.com/%s", bucketDetails.name, bucketDetails.region, p.Key())
+	resolver := s3.NewDefaultEndpointResolverV2()
+	endpoint, err := resolver.ResolveEndpoint(ctx, s3.EndpointParameters{
+		Bucket:       aws.String(bucketDetails.name),
+		Region:       aws.String(bucketDetails.region),
+		UseDualStack: aws.Bool(dualstack),
+	})
+	if err != nil {
+		return "", fmt.Errorf("failed to resolve endpoint for %q: %w", p.String(), err)
 	}
-	return strings.TrimSuffix(url, "/"), nil
+
+	endpoint.URI.Path = path.Join(endpoint.URI.Path, p.Key())
+	return endpoint.URI.String(), nil
 }
 
 func (p *S3Path) IsBucketPublic(ctx context.Context) (bool, error) {

diff --git a/util/pkg/vfs/s3fs_test.go b/util/pkg/vfs/s3fs_test.go
@@ -64,3 +64,77 @@ func Test_S3Path_Parse(t *testing.T) {
 		}
 	}
 }
+
+func TestGetHTTPsUrl(t *testing.T) {
+	grid := []struct {
+		Path        string
+		Dualstack   bool
+		Region      string
+		ExpectedURL string
+	}{
+		{
+			Path:        "s3://bucket",
+			Region:      "us-east-1",
+			ExpectedURL: "https://bucket.s3.us-east-1.amazonaws.com",
+		},
+		{
+			Path:        "s3://bucket.with.forced.path.style/subpath",
+			Region:      "us-east-1",
+			ExpectedURL: "https://s3.us-east-1.amazonaws.com/bucket.with.forced.path.style/subpath",
+		},
+		{
+			Path:        "s3://bucket/path",
+			Region:      "us-east-2",
+			ExpectedURL: "https://bucket.s3.us-east-2.amazonaws.com/path",
+		},
+		{
+			Path:        "s3://bucket2/path/subpath",
+			Region:      "us-east-1",
+			ExpectedURL: "https://bucket2.s3.us-east-1.amazonaws.com/path/subpath",
+		},
+		{
+			Path:        "s3://bucket2-ds/path/subpath",
+			Dualstack:   true,
+			Region:      "us-east-1",
+			ExpectedURL: "https://bucket2-ds.s3.dualstack.us-east-1.amazonaws.com/path/subpath",
+		},
+		{
+			Path:        "s3://bucket2-cn/path/subpath",
+			Region:      "cn-north-1",
+			ExpectedURL: "https://bucket2-cn.s3.cn-north-1.amazonaws.com.cn/path/subpath",
+		},
+		{
+			Path:        "s3://bucket2-cn-ds/path/subpath",
+			Dualstack:   true,
+			Region:      "cn-north-1",
+			ExpectedURL: "https://bucket2-cn-ds.s3.dualstack.cn-north-1.amazonaws.com.cn/path/subpath",
+		},
+		{
+			Path:        "s3://bucket2-gov/path/subpath",
+			Region:      "us-gov-west-1",
+			ExpectedURL: "https://bucket2-gov.s3.us-gov-west-1.amazonaws.com/path/subpath",
+		},
+		{
+			Path:        "s3://bucket2-gov-ds/path/subpath",
+			Dualstack:   true,
+			Region:      "us-gov-west-1",
+			ExpectedURL: "https://bucket2-gov-ds.s3.dualstack.us-gov-west-1.amazonaws.com/path/subpath",
+		},
+	}
+	for _, g := range grid {
+		t.Run(g.Path, func(t *testing.T) {
+			// Must be nonempty in order to force S3_REGION usage
+			// rather than querying S3 for the region.
+			t.Setenv("S3_ENDPOINT", "1")
+			t.Setenv("S3_REGION", g.Region)
+			s3path, _ := Context.buildS3Path(g.Path)
+			url, err := s3path.GetHTTPsUrl(g.Dualstack)
+			if err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+			if url != g.ExpectedURL {
+				t.Fatalf("expected url: %v vs actual url: %v", g.ExpectedURL, url)
+			}
+		})
+	}
+}