[niyomukiza-mechack]

What was changed

Adds the deletion policy field and the decision logic for deleting a Temporal Cloud namespace.

• pkg/deletion: shared logic for any Temporal Cloud resource: resolving the policy, Preflight (decisions that need no Cloud call: Retain, never created).
• controller/namespace/delete.go: the namespace rules (Observed, Decide), matching Temporal Cloud's delete behavior. Only an Active namespace can be deleted; delete protection, Nexus endpoints, and suspension block it; a namespace whose creation time doesn't match the one the operator recorded is never deleted.

Issue #9

[CLAassistant]

All committers have signed the CLA.

[niyomukiza-mechack]

@jaypipes @tomba7 @zainawaisn

1. What should happen on delete when spec.deletionPolicy is not set?
   I'm thinking if the resource was created by the operator, delete and if it was adopted retain.

2. Before sending the delete request, Decide checks for delete protection and Nexus endpoints itself, even though Temporal Cloud already rejects the delete in both cases. Should I keep those checks, or just send the delete and surface Cloud's error? Checking first gives clearer messages and avoids repeated rejected deletes.
   Relying on Cloud means less duplicated logic but its error codes overlap (in both cases temporal returns FailedPrecondition), so we'd have to show its message as is.

[tomba7]

What should happen on delete when spec.deletionPolicy is not set?
I'm thinking if the resource was created by the operator, delete and if it was adopted retain.

IMO we should definitely set a default. My hunch is that most users who are familiar with ACK/Crossplane will expect Delete, since that's the default in those tools. But apart from the familiarity, either option is fine with me. I'd say we go with Delete by default (regardless of how it was created) and then adjust it once we have customer feedback or when we get to the resource adoption implementation.

Before sending the delete request, Decide checks for delete protection and Nexus endpoints itself, even though Temporal Cloud already rejects the delete in both cases. Should I keep those checks, or just send the delete and surface Cloud's error? Checking first gives clearer messages and avoids repeated rejected deletes.
Relying on Cloud means less duplicated logic but its error codes overlap (in both cases temporal returns FailedPrecondition), so we'd have to show its message as is.

On the surface, the delete protection check seems like an obvious one to keep but unsure about the Nexus endpoint one since I haven't reviewed the PR just yet (will do shortly). In general, my preference is to keep the operator's own checks to a minimum, make tcloud the source of truth, and only add operator side checks when it's obvious or necessary (e..g the namespace CRD only has basic validation and leaves the rest to tcloud)

[jaypipes]

Deletion policies for Kubernetes resources are canonically handled as Annotations. See ACK's setup or nine.ch setup or kflare.

I recommend making these annotations cloud.temporal.io/deletion-policy: delete and cloud.temporal.io/deletion-policy: retain. Define the annotation keys and values as constants in the existing file api/cloud/common/const.go

[jaypipes]

Another advantage of using these annotations is now you can use the common kube-temporal/pkg/kube/annotation package and the types.Resource.Annotations() and types.Resource.ReplaceAnnotations() methods.

[niyomukiza-mechack]

nice call out!

[jaypipes]

where did you get the idea to do this? I'm just curious, because it's a pretty complicated interface you're proposing here that doesn't align with the way that resources are deleted in the Temporal Cloud API. For example, deleting a Namespace in the Temporal Cloud API simply returns an asynchronousOperationId which the caller can use to poll on the result of the delete operation. There's no human-in-the-loop "ActionBlock" functionality and the above seems to suspiciously overlap with the concept of deletion protection in the Temporal Cloud API.

I was thinking that cloud-operator's controller-specific reconcilers would use a simpler interface for deleting a resource from Temporal Cloud API that just returns an error that explains whether the deletion operation succeeded or not.

For example, you might implement a ResourceDelete method on pkg/reconciler/base.Reconciler that looks like this:

// ResourceDelete deletes any managed external resources for the supplied
// [types.Resource]. Note that the Reconciler does NOT need to remove the
// finalizer on the associated [client.Object]. The Controller handles that.
// All the Reconciler needs to do is delete any owned resources or
// externally-managed resources associated with the supplied [types.Resource].
func (r *Reconciler) ResourceDelete(
	ctx context.Context,
	desired types.Resource,
) error {
	var err error
	log := ctrlreqlog.FromContext(ctx)
	exit := log.Trace("resource.delete")
	defer func() {
		exit(err)
	}()

	tcc, err := r.CloudClientFor(ctx, desired)
	if err != nil {
		return fmt.Errorf("failed getting tcloud client: %w", err)
	}

	// We may have already called DeleteNamespace for this resource. If so, check
	// that the delete operation has succeeded. If so, return a nil error. Otherwise,
	// mark the resource as being in a state of pending deletion.
	cr := desired.CR().(*tcloudv1alpha1.Namespace)
	status := cr.Status
	resState := status.State
	switch (resState) {
		case tcloudv1alpha1.ResourceStateDeleted:
			// Nothing to do...
			return nil
		case tcloudv1alpha1.Deleting:
			// Check the status of our ongoing deletion operation
			opID := status.AsyncOperationID
			if opID != "" {
				req := tcloudservicev1.GetAsyncOperationRequest{Id: opID}
				opRes, err := tcc.GetAsyncOperation(ctx, &req)
				if err != nil {
					return fmt.Errorf("failed getting async operation status: %w", err)
				}
				if opRes.GetState() == tcloudoperationv1.AsyncOperation_State_STATE_FULFILLED {
					// Deletion succeeded, nothing to do.
					return nil
				} else {
					// Deletion did not succeed. Add a Condition to the resource
					// explaining the deletion has failed.
					reason := fmt.Sprintf(...)
					condition.SetStalled(desired, metav1.ConditionTrue, "ResourceDeleteFailed", reason)
					return fmt.Errorf("failed deleting resource. async operation in state %s", resState.String())
				}
		default:
				// Call the Temporal Cloud API to delete the Namespace.
				req := tcloudservicev1.DeleteNamespaceRequest{...}
				resp, err := tcc.DeleteNamespace(ctx, &req)
				... handle errors...
		}
}

If the deletion failed due to a lifecycle deletion protection being set on the resource, then that error should be placed as a Condition on the resource and an error bubbled back up to prevent the Controller from removing the finalizer on the resource.

[niyomukiza-mechack]

I was actually going to bring this up myself during the crew k8s check in. I was having the same thoughts about whether we should just let Temporal Cloud enforce its own rules and keep the operator simple. But my design came from reading the delete handler in saas-control-plane and trying to mirror its validation so the conditions would be more specific in saying exactly why the delete is stuck instead of a generic failure. but I agree that duplicates what Cloud already enforces.