fix: adding a small amount of validation to cluster-attributes being updated

common/domain/attrValidator.go

@@ -127,7 +127,6 @@ func (d *AttrValidatorImpl) validateDomainReplicationConfigForGlobalDomain(
 	if !isInClusters(activeCluster) {
 		return errActiveClusterNotInClusters
 	}
-
 	// For active-active domains, also validate that all clusters in AttributeScopes are valid
 	if activeClusters != nil && activeClusters.AttributeScopes != nil {
 		for _, scope := range activeClusters.AttributeScopes {
@@ -184,3 +183,33 @@ func (d *AttrValidatorImpl) validateClusterName(
 	}
 	return nil
 }
+
+func (d *AttrValidatorImpl) validateActiveActiveDomainReplicationConfig(
+	activeClusters *types.ActiveClusters,
+) error {
+
+	if activeClusters == nil || activeClusters.AttributeScopes == nil {
+		return nil
+	}
+
+	clusters := d.clusterMetadata.GetEnabledClusterInfo()
+
+	for _, scopeData := range activeClusters.AttributeScopes {
+		for _, activeCluster := range scopeData.ClusterAttributes {
+			_, ok := clusters[activeCluster.ActiveClusterName]
+			if !ok {
+				return &types.BadRequestError{Message: fmt.Sprintf(
+					"Invalid active cluster name: %v",
+					activeCluster.ActiveClusterName,
+				)}
+			}
+			if activeCluster.FailoverVersion < 0 {
+				return &types.BadRequestError{Message: fmt.Sprintf(
+					"invalid failover version: %d",
+					activeCluster.FailoverVersion,
+				)}
+			}
+		}
+	}
+	return nil
+}

common/domain/attrValidator_test.go

@@ -23,9 +23,13 @@ package domain
 import (
 	"testing"
 
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/suite"
 
 	"github.com/uber/cadence/common/cluster"
+	"github.com/uber/cadence/common/config"
+	"github.com/uber/cadence/common/log"
+	"github.com/uber/cadence/common/metrics"
 	"github.com/uber/cadence/common/persistence"
 	"github.com/uber/cadence/common/types"
 )
@@ -341,3 +345,104 @@ func (s *attrValidatorSuite) TestValidateDomainReplicationConfigClustersDoesNotR
 	)
 	s.IsType(&types.BadRequestError{}, err)
 }
+
+func TestValidateActiveActiveDomainReplicationConfig(t *testing.T) {
+	// Define test cluster names
+	const (
+		clusterA = "cluster-a"
+		clusterB = "cluster-b"
+	)
+
+	// Create explicit cluster metadata for the test
+	clusterMetadata := cluster.NewMetadata(
+		config.ClusterGroupMetadata{
+			FailoverVersionIncrement: 10,
+			PrimaryClusterName:       clusterA,
+			CurrentClusterName:       clusterA,
+			ClusterGroup: map[string]config.ClusterInformation{
+				clusterA: {
+					Enabled:                true,
+					InitialFailoverVersion: 1,
+				},
+				clusterB: {
+					Enabled:                true,
+					InitialFailoverVersion: 2,
+				},
+			},
+		},
+		func(d string) bool { return false },
+		metrics.NewNoopMetricsClient(),
+		log.NewNoop(),
+	)
+
+	validator := newAttrValidator(clusterMetadata, 1)
+
+	testCases := []struct {
+		name           string
+		activeClusters *types.ActiveClusters
+		expectedErr    bool
+		errType        interface{}
+	}{
+		{
+			name: "invalid cluster in AttributeScopes",
+			activeClusters: &types.ActiveClusters{
+				AttributeScopes: map[string]types.ClusterAttributeScope{
+					"city": {
+						ClusterAttributes: map[string]types.ActiveClusterInfo{
+							"seattle": {
+								ActiveClusterName: "invalid-cluster",
+								FailoverVersion:   100,
+							},
+						},
+					},
+				},
+			},
+			expectedErr: true,
+			errType:     &types.BadRequestError{},
+		},
+		{
+			name: "empty ActiveClusters - all maps nil",
+			activeClusters: &types.ActiveClusters{
+				AttributeScopes: nil,
+			},
+			expectedErr: false,
+		},
+		{
+			name: "empty ActiveClusters - maps initialized but empty",
+			activeClusters: &types.ActiveClusters{
+				AttributeScopes: map[string]types.ClusterAttributeScope{},
+			},
+			expectedErr: false,
+		},
+		{
+			name: "an invalid failover version should return an error",
+			activeClusters: &types.ActiveClusters{
+				AttributeScopes: map[string]types.ClusterAttributeScope{
+					"city": {
+						ClusterAttributes: map[string]types.ActiveClusterInfo{
+							"seattle": {
+								ActiveClusterName: clusterA,
+								FailoverVersion:   -1,
+							},
+						},
+					},
+				},
+			},
+			expectedErr: true,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			err := validator.validateActiveActiveDomainReplicationConfig(tc.activeClusters)
+			if tc.expectedErr {
+				assert.Error(t, err)
+				if tc.errType != nil {
+					assert.IsType(t, tc.errType, err)
+				}
+			} else {
+				assert.NoError(t, err)
+			}
+		})
+	}
+}

common/domain/handler.go

@@ -1543,16 +1543,23 @@ func (d *handlerImpl) updateDeleteBadBinary(
 	return config, false, nil
 }
 
+// updateReplicationConfig is the function which takes the input request and current state and edits and returns it
+// to the desired state by merging the request values with the current state.
+// replicationConfigChanged being turned on will trigger an increment in the configVersion.
+// activeClusterChanged indicates a failover is happening and a failover version is to be incremented
 func (d *handlerImpl) updateReplicationConfig(
 	domainName string,
 	config *persistence.DomainReplicationConfig,
 	updateRequest *types.UpdateDomainRequest,
-) (*persistence.DomainReplicationConfig, bool, bool, error) {
+) (
+	mutatedCfg *persistence.DomainReplicationConfig,
+	replicationConfigChanged bool,
+	activeClusterChanged bool,
+	err error,
+) {
 
-	clusterUpdated := false
-	activeClusterUpdated := false
 	if len(updateRequest.Clusters) != 0 {
-		clusterUpdated = true
+		replicationConfigChanged = true
 		clustersNew := []*persistence.ClusterReplicationConfig{}
 		for _, clusterConfig := range updateRequest.Clusters {
 			clustersNew = append(clustersNew, &persistence.ClusterReplicationConfig{
@@ -1570,12 +1577,15 @@ func (d *handlerImpl) updateReplicationConfig(
 	}
 
 	if updateRequest.ActiveClusterName != nil {
-		activeClusterUpdated = true
+		activeClusterChanged = true
 		config.ActiveClusterName = *updateRequest.ActiveClusterName
 	}
 
-	// ActiveClustersByRegion field has been removed - this logic is now handled by AttributeScopes
-	// The legacy ActiveClustersByRegion functionality is replaced by using AttributeScopes with "region" scope
+	err = d.domainAttrValidator.validateActiveActiveDomainReplicationConfig(updateRequest.ActiveClusters)
+	if err != nil {
+		return nil, false, false, err
+	}
+
 	if updateRequest != nil && updateRequest.ActiveClusters != nil && updateRequest.ActiveClusters.AttributeScopes != nil {
 		result, isCh := d.buildActiveActiveClusterScopesFromUpdateRequest(updateRequest, config, domainName)
 		if isCh {
@@ -1587,12 +1597,12 @@ func (d *handlerImpl) updateReplicationConfig(
 			}
 
 			config.ActiveClusters.AttributeScopes = result.AttributeScopes
-			activeClusterUpdated = true
-			clusterUpdated = true
+			activeClusterChanged = true
+			replicationConfigChanged = true
 		}
 	}
 
-	return config, clusterUpdated, activeClusterUpdated, nil
+	return config, replicationConfigChanged, activeClusterChanged, nil
 }
 
 func (d *handlerImpl) handleGracefulFailover(