Revert "Use ManagerIdentity API instead of LastModifierIdentity + ignore-last-modifier metadata hack (#220)" (#233)

<!--- Note to EXTERNAL Contributors -->
<!-- Thanks for opening a PR! 
If it is a significant code change, please **make sure there is an open
issue** for this.
We work best with you when we have accepted the idea first before you
code. -->

<!--- For ALL Contributors 👇 -->

## What was changed
This reverts commit fced0ad.

## Why?
Temporary to reduce merge conflicts with a bug fix we are about to merge
to main and pick into some old releases.

## Checklist
<!--- add/delete as needed --->

1. Closes <!-- add issue number here -->

2. How was this tested:
<!--- Please describe how you tested your changes/how we can test them
-->

3. Any docs updates needed?
<!--- update README if applicable
      or point out where to update docs.temporal.io -->

Author: carlydf

api/v1alpha1/worker_types.go

@@ -196,12 +196,6 @@ type TemporalWorkerDeploymentStatus struct {
 	// +optional
 	LastModifierIdentity string `json:"lastModifierIdentity,omitempty"`
 
-	// ManagerIdentity is the identity that has exclusive rights to modify this Worker Deployment's routing config.
-	// When set, clients whose identity does not match will be blocked from making routing changes.
-	// Empty by default. Use `temporal worker deployment manager-identity set/unset` to change.
-	// +optional
-	ManagerIdentity string `json:"managerIdentity,omitempty"`
-
 	// VersionCount is the total number of versions currently known by the worker deployment.
 	// This includes current, target, ramping, and deprecated versions.
 	// +optional

docs/ownership.md

@@ -2,52 +2,57 @@
 
 ## Problem
 
-If the worker controller is managing a Worker Deployment (i.e. updating its routing config), but a user makes a manual
-change via the CLI, SDK, or gRPC API instead of via the `TemporalWorkerDeployment` CRD interface, the controller should 
-not clobber the user's change.
+If a worker controller is managing a Worker Deployment (ie. the controller is updating the RoutingConfig of the Worker
+Deployment), but the user changes something via the CLI (ie. rolls back to the previous current version, or stops the
+new target version from ramping because an issue was detected), the controller should not clobber what the human did.
 
-Once the user has finished their manual intervention, they need a way to hand ownership back to the controller.
+At some point, after this human has handled their urgent rollback, they will want to let the controller know that it is
+authorized to resume making changes to the Routing Config of the Worker Deployment.
 
 ## Solution
 
-The controller uses the Temporal server's `ManagerIdentity` field on Worker Deployments to coordinate exclusive
-ownership of routing changes.
+_Once it is available in OSS v1.29, the controller will be able to coordinate with other users via the `ManagerIdentity`
+field of a Worker Deployment. This runbook will be updated when that is available and implemented by the controller._
 
-When `ManagerIdentity` is set on a Worker Deployment, only clients whose identity matches `ManagerIdentity` can make
-routing changes (set current version, set ramping version). The controller's identity is visible in the
-`managerIdentity` field of the `TemporalWorkerDeployment` status.
+In the meantime, the controller will watch the `LastModifierIdentity` field of a Worker Deployment to detect whether 
+another user has made a change. If another user made a change to the Worker Deployment, the controller will not make
+any more changes to ensure a human's change is not clobbered.
 
-### How the controller claims ownership
+Once you are done making your own changes to the Worker Deployment's current and ramping versions, and you are ready
+for the Worker Controller to take over, you can update the metadata to indicate that.
 
-The first time the controller plans a routing change for a Worker Deployment (i.e. when `ManagerIdentity` is empty),
-it calls `SetManagerIdentity` to claim ownership before applying the change. Subsequent routing changes succeed because
-the controller's identity already matches `ManagerIdentity`.
+There is no Temporal server support for Worker Deployment Version-level metadata, so you'll have to set this value on
+the Current Version of your Worker Deployment.
 
-### Taking manual control
-
-To take manual control away from the controller, set `ManagerIdentity` to your own identity:
+Note: The controller decodes this metadata value as a string. Be sure to set the value to the string "true" (not the boolean true).
 
 ```bash
-temporal worker deployment manager-identity set \
+temporal worker deployment update-metadata-version \
    --deployment-name $MY_DEPLOYMENT \
-   --self
+   --build-id $CURRENT_VERSION_BUILD_ID \
+   --metadata 'temporal.io/ignore-last-modifier="true"'
 ```
-
-The `--self` flag sets `ManagerIdentity` to the identity of the caller (auto-generated by the CLI if not explicitly
-provided via `--identity`; similarly, the SDK uses its own auto-generated or configured identity). After this, the
-controller's routing change attempts will fail and it will retry on a backoff until ownership is returned.
-
-You can then make routing changes freely (the server enforces `ManagerIdentity` for all clients, not just the
-controller).
-
-### Returning ownership to the controller
-
-When you are done with your manual changes and want the controller to resume, clear `ManagerIdentity`:
-
+Alternatively, if your CLI supports JSON input:
 ```bash
-temporal worker deployment manager-identity unset \
-   --deployment-name $MY_DEPLOYMENT
+temporal worker deployment update-metadata-version \
+   --deployment-name $MY_DEPLOYMENT \
+   --build-id $CURRENT_VERSION_BUILD_ID \
+   --metadata-json '{"temporal.io/ignore-last-modifier":"true"}'
+```
+In the rare case that you have a nil Current Version when you are passing back ownership, you should set it on your Ramping Version
+```bash
+temporal worker deployment update-metadata-version \
+   --deployment-name $MY_DEPLOYMENT \
+   --build-id $RAMPING_VERSION_BUILD_ID \
+   --metadata 'temporal.io/ignore-last-modifier="true"'
+```
+Or with JSON:
+```bash
+temporal worker deployment update-metadata-version \
+   --deployment-name $MY_DEPLOYMENT \
+   --build-id $RAMPING_VERSION_BUILD_ID \
+   --metadata-json '{"temporal.io/ignore-last-modifier":"true"}'
 ```
 
-On the next reconcile, the controller will detect that `ManagerIdentity` is empty, claim it for itself, and resume
-managing routing changes.
+In the even rarer case that you have nil Current Version and nil Ramping Version, you'll need to use the CLI or SDK to
+set a Current or Ramping Version and then do as instructed above.

internal/controller/clientpool/clientpool.go

@@ -97,15 +97,13 @@ type NewClientOptions struct {
 	TemporalNamespace string
 	K8sNamespace      string
 	Spec              v1alpha1.TemporalConnectionSpec
-	Identity          string
 }
 
 func (cp *ClientPool) fetchClientUsingMTLSSecret(secret corev1.Secret, opts NewClientOptions) (*sdkclient.Options, *ClientPoolKey, *ClientAuth, error) {
 	clientOpts := sdkclient.Options{
 		Logger:    cp.logger,
 		HostPort:  opts.Spec.HostPort,
 		Namespace: opts.TemporalNamespace,
-		Identity:  opts.Identity,
 	}
 
 	var pemCert []byte
@@ -172,7 +170,6 @@ func (cp *ClientPool) fetchClientUsingAPIKeySecret(secret corev1.Secret, opts Ne
 		Logger:    cp.logger,
 		HostPort:  opts.Spec.HostPort,
 		Namespace: opts.TemporalNamespace,
-		Identity:  opts.Identity,
 		ConnectionOptions: sdkclient.ConnectionOptions{
 			TLS: &tls.Config{},
 		},
@@ -201,7 +198,6 @@ func (cp *ClientPool) fetchClientUsingNoCredentials(opts NewClientOptions) (*sdk
 		Logger:    cp.logger,
 		HostPort:  opts.Spec.HostPort,
 		Namespace: opts.TemporalNamespace,
-		Identity:  opts.Identity,
 	}
 
 	key := ClientPoolKey{

internal/controller/execplan.go

@@ -12,7 +12,6 @@ import (
 
 	"github.com/go-logr/logr"
 	temporaliov1alpha1 "github.com/temporalio/temporal-worker-controller/api/v1alpha1"
-	"github.com/temporalio/temporal-worker-controller/internal/planner"
 	"github.com/temporalio/temporal-worker-controller/internal/temporal"
 	enumspb "go.temporal.io/api/enums/v1"
 	sdkclient "go.temporal.io/sdk/client"
@@ -151,46 +150,12 @@ func (r *TemporalWorkerDeploymentReconciler) startTestWorkflows(ctx context.Cont
 	return nil
 }
 
-func (r *TemporalWorkerDeploymentReconciler) shouldClaimManagerIdentity(vcfg *planner.VersionConfig) bool {
-	return vcfg.ManagerIdentity == ""
-}
-
-func (r *TemporalWorkerDeploymentReconciler) claimManagerIdentity(
-	ctx context.Context,
-	l logr.Logger,
-	workerDeploy *temporaliov1alpha1.TemporalWorkerDeployment,
-	deploymentHandler sdkclient.WorkerDeploymentHandle,
-	vcfg *planner.VersionConfig,
-) error {
-	resp, err := deploymentHandler.SetManagerIdentity(ctx, sdkclient.WorkerDeploymentSetManagerIdentityOptions{
-		Self:          true,
-		ConflictToken: vcfg.ConflictToken,
-		Identity:      getControllerIdentity(),
-	})
-	if err != nil {
-		l.Error(err, "unable to claim manager identity")
-		r.Recorder.Eventf(workerDeploy, corev1.EventTypeWarning, ReasonManagerIdentityClaimFailed,
-			"Failed to claim manager identity: %v", err)
-		return err
-	}
-	l.Info("claimed manager identity", "identity", getControllerIdentity())
-	// Use the updated conflict token for the subsequent routing config change.
-	vcfg.ConflictToken = resp.ConflictToken
-	return nil
-}
-
 func (r *TemporalWorkerDeploymentReconciler) updateVersionConfig(ctx context.Context, l logr.Logger, workerDeploy *temporaliov1alpha1.TemporalWorkerDeployment, deploymentHandler sdkclient.WorkerDeploymentHandle, p *plan) error {
 	vcfg := p.UpdateVersionConfig
 	if vcfg == nil {
 		return nil
 	}
 
-	if r.shouldClaimManagerIdentity(vcfg) {
-		if err := r.claimManagerIdentity(ctx, l, workerDeploy, deploymentHandler, vcfg); err != nil {
-			return fmt.Errorf("unable to claim manager identity: %w", err)
-		}
-	}
-
 	if vcfg.SetCurrent {
 		l.Info("registering new current version", "buildID", vcfg.BuildID)
 		if _, err := deploymentHandler.SetCurrentVersion(ctx, sdkclient.WorkerDeploymentSetCurrentVersionOptions{

internal/controller/genplan.go

@@ -80,7 +80,15 @@ func (r *TemporalWorkerDeploymentReconciler) generatePlan(
 		ScaleDeployments:     make(map[*corev1.ObjectReference]uint32),
 	}
 
+	// Check if we need to force manual strategy due to external modification
 	rolloutStrategy := w.Spec.RolloutStrategy
+	if w.Status.LastModifierIdentity != getControllerIdentity() &&
+		w.Status.LastModifierIdentity != serverDeleteVersionIdentity &&
+		w.Status.LastModifierIdentity != "" &&
+		!temporalState.IgnoreLastModifier {
+		l.Info(fmt.Sprintf("Forcing Manual rollout strategy since Worker Deployment was modified by a user with a different identity '%s'; to allow controller to make changes again, set 'temporal.io/ignore-last-modifier=true' in the metadata of your Current or Ramping Version; see ownership runbook at docs/ownership.md for more details.", w.Status.LastModifierIdentity))
+		rolloutStrategy.Strategy = temporaliov1alpha1.UpdateManual
+	}
 
 	// Resolve gate input if gate is configured
 	var gateInput []byte

internal/controller/reconciler_events_test.go

@@ -627,20 +627,20 @@ func TestUpdateVersionConfig_EmitsEventOnFailure(t *testing.T) {
 		{
 			name:           "SetCurrentFailed",
 			handle:         &stubWDHandle{setCurrentErr: errors.New("simulated SetCurrentVersion failure")},
-			config:         &planner.VersionConfig{BuildID: "build-abc", SetCurrent: true, ManagerIdentity: "some-manager"},
+			config:         &planner.VersionConfig{BuildID: "build-abc", SetCurrent: true},
 			expectedReason: ReasonVersionPromotionFailed,
 		},
 		{
 			name:           "SetRampingFailed",
 			handle:         &stubWDHandle{setRampingErr: errors.New("simulated SetRampingVersion failure")},
-			config:         &planner.VersionConfig{BuildID: "build-abc", RampPercentage: 25, ManagerIdentity: "some-manager"},
+			config:         &planner.VersionConfig{BuildID: "build-abc", RampPercentage: 25},
 			expectedReason: ReasonVersionPromotionFailed,
 		},
 		{
 			// SetCurrentVersion succeeds; UpdateVersionMetadata fails.
 			name:           "MetadataUpdateFailed",
 			handle:         &stubWDHandle{updateMetaErr: errors.New("simulated UpdateVersionMetadata failure")},
-			config:         &planner.VersionConfig{BuildID: "build-abc", SetCurrent: true, ManagerIdentity: "some-manager"},
+			config:         &planner.VersionConfig{BuildID: "build-abc", SetCurrent: true},
 			expectedReason: ReasonMetadataUpdateFailed,
 		},
 	}

internal/controller/state_mapper.go

@@ -34,7 +34,6 @@ func (m *stateMapper) mapToStatus(targetBuildID string) *v1alpha1.TemporalWorker
 	}
 
 	status.LastModifierIdentity = m.temporalState.LastModifierIdentity
-	status.ManagerIdentity = m.temporalState.ManagerIdentity
 
 	// Get build IDs directly from temporal state
 	currentBuildID := m.temporalState.CurrentBuildID

internal/controller/util.go

@@ -18,16 +18,15 @@ import (
 // status API and may change between releases. Do not write alerting or automation
 // that depends on these strings.
 const (
-	ReasonPlanGenerationFailed       = "PlanGenerationFailed"
-	ReasonPlanExecutionFailed        = "PlanExecutionFailed"
-	ReasonDeploymentCreateFailed     = "DeploymentCreateFailed"
-	ReasonDeploymentDeleteFailed     = "DeploymentDeleteFailed"
-	ReasonDeploymentScaleFailed      = "DeploymentScaleFailed"
-	ReasonDeploymentUpdateFailed     = "DeploymentUpdateFailed"
-	ReasonTestWorkflowStartFailed    = "TestWorkflowStartFailed"
-	ReasonVersionPromotionFailed     = "VersionPromotionFailed"
-	ReasonMetadataUpdateFailed       = "MetadataUpdateFailed"
-	ReasonManagerIdentityClaimFailed = "ManagerIdentityClaimFailed"
+	ReasonPlanGenerationFailed    = "PlanGenerationFailed"
+	ReasonPlanExecutionFailed     = "PlanExecutionFailed"
+	ReasonDeploymentCreateFailed  = "DeploymentCreateFailed"
+	ReasonDeploymentDeleteFailed  = "DeploymentDeleteFailed"
+	ReasonDeploymentScaleFailed   = "DeploymentScaleFailed"
+	ReasonDeploymentUpdateFailed  = "DeploymentUpdateFailed"
+	ReasonTestWorkflowStartFailed = "TestWorkflowStartFailed"
+	ReasonVersionPromotionFailed  = "VersionPromotionFailed"
+	ReasonMetadataUpdateFailed    = "MetadataUpdateFailed"
 )
 
 const (

internal/controller/worker_controller.go

@@ -184,7 +184,6 @@ func (r *TemporalWorkerDeploymentReconciler) Reconcile(ctx context.Context, req
 			K8sNamespace:      workerDeploy.Namespace,
 			TemporalNamespace: workerDeploy.Spec.WorkerOptions.TemporalNamespace,
 			Spec:              temporalConnection.Spec,
-			Identity:          getControllerIdentity(),
 		})
 		if err != nil {
 			l.Error(err, "invalid Temporal auth secret")

internal/planner/planner.go

@@ -27,7 +27,6 @@ type Plan struct {
 	ShouldCreateDeployment bool
 	VersionConfig          *VersionConfig
 	TestWorkflows          []WorkflowConfig
-
 	// Build IDs of versions from which the controller should
 	// remove IgnoreLastModifierKey from the version metadata
 	RemoveIgnoreLastModifierBuilds []string
@@ -46,11 +45,6 @@ type VersionConfig struct {
 	SetCurrent bool
 	// Acceptable values [0,100]
 	RampPercentage int32
-
-	// ManagerIdentity is the current manager identity of the worker deployment in Temporal.
-	// An empty string indicates the controller should claim the identity before applying
-	// any routing config changes.
-	ManagerIdentity string
 }
 
 // WorkflowConfig defines a workflow to be started
@@ -552,14 +546,9 @@ func getVersionConfigDiff(
 		}
 	}
 
-	managerIdentity := ""
-	if temporalState != nil {
-		managerIdentity = temporalState.ManagerIdentity
-	}
 	vcfg := &VersionConfig{
-		ConflictToken:   conflictToken,
-		BuildID:         status.TargetVersion.BuildID,
-		ManagerIdentity: managerIdentity,
+		ConflictToken: conflictToken,
+		BuildID:       status.TargetVersion.BuildID,
 	}
 
 	// If there is no current version and presence of unversioned pollers is not confirmed for all