Browse Source

feat(gcp): support multiple replicationLocations on PushSecret (#6225)

Ali Asghar 1 month ago
parent
commit
9b8f10d4bb

+ 3 - 17
providers/v1/gcp/secretmanager/client.go

@@ -196,29 +196,15 @@ func (c *Client) PushSecret(ctx context.Context, secret *corev1.Secret, pushSecr
 		}
 
 		if pushSecretData.GetMetadata() != nil {
-			replica := &secretmanagerpb.Replication_UserManaged_Replica{}
-			var err error
 			meta, err := metadata.ParseMetadataParameters[PushSecretMetadataSpec](pushSecretData.GetMetadata())
 			if err != nil {
 				return fmt.Errorf("failed to parse PushSecret metadata: %w", err)
 			}
-			if meta != nil && meta.Spec.ReplicationLocation != "" {
-				replica.Location = meta.Spec.ReplicationLocation
-			}
-			if meta != nil && meta.Spec.CMEKKeyName != "" {
-				replica.CustomerManagedEncryption = &secretmanagerpb.CustomerManagedEncryption{
-					KmsKeyName: meta.Spec.CMEKKeyName,
+			if meta != nil {
+				if r := buildReplication(meta.Spec); r != nil {
+					replication = r
 				}
 			}
-			replication = &secretmanagerpb.Replication{
-				Replication: &secretmanagerpb.Replication_UserManaged_{
-					UserManaged: &secretmanagerpb.Replication_UserManaged{
-						Replicas: []*secretmanagerpb.Replication_UserManaged_Replica{
-							replica,
-						},
-					},
-				},
-			}
 		}
 		parent := getParentName(c.store.ProjectID, c.store.Location)
 		scrt := &secretmanagerpb.Secret{

+ 76 - 6
providers/v1/gcp/secretmanager/push_secret.go

@@ -41,12 +41,82 @@ const (
 
 // PushSecretMetadataSpec defines the metadata specification for pushed secrets.
 type PushSecretMetadataSpec struct {
-	Annotations         map[string]string             `json:"annotations,omitempty"`
-	Labels              map[string]string             `json:"labels,omitempty"`
-	Topics              []string                      `json:"topics,omitempty"`
-	MergePolicy         PushSecretMetadataMergePolicy `json:"mergePolicy,omitempty"`
-	CMEKKeyName         string                        `json:"cmekKeyName,omitempty"`
-	ReplicationLocation string                        `json:"replicationLocation,omitempty"`
+	Annotations map[string]string             `json:"annotations,omitempty"`
+	Labels      map[string]string             `json:"labels,omitempty"`
+	Topics      []string                      `json:"topics,omitempty"`
+	MergePolicy PushSecretMetadataMergePolicy `json:"mergePolicy,omitempty"`
+	CMEKKeyName string                        `json:"cmekKeyName,omitempty"`
+	// ReplicationLocation defines a single user-managed replication location
+	// for the secret.
+	//
+	// Deprecated: use ReplicationLocations instead. When both fields are set,
+	// ReplicationLocations takes precedence and ReplicationLocation is ignored.
+	ReplicationLocation string `json:"replicationLocation,omitempty"`
+	// ReplicationLocations defines one or more user-managed replication
+	// locations for the secret. This is useful for High Availability across
+	// regions, since Secret Manager does not support multi-regional locations
+	// (e.g. "us", "eu", "ca").
+	ReplicationLocations []string `json:"replicationLocations,omitempty"`
+}
+
+// buildReplication converts a PushSecretMetadataSpec into the
+// secretmanagerpb.Replication to use when creating a new Secret. It returns
+// nil when the spec supplies neither replication locations nor a CMEK key, so
+// the caller keeps the default (Google-managed) automatic replication.
+//
+// Resolution order:
+//
+//   - If ReplicationLocations (or the deprecated ReplicationLocation) are set,
+//     emit UserManaged replication with one replica per location; apply
+//     CMEKKeyName to every replica when it is set.
+//   - If only CMEKKeyName is set, emit Automatic replication carrying the
+//     customer-managed encryption key so the user's choice is not silently
+//     dropped.
+//
+// When both ReplicationLocations and ReplicationLocation are set,
+// ReplicationLocations takes precedence to avoid silently merging values;
+// ReplicationLocation is treated as deprecated.
+func buildReplication(spec PushSecretMetadataSpec) *secretmanagerpb.Replication {
+	locations := spec.ReplicationLocations
+	if len(locations) == 0 && spec.ReplicationLocation != "" {
+		locations = []string{spec.ReplicationLocation}
+	}
+
+	var cmek *secretmanagerpb.CustomerManagedEncryption
+	if spec.CMEKKeyName != "" {
+		cmek = &secretmanagerpb.CustomerManagedEncryption{
+			KmsKeyName: spec.CMEKKeyName,
+		}
+	}
+
+	if cmek != nil && len(locations) == 0 {
+		return &secretmanagerpb.Replication{
+			Replication: &secretmanagerpb.Replication_Automatic_{
+				Automatic: &secretmanagerpb.Replication_Automatic{
+					CustomerManagedEncryption: cmek,
+				},
+			},
+		}
+	}
+
+	if len(locations) == 0 {
+		return nil
+	}
+
+	replicas := make([]*secretmanagerpb.Replication_UserManaged_Replica, 0, len(locations))
+	for _, loc := range locations {
+		replicas = append(replicas, &secretmanagerpb.Replication_UserManaged_Replica{
+			Location:                  loc,
+			CustomerManagedEncryption: cmek,
+		})
+	}
+	return &secretmanagerpb.Replication{
+		Replication: &secretmanagerpb.Replication_UserManaged_{
+			UserManaged: &secretmanagerpb.Replication_UserManaged{
+				Replicas: replicas,
+			},
+		},
+	}
 }
 
 func newPushSecretBuilder(payload []byte, data esv1.PushSecretData) (pushSecretBuilder, error) {

+ 85 - 0
providers/v1/gcp/secretmanager/push_secret_test.go

@@ -19,7 +19,9 @@ package secretmanager
 import (
 	"testing"
 
+	secretmanagerpb "cloud.google.com/go/secretmanager/apiv1/secretmanagerpb"
 	"github.com/stretchr/testify/assert"
+	"google.golang.org/protobuf/proto"
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 
 	testingfake "github.com/external-secrets/external-secrets/runtime/testing/fake"
@@ -144,3 +146,86 @@ func TestBuildMetadata(t *testing.T) {
 		})
 	}
 }
+
+func TestBuildReplication(t *testing.T) {
+	const cmek = "projects/p/locations/global/keyRings/r/cryptoKeys/k"
+
+	userManaged := func(replicas ...*secretmanagerpb.Replication_UserManaged_Replica) *secretmanagerpb.Replication {
+		return &secretmanagerpb.Replication{
+			Replication: &secretmanagerpb.Replication_UserManaged_{
+				UserManaged: &secretmanagerpb.Replication_UserManaged{Replicas: replicas},
+			},
+		}
+	}
+	automatic := func(key string) *secretmanagerpb.Replication {
+		return &secretmanagerpb.Replication{
+			Replication: &secretmanagerpb.Replication_Automatic_{
+				Automatic: &secretmanagerpb.Replication_Automatic{
+					CustomerManagedEncryption: &secretmanagerpb.CustomerManagedEncryption{KmsKeyName: key},
+				},
+			},
+		}
+	}
+	replica := func(loc, key string) *secretmanagerpb.Replication_UserManaged_Replica {
+		r := &secretmanagerpb.Replication_UserManaged_Replica{Location: loc}
+		if key != "" {
+			r.CustomerManagedEncryption = &secretmanagerpb.CustomerManagedEncryption{KmsKeyName: key}
+		}
+		return r
+	}
+
+	tests := []struct {
+		name string
+		spec PushSecretMetadataSpec
+		want *secretmanagerpb.Replication
+	}{
+		{
+			name: "no replication configured",
+			spec: PushSecretMetadataSpec{},
+			want: nil,
+		},
+		{
+			name: "single location via deprecated field",
+			spec: PushSecretMetadataSpec{ReplicationLocation: "us-east1"},
+			want: userManaged(replica("us-east1", "")),
+		},
+		{
+			name: "single location via new field",
+			spec: PushSecretMetadataSpec{ReplicationLocations: []string{"us-east1"}},
+			want: userManaged(replica("us-east1", "")),
+		},
+		{
+			name: "multiple locations",
+			spec: PushSecretMetadataSpec{ReplicationLocations: []string{"us-east1", "europe-west1", "asia-southeast1"}},
+			want: userManaged(replica("us-east1", ""), replica("europe-west1", ""), replica("asia-southeast1", "")),
+		},
+		{
+			name: "new field takes precedence over deprecated when both set",
+			spec: PushSecretMetadataSpec{
+				ReplicationLocation:  "us-east1",
+				ReplicationLocations: []string{"europe-west1", "asia-southeast1"},
+			},
+			want: userManaged(replica("europe-west1", ""), replica("asia-southeast1", "")),
+		},
+		{
+			name: "multiple locations with CMEK applied to all",
+			spec: PushSecretMetadataSpec{
+				ReplicationLocations: []string{"us-east1", "europe-west1"},
+				CMEKKeyName:          cmek,
+			},
+			want: userManaged(replica("us-east1", cmek), replica("europe-west1", cmek)),
+		},
+		{
+			name: "CMEK without locations falls back to automatic replication carrying CMEK",
+			spec: PushSecretMetadataSpec{CMEKKeyName: cmek},
+			want: automatic(cmek),
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := buildReplication(tt.spec)
+			assert.True(t, proto.Equal(tt.want, got), "want %v, got %v", tt.want, got)
+		})
+	}
+}