From 9d0c2e57e328a5e9db6d391240be39419831e991 Mon Sep 17 00:00:00 2001 From: niyomukiza-mechack Date: Sun, 25 Oct 2026 01:48:28 -0700 Subject: [PATCH 1/2] Add deletion policy field and decision logic --- api/cloud/common/deletion_policy.go | 14 ++ api/cloud/v1alpha1/namespace.go | 10 ++ controller/namespace/delete.go | 103 +++++++++++++ .../bases/cloud.temporal.io_namespaces.yaml | 9 ++ pkg/deletion/deletion.go | 135 ++++++++++++++++++ 5 files changed, 271 insertions(+) create mode 100644 api/cloud/common/deletion_policy.go create mode 100644 controller/namespace/delete.go create mode 100644 pkg/deletion/deletion.go diff --git a/api/cloud/common/deletion_policy.go b/api/cloud/common/deletion_policy.go new file mode 100644 index 0000000..0944d30 --- /dev/null +++ b/api/cloud/common/deletion_policy.go @@ -0,0 +1,14 @@ +package common + +// +kubebuilder:validation:Enum=Retain;Delete +type DeletionPolicy string + +const ( + // DeletionPolicyRetain leaves the Temporal Cloud resource in place and + // only removes the custom resource + DeletionPolicyRetain DeletionPolicy = "Retain" + + // DeletionPolicyDelete deletes the Temporal Cloud resource, and removes + // the custom resource after Temporal Cloud finishes deleting it. + DeletionPolicyDelete DeletionPolicy = "Delete" +) diff --git a/api/cloud/v1alpha1/namespace.go b/api/cloud/v1alpha1/namespace.go index 6cabb3f..4ae8fd8 100644 --- a/api/cloud/v1alpha1/namespace.go +++ b/api/cloud/v1alpha1/namespace.go @@ -100,6 +100,16 @@ type NamespaceSpec struct { // Maps to Temporal Cloud proto `NamespaceSpec.description`. // +optional Description string `json:"description,omitempty"` + // TODO(Mechack 10/25/2026): Ask #crew K8s how we should handle delete when this field is not set + // Should the default depend on whether the operator created or adopted the Namespace + // (for example, Retain for adopted, Delete for created), and should an explicit Delete ever be + // honored for an adopted Namespace? + + // DeletionPolicy controls what happens to the Temporal Cloud Namespace + // when this resource is deleted. Retain leaves it in Temporal Cloud. + // Delete permanently deletes it. + // +optional + DeletionPolicy cloudcommon.DeletionPolicy `json:"deletionPolicy,omitempty"` } // ReplicaSpec places a replica of the Namespace in a region. diff --git a/controller/namespace/delete.go b/controller/namespace/delete.go new file mode 100644 index 0000000..b5c8688 --- /dev/null +++ b/controller/namespace/delete.go @@ -0,0 +1,103 @@ +package namespace + +import ( + "fmt" + "time" + + "github.com/temporalio/cloud-operator/api/cloud/common" + "github.com/temporalio/cloud-operator/pkg/deletion" +) + +// Reasons for the decisions Decide makes. +const ( + ReasonDeleted = "Deleted" + ReasonIdentityUnverified = "IdentityUnverified" + ReasonNotCreatedByOperator = "NotCreatedByOperator" + ReasonDeleteProtected = "DeleteProtected" + ReasonNexusEndpointsTarget = "NexusEndpointsTarget" + ReasonDeleting = "Deleting" + ReasonWaiting = "Waiting" + ReasonSuspended = "Suspended" + ReasonUnknownState = "UnknownState" +) + +// Observed is the live state of a Temporal Cloud Namespace +type Observed struct { + // Found is false when Temporal Cloud returns NotFound. + Found bool + // State is the Namespace's state + State common.ResourceState + ResourceVersion string + CreatedTime *time.Time + // DeleteProtection is the live spec.lifecycle.enableDeleteProtection. + DeleteProtection bool + // NexusEndpoints is the number of Nexus endpoints targeting the + // Namespace. + NexusEndpoints int +} + +// Decide decides what to do with a Namespace whose deletion policy is Delete, +// after deletion.Preflight returned false. It follows Temporal Cloud's delete +// rules: only an Active Namespace that isn't delete-protected, suspended or +// targeted by Nexus endpoints can be deleted. +// +// Namespace IDs are reused once a Namespace is gone, so Decide only deletes a +// Namespace whose creation time matches the one the operator recorded. +func Decide(rec deletion.Record, obs Observed) deletion.Decision { + switch { + case !obs.Found, obs.State == common.ResourceStateDeleted: + return decision(deletion.ActionRelease, ReasonDeleted, + "namespace %s is deleted in Temporal Cloud", rec.ID) + case rec.CreatedTime == nil: + return decision(deletion.ActionFail, ReasonIdentityUnverified, + "no creation time is recorded for namespace %s, so the operator can't confirm it created it; %s", rec.ID, deletion.RetainHint) + case !deletion.SameSecond(rec.CreatedTime, obs.CreatedTime): + return decision(deletion.ActionRelease, ReasonNotCreatedByOperator, + "namespace %s in Temporal Cloud is not the one the operator created; leaving it in Temporal Cloud", rec.ID) + } + + switch obs.State { + case common.ResourceStateActive: + if obs.DeleteProtection { + return decision(deletion.ActionBlock, ReasonDeleteProtected, + "namespace %s has delete protection on; turn it off in Temporal Cloud, or %s", rec.ID, deletion.RetainHint) + } + if obs.NexusEndpoints > 0 { + return decision(deletion.ActionBlock, ReasonNexusEndpointsTarget, + "Nexus endpoints target namespace %s; delete them first, or %s", rec.ID, deletion.RetainHint) + } + return decision(deletion.ActionDelete, ReasonDeleting, + "deleting namespace %s in Temporal Cloud", rec.ID) + case common.ResourceStateDeleting, + common.ResourceStateActivating, + common.ResourceStateUpdating: + return decision(deletion.ActionWait, ReasonWaiting, + "namespace %s is %s in Temporal Cloud; waiting", rec.ID, obs.State) + case common.ResourceStateSuspended: + return decision(deletion.ActionBlock, ReasonSuspended, + "namespace %s is suspended, and Temporal Cloud doesn't delete suspended namespaces; contact Temporal support, or %s", rec.ID, deletion.RetainHint) + case common.ResourceStateDeleteFailed, + common.ResourceStateActivationFailed, + common.ResourceStateUpdateFailed, + // Temporal Cloud doesn't report Expired for namespaces today. + common.ResourceStateExpired: + return decision(deletion.ActionFail, "CloudState"+string(obs.State), + "namespace %s is %s in Temporal Cloud and can't be deleted; contact Temporal support, or %s", rec.ID, obs.State, deletion.RetainHint) + default: + return decision(deletion.ActionWait, ReasonUnknownState, + "namespace %s has state %q,waiting", rec.ID, obs.State) + } +} + +func decision( + action deletion.Action, + reason string, + format string, + args ...any, +) deletion.Decision { + return deletion.Decision{ + Action: action, + Reason: reason, + Message: fmt.Sprintf(format, args...), + } +} diff --git a/deploy/kustomize/crd/bases/cloud.temporal.io_namespaces.yaml b/deploy/kustomize/crd/bases/cloud.temporal.io_namespaces.yaml index 018ded9..dc52975 100644 --- a/deploy/kustomize/crd/bases/cloud.temporal.io_namespaces.yaml +++ b/deploy/kustomize/crd/bases/cloud.temporal.io_namespaces.yaml @@ -135,6 +135,15 @@ spec: type: string type: array x-kubernetes-list-type: set + deletionPolicy: + description: |- + DeletionPolicy controls what happens to the Temporal Cloud Namespace + when this resource is deleted. Retain leaves it in Temporal Cloud. + Delete permanently deletes it and all its data. + enum: + - Retain + - Delete + type: string description: description: |- Description is a human-readable description of the Namespace. diff --git a/pkg/deletion/deletion.go b/pkg/deletion/deletion.go new file mode 100644 index 0000000..c9b8898 --- /dev/null +++ b/pkg/deletion/deletion.go @@ -0,0 +1,135 @@ +// Package deletion provides shared logic for handling deleted custom resources. +// It makes no API calls; each resource uses live Temporal Cloud state to decide what to do. +package deletion + +import ( + "fmt" + "time" + + "github.com/temporalio/cloud-operator/api/cloud/common" +) + +// Action is what to do next with a custom resource that is being deleted. +type Action int + +const ( + // ActionRelease means the finalizer may be removed. Temporal Cloud is + // left as it is. + ActionRelease Action = iota + 1 + // ActionWait means Temporal Cloud is busy with the resource. + ActionWait + // ActionBlock means a person must unblock the delete. + ActionBlock + // ActionFail means the resource can't be deleted; stop until + // the custom resource changes. + ActionFail + // ActionDelete means send the delete request to Temporal Cloud. + ActionDelete +) + +// String returns the Action's name. +func (a Action) String() string { + switch a { + case ActionRelease: + return "Release" + case ActionWait: + return "Wait" + case ActionBlock: + return "Block" + case ActionFail: + return "Fail" + case ActionDelete: + return "Delete" + default: + return fmt.Sprintf("Action(%d)", int(a)) + } +} + +// Decision is what to do next, and why. +type Decision struct { + Action Action + // Reason is a condition reason. + Reason string + // Message explains the decision to the user. + Message string +} + +// Record is what the operator recorded about the Temporal Cloud resource it +// created. +type Record struct { + // ID is the resource's Temporal Cloud ID. Only the create path sets it, so + // a non-empty ID means the operator created the resource. + ID string + // CreatedTime is the creation time Temporal Cloud reported for the + // resource. + CreatedTime *time.Time +} + +// Reasons for the decisions Preflight makes. +const ( + ReasonInvalidDeletionPolicy = "InvalidDeletionPolicy" + ReasonRetained = "Retained" + ReasonNeverCreated = "NeverCreated" +) + +// RetainHint tells the user how to remove a custom resource whose Temporal +// Cloud resource can't be deleted. +const RetainHint = "set spec.deletionPolicy to Retain to remove this resource and leave it in Temporal Cloud" + +// Resolve returns the effective deletion policy +func Resolve(policy, def common.DeletionPolicy) (common.DeletionPolicy, error) { + p := policy + if p == "" { + p = def + } + switch p { + case common.DeletionPolicyRetain, common.DeletionPolicyDelete: + return p, nil + default: + return "", fmt.Errorf("unknown deletion policy %q", p) + } +} + +// Preflight decides from the custom resource alone, without reading Temporal +// Cloud. It returns true when the returned Decision is final. When it returns +// false, the caller reads the resource from Temporal Cloud and decides with +// the resource's own rules. +func Preflight( + policy common.DeletionPolicy, + def common.DeletionPolicy, + rec Record, +) (Decision, bool) { + resolved, err := Resolve(policy, def) + if err != nil { + return Decision{ + Action: ActionFail, + Reason: ReasonInvalidDeletionPolicy, + Message: fmt.Sprintf("%v; set spec.deletionPolicy to Retain or Delete", err), + }, true + } + if resolved == common.DeletionPolicyRetain { + return Decision{ + Action: ActionRelease, + Reason: ReasonRetained, + Message: "deletion policy is Retain; leaving the resource in Temporal Cloud", + }, true + } + if rec.ID == "" { + return Decision{ + Action: ActionRelease, + Reason: ReasonNeverCreated, + Message: "the operator never created this resource in Temporal Cloud; nothing to delete", + }, true + } + return Decision{}, false +} + +// SameSecond reports whether recorded and observed are the same time to the +// second. Recorded times are stored as metav1.Time, which keeps whole seconds +// only. A nil time never matches. +func SameSecond(recorded, observed *time.Time) bool { + if recorded == nil || observed == nil { + return false + } + return recorded.Truncate(time.Second).Equal(observed.Truncate(time.Second)) +} From 1473a1d1f0755b14d295fb1dd476a1688997d8d5 Mon Sep 17 00:00:00 2001 From: niyomukiza-mechack Date: Tue, 27 Oct 2026 00:29:33 -0700 Subject: [PATCH 2/2] Simplify namespace delete path per review --- api/cloud/common/const.go | 8 + api/cloud/common/deletion_policy.go | 14 - api/cloud/v1alpha1/namespace.go | 10 - controller/namespace/delete.go | 254 ++++++++++++------ controller/namespace/reconcile.go | 27 -- .../bases/cloud.temporal.io_namespaces.yaml | 9 - pkg/deletion/deletion.go | 135 ---------- .../temporal/cloud/fake/server/namespace.go | 66 +++++ .../temporal/cloud/fake/server/operation.go | 74 +++++ .../temporal/cloud/fake/server/server.go | 4 +- 10 files changed, 323 insertions(+), 278 deletions(-) delete mode 100644 api/cloud/common/deletion_policy.go delete mode 100644 pkg/deletion/deletion.go create mode 100644 pkg/testutil/temporal/cloud/fake/server/operation.go diff --git a/api/cloud/common/const.go b/api/cloud/common/const.go index c81a72b..dcf0eff 100644 --- a/api/cloud/common/const.go +++ b/api/cloud/common/const.go @@ -3,4 +3,12 @@ package common const ( AnnotationKeyPrefixTemporalCloud = "cloud.temporal.io/" AnnotationKeyTemporalCloudHostPort = AnnotationKeyPrefixTemporalCloud + "hostport" + + AnnotationKeyDeletionPolicy = AnnotationKeyPrefixTemporalCloud + "deletion-policy" + // DeletionPolicyDelete deletes the Temporal Cloud resource, and removes + // the custom resource once Temporal Cloud has finished deleting it. + DeletionPolicyDelete = "delete" + // DeletionPolicyRetain leaves the Temporal Cloud resource in place and + // only removes the custom resource. + DeletionPolicyRetain = "retain" ) diff --git a/api/cloud/common/deletion_policy.go b/api/cloud/common/deletion_policy.go deleted file mode 100644 index 0944d30..0000000 --- a/api/cloud/common/deletion_policy.go +++ /dev/null @@ -1,14 +0,0 @@ -package common - -// +kubebuilder:validation:Enum=Retain;Delete -type DeletionPolicy string - -const ( - // DeletionPolicyRetain leaves the Temporal Cloud resource in place and - // only removes the custom resource - DeletionPolicyRetain DeletionPolicy = "Retain" - - // DeletionPolicyDelete deletes the Temporal Cloud resource, and removes - // the custom resource after Temporal Cloud finishes deleting it. - DeletionPolicyDelete DeletionPolicy = "Delete" -) diff --git a/api/cloud/v1alpha1/namespace.go b/api/cloud/v1alpha1/namespace.go index 4ae8fd8..6cabb3f 100644 --- a/api/cloud/v1alpha1/namespace.go +++ b/api/cloud/v1alpha1/namespace.go @@ -100,16 +100,6 @@ type NamespaceSpec struct { // Maps to Temporal Cloud proto `NamespaceSpec.description`. // +optional Description string `json:"description,omitempty"` - // TODO(Mechack 10/25/2026): Ask #crew K8s how we should handle delete when this field is not set - // Should the default depend on whether the operator created or adopted the Namespace - // (for example, Retain for adopted, Delete for created), and should an explicit Delete ever be - // honored for an adopted Namespace? - - // DeletionPolicy controls what happens to the Temporal Cloud Namespace - // when this resource is deleted. Retain leaves it in Temporal Cloud. - // Delete permanently deletes it. - // +optional - DeletionPolicy cloudcommon.DeletionPolicy `json:"deletionPolicy,omitempty"` } // ReplicaSpec places a replica of the Namespace in a region. diff --git a/controller/namespace/delete.go b/controller/namespace/delete.go index b5c8688..ef6fa77 100644 --- a/controller/namespace/delete.go +++ b/controller/namespace/delete.go @@ -1,103 +1,193 @@ package namespace import ( + "context" + "errors" "fmt" "time" - "github.com/temporalio/cloud-operator/api/cloud/common" - "github.com/temporalio/cloud-operator/pkg/deletion" + ctrlreqlog "github.com/temporalio/kube-temporal/pkg/controller/request/log" + "github.com/temporalio/kube-temporal/pkg/controller/requeue" + "github.com/temporalio/kube-temporal/pkg/kube/annotation" + "github.com/temporalio/kube-temporal/pkg/kube/condition" + "github.com/temporalio/kube-temporal/pkg/types" + tcloudservicev1 "go.temporal.io/cloud-sdk/api/cloudservice/v1" + operationv1 "go.temporal.io/cloud-sdk/api/operation/v1" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + tcloudcommon "github.com/temporalio/cloud-operator/api/cloud/common" + tcloudv1alpha1 "github.com/temporalio/cloud-operator/api/cloud/v1alpha1" ) -// Reasons for the decisions Decide makes. const ( - ReasonDeleted = "Deleted" - ReasonIdentityUnverified = "IdentityUnverified" - ReasonNotCreatedByOperator = "NotCreatedByOperator" - ReasonDeleteProtected = "DeleteProtected" - ReasonNexusEndpointsTarget = "NexusEndpointsTarget" - ReasonDeleting = "Deleting" - ReasonWaiting = "Waiting" - ReasonSuspended = "Suspended" - ReasonUnknownState = "UnknownState" -) + // ReasonResourceDeleteFailed is the Stalled condition reason when Temporal + // Cloud failed to delete the Namespace. + ReasonResourceDeleteFailed = "ResourceDeleteFailed" -// Observed is the live state of a Temporal Cloud Namespace -type Observed struct { - // Found is false when Temporal Cloud returns NotFound. - Found bool - // State is the Namespace's state - State common.ResourceState - ResourceVersion string - CreatedTime *time.Time - // DeleteProtection is the live spec.lifecycle.enableDeleteProtection. - DeleteProtection bool - // NexusEndpoints is the number of Nexus endpoints targeting the - // Namespace. - NexusEndpoints int -} + // deletePollInterval is how often to check on a delete in progress. + deletePollInterval = 15 * time.Second +) -// Decide decides what to do with a Namespace whose deletion policy is Delete, -// after deletion.Preflight returned false. It follows Temporal Cloud's delete -// rules: only an Active Namespace that isn't delete-protected, suspended or -// targeted by Nexus endpoints can be deleted. +// ResourceDelete deletes the Temporal Cloud Namespace for the supplied +// [types.Resource], unless its deletion policy is retain. It returns nil once +// the Namespace is gone, and an error while the delete is in progress or has +// failed. // -// Namespace IDs are reused once a Namespace is gone, so Decide only deletes a -// Namespace whose creation time matches the one the operator recorded. -func Decide(rec deletion.Record, obs Observed) deletion.Decision { +// TODO(Mechack): this relies on the kube-temporal Controller persisting the +// status set here (State and AsyncOperationID) and treating nil as "remove +// the finalizer" and a requeue error as "keep it and requeue". Today it passes +// desired and drops the result on error. Remove this once that's fixed. +// +// TODO(Mechack): status.id and status.resourceVersion are set by the create path. +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) + }() + + policy, err := deletionPolicy(desired) + if err != nil { + return err + } + if policy == tcloudcommon.DeletionPolicyRetain { + return nil + } + + cr := desired.CR().(*tcloudv1alpha1.Namespace) + if cr.Status.ID == "" { + // The operator never created a Namespace in Temporal Cloud. + return nil + } + + tcc, err := r.CloudClientFor(ctx, desired) + if err != nil { + err = fmt.Errorf("failed getting tcloud client: %w", err) + return err + } + + // We may have already called DeleteNamespace for this resource. If so, + // check whether the delete operation has finished. switch { - case !obs.Found, obs.State == common.ResourceStateDeleted: - return decision(deletion.ActionRelease, ReasonDeleted, - "namespace %s is deleted in Temporal Cloud", rec.ID) - case rec.CreatedTime == nil: - return decision(deletion.ActionFail, ReasonIdentityUnverified, - "no creation time is recorded for namespace %s, so the operator can't confirm it created it; %s", rec.ID, deletion.RetainHint) - case !deletion.SameSecond(rec.CreatedTime, obs.CreatedTime): - return decision(deletion.ActionRelease, ReasonNotCreatedByOperator, - "namespace %s in Temporal Cloud is not the one the operator created; leaving it in Temporal Cloud", rec.ID) + case cr.Status.State == tcloudcommon.ResourceStateDeleted: + return nil + case cr.Status.State == tcloudcommon.ResourceStateDeleting && + cr.Status.AsyncOperationID != "": + err = checkDelete(ctx, tcc, desired, cr) + return err } - switch obs.State { - case common.ResourceStateActive: - if obs.DeleteProtection { - return decision(deletion.ActionBlock, ReasonDeleteProtected, - "namespace %s has delete protection on; turn it off in Temporal Cloud, or %s", rec.ID, deletion.RetainHint) - } - if obs.NexusEndpoints > 0 { - return decision(deletion.ActionBlock, ReasonNexusEndpointsTarget, - "Nexus endpoints target namespace %s; delete them first, or %s", rec.ID, deletion.RetainHint) - } - return decision(deletion.ActionDelete, ReasonDeleting, - "deleting namespace %s in Temporal Cloud", rec.ID) - case common.ResourceStateDeleting, - common.ResourceStateActivating, - common.ResourceStateUpdating: - return decision(deletion.ActionWait, ReasonWaiting, - "namespace %s is %s in Temporal Cloud; waiting", rec.ID, obs.State) - case common.ResourceStateSuspended: - return decision(deletion.ActionBlock, ReasonSuspended, - "namespace %s is suspended, and Temporal Cloud doesn't delete suspended namespaces; contact Temporal support, or %s", rec.ID, deletion.RetainHint) - case common.ResourceStateDeleteFailed, - common.ResourceStateActivationFailed, - common.ResourceStateUpdateFailed, - // Temporal Cloud doesn't report Expired for namespaces today. - common.ResourceStateExpired: - return decision(deletion.ActionFail, "CloudState"+string(obs.State), - "namespace %s is %s in Temporal Cloud and can't be deleted; contact Temporal support, or %s", rec.ID, obs.State, deletion.RetainHint) + // TODO(Mechack): uses the CR's last recorded resourceVersion. If it's stale, + // Temporal Cloud rejects the delete with "resource version mismatch". Switch to + // the version from a fresh GetNamespace if that becomes a problem. + resp, err := tcc.DeleteNamespace(ctx, &tcloudservicev1.DeleteNamespaceRequest{ + Namespace: cr.Status.ID, + ResourceVersion: cr.Status.ResourceVersion, + }) + if status.Code(err) == codes.NotFound { + // Already gone. + err = nil + return nil + } + if err != nil { + // The gRPC error includes Temporal Cloud's message, for example that + // deletion protection is enabled. + err = fmt.Errorf("failed deleting tcloud namespace %s: %w", cr.Status.ID, err) + condition.SetStalled( + desired, metav1.ConditionTrue, err.Error(), ReasonResourceDeleteFailed, + ) + return err + } + + cr.Status.State = tcloudcommon.ResourceStateDeleting + cr.Status.AsyncOperationID = resp.GetAsyncOperation().GetId() + err = requeue.NeededAfter( + fmt.Errorf( + "deleting tcloud namespace %s (async operation %s)", + cr.Status.ID, cr.Status.AsyncOperationID, + ), + deletePollInterval, + ) + return err +} + +// checkDelete checks the async operation for a delete already sent to +// Temporal Cloud. It returns nil once the operation is fulfilled, a requeue +// error while it is running, and an error with the Stalled condition set if +// it failed. +func checkDelete( + ctx context.Context, + tcc tcloudservicev1.CloudServiceClient, + desired types.Resource, + cr *tcloudv1alpha1.Namespace, +) error { + resp, err := tcc.GetAsyncOperation(ctx, &tcloudservicev1.GetAsyncOperationRequest{ + AsyncOperationId: cr.Status.AsyncOperationID, + }) + if err != nil { + return fmt.Errorf( + "failed getting tcloud async operation %s: %w", + cr.Status.AsyncOperationID, err, + ) + } + op := resp.GetAsyncOperation() + switch op.GetState() { + case operationv1.AsyncOperation_STATE_FULFILLED: + return nil + case operationv1.AsyncOperation_STATE_FAILED, + operationv1.AsyncOperation_STATE_REJECTED, + operationv1.AsyncOperation_STATE_CANCELLED: + msg := fmt.Sprintf( + "deleting tcloud namespace %s failed: async operation %s is %s: %s", + cr.Status.ID, op.GetId(), op.GetState(), op.GetFailureReason(), + ) + condition.SetStalled( + desired, metav1.ConditionTrue, msg, ReasonResourceDeleteFailed, + ) + return errors.New(msg) default: - return decision(deletion.ActionWait, ReasonUnknownState, - "namespace %s has state %q,waiting", rec.ID, obs.State) + // Pending. + return requeue.NeededAfter( + fmt.Errorf( + "waiting for tcloud async operation %s (%s)", + op.GetId(), op.GetState(), + ), + deletePollInterval, + ) } } -func decision( - action deletion.Action, - reason string, - format string, - args ...any, -) deletion.Decision { - return deletion.Decision{ - Action: action, - Reason: reason, - Message: fmt.Sprintf(format, args...), +// deletionPolicy returns the subject's deletion policy from its +// cloud.temporal.io/deletion-policy annotation. An unset annotation means +// delete. Any other value is an error and never falls back to delete. +// +// TODO(Mechack/10/26/2026): once adoption is supported, the default for an adopted +// Namespace becomes retain. An explicit delete is honored either way. +func deletionPolicy(subject types.Resource) (string, error) { + key := tcloudcommon.AnnotationKeyDeletionPolicy + v, ok := annotation.Value(subject, key) + if !ok { + // annotation.Value reports false when more than one key matches + // case-insensitively, so check that the key is really absent. + if annotation.HasKey(subject, key) { + return "", fmt.Errorf("more than one %s annotation", key) + } + return tcloudcommon.DeletionPolicyDelete, nil + } + switch v { + case tcloudcommon.DeletionPolicyDelete, tcloudcommon.DeletionPolicyRetain: + return v, nil + default: + return "", fmt.Errorf( + "invalid %s annotation %q: must be %q or %q", + key, v, + tcloudcommon.DeletionPolicyDelete, tcloudcommon.DeletionPolicyRetain, + ) } } diff --git a/controller/namespace/reconcile.go b/controller/namespace/reconcile.go index 49b3fe6..aa0273e 100644 --- a/controller/namespace/reconcile.go +++ b/controller/namespace/reconcile.go @@ -160,33 +160,6 @@ func (r *Reconciler) ResourceSync( return err } -// 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) - } - _ = tcc - - // TODO: call the Temporal Cloud API to delete the Namespace. - - return err -} - // compile-time check that Reconciler satisfies the [types.Reconciler] // contract. var _ types.Reconciler = (*Reconciler)(nil) diff --git a/deploy/kustomize/crd/bases/cloud.temporal.io_namespaces.yaml b/deploy/kustomize/crd/bases/cloud.temporal.io_namespaces.yaml index dc52975..018ded9 100644 --- a/deploy/kustomize/crd/bases/cloud.temporal.io_namespaces.yaml +++ b/deploy/kustomize/crd/bases/cloud.temporal.io_namespaces.yaml @@ -135,15 +135,6 @@ spec: type: string type: array x-kubernetes-list-type: set - deletionPolicy: - description: |- - DeletionPolicy controls what happens to the Temporal Cloud Namespace - when this resource is deleted. Retain leaves it in Temporal Cloud. - Delete permanently deletes it and all its data. - enum: - - Retain - - Delete - type: string description: description: |- Description is a human-readable description of the Namespace. diff --git a/pkg/deletion/deletion.go b/pkg/deletion/deletion.go deleted file mode 100644 index c9b8898..0000000 --- a/pkg/deletion/deletion.go +++ /dev/null @@ -1,135 +0,0 @@ -// Package deletion provides shared logic for handling deleted custom resources. -// It makes no API calls; each resource uses live Temporal Cloud state to decide what to do. -package deletion - -import ( - "fmt" - "time" - - "github.com/temporalio/cloud-operator/api/cloud/common" -) - -// Action is what to do next with a custom resource that is being deleted. -type Action int - -const ( - // ActionRelease means the finalizer may be removed. Temporal Cloud is - // left as it is. - ActionRelease Action = iota + 1 - // ActionWait means Temporal Cloud is busy with the resource. - ActionWait - // ActionBlock means a person must unblock the delete. - ActionBlock - // ActionFail means the resource can't be deleted; stop until - // the custom resource changes. - ActionFail - // ActionDelete means send the delete request to Temporal Cloud. - ActionDelete -) - -// String returns the Action's name. -func (a Action) String() string { - switch a { - case ActionRelease: - return "Release" - case ActionWait: - return "Wait" - case ActionBlock: - return "Block" - case ActionFail: - return "Fail" - case ActionDelete: - return "Delete" - default: - return fmt.Sprintf("Action(%d)", int(a)) - } -} - -// Decision is what to do next, and why. -type Decision struct { - Action Action - // Reason is a condition reason. - Reason string - // Message explains the decision to the user. - Message string -} - -// Record is what the operator recorded about the Temporal Cloud resource it -// created. -type Record struct { - // ID is the resource's Temporal Cloud ID. Only the create path sets it, so - // a non-empty ID means the operator created the resource. - ID string - // CreatedTime is the creation time Temporal Cloud reported for the - // resource. - CreatedTime *time.Time -} - -// Reasons for the decisions Preflight makes. -const ( - ReasonInvalidDeletionPolicy = "InvalidDeletionPolicy" - ReasonRetained = "Retained" - ReasonNeverCreated = "NeverCreated" -) - -// RetainHint tells the user how to remove a custom resource whose Temporal -// Cloud resource can't be deleted. -const RetainHint = "set spec.deletionPolicy to Retain to remove this resource and leave it in Temporal Cloud" - -// Resolve returns the effective deletion policy -func Resolve(policy, def common.DeletionPolicy) (common.DeletionPolicy, error) { - p := policy - if p == "" { - p = def - } - switch p { - case common.DeletionPolicyRetain, common.DeletionPolicyDelete: - return p, nil - default: - return "", fmt.Errorf("unknown deletion policy %q", p) - } -} - -// Preflight decides from the custom resource alone, without reading Temporal -// Cloud. It returns true when the returned Decision is final. When it returns -// false, the caller reads the resource from Temporal Cloud and decides with -// the resource's own rules. -func Preflight( - policy common.DeletionPolicy, - def common.DeletionPolicy, - rec Record, -) (Decision, bool) { - resolved, err := Resolve(policy, def) - if err != nil { - return Decision{ - Action: ActionFail, - Reason: ReasonInvalidDeletionPolicy, - Message: fmt.Sprintf("%v; set spec.deletionPolicy to Retain or Delete", err), - }, true - } - if resolved == common.DeletionPolicyRetain { - return Decision{ - Action: ActionRelease, - Reason: ReasonRetained, - Message: "deletion policy is Retain; leaving the resource in Temporal Cloud", - }, true - } - if rec.ID == "" { - return Decision{ - Action: ActionRelease, - Reason: ReasonNeverCreated, - Message: "the operator never created this resource in Temporal Cloud; nothing to delete", - }, true - } - return Decision{}, false -} - -// SameSecond reports whether recorded and observed are the same time to the -// second. Recorded times are stored as metav1.Time, which keeps whole seconds -// only. A nil time never matches. -func SameSecond(recorded, observed *time.Time) bool { - if recorded == nil || observed == nil { - return false - } - return recorded.Truncate(time.Second).Equal(observed.Truncate(time.Second)) -} diff --git a/pkg/testutil/temporal/cloud/fake/server/namespace.go b/pkg/testutil/temporal/cloud/fake/server/namespace.go index 95e126a..93f5e4b 100644 --- a/pkg/testutil/temporal/cloud/fake/server/namespace.go +++ b/pkg/testutil/temporal/cloud/fake/server/namespace.go @@ -61,3 +61,69 @@ func (s fakeServer) GetNamespace( } return s.UnimplementedCloudServiceServer.GetNamespace(ctx, req) } + +type DeleteNamespaceFunc func( + context.Context, + *tcloudservicev1.DeleteNamespaceRequest, +) (*tcloudservicev1.DeleteNamespaceResponse, error) + +// WithDeleteNamespace the fake server's DeleteNamespace function +func WithDeleteNamespace(fn DeleteNamespaceFunc) NewOption { + return func(s *fakeServer) { + s.deleteNamespaceFunc = fn + } +} + +// OnDeleteNamespace lets tests validate arguments passed to DeleteNamespace +// and set its response. +func OnDeleteNamespace( + asserter func(*tcloudservicev1.DeleteNamespaceRequest), + returner func() (*tcloudservicev1.DeleteNamespaceResponse, error), +) NewOption { + return func(s *fakeServer) { + fn := func( + ctx context.Context, req *tcloudservicev1.DeleteNamespaceRequest, + ) (*tcloudservicev1.DeleteNamespaceResponse, error) { + if asserter != nil { + asserter(req) + } + if returner == nil { + return &tcloudservicev1.DeleteNamespaceResponse{}, nil + } + resp, err := returner() + if resp == nil && err == nil { + resp = &tcloudservicev1.DeleteNamespaceResponse{} + } + return resp, err + } + s.deleteNamespaceFunc = fn + } +} + +// OnDeleteNamespaceReturn sets the response returned by DeleteNamespace. +func OnDeleteNamespaceReturn( + resp *tcloudservicev1.DeleteNamespaceResponse, + err error, +) NewOption { + if resp == nil && err == nil { + resp = &tcloudservicev1.DeleteNamespaceResponse{} + } + return func(s *fakeServer) { + fn := func( + context.Context, *tcloudservicev1.DeleteNamespaceRequest, + ) (*tcloudservicev1.DeleteNamespaceResponse, error) { + return resp, err + } + s.deleteNamespaceFunc = fn + } +} + +func (s fakeServer) DeleteNamespace( + ctx context.Context, + req *tcloudservicev1.DeleteNamespaceRequest, +) (*tcloudservicev1.DeleteNamespaceResponse, error) { + if s.deleteNamespaceFunc != nil { + return s.deleteNamespaceFunc(ctx, req) + } + return s.UnimplementedCloudServiceServer.DeleteNamespace(ctx, req) +} diff --git a/pkg/testutil/temporal/cloud/fake/server/operation.go b/pkg/testutil/temporal/cloud/fake/server/operation.go new file mode 100644 index 0000000..dd5094e --- /dev/null +++ b/pkg/testutil/temporal/cloud/fake/server/operation.go @@ -0,0 +1,74 @@ +package server + +import ( + "context" + + tcloudservicev1 "go.temporal.io/cloud-sdk/api/cloudservice/v1" +) + +type GetAsyncOperationFunc func( + context.Context, + *tcloudservicev1.GetAsyncOperationRequest, +) (*tcloudservicev1.GetAsyncOperationResponse, error) + +// WithGetAsyncOperation sets the fake server's GetAsyncOperation function +// override. +func WithGetAsyncOperation(fn GetAsyncOperationFunc) NewOption { + return func(s *fakeServer) { + s.getAsyncOperationFunc = fn + } +} + +// OnGetAsyncOperation lets tests validate arguments passed to GetAsyncOperation +// and set its response. +func OnGetAsyncOperation( + asserter func(*tcloudservicev1.GetAsyncOperationRequest), + returner func() (*tcloudservicev1.GetAsyncOperationResponse, error), +) NewOption { + return func(s *fakeServer) { + fn := func( + ctx context.Context, req *tcloudservicev1.GetAsyncOperationRequest, + ) (*tcloudservicev1.GetAsyncOperationResponse, error) { + if asserter != nil { + asserter(req) + } + if returner == nil { + return &tcloudservicev1.GetAsyncOperationResponse{}, nil + } + resp, err := returner() + if resp == nil && err == nil { + resp = &tcloudservicev1.GetAsyncOperationResponse{} + } + return resp, err + } + s.getAsyncOperationFunc = fn + } +} + +// OnGetAsyncOperationReturn sets the response returned by GetAsyncOperation. +func OnGetAsyncOperationReturn( + resp *tcloudservicev1.GetAsyncOperationResponse, + err error, +) NewOption { + if resp == nil && err == nil { + resp = &tcloudservicev1.GetAsyncOperationResponse{} + } + return func(s *fakeServer) { + fn := func( + context.Context, *tcloudservicev1.GetAsyncOperationRequest, + ) (*tcloudservicev1.GetAsyncOperationResponse, error) { + return resp, err + } + s.getAsyncOperationFunc = fn + } +} + +func (s fakeServer) GetAsyncOperation( + ctx context.Context, + req *tcloudservicev1.GetAsyncOperationRequest, +) (*tcloudservicev1.GetAsyncOperationResponse, error) { + if s.getAsyncOperationFunc != nil { + return s.getAsyncOperationFunc(ctx, req) + } + return s.UnimplementedCloudServiceServer.GetAsyncOperation(ctx, req) +} diff --git a/pkg/testutil/temporal/cloud/fake/server/server.go b/pkg/testutil/temporal/cloud/fake/server/server.go index cc57d0f..22b829f 100644 --- a/pkg/testutil/temporal/cloud/fake/server/server.go +++ b/pkg/testutil/temporal/cloud/fake/server/server.go @@ -14,7 +14,9 @@ type fakeServer struct { lis net.Listener tcloudservicev1.UnimplementedCloudServiceServer - getNamespaceFunc GetNamespaceFunc + getNamespaceFunc GetNamespaceFunc + deleteNamespaceFunc DeleteNamespaceFunc + getAsyncOperationFunc GetAsyncOperationFunc } // HostPort returns the address of the TCP localhost port that the fake server