refactor to use regex + e2e testing

operator/internal/handlers/internal/storage/secrets.go

@@ -10,6 +10,7 @@ import (
 	"fmt"
 	"io"
 	"net/url"
+	"regexp"
 	"sort"
 	"strings"
 
@@ -42,9 +43,10 @@ var (
 	errS3EndpointUnparseable       = errors.New("can not parse S3 endpoint as URL")
 	errS3EndpointNoURL             = errors.New("endpoint for S3 must be an HTTP or HTTPS URL")
 	errS3EndpointUnsupportedScheme = errors.New("scheme of S3 endpoint URL is unsupported")
-	errS3EndpointAWSInvalid        = errors.New("endpoint for AWS S3 must include correct region")
+	errS3EndpointAWSNoRegion       = errors.New("endpoint for AWS S3 must include correct region")
+	errS3EndpointNoBucketName      = errors.New("bucket name must not be included in AWS S3 endpoint URL")
+	errS3EndpointAWSInvalid        = errors.New("endpoint for AWS S3 is invalid, must match either https://s3.region.amazonaws.com or https://vpce-id.s3.region.vpce.amazonaws.com")
 	errS3ForcePathStyleInvalid     = errors.New(`forcepathstyle must be "true" or "false"`)
-	errS3VPCBucketName             = errors.New("bucket name must not be included in VPC endpoint URL")
 
 	errGCPParseCredentialsFile      = errors.New("gcp storage secret cannot be parsed from JSON content")
 	errGCPWrongCredentialSourceFile = errors.New("credential source in secret needs to point to token file")
@@ -63,6 +65,21 @@ const (
 	gcpAccountTypeExternal = "external_account"
 )
 
+var (
+	// Regular AWS S3 endpoint: https://s3.{region}.amazonaws.com
+	awsS3EndpointRegex = regexp.MustCompile(`^https://s3\.([a-z0-9-]+)\.amazonaws\.com$`)
+
+	// VPC endpoint: https://vpce-{id}.s3.{region}.vpce.amazonaws.com
+	awsVPCEndpointRegex = regexp.MustCompile(`^https://bucket.vpce-[a-z0-9-]+\.s3\.([a-z0-9-]+)\.vpce\.amazonaws\.com$`)
+
+	// Invalid patterns with bucket names (to detect and reject)
+	// Regular S3 with bucket: https://bucket-name.s3.region.amazonaws.com
+	awsS3WithBucketRegex = regexp.MustCompile(`^https://([a-z0-9.-]+)\.s3\.([a-z0-9-]+)\.amazonaws\.com$`)
+
+	// VPC with bucket: https://bucket-name.vpce-id.s3.region.vpce.amazonaws.com
+	awsVPCWithBucketRegex = regexp.MustCompile(`^https://([a-z0-9.-]+)\.bucket.vpce-[a-z0-9-]+\.s3\.([a-z0-9-]+)\.vpce\.amazonaws\.com$`)
+)
+
 func getSecrets(ctx context.Context, k k8s.Client, stack *lokiv1.LokiStack, fg configv1.FeatureGates) (*corev1.Secret, *corev1.Secret, error) {
 	var (
 		storageSecret      corev1.Secret
@@ -500,31 +517,36 @@ func validateS3Endpoint(endpoint, region string) error {
 	}
 
 	// Non-AWS S3 compatible endpoints (e.g., MinIO) - no further validation needed
-	if !strings.HasSuffix(endpoint, awsEndpointSuffix) {
-		return nil
-	}
+	if strings.HasSuffix(endpoint, awsEndpointSuffix) {
+		// AWS endpoint validation
+		if len(region) == 0 {
+			return fmt.Errorf("%w: %s", errSecretMissingField, storage.KeyAWSRegion)
+		}
 
-	// AWS endpoint validation
-	if len(region) == 0 {
-		return fmt.Errorf("%w: %s", errSecretMissingField, storage.KeyAWSRegion)
-	}
+		if awsS3WithBucketRegex.MatchString(endpoint) || awsVPCWithBucketRegex.MatchString(endpoint) {
+			return errS3EndpointNoBucketName
+		}
 
-	// Check standard AWS S3 endpoint
-	validEndpoint := fmt.Sprintf("https://s3.%s%s", region, awsEndpointSuffix)
-	if endpoint == validEndpoint {
-		return nil
-	}
+		// Validate correct endpoint formats
+		if matches := awsS3EndpointRegex.FindStringSubmatch(endpoint); matches != nil {
+			if extractedRegion := matches[1]; extractedRegion != region {
+				return fmt.Errorf("%w: expected region %s, got %s", errS3EndpointAWSNoRegion, region, extractedRegion)
+			}
+			return nil
+		}
 
-	// Check VPC endpoint format
-	host := parsedURL.Hostname()
-	if strings.Contains(host, ".vpce.amazonaws.com") && strings.Contains(host, region) {
-		if !strings.HasPrefix(host, "vpce-") {
-			return errS3VPCBucketName
+		if matches := awsVPCEndpointRegex.FindStringSubmatch(endpoint); matches != nil {
+			if extractedRegion := matches[1]; extractedRegion != region {
+				return fmt.Errorf("%w: expected region %s, got %s", errS3EndpointAWSNoRegion, region, extractedRegion)
+			}
+			return nil
 		}
-		return nil
+
+		// If doesn't match any of the valid patterns, just return the error
+		return fmt.Errorf("%w: %s", errS3EndpointAWSInvalid, endpoint)
 	}
 
-	return fmt.Errorf("%w: %s", errS3EndpointAWSInvalid, validEndpoint)
+	return nil
 }
 
 func extractS3SSEConfig(d map[string][]byte) (storage.S3SSEConfig, error) {

operator/internal/handlers/internal/storage/secrets_test.go

@@ -615,7 +615,7 @@ func TestS3Extract(t *testing.T) {
 					"access_key_secret": []byte("secret"),
 				},
 			},
-			wantError: "endpoint for AWS S3 must include correct region: https://s3.region.amazonaws.com",
+			wantError: "endpoint for AWS S3 must include correct region: expected region region, got wrong",
 		},
 		{
 			name: "s3 endpoint format is not a valid s3 URL",
@@ -629,7 +629,49 @@ func TestS3Extract(t *testing.T) {
 					"access_key_secret": []byte("secret"),
 				},
 			},
-			wantError: "endpoint for AWS S3 must include correct region: https://s3.region.amazonaws.com",
+			wantError: "endpoint for AWS S3 is invalid, must match either https://s3.region.amazonaws.com or https://vpce-id.s3.region.vpce.amazonaws.com: http://region.amazonaws.com",
+		},
+		{
+			name: "valid aws s3 vpc endpoint",
+			secret: &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{Name: "test"},
+				Data: map[string][]byte{
+					"endpoint":          []byte("https://vpce-1234567abc.s3.us-east-1.vpce.amazonaws.com"),
+					"region":            []byte("us-east-1"),
+					"bucketnames":       []byte("this,that"),
+					"access_key_id":     []byte("id"),
+					"access_key_secret": []byte("secret"),
+				},
+			},
+			wantCredentialMode: lokiv1.CredentialModeStatic,
+		},
+		{
+			name: "aws s3 vpc endpoint with bucket name should fail",
+			secret: &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{Name: "test"},
+				Data: map[string][]byte{
+					"endpoint":          []byte("https://bucket.vpce-1234567abc.s3.us-east-1.vpce.amazonaws.com"),
+					"region":            []byte("us-east-1"),
+					"bucketnames":       []byte("this,that"),
+					"access_key_id":     []byte("id"),
+					"access_key_secret": []byte("secret"),
+				},
+			},
+			wantError: "bucket name must not be included in AWS S3 endpoint URL",
+		},
+		{
+			name: "aws s3 vpc endpoint wrong region",
+			secret: &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{Name: "test"},
+				Data: map[string][]byte{
+					"endpoint":          []byte("https://vpce-1234567abc.s3.eu-east-2.vpce.amazonaws.com"),
+					"region":            []byte("us-east-1"),
+					"bucketnames":       []byte("this,that"),
+					"access_key_id":     []byte("id"),
+					"access_key_secret": []byte("secret"),
+				},
+			},
+			wantError: "endpoint for AWS S3 must include correct region: expected region us-east-1, got eu-east-2",
 		},
 	}
 	for _, tst := range table {
@@ -720,53 +762,39 @@ func TestS3Extract_ForcePathStyle(t *testing.T) {
 			},
 		},
 		{
-			desc: "aws s3 vpc endpoint with bucket name should fail",
+			desc: "invalid forcepathstyle value",
 			secret: &corev1.Secret{
 				ObjectMeta: metav1.ObjectMeta{Name: "test"},
 				Data: map[string][]byte{
-					"endpoint":          []byte("https://bucket.vpce-1234567-us-east-1c.s3.us-east-1.vpce.amazonaws.com"),
-					"region":            []byte("us-east-1"),
+					"endpoint":          []byte("https://s3.region.amazonaws.com"),
+					"region":            []byte("region"),
 					"bucketnames":       []byte("this,that"),
 					"access_key_id":     []byte("id"),
 					"access_key_secret": []byte("secret"),
+					"forcepathstyle":    []byte("yes"),
 				},
 			},
-			wantError: "bucket name must not be included in VPC endpoint URL",
+			wantError: `forcepathstyle must be "true" or "false": yes`,
 		},
 		{
-			desc: "aws s3 vpc endpoint without bucket prefix",
+			desc: "aws s3 vpc endpoint",
 			secret: &corev1.Secret{
 				ObjectMeta: metav1.ObjectMeta{Name: "test"},
 				Data: map[string][]byte{
-					"endpoint":          []byte("https://vpce-1234567-us-east-1c.s3.us-east-1.vpce.amazonaws.com"),
+					"endpoint":          []byte("https://vpce-1234567abc.s3.us-east-1.vpce.amazonaws.com"),
 					"region":            []byte("us-east-1"),
 					"bucketnames":       []byte("this,that"),
 					"access_key_id":     []byte("id"),
 					"access_key_secret": []byte("secret"),
 				},
 			},
 			wantOptions: &storage.S3StorageConfig{
-				Endpoint:       "https://vpce-1234567-us-east-1c.s3.us-east-1.vpce.amazonaws.com",
+				Endpoint:       "https://vpce-1234567abc.s3.us-east-1.vpce.amazonaws.com",
 				Region:         "us-east-1",
 				Buckets:        "this,that",
 				ForcePathStyle: false,
 			},
 		},
-		{
-			desc: "invalid forcepathstyle value",
-			secret: &corev1.Secret{
-				ObjectMeta: metav1.ObjectMeta{Name: "test"},
-				Data: map[string][]byte{
-					"endpoint":          []byte("https://s3.region.amazonaws.com"),
-					"region":            []byte("region"),
-					"bucketnames":       []byte("this,that"),
-					"access_key_id":     []byte("id"),
-					"access_key_secret": []byte("secret"),
-					"forcepathstyle":    []byte("yes"),
-				},
-			},
-			wantError: `forcepathstyle must be "true" or "false": yes`,
-		},
 	}
 
 	for _, tc := range tt {