chore: Refactoring/domain handler update 1/n (#7395)

<!-- Describe what has changed in this PR -->
**What changed?**

This is a refactoring of the domain-handler with the intent of making
the pathing complexity in that method more tractable. The idea is pretty
simple: Separate out the following pathways:

- Local-domain updates (in this PR)
- Global-domain configuration updates (in following PRs)
- Failovers
  - Normal active/passive failover
  - AA failover

and in doing so, unify the UpdateDomain Failover functionality and the
new Failover endpoint, which is really a subset of functionality of
UpdateDomain. The reason for this is because the handler's extremely
hard to follow and has a lot of needless complexity through it's
overloaded use-cases.

The idea being that by splitting out the pathways as mentioned above,
they can follow a much more standard pattern of:

- Request validation
- Fetching existing data 
- performing writes

Without huge amounts of conditionals and guards. This should make
validation a lot easier (right now it's really hard to work out what's
permissable). It should make future edits easier. Importantly for my
work it should unblock the ability to write changes to the
failoverHistory endpoint, since otherwise crowbaring that it is too
hard.


<!-- How have you verified this change? Tested locally? Added a unit
test? Checked in staging env? -->
**How did you test it?**

Unit tests and manual local testing

<!-- Assuming the worst case, what can be broken when deploying this
change to production? -->
**Potential risks**

This could break domain updates, failover, registration if there's a
bug, so some care is warranted.

---------

Signed-off-by: David Porter <[email protected]>

Author: davidporter-id-au

common/domain/attrValidator.go

@@ -48,6 +48,16 @@ func newAttrValidator(
 	}
 }
 
+func (d *AttrValidatorImpl) validateLocalDomainUpdateRequest(updateRequest *types.UpdateDomainRequest) error {
+	if updateRequest.ActiveClusterName != nil || updateRequest.ActiveClusters != nil || updateRequest.Clusters != nil {
+		return errLocalDomainsCannotFailover
+	}
+	if updateRequest.FailoverTimeoutInSeconds != nil {
+		return errLocalDomainsCannotFailover
+	}
+	return nil
+}
+
 func (d *AttrValidatorImpl) validateDomainConfig(config *persistence.DomainConfig) error {
 	if config.Retention < int32(d.minRetentionDays) {
 		return errInvalidRetentionPeriod

common/domain/errors.go

@@ -33,6 +33,7 @@ var (
 	errOngoingGracefulFailover             = &types.BadRequestError{Message: "Cannot start concurrent graceful failover."}
 	errInvalidGracefulFailover             = &types.BadRequestError{Message: "Cannot start graceful failover without updating active cluster or in local domain."}
 	errActiveClusterNameRequired           = &types.BadRequestError{Message: "ActiveClusterName is required for all global domains."}
+	errLocalDomainsCannotFailover          = &types.BadRequestError{Message: "Local domains cannot perform failovers or change replication configuration"}
 
 	errInvalidRetentionPeriod = &types.BadRequestError{Message: "A valid retention period is not set on request."}
 	errInvalidArchivalConfig  = &types.BadRequestError{Message: "Invalid to enable archival without specifying a uri."}

common/domain/handler.go

@@ -427,6 +427,7 @@ func (d *handlerImpl) UpdateDomain(
 		return nil, err
 	}
 
+	// todo (david.porter) remove this and push the deepcopy into each of the branches
 	getResponse := currentDomainState.DeepCopy()
 
 	info := getResponse.Info
@@ -439,10 +440,15 @@ func (d *handlerImpl) UpdateDomain(
 	isGlobalDomain := getResponse.IsGlobalDomain
 	gracefulFailoverEndTime := getResponse.FailoverEndTime
 	currentActiveCluster := replicationConfig.ActiveClusterName
-	currentActiveClusters := replicationConfig.ActiveClusters.DeepCopy()
 	previousFailoverVersion := getResponse.PreviousFailoverVersion
 	lastUpdatedTime := time.Unix(0, getResponse.LastUpdatedTime)
 
+	if !isGlobalDomain {
+		return d.updateLocalDomain(ctx, updateRequest, getResponse, notificationVersion)
+	}
+	// todo (active-active) refactor the rest of this method to remove all branching for global domain variations
+	// and the split between domain update and failover
+
 	// whether history archival config changed
 	historyArchivalConfigChanged := false
 	// whether visibility archival config changed
@@ -632,7 +638,7 @@ func (d *handlerImpl) UpdateDomain(
 					failoverType,
 					&currentActiveCluster,
 					updateRequest.ActiveClusterName,
-					currentActiveClusters,
+					currentDomainState.ReplicationConfig.GetActiveClusters(),
 					replicationConfig.ActiveClusters,
 				))
 				if err != nil {
@@ -663,6 +669,8 @@ func (d *handlerImpl) UpdateDomain(
 			return nil, err
 		}
 	}
+	// todo (david.porter) remove this - all domains at this point are global
+	// leaving during the refactor just for clarity
 	if isGlobalDomain {
 		if err = d.domainReplicator.HandleTransmissionTask(
 			ctx,
@@ -691,6 +699,116 @@ func (d *handlerImpl) UpdateDomain(
 	return response, nil
 }
 
+func (d *handlerImpl) updateLocalDomain(ctx context.Context,
+	updateRequest *types.UpdateDomainRequest,
+	currentState *persistence.GetDomainResponse,
+	notificationVersion int64,
+) (*types.UpdateDomainResponse, error) {
+
+	err := d.domainAttrValidator.validateLocalDomainUpdateRequest(updateRequest)
+	if err != nil {
+		return nil, err
+	}
+
+	// whether history archival config changed
+	historyArchivalConfigChanged := false
+	// whether visibility archival config changed
+	visibilityArchivalConfigChanged := false
+	// whether anything other than active cluster is changed
+	configurationChanged := false
+
+	intendedDomainState := currentState.DeepCopy()
+
+	configVersion := currentState.ConfigVersion
+
+	now := d.timeSource.Now()
+
+	lastUpdatedTime := time.Unix(0, currentState.LastUpdatedTime)
+
+	// Update history archival state
+	historyArchivalConfigChanged, err = d.updateHistoryArchivalState(intendedDomainState.Config, updateRequest)
+	if err != nil {
+		return nil, err
+	}
+
+	// Update visibility archival state
+	visibilityArchivalConfigChanged, err = d.updateVisibilityArchivalState(intendedDomainState.Config, updateRequest)
+	if err != nil {
+		return nil, err
+	}
+
+	// Update domain info
+	info, domainInfoChanged := d.updateDomainInfo(
+		updateRequest,
+		intendedDomainState.Info,
+	)
+
+	// Update domain config
+	config, domainConfigChanged, err := d.updateDomainConfiguration(
+		updateRequest.GetName(),
+		intendedDomainState.Config,
+		updateRequest,
+	)
+	if err != nil {
+		return nil, err
+	}
+
+	// Update domain bad binary
+	config, deleteBinaryChanged, err := d.updateDeleteBadBinary(
+		config,
+		updateRequest.DeleteBadBinary,
+	)
+	if err != nil {
+		return nil, err
+	}
+
+	configurationChanged = historyArchivalConfigChanged || visibilityArchivalConfigChanged || domainInfoChanged || domainConfigChanged || deleteBinaryChanged
+
+	if err = d.domainAttrValidator.validateDomainConfig(config); err != nil {
+		return nil, err
+	}
+
+	if err = d.domainAttrValidator.validateDomainReplicationConfigForLocalDomain(
+		intendedDomainState.ReplicationConfig,
+	); err != nil {
+		return nil, err
+	}
+
+	if configurationChanged {
+		// set the versions
+		if configurationChanged {
+			configVersion = intendedDomainState.ConfigVersion + 1
+		}
+
+		lastUpdatedTime = now
+
+		updateReq := createUpdateRequest(
+			info,
+			config,
+			intendedDomainState.ReplicationConfig,
+			configVersion,
+			intendedDomainState.FailoverVersion,
+			intendedDomainState.FailoverNotificationVersion,
+			intendedDomainState.FailoverEndTime,
+			intendedDomainState.PreviousFailoverVersion,
+			lastUpdatedTime,
+			notificationVersion,
+		)
+
+		err = d.domainManager.UpdateDomain(ctx, &updateReq)
+		if err != nil {
+			return nil, err
+		}
+	}
+	response := &types.UpdateDomainResponse{
+		IsGlobalDomain:  false,
+		FailoverVersion: intendedDomainState.FailoverVersion,
+	}
+	response.DomainInfo, response.Configuration, response.ReplicationConfiguration = d.createResponse(info, config, intendedDomainState.ReplicationConfig)
+
+	return response, nil
+}
+
 // FailoverDomain handles failover of the domain to a different cluster
 func (d *handlerImpl) FailoverDomain(
 	ctx context.Context,

common/domain/handler_MasterCluster_test.go

@@ -427,8 +427,6 @@ func (s *domainHandlerGlobalDomainEnabledPrimaryClusterSuite) TestUpdateGetDomai
 		VisibilityArchivalStatus:               types.ArchivalStatusDisabled.Ptr(),
 		VisibilityArchivalURI:                  common.StringPtr(""),
 		BadBinaries:                            &types.BadBinaries{Binaries: map[string]*types.BadBinaryInfo{}},
-		ActiveClusterName:                      common.StringPtr(s.ClusterMetadata.GetCurrentClusterName()),
-		Clusters:                               clusters,
 	})
 	s.Nil(err)
 	fnTest(

common/domain/handler_NotMasterCluster_test.go

@@ -398,8 +398,6 @@ func (s *domainHandlerGlobalDomainEnabledNotPrimaryClusterSuite) TestUpdateGetDo
 		VisibilityArchivalStatus:               types.ArchivalStatusDisabled.Ptr(),
 		VisibilityArchivalURI:                  common.StringPtr(""),
 		BadBinaries:                            &types.BadBinaries{Binaries: map[string]*types.BadBinaryInfo{}},
-		ActiveClusterName:                      common.StringPtr(s.ClusterMetadata.GetCurrentClusterName()),
-		Clusters:                               clusters,
 	})
 	s.Nil(err)
 	fnTest(

common/domain/handler_test.go

@@ -2210,7 +2210,7 @@ func TestHandler_UpdateDomain(t *testing.T) {
 			},
 		},
 		{
-			name: "Success case - local domain force failover",
+			name: "Error case - local domain force failover - shoudl not be able to failover a local domain",
 			setupMock: func(domainManager *persistence.MockDomainManager, updateRequest *types.UpdateDomainRequest, archivalMetadata *archiver.MockArchivalMetadata, timeSource clock.MockedTimeSource, _ *MockReplicator) {
 				domainResponse := &persistence.GetDomainResponse{
 					ReplicationConfig: &persistence.DomainReplicationConfig{
@@ -2239,17 +2239,6 @@ func TestHandler_UpdateDomain(t *testing.T) {
 				domainManager.EXPECT().GetMetadata(ctx).Return(&persistence.GetMetadataResponse{}, nil).Times(1)
 				domainManager.EXPECT().GetDomain(ctx, &persistence.GetDomainRequest{Name: updateRequest.GetName()}).
 					Return(domainResponse, nil).Times(1)
-				archivalConfig := archiver.NewArchivalConfig(
-					commonconstants.ArchivalDisabled,
-					dynamicproperties.GetStringPropertyFn(commonconstants.ArchivalDisabled),
-					false,
-					dynamicproperties.GetBoolPropertyFn(false),
-					commonconstants.ArchivalDisabled,
-					"")
-				archivalMetadata.On("GetHistoryConfig").Return(archivalConfig).Times(1)
-				archivalMetadata.On("GetVisibilityConfig").Return(archivalConfig).Times(1)
-				timeSource.Advance(time.Hour)
-				domainManager.EXPECT().UpdateDomain(ctx, gomock.Any()).Return(nil).Times(1)
 			},
 			request: &types.UpdateDomainRequest{
 				Name:              constants.TestDomainName,
@@ -2281,6 +2270,7 @@ func TestHandler_UpdateDomain(t *testing.T) {
 					},
 				}
 			},
+			err: errLocalDomainsCannotFailover,
 		},
 		{
 			name: "Error case - GetMetadata error",
@@ -2417,7 +2407,7 @@ func TestHandler_UpdateDomain(t *testing.T) {
 			},
 		},
 		{
-			name: "Error case - handleGracefulFailover error",
+			name: "Error case - handleGracefulFailover error in the case of a global domain - it should return an error to the user",
 			setupMock: func(domainManager *persistence.MockDomainManager, updateRequest *types.UpdateDomainRequest, archivalMetadata *archiver.MockArchivalMetadata, _ clock.MockedTimeSource, _ *MockReplicator) {
 				domainManager.EXPECT().GetMetadata(ctx).Return(&persistence.GetMetadataResponse{}, nil).Times(1)
 				domainManager.EXPECT().GetDomain(ctx, &persistence.GetDomainRequest{Name: updateRequest.GetName()}).
@@ -2427,6 +2417,7 @@ func TestHandler_UpdateDomain(t *testing.T) {
 						},
 						ReplicationConfig: &persistence.DomainReplicationConfig{},
 						Config:            &persistence.DomainConfig{},
+						IsGlobalDomain:    true,
 					}, nil).Times(1)
 				archivalConfig := archiver.NewArchivalConfig(
 					commonconstants.ArchivalDisabled,
@@ -2452,6 +2443,7 @@ func TestHandler_UpdateDomain(t *testing.T) {
 					Return(&persistence.GetDomainResponse{
 						ReplicationConfig: &persistence.DomainReplicationConfig{},
 						Config:            &persistence.DomainConfig{},
+						IsGlobalDomain:    true,
 						Info: &persistence.DomainInfo{
 							Name: constants.TestDomainName,
 						},

common/persistence/data_manager_interfaces.go

@@ -1933,6 +1933,13 @@ func (t *TimerTaskInfo) ToTask() (Task, error) {
 	}
 }
 
+func (c *DomainReplicationConfig) GetActiveClusters() *types.ActiveClusters {
+	if c != nil && c.ActiveClusters != nil {
+		return c.ActiveClusters
+	}
+	return nil
+}
+
 // ToNilSafeCopy
 // TODO: it seems that we just need a nil safe shardInfo, deep copy is not necessary
 func (s *ShardInfo) ToNilSafeCopy() *ShardInfo {