From b86360d3fabfe2b1b5e1b436904aab7315ec3bc1 Mon Sep 17 00:00:00 2001 From: niyomukiza-mechack Date: Sun, 25 Oct 2026 01:48:28 -0700 Subject: [PATCH 1/9] 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 5161f25..195cf66 100644 --- a/api/cloud/v1alpha1/namespace.go +++ b/api/cloud/v1alpha1/namespace.go @@ -139,6 +139,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"` } // FromProto sets the spec from the supplied Temporal Cloud NamespaceSpec. 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 0126b6c624d21c504a79d412edf2ef8f47959ac2 Mon Sep 17 00:00:00 2001 From: niyomukiza-mechack Date: Tue, 27 Oct 2026 00:29:33 -0700 Subject: [PATCH 2/9] 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 | 32 --- .../bases/cloud.temporal.io_namespaces.yaml | 9 - pkg/deletion/deletion.go | 135 ---------- .../temporal/cloud/fake/server/namespace.go | 67 +++++ .../temporal/cloud/fake/server/operation.go | 74 +++++ .../temporal/cloud/fake/server/server.go | 6 +- 10 files changed, 325 insertions(+), 284 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 195cf66..5161f25 100644 --- a/api/cloud/v1alpha1/namespace.go +++ b/api/cloud/v1alpha1/namespace.go @@ -139,16 +139,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"` } // FromProto sets the spec from the supplied Temporal Cloud NamespaceSpec. 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 f6cdc27..5e93810 100644 --- a/controller/namespace/reconcile.go +++ b/controller/namespace/reconcile.go @@ -397,38 +397,6 @@ func tagsDiff( return upsert, remove } -// 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]. -// -// It returns a copy of the supplied [types.Resource] with any status -// mutations applied to the copy. `original` is not mutated. -func (r *Reconciler) ResourceDelete( - ctx context.Context, - original types.Resource, -) (types.Resource, error) { - var err error - log := ctrlreqlog.FromContext(ctx) - exit := log.Trace("resource.delete") - defer func() { - exit(err) - }() - - latest := original.DeepCopy() - - tcc, err := r.CloudClientFor(ctx, original) - if err != nil { - return latest, fmt.Errorf("failed getting tcloud client: %w", err) - } - _ = tcc - - // TODO: call the Temporal Cloud API to delete the Namespace. - - return latest, 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 f18689b..bb181a1 100644 --- a/pkg/testutil/temporal/cloud/fake/server/namespace.go +++ b/pkg/testutil/temporal/cloud/fake/server/namespace.go @@ -117,3 +117,70 @@ func (s fakeServer) GetNamespaces( } return s.UnimplementedCloudServiceServer.GetNamespaces(ctx, req) } + +type DeleteNamespaceFunc func( + context.Context, + *tcloudservicev1.DeleteNamespaceRequest, +) (*tcloudservicev1.DeleteNamespaceResponse, error) + +// WithDeleteNamespace sets the fake server's DeleteNamespace function +// override. +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) +} \ No newline at end of file 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 d485185..94a864d 100644 --- a/pkg/testutil/temporal/cloud/fake/server/server.go +++ b/pkg/testutil/temporal/cloud/fake/server/server.go @@ -14,8 +14,10 @@ type fakeServer struct { lis net.Listener tcloudservicev1.UnimplementedCloudServiceServer - getNamespaceFunc GetNamespaceFunc - getNamespacesFunc GetNamespacesFunc + getNamespaceFunc GetNamespaceFunc + getNamespacesFunc GetNamespacesFunc + deleteNamespaceFunc DeleteNamespaceFunc + getAsyncOperationFunc GetAsyncOperationFunc } // HostPort returns the address of the TCP localhost port that the fake server From e1109dc04b35c914c4352be3bc4324f04fdb6fa3 Mon Sep 17 00:00:00 2001 From: niyomukiza-mechack Date: Wed, 28 Oct 2026 09:16:46 -0700 Subject: [PATCH 3/9] Address review: use kube-temporal v0.6.0, deletion policy type, delete tests --- api/cloud/common/const.go | 37 ++- controller/namespace/delete.go | 132 ++++---- controller/namespace/delete_test.go | 469 ++++++++++++++++++++++++++++ 3 files changed, 570 insertions(+), 68 deletions(-) create mode 100644 controller/namespace/delete_test.go diff --git a/api/cloud/common/const.go b/api/cloud/common/const.go index dcf0eff..aa4d898 100644 --- a/api/cloud/common/const.go +++ b/api/cloud/common/const.go @@ -1,14 +1,47 @@ package common +import ( + "fmt" + "strings" +) + const ( AnnotationKeyPrefixTemporalCloud = "cloud.temporal.io/" AnnotationKeyTemporalCloudHostPort = AnnotationKeyPrefixTemporalCloud + "hostport" + // AnnotationKeyDeletionPolicy controls what happens to the Temporal Cloud + // resource when the custom resource that manages it is deleted. Its value + // is a DeletionPolicy. AnnotationKeyDeletionPolicy = AnnotationKeyPrefixTemporalCloud + "deletion-policy" + + // ReasonResourceDeleteFailed is the Stalled condition reason when Temporal + // Cloud failed to delete the backend resource. + ReasonResourceDeleteFailed = "ResourceDeleteFailed" +) + +// DeletionPolicy describes the expected behaviour of the controller when +// deleting a Temporal Cloud resource associated with a CR. +type DeletionPolicy string + +const ( // DeletionPolicyDelete deletes the Temporal Cloud resource, and removes // the custom resource once Temporal Cloud has finished deleting it. - DeletionPolicyDelete = "delete" + DeletionPolicyDelete DeletionPolicy = "delete" // DeletionPolicyRetain leaves the Temporal Cloud resource in place and // only removes the custom resource. - DeletionPolicyRetain = "retain" + DeletionPolicyRetain DeletionPolicy = "retain" ) + +// Validate returns an error if the DeletionPolicy is not valid. +func (p DeletionPolicy) Validate() error { + switch p { + case DeletionPolicyRetain, DeletionPolicyDelete: + return nil + default: + allowed := strings.Join( + []string{string(DeletionPolicyRetain), string(DeletionPolicyDelete)}, + ", ", + ) + return fmt.Errorf("invalid deletion policy %q. allowed values: %s", p, allowed) + } +} diff --git a/controller/namespace/delete.go b/controller/namespace/delete.go index ef6fa77..211c1a9 100644 --- a/controller/namespace/delete.go +++ b/controller/namespace/delete.go @@ -4,7 +4,6 @@ import ( "context" "errors" "fmt" - "time" ctrlreqlog "github.com/temporalio/kube-temporal/pkg/controller/request/log" "github.com/temporalio/kube-temporal/pkg/controller/requeue" @@ -19,15 +18,7 @@ import ( tcloudcommon "github.com/temporalio/cloud-operator/api/cloud/common" tcloudv1alpha1 "github.com/temporalio/cloud-operator/api/cloud/v1alpha1" -) - -const ( - // ReasonResourceDeleteFailed is the Stalled condition reason when Temporal - // Cloud failed to delete the Namespace. - ReasonResourceDeleteFailed = "ResourceDeleteFailed" - - // deletePollInterval is how often to check on a delete in progress. - deletePollInterval = 15 * time.Second + ctrlerrors "github.com/temporalio/kube-temporal/pkg/controller/errors" ) // ResourceDelete deletes the Temporal Cloud Namespace for the supplied @@ -35,16 +26,17 @@ const ( // the Namespace is gone, and an error while the delete is in progress or has // failed. // -// 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. +// It works on a copy of `original` and returns that copy, with any status +// changes (State, AsyncOperationID, conditions), as the latest state. +// `original` is never mutated. // -// TODO(Mechack): status.id and status.resourceVersion are set by the create path. +// TODO(Mechack): status.id and status.resourceVersion are only set once the +// create path is merged. Until then they're empty, so ResourceDelete treats +// every Namespace as never created. Delete this TODO when it lands. func (r *Reconciler) ResourceDelete( ctx context.Context, - desired types.Resource, -) error { + original types.Resource, +) (types.Resource, error) { var err error log := ctrlreqlog.FromContext(ctx) exit := log.Trace("resource.delete") @@ -52,40 +44,61 @@ func (r *Reconciler) ResourceDelete( exit(err) }() - policy, err := deletionPolicy(desired) + latest := original.DeepCopy() + cr := latest.CR().(*tcloudv1alpha1.Namespace) + if cr.Status.ID == "" { + // The user deleted the CR before the operator created a Namespace in + // Temporal Cloud, so there's nothing to delete. + return latest, nil + } + + policy, err := deletionPolicy(latest) if err != nil { - return err + return latest, err } if policy == tcloudcommon.DeletionPolicyRetain { - return nil + // TODO(Mechack): log that the Namespace is retained because of the + // retain deletion policy, once ctrlreqlog's log.Info no longer panics + // (ToContext never stores the request logger). + return latest, 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) + tcc, err := r.CloudClientFor(ctx, latest) if err != nil { err = fmt.Errorf("failed getting tcloud client: %w", err) - return err + return latest, err } // We may have already called DeleteNamespace for this resource. If so, // check whether the delete operation has finished. switch { case cr.Status.State == tcloudcommon.ResourceStateDeleted: - return nil + return latest, nil + case cr.Status.State == tcloudcommon.ResourceStateDeleteFailed: + // A previous delete failed. Don't send another one automatically. + err = ctrlerrors.NewTerminalError(fmt.Errorf( + "deleting Temporal Cloud namespace %s previously failed (async operation %s); not retrying", + cr.Status.ID, cr.Status.AsyncOperationID, + )) + return latest, err case cr.Status.State == tcloudcommon.ResourceStateDeleting && cr.Status.AsyncOperationID != "": - err = checkDelete(ctx, tcc, desired, cr) - return err + err = checkDelete(ctx, tcc, latest, cr) + return latest, err + case cr.Status.State == tcloudcommon.ResourceStateActivating, + cr.Status.State == tcloudcommon.ResourceStateUpdating: + // A create or update on this custom resource is still in flight, and Temporal Cloud rejects a + // delete until the Namespace is Active. Check again later. + err = requeue.NeededAfter( + fmt.Errorf( + "waiting for tcloud namespace %s to finish %s before deleting", + cr.Status.ID, cr.Status.State, + ), + requeue.DefaultRequeueAfterDuration, + ) + return latest, err } - // 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, @@ -93,16 +106,16 @@ func (r *Reconciler) ResourceDelete( if status.Code(err) == codes.NotFound { // Already gone. err = nil - return nil + return latest, 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, + latest, metav1.ConditionTrue, err.Error(), tcloudcommon.ReasonResourceDeleteFailed, ) - return err + return latest, err } cr.Status.State = tcloudcommon.ResourceStateDeleting @@ -112,19 +125,19 @@ func (r *Reconciler) ResourceDelete( "deleting tcloud namespace %s (async operation %s)", cr.Status.ID, cr.Status.AsyncOperationID, ), - deletePollInterval, + requeue.DefaultRequeueAfterDuration, ) - return err + return latest, 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. +// error while it is running, and an error with the Stalled condition set on +// latest if it failed. func checkDelete( ctx context.Context, tcc tcloudservicev1.CloudServiceClient, - desired types.Resource, + latest types.Resource, cr *tcloudv1alpha1.Namespace, ) error { resp, err := tcc.GetAsyncOperation(ctx, &tcloudservicev1.GetAsyncOperationRequest{ @@ -148,46 +161,33 @@ func checkDelete( cr.Status.ID, op.GetId(), op.GetState(), op.GetFailureReason(), ) condition.SetStalled( - desired, metav1.ConditionTrue, msg, ReasonResourceDeleteFailed, + latest, metav1.ConditionTrue, msg, tcloudcommon.ReasonResourceDeleteFailed, ) - return errors.New(msg) + cr.Status.State = tcloudcommon.ResourceStateDeleteFailed + return ctrlerrors.NewTerminalError(errors.New(msg)) default: - // Pending. + // Pending, in progress, or a state this SDK doesn't know. return requeue.NeededAfter( fmt.Errorf( "waiting for tcloud async operation %s (%s)", op.GetId(), op.GetState(), ), - deletePollInterval, + requeue.DefaultRequeueAfterDuration, ) } } // 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. +// delete. An invalid value is an error and never falls back to delete. // -// TODO(Mechack/10/26/2026): once adoption is supported, the default for an adopted +// TODO(Mechack): 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) +func deletionPolicy(subject types.Resource) (tcloudcommon.DeletionPolicy, error) { + v, ok := annotation.Value(subject, tcloudcommon.AnnotationKeyDeletionPolicy) 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, - ) - } + p := tcloudcommon.DeletionPolicy(v) + return p, p.Validate() } diff --git a/controller/namespace/delete_test.go b/controller/namespace/delete_test.go new file mode 100644 index 0000000..2c3f0f0 --- /dev/null +++ b/controller/namespace/delete_test.go @@ -0,0 +1,469 @@ +package namespace_test + +import ( + "context" + "errors" + "sync" + "testing" + + "github.com/stretchr/testify/require" + ctrlerrors "github.com/temporalio/kube-temporal/pkg/controller/errors" + "github.com/temporalio/kube-temporal/pkg/controller/requeue" + "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" + tcclient "go.temporal.io/cloud-sdk/cloudclient" + "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" + "github.com/temporalio/cloud-operator/controller/namespace" + "github.com/temporalio/cloud-operator/pkg/testutil/kube" + fakeserver "github.com/temporalio/cloud-operator/pkg/testutil/temporal/cloud/fake/server" +) + +const ( + deleteTestName = "tmprl-ns" + deleteTestID = "tmprl-ns.acct" + deleteTestRV = "rv-1" + deleteTestAsyncOpID = "async-op-1" +) + +var ( + statusActive = tcloudcommon.ResourceStatus{ + ID: deleteTestID, + ResourceVersion: deleteTestRV, + State: tcloudcommon.ResourceStateActive, + } + + statusDeleting = tcloudcommon.ResourceStatus{ + ID: deleteTestID, + ResourceVersion: deleteTestRV, + State: tcloudcommon.ResourceStateDeleting, + AsyncOperationID: deleteTestAsyncOpID, + } + + statusDeleteFailed = tcloudcommon.ResourceStatus{ + ID: deleteTestID, + ResourceVersion: deleteTestRV, + State: tcloudcommon.ResourceStateDeleteFailed, + AsyncOperationID: deleteTestAsyncOpID, + } + + deleteAccepted = &tcloudservicev1.DeleteNamespaceResponse{ + AsyncOperation: &operationv1.AsyncOperation{Id: deleteTestAsyncOpID}, + } +) + +// deleteCase describes one call to ResourceDelete: the resource passed in, +// what the fake Temporal Cloud server returns, and what's expected back. +type deleteCase struct { + name string + // policy is the deletion-policy annotation value. Empty means no + // annotation. + policy string + status tcloudcommon.ResourceStatus + + deleteResp *tcloudservicev1.DeleteNamespaceResponse + deleteErr error + asyncOp *operationv1.AsyncOperation + + // expDelete is whether DeleteNamespace is called (once) with the status + // ID and resource version. + expDelete bool + // expGetAsyncOp is whether GetAsyncOperation is called (once) with the + // status async operation ID. + expGetAsyncOp bool + expErr string + expRequeue bool + // expTerminal is whether the error is a terminal error. + expTerminal bool + // expStatus is the returned resource's Temporal Cloud status. Nil means + // unchanged from status. + expStatus *tcloudcommon.ResourceStatus + // expStalled is a substring of the Stalled=True condition message. Empty + // means no Stalled condition. + expStalled string +} + +func TestReconciler_ResourceDelete_DeletionPolicy(t *testing.T) { + cases := []deleteCase{ + { + name: "retain makes no cloud calls", + policy: "retain", + status: statusActive, + }, + { + name: "no annotation defaults to delete", + status: statusActive, + deleteResp: deleteAccepted, + expDelete: true, + expRequeue: true, + expStatus: &statusDeleting, + }, + { + name: "delete calls DeleteNamespace", + policy: "delete", + status: statusActive, + deleteResp: deleteAccepted, + expDelete: true, + expRequeue: true, + expStatus: &statusDeleting, + }, + { + name: "invalid policy errors without cloud calls", + policy: "destroy", + status: statusActive, + expErr: `invalid deletion policy "destroy"`, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(tt *testing.T) { + runDeleteCase(tt, tc) + }) + } +} + +func TestReconciler_ResourceDelete_NoStatusID(t *testing.T) { + cases := []deleteCase{ + {name: "delete policy makes no cloud calls", policy: "delete"}, + {name: "retain policy makes no cloud calls", policy: "retain"}, + {name: "invalid policy makes no cloud calls", policy: "destroy"}, + } + for _, tc := range cases { + t.Run(tc.name, func(tt *testing.T) { + runDeleteCase(tt, tc) + }) + } +} + +func TestReconciler_ResourceDelete_SendDelete(t *testing.T) { + cases := []deleteCase{ + { + name: "success sets deleting and requeues", + status: statusActive, + deleteResp: deleteAccepted, + expDelete: true, + expRequeue: true, + expStatus: &statusDeleting, + }, + { + name: "NotFound means already gone", + status: statusActive, + deleteErr: status.Error(codes.NotFound, "namespace not found"), + expDelete: true, + }, + { + name: "FailedPrecondition sets stalled", + status: statusActive, + deleteErr: status.Error( + codes.FailedPrecondition, "delete protection is enabled", + ), + expDelete: true, + expErr: "delete protection is enabled", + expStalled: "delete protection is enabled", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(tt *testing.T) { + runDeleteCase(tt, tc) + }) + } +} + +func TestReconciler_ResourceDelete_DeleteInProgress(t *testing.T) { + cases := []deleteCase{ + { + name: "operation fulfilled", + status: statusDeleting, + asyncOp: asyncOperation(operationv1.AsyncOperation_STATE_FULFILLED, ""), + expGetAsyncOp: true, + }, + { + name: "operation pending requeues", + status: statusDeleting, + asyncOp: asyncOperation(operationv1.AsyncOperation_STATE_PENDING, ""), + expGetAsyncOp: true, + expRequeue: true, + }, + { + name: "operation failed sets delete failed and stalled", + status: statusDeleting, + asyncOp: asyncOperation( + operationv1.AsyncOperation_STATE_FAILED, "nexus endpoints exist", + ), + expGetAsyncOp: true, + expErr: "nexus endpoints exist", + expTerminal: true, + expStatus: &statusDeleteFailed, + expStalled: "nexus endpoints exist", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(tt *testing.T) { + runDeleteCase(tt, tc) + }) + } +} + +func TestReconciler_ResourceDelete_DeleteFailed(t *testing.T) { + runDeleteCase(t, deleteCase{ + name: "delete failed is not retried", + status: statusDeleteFailed, + expErr: "previously failed", + expTerminal: true, + }) +} + +func TestReconciler_ResourceDelete_InFlightStates(t *testing.T) { + cases := []deleteCase{ + { + name: "activating requeues without cloud calls", + status: statusWithState(tcloudcommon.ResourceStateActivating), + expRequeue: true, + }, + { + name: "updating requeues without cloud calls", + status: statusWithState(tcloudcommon.ResourceStateUpdating), + expRequeue: true, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(tt *testing.T) { + runDeleteCase(tt, tc) + }) + } +} + +// runDeleteCase calls ResourceDelete for tc against a fake Temporal Cloud +// server and checks the returned resource, the error, and the Cloud calls +// made. +func runDeleteCase(t *testing.T, tc deleteCase) { + t.Helper() + require := require.New(t) + + tns := newDeleteTestNamespace(tc.policy, tc.status) + cloud := startDeleteFakeCloud(t, tc) + r := newDeleteTestReconciler(cloud, tns) + + original := namespace.NewResource(tns.DeepCopy()) + before := original.DeepCopy() + + got, err := r.ResourceDelete(context.TODO(), original) + + requireNotMutated(t, before, original) + require.NotNil(got) + requireDeleteError(t, tc, err) + + gotCR := got.CR().(*tcloudv1alpha1.Namespace) + expStatus := tc.status + if tc.expStatus != nil { + expStatus = *tc.expStatus + } + require.Equal(expStatus, gotCR.Status.ResourceStatus) + + requireStalled(t, got, tc.expStalled) + cloud.requireCalls(t, tc.expDelete, tc.expGetAsyncOp) +} + +// newDeleteTestNamespace returns a Namespace CR with the supplied +// deletion-policy annotation (none if empty) and Temporal Cloud status. +func newDeleteTestNamespace( + policy string, + rs tcloudcommon.ResourceStatus, +) *tcloudv1alpha1.Namespace { + tns := &tcloudv1alpha1.Namespace{} + tns.Name = deleteTestName + tns.Namespace = kubeNamespace + if policy != "" { + tns.Annotations = map[string]string{ + tcloudcommon.AnnotationKeyDeletionPolicy: policy, + } + } + tns.Status.ResourceStatus = rs + return tns +} + +// statusWithState returns statusActive with its state replaced. +func statusWithState(s tcloudcommon.ResourceState) tcloudcommon.ResourceStatus { + rs := statusActive + rs.State = s + return rs +} + +// asyncOperation returns the test async operation in the supplied state. +func asyncOperation( + state operationv1.AsyncOperation_State, + failureReason string, +) *operationv1.AsyncOperation { + return &operationv1.AsyncOperation{ + Id: deleteTestAsyncOpID, + State: state, + FailureReason: failureReason, + } +} + +// deleteFakeCloud is a started fake Temporal Cloud server that records the +// requests it receives. Its handlers run on the server goroutine, so they only +// record under the mutex, and the test goroutine asserts. +type deleteFakeCloud struct { + client *tcclient.Client + + mu sync.Mutex + getNamespace int + deleteNamespace []*tcloudservicev1.DeleteNamespaceRequest + getAsyncOperation []*tcloudservicev1.GetAsyncOperationRequest +} + +// startDeleteFakeCloud starts a fake Temporal Cloud server that returns tc's +// DeleteNamespace and GetAsyncOperation responses, and stops it when the test +// ends. +func startDeleteFakeCloud(t *testing.T, tc deleteCase) *deleteFakeCloud { + t.Helper() + require := require.New(t) + + c := &deleteFakeCloud{} + srv, err := fakeserver.New( + fakeserver.WithGetNamespace(func( + context.Context, *tcloudservicev1.GetNamespaceRequest, + ) (*tcloudservicev1.GetNamespaceResponse, error) { + c.mu.Lock() + defer c.mu.Unlock() + c.getNamespace++ + return &tcloudservicev1.GetNamespaceResponse{}, nil + }), + fakeserver.WithDeleteNamespace(func( + _ context.Context, req *tcloudservicev1.DeleteNamespaceRequest, + ) (*tcloudservicev1.DeleteNamespaceResponse, error) { + c.mu.Lock() + defer c.mu.Unlock() + c.deleteNamespace = append(c.deleteNamespace, req) + if tc.deleteErr != nil { + return nil, tc.deleteErr + } + if tc.deleteResp == nil { + return &tcloudservicev1.DeleteNamespaceResponse{}, nil + } + return tc.deleteResp, nil + }), + fakeserver.WithGetAsyncOperation(func( + _ context.Context, req *tcloudservicev1.GetAsyncOperationRequest, + ) (*tcloudservicev1.GetAsyncOperationResponse, error) { + c.mu.Lock() + defer c.mu.Unlock() + c.getAsyncOperation = append(c.getAsyncOperation, req) + return &tcloudservicev1.GetAsyncOperationResponse{ + AsyncOperation: tc.asyncOp, + }, nil + }), + ) + require.Nil(err) + stop := srv.Start() + t.Cleanup(stop) + + c.client, err = srv.Client() + require.Nil(err) + return c +} + +// requireCalls checks which Temporal Cloud calls the server received. +// GetNamespace is never expected. +func (c *deleteFakeCloud) requireCalls( + t *testing.T, + expDelete bool, + expGetAsyncOp bool, +) { + t.Helper() + require := require.New(t) + + c.mu.Lock() + defer c.mu.Unlock() + require.Zero(c.getNamespace) + if expDelete { + require.Len(c.deleteNamespace, 1) + require.Equal(deleteTestID, c.deleteNamespace[0].GetNamespace()) + require.Equal(deleteTestRV, c.deleteNamespace[0].GetResourceVersion()) + } else { + require.Empty(c.deleteNamespace) + } + if expGetAsyncOp { + require.Len(c.getAsyncOperation, 1) + require.Equal( + deleteTestAsyncOpID, c.getAsyncOperation[0].GetAsyncOperationId(), + ) + } else { + require.Empty(c.getAsyncOperation) + } +} + +// newDeleteTestReconciler returns a Namespace Reconciler that talks to the +// fake Temporal Cloud server and a fake kube client holding tns. +func newDeleteTestReconciler( + cloud *deleteFakeCloud, + tns *tcloudv1alpha1.Namespace, +) *namespace.Reconciler { + return namespace.NewReconciler( + namespace.WithDefaultCloudClient(*cloud.client), + namespace.WithKubeClient(kube.NewFakeClient(tns)), + ) +} + +// requireNotMutated checks that ResourceDelete left the resource passed in +// unchanged. +func requireNotMutated(t *testing.T, before, original types.Resource) { + t.Helper() + require.Equal(t, before, original, "ResourceDelete mutated original") +} + +// requireDeleteError checks err against tc's expected requeue, terminal or +// plain error, or no error. +func requireDeleteError(t *testing.T, tc deleteCase, err error) { + t.Helper() + require := require.New(t) + + var requeueAfter *requeue.RequeueNeededAfter + switch { + case tc.expRequeue: + require.True( + errors.As(err, &requeueAfter), + "expected requeue error, got %v", err, + ) + require.Equal( + requeue.DefaultRequeueAfterDuration, requeueAfter.Duration(), + ) + case tc.expErr != "": + require.ErrorContains(err, tc.expErr) + require.False( + errors.As(err, &requeueAfter), + "expected non-requeue error, got %v", err, + ) + var terminal *ctrlerrors.TerminalError + require.Equal( + tc.expTerminal, errors.As(err, &terminal), + "terminal error mismatch, got %T: %v", err, err, + ) + default: + require.NoError(err) + } +} + +// requireStalled checks the resource's Stalled condition. An empty msg means +// no Stalled condition is expected; otherwise Stalled is True with the delete +// failed reason and a message containing msg. +func requireStalled(t *testing.T, got types.Resource, msg string) { + t.Helper() + require := require.New(t) + + stalled := condition.Stalled(got) + if msg == "" { + require.Nil(stalled) + return + } + require.NotNil(stalled) + require.Equal(metav1.ConditionTrue, stalled.Status) + require.Equal(tcloudcommon.ReasonResourceDeleteFailed, stalled.Reason) + require.Contains(stalled.Message, msg) +} From 63d97ba6066afe598be55b05ca8636ffe6715516 Mon Sep 17 00:00:00 2001 From: niyomukiza-mechack Date: Wed, 28 Oct 2026 10:03:24 -0700 Subject: [PATCH 4/9] Clean up delete.go comments and imports --- controller/namespace/delete.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/controller/namespace/delete.go b/controller/namespace/delete.go index 211c1a9..75da207 100644 --- a/controller/namespace/delete.go +++ b/controller/namespace/delete.go @@ -29,10 +29,7 @@ import ( // It works on a copy of `original` and returns that copy, with any status // changes (State, AsyncOperationID, conditions), as the latest state. // `original` is never mutated. -// -// TODO(Mechack): status.id and status.resourceVersion are only set once the -// create path is merged. Until then they're empty, so ResourceDelete treats -// every Namespace as never created. Delete this TODO when it lands. + func (r *Reconciler) ResourceDelete( ctx context.Context, original types.Resource, From d10f12f66c694992e829c43cc9aff6c9f350deb9 Mon Sep 17 00:00:00 2001 From: niyomukiza-mechack Date: Wed, 28 Oct 2026 10:17:44 -0700 Subject: [PATCH 5/9] gofmt fake server namespace.go --- pkg/testutil/temporal/cloud/fake/server/namespace.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/testutil/temporal/cloud/fake/server/namespace.go b/pkg/testutil/temporal/cloud/fake/server/namespace.go index bb181a1..2ee2a3f 100644 --- a/pkg/testutil/temporal/cloud/fake/server/namespace.go +++ b/pkg/testutil/temporal/cloud/fake/server/namespace.go @@ -183,4 +183,4 @@ func (s fakeServer) DeleteNamespace( return s.deleteNamespaceFunc(ctx, req) } return s.UnimplementedCloudServiceServer.DeleteNamespace(ctx, req) -} \ No newline at end of file +} From 94d2b66dc7a23f4ecc4fb212083ca5c8e1e590b3 Mon Sep 17 00:00:00 2001 From: niyomukiza-mechack Date: Wed, 28 Oct 2026 17:08:33 -0700 Subject: [PATCH 6/9] Surface invalid deletion policy as Stalled and requeue; accept any case --- controller/namespace/delete.go | 14 ++++++++++++++ controller/namespace/delete_test.go | 21 ++++++++++++++++++--- 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/controller/namespace/delete.go b/controller/namespace/delete.go index 75da207..6990e46 100644 --- a/controller/namespace/delete.go +++ b/controller/namespace/delete.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "strings" ctrlreqlog "github.com/temporalio/kube-temporal/pkg/controller/request/log" "github.com/temporalio/kube-temporal/pkg/controller/requeue" @@ -51,6 +52,17 @@ func (r *Reconciler) ResourceDelete( policy, err := deletionPolicy(latest) if err != nil { + // Never fall back to delete on an invalid value. Annotation edits + // don't trigger a reconcile, so requeue to pick up a corrected value. + msg := fmt.Sprintf( + "%s. Set the %s annotation to %q or %q", + err, tcloudcommon.AnnotationKeyDeletionPolicy, + tcloudcommon.DeletionPolicyRetain, tcloudcommon.DeletionPolicyDelete, + ) + condition.SetStalled( + latest, metav1.ConditionTrue, msg, tcloudcommon.ReasonResourceDeleteFailed, + ) + err = requeue.NeededAfter(errors.New(msg), requeue.DefaultRequeueAfterDuration) return latest, err } if policy == tcloudcommon.DeletionPolicyRetain { @@ -185,6 +197,8 @@ func deletionPolicy(subject types.Resource) (tcloudcommon.DeletionPolicy, error) if !ok { return tcloudcommon.DeletionPolicyDelete, nil } + + v = strings.ToLower(strings.TrimSpace(v)) p := tcloudcommon.DeletionPolicy(v) return p, p.Validate() } diff --git a/controller/namespace/delete_test.go b/controller/namespace/delete_test.go index 2c3f0f0..d16ef7a 100644 --- a/controller/namespace/delete_test.go +++ b/controller/namespace/delete_test.go @@ -114,10 +114,22 @@ func TestReconciler_ResourceDelete_DeletionPolicy(t *testing.T) { expStatus: &statusDeleting, }, { - name: "invalid policy errors without cloud calls", - policy: "destroy", + name: "capitalized retain makes no cloud calls", + policy: "Retain", status: statusActive, - expErr: `invalid deletion policy "destroy"`, + }, + { + name: "retain with surrounding whitespace makes no cloud calls", + policy: " retain ", + status: statusActive, + }, + { + name: "invalid policy sets stalled and requeues without cloud calls", + policy: "destroy", + status: statusActive, + expErr: `invalid deletion policy "destroy"`, + expRequeue: true, + expStalled: `invalid deletion policy "destroy"`, }, } for _, tc := range cases { @@ -434,6 +446,9 @@ func requireDeleteError(t *testing.T, tc deleteCase, err error) { require.Equal( requeue.DefaultRequeueAfterDuration, requeueAfter.Duration(), ) + if tc.expErr != "" { + require.ErrorContains(err, tc.expErr) + } case tc.expErr != "": require.ErrorContains(err, tc.expErr) require.False( From 48bf442a52c674d401f26428648d56af9084de82 Mon Sep 17 00:00:00 2001 From: niyomukiza-mechack Date: Wed, 28 Oct 2026 20:21:01 -0700 Subject: [PATCH 7/9] Block delete when deletion protection is enabled --- controller/namespace/delete.go | 18 +++++++++++ controller/namespace/delete_test.go | 46 +++++++++++++++++++++++++++-- 2 files changed, 62 insertions(+), 2 deletions(-) diff --git a/controller/namespace/delete.go b/controller/namespace/delete.go index 6990e46..41e1789 100644 --- a/controller/namespace/delete.go +++ b/controller/namespace/delete.go @@ -108,6 +108,24 @@ func (r *Reconciler) ResourceDelete( return latest, err } + // Temporal Cloud rejects deleting a protected Namespace, so no need to send the + // delete request. Changing the spec triggers a reconcile, but switching the + // annotation to retain doesn't, so requeue to pick that up. + if cr.Spec.Lifecycle != nil && cr.Spec.Lifecycle.EnableDeleteProtection { + msg := fmt.Sprintf( + "tcloud namespace %s has deletion protection enabled; set "+ + "spec.lifecycle.enableDeleteProtection to false to delete it, "+ + "or set the %s annotation to %q to keep it", + cr.Status.ID, tcloudcommon.AnnotationKeyDeletionPolicy, + tcloudcommon.DeletionPolicyRetain, + ) + condition.SetStalled( + latest, metav1.ConditionTrue, msg, tcloudcommon.ReasonResourceDeleteFailed, + ) + err = requeue.NeededAfter(errors.New(msg), requeue.DefaultRequeueAfterDuration) + return latest, err + } + resp, err := tcc.DeleteNamespace(ctx, &tcloudservicev1.DeleteNamespaceRequest{ Namespace: cr.Status.ID, ResourceVersion: cr.Status.ResourceVersion, diff --git a/controller/namespace/delete_test.go b/controller/namespace/delete_test.go index d16ef7a..efca613 100644 --- a/controller/namespace/delete_test.go +++ b/controller/namespace/delete_test.go @@ -66,6 +66,8 @@ type deleteCase struct { // annotation. policy string status tcloudcommon.ResourceStatus + // protected sets spec.lifecycle.enableDeleteProtection. + protected bool deleteResp *tcloudservicev1.DeleteNamespaceResponse deleteErr error @@ -230,6 +232,39 @@ func TestReconciler_ResourceDelete_DeleteFailed(t *testing.T) { }) } +func TestReconciler_ResourceDelete_DeleteProtection(t *testing.T) { + cases := []deleteCase{ + { + name: "delete policy sets stalled and requeues without cloud calls", + policy: "delete", + status: statusActive, + protected: true, + expErr: "enableDeleteProtection", + expRequeue: true, + expStalled: "enableDeleteProtection", + }, + { + name: "retain policy makes no cloud calls", + policy: "retain", + status: statusActive, + protected: true, + }, + { + name: "delete in progress is still polled", + status: statusDeleting, + protected: true, + asyncOp: asyncOperation(operationv1.AsyncOperation_STATE_PENDING, ""), + expGetAsyncOp: true, + expRequeue: true, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(tt *testing.T) { + runDeleteCase(tt, tc) + }) + } +} + func TestReconciler_ResourceDelete_InFlightStates(t *testing.T) { cases := []deleteCase{ { @@ -257,7 +292,7 @@ func runDeleteCase(t *testing.T, tc deleteCase) { t.Helper() require := require.New(t) - tns := newDeleteTestNamespace(tc.policy, tc.status) + tns := newDeleteTestNamespace(tc.policy, tc.status, tc.protected) cloud := startDeleteFakeCloud(t, tc) r := newDeleteTestReconciler(cloud, tns) @@ -282,10 +317,12 @@ func runDeleteCase(t *testing.T, tc deleteCase) { } // newDeleteTestNamespace returns a Namespace CR with the supplied -// deletion-policy annotation (none if empty) and Temporal Cloud status. +// deletion-policy annotation (none if empty), Temporal Cloud status and +// deletion protection. func newDeleteTestNamespace( policy string, rs tcloudcommon.ResourceStatus, + protected bool, ) *tcloudv1alpha1.Namespace { tns := &tcloudv1alpha1.Namespace{} tns.Name = deleteTestName @@ -296,6 +333,11 @@ func newDeleteTestNamespace( } } tns.Status.ResourceStatus = rs + if protected { + tns.Spec.Lifecycle = &tcloudcommon.LifecycleSpec{ + EnableDeleteProtection: true, + } + } return tns } From 02c02d3078573b7b733cabba6fe45a25eb8c7afd Mon Sep 17 00:00:00 2001 From: niyomukiza-mechack Date: Wed, 28 Oct 2026 20:58:10 -0700 Subject: [PATCH 8/9] Turn off deletion protection in Temporal Cloud before deleting --- controller/namespace/delete.go | 57 ++++- controller/namespace/delete_test.go | 228 ++++++++++++++---- .../temporal/cloud/fake/server/namespace.go | 67 +++++ .../temporal/cloud/fake/server/server.go | 1 + 4 files changed, 301 insertions(+), 52 deletions(-) diff --git a/controller/namespace/delete.go b/controller/namespace/delete.go index 41e1789..a587441 100644 --- a/controller/namespace/delete.go +++ b/controller/namespace/delete.go @@ -12,9 +12,11 @@ import ( "github.com/temporalio/kube-temporal/pkg/kube/condition" "github.com/temporalio/kube-temporal/pkg/types" tcloudservicev1 "go.temporal.io/cloud-sdk/api/cloudservice/v1" + tcloudnamespacev1 "go.temporal.io/cloud-sdk/api/namespace/v1" operationv1 "go.temporal.io/cloud-sdk/api/operation/v1" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" + "google.golang.org/protobuf/proto" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" tcloudcommon "github.com/temporalio/cloud-operator/api/cloud/common" @@ -126,9 +128,32 @@ func (r *Reconciler) ResourceDelete( return latest, err } + // Read the namespace so deletion protection in Temporal Cloud can be made + // to match the CR, and so the delete uses the current resource version. + getResp, err := tcc.GetNamespace(ctx, &tcloudservicev1.GetNamespaceRequest{ + Namespace: cr.Status.ID, + }) + if status.Code(err) == codes.NotFound { + // Already gone. + err = nil + return latest, nil + } + if err != nil { + err = fmt.Errorf("failed getting tcloud namespace %s: %w", cr.Status.ID, err) + return latest, err + } + tns := getResp.GetNamespace() + + if tns.GetSpec().GetLifecycle().GetEnableDeleteProtection() { + // The CR's spec says protection is off, so make + // Temporal Cloud match it before deleting. + err = disableDeleteProtection(ctx, tcc, tns) + return latest, err + } + resp, err := tcc.DeleteNamespace(ctx, &tcloudservicev1.DeleteNamespaceRequest{ Namespace: cr.Status.ID, - ResourceVersion: cr.Status.ResourceVersion, + ResourceVersion: tns.GetResourceVersion(), }) if status.Code(err) == codes.NotFound { // Already gone. @@ -157,6 +182,36 @@ func (r *Reconciler) ResourceDelete( return latest, err } +// disableDeleteProtection turns off deletion protection on the +// Temporal Cloud Namespace. It sends the Namespace's current spec from +// Temporal Cloud with only that setting changed, so other pending changes to +// the CR's spec aren't pushed right before a delete. On success it returns a +// requeue error, and the next reconcile re-reads the Namespace rather than +// tracking the update's async operation. +func disableDeleteProtection( + ctx context.Context, + tcc tcloudservicev1.CloudServiceClient, + tns *tcloudnamespacev1.Namespace, +) error { + spec := proto.Clone(tns.GetSpec()).(*tcloudnamespacev1.NamespaceSpec) + spec.GetLifecycle().EnableDeleteProtection = false + _, err := tcc.UpdateNamespace(ctx, &tcloudservicev1.UpdateNamespaceRequest{ + Namespace: tns.GetNamespace(), + Spec: spec, + ResourceVersion: tns.GetResourceVersion(), + }) + if err != nil { + return cloudCallError("turning off tcloud namespace deletion protection", err) + } + return requeue.NeededAfter( + fmt.Errorf( + "turned off deletion protection on tcloud namespace %s, waiting for update to complete", + tns.GetNamespace(), + ), + requeue.DefaultRequeueAfterDuration, + ) +} + // 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 on diff --git a/controller/namespace/delete_test.go b/controller/namespace/delete_test.go index efca613..81c6a0f 100644 --- a/controller/namespace/delete_test.go +++ b/controller/namespace/delete_test.go @@ -12,10 +12,13 @@ import ( "github.com/temporalio/kube-temporal/pkg/kube/condition" "github.com/temporalio/kube-temporal/pkg/types" tcloudservicev1 "go.temporal.io/cloud-sdk/api/cloudservice/v1" + tcloudnamespacev1 "go.temporal.io/cloud-sdk/api/namespace/v1" operationv1 "go.temporal.io/cloud-sdk/api/operation/v1" + tcloudresourcev1 "go.temporal.io/cloud-sdk/api/resource/v1" tcclient "go.temporal.io/cloud-sdk/cloudclient" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" + "google.golang.org/protobuf/proto" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" tcloudcommon "github.com/temporalio/cloud-operator/api/cloud/common" @@ -26,9 +29,12 @@ import ( ) const ( - deleteTestName = "tmprl-ns" - deleteTestID = "tmprl-ns.acct" - deleteTestRV = "rv-1" + deleteTestName = "tmprl-ns" + deleteTestID = "tmprl-ns.acct" + deleteTestRV = "rv-1" + // deleteTestCloudRV is the resource version Temporal Cloud reports. It + // differs from the CR's saved one so tests can tell which was sent. + deleteTestCloudRV = "rv-cloud" deleteTestAsyncOpID = "async-op-1" ) @@ -69,12 +75,23 @@ type deleteCase struct { // protected sets spec.lifecycle.enableDeleteProtection. protected bool + // cloudNS is the namespace GetNamespace returns. Nil means an Active, + // unprotected namespace. + cloudNS *tcloudnamespacev1.Namespace + getErr error + updateErr error deleteResp *tcloudservicev1.DeleteNamespaceResponse deleteErr error asyncOp *operationv1.AsyncOperation + // expGetNamespace is whether GetNamespace is called (once) with the + // status ID. + expGetNamespace bool + // expUpdate is whether UpdateNamespace is called (once) to turn off + // deletion protection, with Temporal Cloud's resource version. + expUpdate bool // expDelete is whether DeleteNamespace is called (once) with the status - // ID and resource version. + // ID and Temporal Cloud's resource version. expDelete bool // expGetAsyncOp is whether GetAsyncOperation is called (once) with the // status async operation ID. @@ -99,21 +116,23 @@ func TestReconciler_ResourceDelete_DeletionPolicy(t *testing.T) { status: statusActive, }, { - name: "no annotation defaults to delete", - status: statusActive, - deleteResp: deleteAccepted, - expDelete: true, - expRequeue: true, - expStatus: &statusDeleting, + name: "no annotation defaults to delete", + status: statusActive, + deleteResp: deleteAccepted, + expGetNamespace: true, + expDelete: true, + expRequeue: true, + expStatus: &statusDeleting, }, { - name: "delete calls DeleteNamespace", - policy: "delete", - status: statusActive, - deleteResp: deleteAccepted, - expDelete: true, - expRequeue: true, - expStatus: &statusDeleting, + name: "delete calls DeleteNamespace", + policy: "delete", + status: statusActive, + deleteResp: deleteAccepted, + expGetNamespace: true, + expDelete: true, + expRequeue: true, + expStatus: &statusDeleting, }, { name: "capitalized retain makes no cloud calls", @@ -157,28 +176,44 @@ func TestReconciler_ResourceDelete_NoStatusID(t *testing.T) { func TestReconciler_ResourceDelete_SendDelete(t *testing.T) { cases := []deleteCase{ { - name: "success sets deleting and requeues", - status: statusActive, - deleteResp: deleteAccepted, - expDelete: true, - expRequeue: true, - expStatus: &statusDeleting, + name: "success sets deleting and requeues", + status: statusActive, + deleteResp: deleteAccepted, + expGetNamespace: true, + expDelete: true, + expRequeue: true, + expStatus: &statusDeleting, }, { - name: "NotFound means already gone", - status: statusActive, - deleteErr: status.Error(codes.NotFound, "namespace not found"), - expDelete: true, + name: "NotFound means already gone", + status: statusActive, + deleteErr: status.Error(codes.NotFound, "namespace not found"), + expGetNamespace: true, + expDelete: true, }, { name: "FailedPrecondition sets stalled", status: statusActive, deleteErr: status.Error( - codes.FailedPrecondition, "delete protection is enabled", + codes.FailedPrecondition, "namespace has nexus endpoints", ), - expDelete: true, - expErr: "delete protection is enabled", - expStalled: "delete protection is enabled", + expGetNamespace: true, + expDelete: true, + expErr: "namespace has nexus endpoints", + expStalled: "namespace has nexus endpoints", + }, + { + name: "GetNamespace NotFound releases the CR", + status: statusActive, + getErr: status.Error(codes.NotFound, "namespace not found"), + expGetNamespace: true, + }, + { + name: "GetNamespace error returns a plain error", + status: statusActive, + getErr: status.Error(codes.PermissionDenied, "api key lacks access"), + expGetNamespace: true, + expErr: "api key lacks access", }, } for _, tc := range cases { @@ -235,7 +270,7 @@ func TestReconciler_ResourceDelete_DeleteFailed(t *testing.T) { func TestReconciler_ResourceDelete_DeleteProtection(t *testing.T) { cases := []deleteCase{ { - name: "delete policy sets stalled and requeues without cloud calls", + name: "spec on sets stalled and requeues without cloud calls", policy: "delete", status: statusActive, protected: true, @@ -244,19 +279,51 @@ func TestReconciler_ResourceDelete_DeleteProtection(t *testing.T) { expStalled: "enableDeleteProtection", }, { - name: "retain policy makes no cloud calls", + name: "spec on with retain policy makes no cloud calls", policy: "retain", status: statusActive, protected: true, }, { - name: "delete in progress is still polled", + name: "spec on with delete in progress is still polled", status: statusDeleting, protected: true, asyncOp: asyncOperation(operationv1.AsyncOperation_STATE_PENDING, ""), expGetAsyncOp: true, expRequeue: true, }, + { + name: "cloud on with spec off turns protection off and requeues", + status: statusActive, + cloudNS: cloudNamespace(tcloudresourcev1.ResourceState_RESOURCE_STATE_ACTIVE, true), + expGetNamespace: true, + expUpdate: true, + expRequeue: true, + }, + { + name: "stale protection update requeues", + status: statusActive, + cloudNS: cloudNamespace(tcloudresourcev1.ResourceState_RESOURCE_STATE_ACTIVE, true), + updateErr: status.Error( + codes.FailedPrecondition, "resource version mismatch", + ), + expGetNamespace: true, + expUpdate: true, + expErr: "resource version mismatch", + expRequeue: true, + }, + { + name: "invalid protection update is terminal", + status: statusActive, + cloudNS: cloudNamespace(tcloudresourcev1.ResourceState_RESOURCE_STATE_ACTIVE, true), + updateErr: status.Error( + codes.InvalidArgument, "bad spec", + ), + expGetNamespace: true, + expUpdate: true, + expErr: "bad spec", + expTerminal: true, + }, } for _, tc := range cases { t.Run(tc.name, func(tt *testing.T) { @@ -313,7 +380,7 @@ func runDeleteCase(t *testing.T, tc deleteCase) { require.Equal(expStatus, gotCR.Status.ResourceStatus) requireStalled(t, got, tc.expStalled) - cloud.requireCalls(t, tc.expDelete, tc.expGetAsyncOp) + cloud.requireCalls(t, tc) } // newDeleteTestNamespace returns a Namespace CR with the supplied @@ -348,6 +415,28 @@ func statusWithState(s tcloudcommon.ResourceState) tcloudcommon.ResourceStatus { return rs } +// cloudNamespace returns the namespace Temporal Cloud reports, in the +// supplied state and with deletion protection on or off. Its spec has other +// settings too, so tests can check that only protection changes. +func cloudNamespace( + state tcloudresourcev1.ResourceState, + protected bool, +) *tcloudnamespacev1.Namespace { + return &tcloudnamespacev1.Namespace{ + Namespace: deleteTestID, + ResourceVersion: deleteTestCloudRV, + State: state, + Spec: &tcloudnamespacev1.NamespaceSpec{ + Name: deleteTestName, + Regions: []string{"aws-us-east-1"}, + RetentionDays: 30, + Lifecycle: &tcloudnamespacev1.LifecycleSpec{ + EnableDeleteProtection: protected, + }, + }, + } +} + // asyncOperation returns the test async operation in the supplied state. func asyncOperation( state operationv1.AsyncOperation_State, @@ -367,14 +456,15 @@ type deleteFakeCloud struct { client *tcclient.Client mu sync.Mutex - getNamespace int + getNamespace []*tcloudservicev1.GetNamespaceRequest + updateNamespace []*tcloudservicev1.UpdateNamespaceRequest deleteNamespace []*tcloudservicev1.DeleteNamespaceRequest getAsyncOperation []*tcloudservicev1.GetAsyncOperationRequest } // startDeleteFakeCloud starts a fake Temporal Cloud server that returns tc's -// DeleteNamespace and GetAsyncOperation responses, and stops it when the test -// ends. +// GetNamespace, UpdateNamespace, DeleteNamespace and GetAsyncOperation +// responses, and stops it when the test ends. func startDeleteFakeCloud(t *testing.T, tc deleteCase) *deleteFakeCloud { t.Helper() require := require.New(t) @@ -382,12 +472,32 @@ func startDeleteFakeCloud(t *testing.T, tc deleteCase) *deleteFakeCloud { c := &deleteFakeCloud{} srv, err := fakeserver.New( fakeserver.WithGetNamespace(func( - context.Context, *tcloudservicev1.GetNamespaceRequest, + _ context.Context, req *tcloudservicev1.GetNamespaceRequest, ) (*tcloudservicev1.GetNamespaceResponse, error) { c.mu.Lock() defer c.mu.Unlock() - c.getNamespace++ - return &tcloudservicev1.GetNamespaceResponse{}, nil + c.getNamespace = append(c.getNamespace, req) + if tc.getErr != nil { + return nil, tc.getErr + } + ns := tc.cloudNS + if ns == nil { + ns = cloudNamespace( + tcloudresourcev1.ResourceState_RESOURCE_STATE_ACTIVE, false, + ) + } + return &tcloudservicev1.GetNamespaceResponse{Namespace: ns}, nil + }), + fakeserver.WithUpdateNamespace(func( + _ context.Context, req *tcloudservicev1.UpdateNamespaceRequest, + ) (*tcloudservicev1.UpdateNamespaceResponse, error) { + c.mu.Lock() + defer c.mu.Unlock() + c.updateNamespace = append(c.updateNamespace, req) + if tc.updateErr != nil { + return nil, tc.updateErr + } + return &tcloudservicev1.UpdateNamespaceResponse{}, nil }), fakeserver.WithDeleteNamespace(func( _ context.Context, req *tcloudservicev1.DeleteNamespaceRequest, @@ -424,26 +534,42 @@ func startDeleteFakeCloud(t *testing.T, tc deleteCase) *deleteFakeCloud { } // requireCalls checks which Temporal Cloud calls the server received. -// GetNamespace is never expected. -func (c *deleteFakeCloud) requireCalls( - t *testing.T, - expDelete bool, - expGetAsyncOp bool, -) { +func (c *deleteFakeCloud) requireCalls(t *testing.T, tc deleteCase) { t.Helper() require := require.New(t) c.mu.Lock() defer c.mu.Unlock() - require.Zero(c.getNamespace) - if expDelete { + if tc.expGetNamespace { + require.Len(c.getNamespace, 1) + require.Equal(deleteTestID, c.getNamespace[0].GetNamespace()) + } else { + require.Empty(c.getNamespace) + } + if tc.expUpdate { + require.Len(c.updateNamespace, 1) + req := c.updateNamespace[0] + require.Equal(deleteTestID, req.GetNamespace()) + require.Equal(deleteTestCloudRV, req.GetResourceVersion()) + // Only deletion protection changes; the rest is Temporal Cloud's + // spec as read. + exp := proto.Clone(tc.cloudNS.GetSpec()).(*tcloudnamespacev1.NamespaceSpec) + exp.Lifecycle.EnableDeleteProtection = false + require.True( + proto.Equal(exp, req.GetSpec()), + "unexpected update spec: %v", req.GetSpec(), + ) + } else { + require.Empty(c.updateNamespace) + } + if tc.expDelete { require.Len(c.deleteNamespace, 1) require.Equal(deleteTestID, c.deleteNamespace[0].GetNamespace()) - require.Equal(deleteTestRV, c.deleteNamespace[0].GetResourceVersion()) + require.Equal(deleteTestCloudRV, c.deleteNamespace[0].GetResourceVersion()) } else { require.Empty(c.deleteNamespace) } - if expGetAsyncOp { + if tc.expGetAsyncOp { require.Len(c.getAsyncOperation, 1) require.Equal( deleteTestAsyncOpID, c.getAsyncOperation[0].GetAsyncOperationId(), diff --git a/pkg/testutil/temporal/cloud/fake/server/namespace.go b/pkg/testutil/temporal/cloud/fake/server/namespace.go index 2ee2a3f..188ea8c 100644 --- a/pkg/testutil/temporal/cloud/fake/server/namespace.go +++ b/pkg/testutil/temporal/cloud/fake/server/namespace.go @@ -184,3 +184,70 @@ func (s fakeServer) DeleteNamespace( } return s.UnimplementedCloudServiceServer.DeleteNamespace(ctx, req) } + +type UpdateNamespaceFunc func( + context.Context, + *tcloudservicev1.UpdateNamespaceRequest, +) (*tcloudservicev1.UpdateNamespaceResponse, error) + +// WithUpdateNamespace sets the fake server's UpdateNamespace function +// override. +func WithUpdateNamespace(fn UpdateNamespaceFunc) NewOption { + return func(s *fakeServer) { + s.updateNamespaceFunc = fn + } +} + +// OnUpdateNamespace lets tests validate arguments passed to UpdateNamespace +// and set its response. +func OnUpdateNamespace( + asserter func(*tcloudservicev1.UpdateNamespaceRequest), + returner func() (*tcloudservicev1.UpdateNamespaceResponse, error), +) NewOption { + return func(s *fakeServer) { + fn := func( + ctx context.Context, req *tcloudservicev1.UpdateNamespaceRequest, + ) (*tcloudservicev1.UpdateNamespaceResponse, error) { + if asserter != nil { + asserter(req) + } + if returner == nil { + return &tcloudservicev1.UpdateNamespaceResponse{}, nil + } + resp, err := returner() + if resp == nil && err == nil { + resp = &tcloudservicev1.UpdateNamespaceResponse{} + } + return resp, err + } + s.updateNamespaceFunc = fn + } +} + +// OnUpdateNamespaceReturn sets the response returned by UpdateNamespace. +func OnUpdateNamespaceReturn( + resp *tcloudservicev1.UpdateNamespaceResponse, + err error, +) NewOption { + if resp == nil && err == nil { + resp = &tcloudservicev1.UpdateNamespaceResponse{} + } + return func(s *fakeServer) { + fn := func( + context.Context, *tcloudservicev1.UpdateNamespaceRequest, + ) (*tcloudservicev1.UpdateNamespaceResponse, error) { + return resp, err + } + s.updateNamespaceFunc = fn + } +} + +func (s fakeServer) UpdateNamespace( + ctx context.Context, + req *tcloudservicev1.UpdateNamespaceRequest, +) (*tcloudservicev1.UpdateNamespaceResponse, error) { + if s.updateNamespaceFunc != nil { + return s.updateNamespaceFunc(ctx, req) + } + return s.UnimplementedCloudServiceServer.UpdateNamespace(ctx, req) +} diff --git a/pkg/testutil/temporal/cloud/fake/server/server.go b/pkg/testutil/temporal/cloud/fake/server/server.go index 94a864d..7f691ac 100644 --- a/pkg/testutil/temporal/cloud/fake/server/server.go +++ b/pkg/testutil/temporal/cloud/fake/server/server.go @@ -16,6 +16,7 @@ type fakeServer struct { getNamespaceFunc GetNamespaceFunc getNamespacesFunc GetNamespacesFunc + updateNamespaceFunc UpdateNamespaceFunc deleteNamespaceFunc DeleteNamespaceFunc getAsyncOperationFunc GetAsyncOperationFunc } From b772b760183864525df53ed3cf47e01d2dd34b1e Mon Sep 17 00:00:00 2001 From: niyomukiza-mechack Date: Thu, 29 Oct 2026 10:42:37 -0700 Subject: [PATCH 9/9] Address review: kube-temporal v0.7.0, retain log, case-insensitive Validate --- api/cloud/common/const.go | 30 ++++++++++++++++------- api/cloud/common/const_test.go | 44 ++++++++++++++++++++++++++++++++++ controller/namespace/delete.go | 10 ++++---- go.mod | 15 ++++++------ go.sum | 32 ++++++++++++------------- 5 files changed, 94 insertions(+), 37 deletions(-) create mode 100644 api/cloud/common/const_test.go diff --git a/api/cloud/common/const.go b/api/cloud/common/const.go index aa4d898..4c9db58 100644 --- a/api/cloud/common/const.go +++ b/api/cloud/common/const.go @@ -3,6 +3,8 @@ package common import ( "fmt" "strings" + + "github.com/samber/lo" ) const ( @@ -32,16 +34,26 @@ const ( DeletionPolicyRetain DeletionPolicy = "retain" ) -// Validate returns an error if the DeletionPolicy is not valid. +// validDeletionPolicies are the valid DeletionPolicy values. +var validDeletionPolicies = []string{ + string(DeletionPolicyRetain), + string(DeletionPolicyDelete), +} + +// Validate returns an error if the DeletionPolicy is not one of the valid +// values. func (p DeletionPolicy) Validate() error { - switch p { - case DeletionPolicyRetain, DeletionPolicyDelete: + if ContainsFold(string(p), validDeletionPolicies) { return nil - default: - allowed := strings.Join( - []string{string(DeletionPolicyRetain), string(DeletionPolicyDelete)}, - ", ", - ) - return fmt.Errorf("invalid deletion policy %q. allowed values: %s", p, allowed) } + allowed := strings.Join(validDeletionPolicies, ", ") + return fmt.Errorf("invalid deletion policy %q. allowed values: %s", p, allowed) +} + +// ContainsFold returns true if the subject matches one of the +// supplied valid values. +func ContainsFold(subject string, validValues []string) bool { + return lo.ContainsBy(validValues, func(v string) bool { + return strings.EqualFold(subject, v) + }) } diff --git a/api/cloud/common/const_test.go b/api/cloud/common/const_test.go new file mode 100644 index 0000000..43b3f5d --- /dev/null +++ b/api/cloud/common/const_test.go @@ -0,0 +1,44 @@ +package common + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestDeletionPolicyValidate(t *testing.T) { + cases := []struct { + in DeletionPolicy + expErr bool + }{ + {"retain", false}, + {"delete", false}, + {"Retain", false}, + {"RETAIN", false}, + {"Delete", false}, + {"dElEtE", false}, + {"", true}, + {"orphan", true}, + {"retained", true}, + {" retain ", true}, + } + for _, tc := range cases { + t.Run(string(tc.in), func(t *testing.T) { + err := tc.in.Validate() + if !tc.expErr { + require.NoError(t, err) + return + } + require.EqualError(t, err, `invalid deletion policy "`+string(tc.in)+`". allowed values: retain, delete`) + }) + } +} + +func TestContainsFold(t *testing.T) { + validValues := []string{"retain", "delete"} + require.True(t, ContainsFold("retain", validValues)) + require.True(t, ContainsFold("Retain", validValues)) + require.True(t, ContainsFold("DELETE", validValues)) + require.False(t, ContainsFold("orphan", validValues)) + require.False(t, ContainsFold("retain", nil)) +} diff --git a/controller/namespace/delete.go b/controller/namespace/delete.go index a587441..db95def 100644 --- a/controller/namespace/delete.go +++ b/controller/namespace/delete.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "log/slog" "strings" ctrlreqlog "github.com/temporalio/kube-temporal/pkg/controller/request/log" @@ -32,7 +33,6 @@ import ( // It works on a copy of `original` and returns that copy, with any status // changes (State, AsyncOperationID, conditions), as the latest state. // `original` is never mutated. - func (r *Reconciler) ResourceDelete( ctx context.Context, original types.Resource, @@ -68,9 +68,11 @@ func (r *Reconciler) ResourceDelete( return latest, err } if policy == tcloudcommon.DeletionPolicyRetain { - // TODO(Mechack): log that the Namespace is retained because of the - // retain deletion policy, once ctrlreqlog's log.Info no longer panics - // (ToContext never stores the request logger). + log.Info( + "retaining Temporal Cloud Namespace because of the retain deletion policy", + slog.String("namespace_id", cr.Status.ID), + slog.String("deletion_policy", string(policy)), + ) return latest, nil } diff --git a/go.mod b/go.mod index c99afb5..cd09e73 100644 --- a/go.mod +++ b/go.mod @@ -7,7 +7,7 @@ require ( github.com/samber/lo v1.53.0 github.com/spf13/pflag v1.0.10 github.com/stretchr/testify v1.12.1 - github.com/temporalio/kube-temporal v0.6.0 + github.com/temporalio/kube-temporal v0.7.0 go.temporal.io/cloud-sdk v0.21.0 google.golang.org/grpc v1.84.0 google.golang.org/protobuf v1.36.12 @@ -28,10 +28,10 @@ require ( github.com/emicklei/go-restful/v3 v3.13.0 // indirect github.com/evanphx/json-patch/v5 v5.9.11 // indirect github.com/felixge/httpsnoop v1.1.0 // indirect - github.com/fsnotify/fsnotify v1.10.0 // indirect - github.com/fxamacker/cbor/v2 v2.9.1 // indirect + github.com/fsnotify/fsnotify v1.10.1 // indirect + github.com/fxamacker/cbor/v2 v2.9.3 // indirect github.com/go-logr/stdr v1.2.2 // indirect - github.com/go-openapi/jsonpointer v1.0.1 // indirect + github.com/go-openapi/jsonpointer v1.0.2 // indirect github.com/go-openapi/jsonreference v1.0.1 // indirect github.com/go-openapi/swag v0.29.2 // indirect github.com/go-openapi/swag/cmdutils v0.29.2 // indirect @@ -48,7 +48,6 @@ require ( github.com/google/cel-go v0.29.2 // indirect github.com/google/gnostic-models v0.7.1 // indirect github.com/google/go-cmp v0.7.0 // indirect - github.com/google/pprof v0.0.0-20260402051712-545e8a4df936 // indirect github.com/google/uuid v1.6.0 // indirect github.com/grpc-ecosystem/go-grpc-middleware/v2 v2.3.4 // indirect github.com/grpc-ecosystem/grpc-gateway/v2 v2.31.0 // indirect @@ -77,7 +76,7 @@ require ( go.yaml.in/yaml/v2 v2.4.4 // indirect go.yaml.in/yaml/v3 v3.0.5 // indirect golang.org/x/exp v0.0.0-20260908205506-85c1c2202aba // indirect - golang.org/x/net v0.59.0 // indirect + golang.org/x/net v0.60.0 // indirect golang.org/x/oauth2 v0.37.0 // indirect golang.org/x/sync v0.23.0 // indirect golang.org/x/sys v0.48.0 // indirect @@ -93,9 +92,9 @@ require ( k8s.io/apiserver v0.37.0 // indirect k8s.io/component-base v0.37.0 // indirect k8s.io/klog/v2 v2.140.0 // indirect - k8s.io/kube-openapi v0.0.0-20260721132016-d427ff9ee9ad // indirect + k8s.io/kube-openapi v0.0.0-20260821135717-be32def86098 // indirect k8s.io/streaming v0.37.1 // indirect - k8s.io/utils v0.0.0-20260626114624-be93311217bd // indirect + k8s.io/utils v0.0.0-20260707023825-cf1189d6abe3 // indirect sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.36.0 // indirect sigs.k8s.io/cli-utils v0.37.2 // indirect sigs.k8s.io/json v0.0.0-20250730193827-2d320260d730 // indirect diff --git a/go.sum b/go.sum index cae5a44..110dc51 100644 --- a/go.sum +++ b/go.sum @@ -25,10 +25,10 @@ github.com/evanphx/json-patch/v5 v5.9.11 h1:/8HVnzMq13/3x9TPvjG08wUGqBTmZBsCWzjT github.com/evanphx/json-patch/v5 v5.9.11/go.mod h1:3j+LviiESTElxA4p3EMKAB9HXj3/XEtnUf6OZxqIQTM= github.com/felixge/httpsnoop v1.1.0 h1:3YtUj32ZZkqZtt3sZZsClsymw/QDuVfpNhoA31zeORc= github.com/felixge/httpsnoop v1.1.0/go.mod h1:Zqxgdd+1Rkcz8euOqdr7lqgCRJztwr5hp9vDSi5UZCE= -github.com/fsnotify/fsnotify v1.10.0 h1:Xx/5Ydg9CeBDX/wi4VJqStNtohYjitZhhlHt4h3St1M= -github.com/fsnotify/fsnotify v1.10.0/go.mod h1:TLheqan6HD6GBK6PrDWyDPBaEV8LspOxvPSjC+bVfgo= -github.com/fxamacker/cbor/v2 v2.9.1 h1:2rWm8B193Ll4VdjsJY28jxs70IdDsHRWgQYAI80+rMQ= -github.com/fxamacker/cbor/v2 v2.9.1/go.mod h1:vM4b+DJCtHn+zz7h3FFp/hDAI9WNWCsZj23V5ytsSxQ= +github.com/fsnotify/fsnotify v1.10.1 h1:b0/UzAf9yR5rhf3RPm9gf3ehBPpf0oZKIjtpKrx59Ho= +github.com/fsnotify/fsnotify v1.10.1/go.mod h1:TLheqan6HD6GBK6PrDWyDPBaEV8LspOxvPSjC+bVfgo= +github.com/fxamacker/cbor/v2 v2.9.3 h1:oQBnFATpNdY8gJHTndDDv5Xl4QqNaz51G5LLEPhng3Q= +github.com/fxamacker/cbor/v2 v2.9.3/go.mod h1:vM4b+DJCtHn+zz7h3FFp/hDAI9WNWCsZj23V5ytsSxQ= github.com/go-logr/logr v1.2.2/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A= github.com/go-logr/logr v1.4.4 h1:tG4xh9yMsRCAiodLVTxyrkzSZ9+o0L1Kg/+cPVcbP/8= github.com/go-logr/logr v1.4.4/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY= @@ -36,8 +36,8 @@ github.com/go-logr/stdr v1.2.2 h1:hSWxHoqTgW2S2qGc0LTAI563KZ5YKYRhT3MFKZMbjag= github.com/go-logr/stdr v1.2.2/go.mod h1:mMo/vtBO5dYbehREoey6XUKy/eSumjCCveDpRre4VKE= github.com/go-logr/zapr v1.3.0 h1:XGdV8XW8zdwFiwOA2Dryh1gj2KRQyOOoNmBy4EplIcQ= github.com/go-logr/zapr v1.3.0/go.mod h1:YKepepNBd1u/oyhd/yQmtjVXmm9uML4IXUgMOwR8/Gg= -github.com/go-openapi/jsonpointer v1.0.1 h1:2KxywRmNwJkT/FMBa3iRNHEaAxSJvjqoufQZy3au1Mg= -github.com/go-openapi/jsonpointer v1.0.1/go.mod h1:wI7ZYsFmbIi9nBXOZqgDaS/bqOchRGZjqxFli7FBYxY= +github.com/go-openapi/jsonpointer v1.0.2 h1:NbW7xT2b+l7x+JefiPGDsZWsrLk1ogrz4AHK1TTzMt8= +github.com/go-openapi/jsonpointer v1.0.2/go.mod h1:N0rI4MbNKD7xAFTiHtB+5Nhrg2j/wwYg7m7Fmb4/jk8= github.com/go-openapi/jsonreference v1.0.1 h1:4zJ7AmYDKNmD3aSpfPnFNCFA5E80/xMHUNKgydaLh38= github.com/go-openapi/jsonreference v1.0.1/go.mod h1:dYplQXa6p5lXprLcJ8LE2iU7vNpXsAHDQ5ZAgL+Qx3A= github.com/go-openapi/swag v0.29.2 h1:9n8frkcsuQRA0INU31VbxNxt1zBaHxrHYGEf5BUjFi4= @@ -83,8 +83,8 @@ github.com/google/go-cmp v0.7.0/go.mod h1:pXiqmnSA92OHEEa9HXL2W4E7lf9JzCmGVUdgjX github.com/google/gofuzz v1.0.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg= github.com/google/gofuzz v1.2.0 h1:xRy4A+RhZaiKjJ1bPfwQ8sedCA+YS2YcCHW6ec7JMi0= github.com/google/gofuzz v1.2.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg= -github.com/google/pprof v0.0.0-20260402051712-545e8a4df936 h1:EwtI+Al+DeppwYX2oXJCETMO23COyaKGP6fHVpkpWpg= -github.com/google/pprof v0.0.0-20260402051712-545e8a4df936/go.mod h1:MxpfABSjhmINe3F1It9d+8exIHFvUqtLIRCdOGNXqiI= +github.com/google/pprof v0.0.0-20260709232956-b9395ee17fa0 h1:du0WGc8xSKq/++e0cglxhS/mXVqsR7+c7jLEi5Vqduw= +github.com/google/pprof v0.0.0-20260709232956-b9395ee17fa0/go.mod h1:MxpfABSjhmINe3F1It9d+8exIHFvUqtLIRCdOGNXqiI= github.com/google/uuid v1.6.0 h1:NIvaJDMOsjHA8n1jAhLSgzrAzy1Hgr+hNrb57e+94F0= github.com/google/uuid v1.6.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= github.com/grpc-ecosystem/go-grpc-middleware/v2 v2.3.4 h1:9ZJYjPEJpcleIKcqysXOo14NLQYwu23fzmKRFpIjPjc= @@ -136,8 +136,8 @@ github.com/stretchr/objx v0.5.3/go.mod h1:rDQraq+vQZU7Fde9LOZLr8Tax6zZvy4kuNKF+Q github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI= github.com/stretchr/testify v1.12.1 h1:EuwCh5fleGS7H32xRwO3wRGT7DxrDhLAT6FF8MpWDWE= github.com/stretchr/testify v1.12.1/go.mod h1:MDEgiDPPsNp5cuIrHPPCyornHKgEVbtFUmoNlxoYthg= -github.com/temporalio/kube-temporal v0.6.0 h1:Zp4qbIqOE+u3j040EZitjX4MRSb/IZbEG1xzlsC8lDI= -github.com/temporalio/kube-temporal v0.6.0/go.mod h1:1rjIEYZPbs6ahnxptA7Z5p549s2oZYeU6P337Bz50gY= +github.com/temporalio/kube-temporal v0.7.0 h1:6fPfa2SUJy9JWcEMMW3pM8J+z9nK4jr2eThG6+fL7zA= +github.com/temporalio/kube-temporal v0.7.0/go.mod h1:oaDFjrRC1LJl56dg5dVUORZ9YwmVMGIuNEMV53PitrQ= github.com/x448/float16 v0.8.4 h1:qLwI1I70+NjRFUR3zs1JPUCgaCXSh3SW62uAKT1mSBM= github.com/x448/float16 v0.8.4/go.mod h1:14CWIYCyZA/cWjXOioeEpHeN/83MdbZDRQHoFcYsOfg= go.opentelemetry.io/auto/sdk v1.2.1 h1:jXsnJ4Lmnqd11kwkBV2LgLoFMZKizbCi5fNZ/ipaZ64= @@ -179,8 +179,8 @@ golang.org/x/exp v0.0.0-20260908205506-85c1c2202aba h1:Ck8QetSgk912qxWLMCKxd0in+ golang.org/x/exp v0.0.0-20260908205506-85c1c2202aba/go.mod h1:50RgIsmK7OwqzTTeqcSXQW8SswW0o8fRcDxmqGluJ8E= golang.org/x/mod v0.41.0 h1:qJmnOUb4YB+FsEuM3HcWucdZASCPGhsX6uljO6pog0c= golang.org/x/mod v0.41.0/go.mod h1:Ek9pY8RKWXwsWvd3rQiHYtMqkjSUV+s1Rj7j4H5Ur6o= -golang.org/x/net v0.59.0 h1:5zfYln+w5XCxwrnMMJPufRgNoXEaGxl0wo5GqPXyues= -golang.org/x/net v0.59.0/go.mod h1:2DA/G1UfVbCpQPeWTmMPGY7Cs2PkBkwu743bVX5PIVg= +golang.org/x/net v0.60.0 h1:79p50tfZlm0J9YfoDsSi639qSXNGVwEzOPLCxM2FsYU= +golang.org/x/net v0.60.0/go.mod h1:2DA/G1UfVbCpQPeWTmMPGY7Cs2PkBkwu743bVX5PIVg= golang.org/x/oauth2 v0.37.0 h1:JUlcxA8oAtauLfiH8FX2/FkAWHAdi0QtGCGc+hofE98= golang.org/x/oauth2 v0.37.0/go.mod h1:IxwZNxUULJmpBFf9K/9NTMSIfZZuvuTy1gGxhigP/58= golang.org/x/sync v0.23.0 h1:KameEIfc1IkluZyXWLn39Wd4tURc6GbCiISGiZm2bQk= @@ -226,12 +226,12 @@ k8s.io/component-base v0.37.0 h1:3SdSa4+itMdFTDFTeR8CxKGmSTSMXFlKL4ky8OqjguM= k8s.io/component-base v0.37.0/go.mod h1:LjOebp4R9y6LODWZQv102ZQxGheLcDO2ZJLAw6bbh4I= k8s.io/klog/v2 v2.140.0 h1:Tf+J3AH7xnUzZyVVXhTgGhEKnFqye14aadWv7bzXdzc= k8s.io/klog/v2 v2.140.0/go.mod h1:o+/RWfJ6PwpnFn7OyAG3QnO47BFsymfEfrz6XyYSSp0= -k8s.io/kube-openapi v0.0.0-20260721132016-d427ff9ee9ad h1:oXImqH8mQNk7PmvzKhmN3ddJoY6OnyM225MXwGHPm0A= -k8s.io/kube-openapi v0.0.0-20260721132016-d427ff9ee9ad/go.mod h1:0/mqHCVhlumdJ3BhCfnjSZQE037nAhNodh1/hK0T8/I= +k8s.io/kube-openapi v0.0.0-20260821135717-be32def86098 h1:z5+pcu1jTyKK5mNTe2/+x+U6Uuv9jRVOJQLaBJJMpeI= +k8s.io/kube-openapi v0.0.0-20260821135717-be32def86098/go.mod h1:0/mqHCVhlumdJ3BhCfnjSZQE037nAhNodh1/hK0T8/I= k8s.io/streaming v0.37.1 h1:TpzVfQeFuVndn2g9mFqxy1UcUYPwDzqjUmwR/IzJCWc= k8s.io/streaming v0.37.1/go.mod h1:APlJR26ZWRcVy5bIEj0QRrKUXROtBHPcxl2NT7EAzPU= -k8s.io/utils v0.0.0-20260626114624-be93311217bd h1:Ea7fgQ5we8Y9T0OX5o0dAHzQOBRI07D/dEYRaB9ZZEs= -k8s.io/utils v0.0.0-20260626114624-be93311217bd/go.mod h1:xDxuJ0whA3d0I4mf/C4ppKHxXynQ+fxnkmQH0vTHnuk= +k8s.io/utils v0.0.0-20260707023825-cf1189d6abe3 h1:jVkFFVfXdXP74B/zbO3hM3hpSFD0xvhQ5U686DPurkE= +k8s.io/utils v0.0.0-20260707023825-cf1189d6abe3/go.mod h1:M2s5JB1lIYP3jzZdorPLHXIPJzt9vv2muW5a6L9DtNM= sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.36.0 h1:/YpDJ4vReG7ZmzSpBGxduXgywWkJU9zHubgJG03MT+Y= sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.36.0/go.mod h1:tJo1aepTXyR+8Xs3sUsGBDk4Ub2AM5dPAPKJx0mpm5c= sigs.k8s.io/cli-utils v0.37.2 h1:GOfKw5RV2HDQZDJlru5KkfLO1tbxqMoyn1IYUxqBpNg=