From b402b4bf3a78418ebd54f223424d984091e26bf8 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 23:20:49 +0000 Subject: [PATCH 1/7] test: a CoderProvisioner deletes keys at earlier targets while the current target is not ready (#416) Change-Id: Id023e1f2a68b9c6bd89a56688f7f746c79ae4f37 Signed-off-by: Thomas Kosiewski --- .../coderprovisioner_earliertargets_test.go | 162 ++++++++++++++++++ 1 file changed, 162 insertions(+) create mode 100644 internal/controller/coderprovisioner_earliertargets_test.go diff --git a/internal/controller/coderprovisioner_earliertargets_test.go b/internal/controller/coderprovisioner_earliertargets_test.go new file mode 100644 index 00000000..ce83829b --- /dev/null +++ b/internal/controller/coderprovisioner_earliertargets_test.go @@ -0,0 +1,162 @@ +package controller_test + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/coder/coder/v2/codersdk" + "github.com/google/uuid" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" + "github.com/coder/coder-k8s/internal/coderbootstrap" +) + +// coderCallLog records the Coder URL of every call, and can fail the deletes +// of one key. +type coderCallLog struct { + coderbootstrap.Client + urls []string + failDelete uuid.UUID + failedDeletes int +} + +func (c *coderCallLog) EnsureProvisionerKey(ctx context.Context, req coderbootstrap.EnsureProvisionerKeyRequest) (coderbootstrap.EnsureProvisionerKeyResponse, error) { + c.urls = append(c.urls, req.CoderURL) + return c.Client.EnsureProvisionerKey(ctx, req) +} + +func (c *coderCallLog) DeleteProvisionerKeyByID(ctx context.Context, req coderbootstrap.DeleteProvisionerKeyByIDRequest) error { + c.urls = append(c.urls, req.CoderURL) + if req.KeyID == c.failDelete { + c.failedDeletes++ + return errors.New("injected delete failure") + } + return c.Client.DeleteProvisionerKeyByID(ctx, req) +} + +func (c *coderCallLog) ProvisionerKeyByID(ctx context.Context, req coderbootstrap.ProvisionerKeyByIDRequest) (*codersdk.ProvisionerKey, error) { + c.urls = append(c.urls, req.CoderURL) + return c.Client.ProvisionerKeyByID(ctx, req) +} + +func (c *coderCallLog) ProvisionerKeyByName(ctx context.Context, req coderbootstrap.ProvisionerKeyByNameRequest) (*codersdk.ProvisionerKey, error) { + c.urls = append(c.urls, req.CoderURL) + return c.Client.ProvisionerKeyByName(ctx, req) +} + +func (c *coderCallLog) Entitlements(ctx context.Context, coderURL, sessionToken string) (codersdk.Entitlements, error) { + c.urls = append(c.urls, coderURL) + return c.Client.Entitlements(ctx, coderURL, sessionToken) +} + +// TestCoderProvisionerKey_EarlierTargetRevokedWhileTargetNotReady checks a +// key that a rotation from control plane A to B replaced and could not delete. +// When the provisioner moves on to a control plane C that is not ready, the +// key at A must still be deleted at A with the operator token of A, and C must +// get no request. Key B, which the Secret holds, must stay. A failed delete is +// retried at the pending key cadence, not on every reconcile. +func TestCoderProvisionerKey_EarlierTargetRevokedWhileTargetNotReady(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + // notReady makes control plane C fail one of its checks. + notReady func(ctx context.Context, t *testing.T, namespace string) + // deleteFails fails every delete of key A at C. + deleteFails bool + }{ + {name: "target not entitled", notReady: markNotEntitled}, + {name: "target operator token missing", notReady: deleteOperatorToken}, + {name: "delete fails at the earlier target", notReady: markNotEntitled, deleteFails: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + f := newProvisionerCredentialFixture(ctx, t, "cprov-earlier") + createControlPlaneWithToken(ctx, t, f.namespace, "coder-b", "https://coder-b.example.com", "operator-token-b") + createControlPlaneWithToken(ctx, t, f.namespace, "coder-c", "https://coder-c.example.com", "operator-token-c") + apiURL := func(controlPlane string) string { + return "http://" + controlPlane + "." + f.namespace + ".svc.cluster.local:80" + } + keyA := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: uuid.New(), KeyID: uuid.New(), KeyName: "provisioner", Key: "material-a"} + keyB := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: uuid.New(), KeyID: uuid.New(), KeyName: "provisioner", Key: "material-b"} + f.bootstrapClient.provisionerKeyResponses = []coderbootstrap.EnsureProvisionerKeyResponse{keyA, keyB} + calls := &coderCallLog{Client: f.bootstrapClient} + f.reconciler.BootstrapClient = calls + + provisioner := f.createProvisioner(ctx, t, nil, nil) + name := types.NamespacedName{Name: provisioner.Name, Namespace: provisioner.Namespace} + moveTo := func(controlPlane string) { + latest := f.get(ctx, t, provisioner) + latest.Spec.ControlPlaneRef.Name = controlPlane + require.NoError(t, k8sClient.Update(ctx, latest)) + } + reconcile := func() { + _, err := f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: name}) + require.NoError(t, err) + } + + f.reconcile(ctx, t, provisioner) + f.reconcile(ctx, t, provisioner) + require.Equal(t, keyA.KeyID.String(), f.get(ctx, t, provisioner).Status.ProvisionerKeyID) + + moveTo("coder-b") + calls.failDelete = keyA.KeyID + reconcile() + atB := f.get(ctx, t, provisioner) + require.Equal(t, keyB.KeyID.String(), atB.Status.ProvisionerKeyID) + require.True(t, pendingKeyRecorded(atB, keyA.KeyID), "the failed delete must keep the record of key A") + + if !tt.deleteFails { + calls.failDelete = uuid.Nil + } + calls.failedDeletes = 0 + calls.urls = nil + f.bootstrapClient.deleteKeyByIDRequests = nil + tt.notReady(ctx, t, f.namespace) + moveTo("coder-c") + reconcile() + if tt.deleteFails { + reconcile() + } + + require.NotContains(t, calls.urls, apiURL("coder-c"), "control plane C must get no request while it is not ready") + require.NotContains(t, keyDeletes(f.bootstrapClient), keyDelete{coderURL: apiURL("coder-b"), token: "operator-token-b", keyID: keyB.KeyID.String()}, + "nothing may delete key B, which the Secret holds") + final := f.get(ctx, t, provisioner) + require.Equal(t, keyB.KeyID.String(), final.Status.ProvisionerKeyID) + if tt.deleteFails { + require.Equal(t, 1, calls.failedDeletes, "a failed delete must wait for the pending key cadence") + require.True(t, pendingKeyRecorded(final, keyA.KeyID), "a failed delete must keep the record of key A") + return + } + require.Equal(t, []keyDelete{{coderURL: apiURL("coder"), token: testOperatorToken, keyID: keyA.KeyID.String()}}, keyDeletes(f.bootstrapClient), + "key A must be deleted at A with the operator token of A") + require.False(t, pendingKeyRecorded(final, keyA.KeyID), "the record of key A must go once Coder confirmed the delete") + }) + } +} + +func markNotEntitled(ctx context.Context, t *testing.T, namespace string) { + t.Helper() + controlPlane := &coderv1alpha1.CoderControlPlane{} + require.NoError(t, k8sClient.Get(ctx, types.NamespacedName{Name: "coder-c", Namespace: namespace}, controlPlane)) + now := metav1.NewTime(time.Now()) + controlPlane.Status.EntitlementsLastChecked = &now + controlPlane.Status.ExternalProvisionerDaemonsEntitlement = string(codersdk.EntitlementNotEntitled) + require.NoError(t, k8sClient.Status().Update(ctx, controlPlane)) +} + +func deleteOperatorToken(ctx context.Context, t *testing.T, namespace string) { + t.Helper() + require.NoError(t, k8sClient.Delete(ctx, &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "coder-c-operator-token", Namespace: namespace}})) +} From 04c108c1fc9fd30d3668f6387ef58af162e10aaa Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 23:28:12 +0000 Subject: [PATCH 2/7] fix: a CoderProvisioner deletes keys at earlier targets while the current target fails its checks (#416) Change-Id: I6e4a5a65a893bba9aaf5ea07a96bae54b72ba0c9 Signed-off-by: Thomas Kosiewski --- docs/tutorials/deploy-coderprovisioner.md | 2 +- .../controller/coderprovisioner_controller.go | 159 +++++++++++++----- .../coderprovisioner_earliertargets_test.go | 53 +++++- .../coderprovisioner_pendingkeys_test.go | 2 + internal/controller/export_test.go | 6 + 5 files changed, 169 insertions(+), 53 deletions(-) diff --git a/docs/tutorials/deploy-coderprovisioner.md b/docs/tutorials/deploy-coderprovisioner.md index 5de36a7d..e84b8b29 100644 --- a/docs/tutorials/deploy-coderprovisioner.md +++ b/docs/tutorials/deploy-coderprovisioner.md @@ -82,7 +82,7 @@ kubectl delete coderprovisioner coderprovisioner-sample -n coder The operator then deletes the provisioner keys in Coder that this provisioner created. It finds the key in the provisioner key Secret by the organization and key IDs in `status.organizationID` and `status.provisionerKeyID`, not by the names in the spec. It also deletes the key of each entry in `status.pendingKeys` that has an `organizationID` and a `keyID`. The entries name keys that the operator created but did not write to the Secret, for example because a reconcile failed, and keys that a rotation replaced but that Coder did not confirm as deleted. Each entry names the Coder URL of the key and the control plane whose operator token the operator uses to delete it. An entry without these IDs names a key that the operator started to create before Coder returned its ID. For such an entry, the operator deletes no key, because another provisioner or a person can use the same key name in that organization. It removes the entry when Coder has no key with that name, or when that key is the key in `status.provisionerKeyID`. Otherwise, and while the operator cannot reach Coder for that check, the entry stays, the condition `UnverifiedKeyKept` is `True` and names the entry, and deletion of the provisioner leaves the key in Coder. If no other provisioner or person uses the key, delete it with `coder provisioner keys delete --org `. The operator checks again every 2 minutes, and removes the entry and the condition once the key is gone. When the status has no key, for example because the provisioner never passed its checks, the operator removes its finalizer without a call to Coder. -The same records apply while the provisioner runs. When `spec.controlPlaneRef`, the control plane URL, the organization, the key name, or the tags change, the operator creates a new key and deletes the replaced keys by their recorded organization and key IDs, each at its own Coder URL, so a renamed Coder organization does not hide a replaced key. When a rotation fails after Coder created the new key, the next reconcile writes another new key to the Secret, also when you reverted the change. An entry stays in `status.pendingKeys` until Coder confirms that its key is gone, and the operator tries again every 2 minutes. Each entry and `status.controlPlaneUID` also record the UID of the control plane. The operator sends the operator token of a control plane for a record only while the control plane has the recorded UID. When a control plane is deleted and created again under the same name, the new one can run another Coder backend, also at the same URL. The operator then creates a new key for the provisioner, and sends the new token for no key of the old control plane. Such an entry stays, the condition `ControlPlaneChangedKeyKept` is `True` and names it, and deletion of the provisioner leaves that key in Coder. A record without a UID, from an operator version that did not record it, counts as bound to its control plane only at the current URL of that control plane. For status without `status.controlPlaneUID`, the next reconcile looks the applied key up at the current control plane by `status.organizationID` and `status.provisionerKeyID`, so a renamed organization does not hide it. When Coder holds the key there, the operator records the UID of that control plane. When Coder answers that it has no such key, the operator creates a new key. When a change of the spec or the control plane replaces the key of such status, its entry in `status.pendingKeys` records the UID of its control plane while that control plane has the recorded URL. An entry whose control plane was deleted, or that `ControlPlaneChangedKeyKept` names, stays for good: delete its key by hand in Coder, then remove the entry, for example the first one with `kubectl patch coderprovisioner coderprovisioner-sample -n coder --subresource=status --type=json -p '[{"op":"remove","path":"/status/pendingKeys/0"}]'`. +The same records apply while the provisioner runs. When `spec.controlPlaneRef`, the control plane URL, the organization, the key name, or the tags change, the operator creates a new key and deletes the replaced keys by their recorded organization and key IDs, each at its own Coder URL, so a renamed Coder organization does not hide a replaced key. When a rotation fails after Coder created the new key, the next reconcile writes another new key to the Secret, also when you reverted the change. An entry stays in `status.pendingKeys` until Coder confirms that its key is gone, and the operator tries again every 2 minutes. The operator also deletes the keys of entries for earlier targets while the current target is not ready, for example while its operator token is missing or its Coder deployment is not entitled to external provisioners, and sends nothing to the current target then. An entry with `unpublished: true`, and an entry for the key in the Secret, waits for the current target, because the running Deployment can still use its key. Each entry and `status.controlPlaneUID` also record the UID of the control plane. The operator sends the operator token of a control plane for a record only while the control plane has the recorded UID. When a control plane is deleted and created again under the same name, the new one can run another Coder backend, also at the same URL. The operator then creates a new key for the provisioner, and sends the new token for no key of the old control plane. Such an entry stays, the condition `ControlPlaneChangedKeyKept` is `True` and names it, and deletion of the provisioner leaves that key in Coder. A record without a UID, from an operator version that did not record it, counts as bound to its control plane only at the current URL of that control plane. For status without `status.controlPlaneUID`, the next reconcile looks the applied key up at the current control plane by `status.organizationID` and `status.provisionerKeyID`, so a renamed organization does not hide it. When Coder holds the key there, the operator records the UID of that control plane. When Coder answers that it has no such key, the operator creates a new key. When a change of the spec or the control plane replaces the key of such status, its entry in `status.pendingKeys` records the UID of its control plane while that control plane has the recorded URL. An entry whose control plane was deleted, or that `ControlPlaneChangedKeyKept` names, stays for good: delete its key by hand in Coder, then remove the entry, for example the first one with `kubectl patch coderprovisioner coderprovisioner-sample -n coder --subresource=status --type=json -p '[{"op":"remove","path":"/status/pendingKeys/0"}]'`. This cleanup is best-effort. When the operator cannot reach Coder, for example because the control plane or its operator token is gone, or when the delete call fails, the operator still removes its finalizer, and the key stays valid in Coder. Delete the provisioner while the control plane runs. If cleanup could not reach Coder, list the keys with `coder provisioner keys list --org `, and delete the key by hand with `coder provisioner keys delete --org `. diff --git a/internal/controller/coderprovisioner_controller.go b/internal/controller/coderprovisioner_controller.go index 95134ab7..2052c43c 100644 --- a/internal/controller/coderprovisioner_controller.go +++ b/internal/controller/coderprovisioner_controller.go @@ -80,6 +80,10 @@ type CoderProvisionerReconciler struct { client.Client Scheme *runtime.Scheme BootstrapClient coderbootstrap.Client + + // earlierKeyRetryAt maps a provisioner UID to the time.Time before which + // revokeEarlierTargetKeys does not try again. + earlierKeyRetryAt sync.Map } // +kubebuilder:rbac:groups=coder.com,resources=coderprovisioners,verbs=get;list;watch;create;update;patch;delete @@ -205,6 +209,38 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req organizationName := provisionerOrganizationName(provisioner.Spec.OrganizationName) keyName, keySecretName, keySecretKey := provisionerKeyConfig(provisioner) + desiredControlPlaneRefName := provisioner.Spec.ControlPlaneRef.Name + + coderURL, err := coderAPIURL(controlPlane) + if err != nil { + return ctrl.Result{}, err + } + + // Every key that can exist in Coder has a record in status: the applied + // fields for the key in the Secret, status.pendingKeys for the others. A + // key to create is recorded by name before the create, because Coder + // returns its ID only in the create response. Keys recorded with IDs in + // another slot than the target are deleted here. + target := coderv1alpha1.CoderProvisionerKeyRecord{ + ControlPlaneRefName: desiredControlPlaneRefName, + ControlPlaneUID: string(controlPlane.UID), + CoderURL: coderURL, + OrganizationName: organizationName, + KeyName: keyName, + Unpublished: true, + } + // Each earlier target record names its own URL and control plane, so its + // cleanup does not wait for the checks of the current target below. + earlierKeysUpdated, err := r.revokeEarlierTargetKeys(ctx, provisioner, target) + if err != nil { + return ctrl.Result{}, err + } + if earlierKeysUpdated { + statusSnapshot = provisioner.Status.DeepCopy() + if statusSnapshot == nil { + return ctrl.Result{}, fmt.Errorf("assertion failed: status snapshot must not be nil after earlier target key status update") + } + } sessionToken, operatorAccessResult, operatorAccessReason, operatorAccessMessage, err := r.readOperatorSessionToken(ctx, provisioner, controlPlane) if err != nil { @@ -229,11 +265,6 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req operatorAccessMessage, ) - coderURL, err := coderAPIURL(controlPlane) - if err != nil { - return ctrl.Result{}, err - } - entitlementResult, entitlementErr := r.reconcileExternalProvisionerEntitlement(ctx, provisioner, controlPlane, sessionToken) if entitlementErr != nil { if statusSnapshot == nil { @@ -255,7 +286,6 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req } desiredTagsHash := hashProvisionerTags(provisioner.Spec.Tags) - desiredControlPlaneRefName := provisioner.Spec.ControlPlaneRef.Name status := provisioner.Status orgDrift := status.OrganizationName != "" && status.OrganizationName != organizationName keyNameDrift := status.ProvisionerKeyName != "" && status.ProvisionerKeyName != keyName @@ -295,19 +325,6 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req metadataMissing := status.OrganizationName == "" || status.TagsHash == "" || status.ControlPlaneRefName == "" || status.ControlPlaneURL == "" - // Every key that can exist in Coder has a record in status: the applied - // fields for the key in the Secret, status.pendingKeys for the others. A - // key to create is recorded by name before the create, because Coder - // returns its ID only in the create response. Keys recorded with IDs in - // another slot than the target are deleted here. - target := coderv1alpha1.CoderProvisionerKeyRecord{ - ControlPlaneRefName: desiredControlPlaneRefName, - ControlPlaneUID: string(controlPlane.UID), - CoderURL: coderURL, - OrganizationName: organizationName, - KeyName: keyName, - Unpublished: true, - } // An unpublished record names a key that an earlier reconcile created, or // started to create, for the Secret and did not record as applied. The // Secret can hold that key, or a key that the rotation already deleted, @@ -369,15 +386,6 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req return ctrl.Result{}, fmt.Errorf("look up provisioner key %q: %w", keyName, err) } if existingKey != nil && !recordsKeyID(provisioner.Status, existingKey.ID) { - // Keys recorded at earlier targets do not wait for this name. - // Unpublished records stay, because the Secret can hold their key. - kept, _, revokeErr := r.revokePendingKeys(ctx, provisioner, provisioner.Status.PendingKeys, target, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { - return record.Unpublished - }) - if revokeErr != nil { - return ctrl.Result{}, revokeErr - } - provisioner.Status.PendingKeys = kept return r.refuseKeyNameInUse(ctx, provisioner, coderURL, organizationName, keyName, existingKey.ID) } } @@ -405,22 +413,24 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req return sameKeySlot(record, target) && record.KeyID == "" }) pending = append(pending, target) - } - // The replacement below deletes the key that the target holds. - kept, conditionChanged, err := r.revokePendingKeys(ctx, provisioner, pending, target, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { - return existingKey != nil && isKeyID(record.KeyID, existingKey.ID) - }) - if err != nil { - return ctrl.Result{}, err - } - if conditionChanged || !slices.Equal(kept, provisioner.Status.PendingKeys) { - provisioner.Status.PendingKeys = kept - if err := r.Status().Update(ctx, provisioner); err != nil { - return ctrl.Result{}, fmt.Errorf("record pending provisioner keys in status: %w", err) + // Without a replacement, revokeEarlierTargetKeys above handles every + // record here. The replacement below deletes the key that the target + // holds. + kept, conditionChanged, err := r.revokePendingKeys(ctx, provisioner, pending, target, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { + return existingKey != nil && isKeyID(record.KeyID, existingKey.ID) + }) + if err != nil { + return ctrl.Result{}, err } - statusSnapshot = provisioner.Status.DeepCopy() - if statusSnapshot == nil { - return ctrl.Result{}, fmt.Errorf("assertion failed: status snapshot must not be nil after pending key status update") + if conditionChanged || !slices.Equal(kept, provisioner.Status.PendingKeys) { + provisioner.Status.PendingKeys = kept + if err := r.Status().Update(ctx, provisioner); err != nil { + return ctrl.Result{}, fmt.Errorf("record pending provisioner keys in status: %w", err) + } + statusSnapshot = provisioner.Status.DeepCopy() + if statusSnapshot == nil { + return ctrl.Result{}, fmt.Errorf("assertion failed: status snapshot must not be nil after pending key status update") + } } } @@ -652,6 +662,7 @@ func (r *CoderProvisionerReconciler) reconcileDeletion(ctx context.Context, prov } log := ctrl.LoggerFrom(ctx) + r.earlierKeyRetryAt.Delete(provisioner.UID) // Delete only the keys that status records for this resource. A // provisioner that never sent a key create (refused, not entitled, never @@ -1019,7 +1030,9 @@ func (r *CoderProvisionerReconciler) revokeKey( // target slot, and returns the records to keep: the target slot, the records // that keep reports, and the records whose key Coder did not confirm gone. It // sets the UnverifiedKeyKept and ControlPlaneChangedKeyKept conditions for the -// records it tried, and reports whether the conditions changed. +// records it tried, and reports whether the conditions changed. When keep +// holds back a record outside the target slot, the conditions could not name +// that record, so they stay as they are. func (r *CoderProvisionerReconciler) revokePendingKeys( ctx context.Context, provisioner *coderv1alpha1.CoderProvisioner, @@ -1029,11 +1042,17 @@ func (r *CoderProvisionerReconciler) revokePendingKeys( ) ([]coderv1alpha1.CoderProvisionerKeyRecord, bool, error) { var kept []coderv1alpha1.CoderProvisionerKeyRecord var unverified, controlPlaneChanged []string + heldBack := false for _, record := range pending { - if sameKeySlot(record, target) || keep(record) { + if sameKeySlot(record, target) { kept = append(kept, record) continue } + if keep(record) { + kept = append(kept, record) + heldBack = true + continue + } cleanup, err := r.revokeKey(ctx, provisioner, record) if err != nil { return nil, false, err @@ -1055,11 +1074,61 @@ func (r *CoderProvisionerReconciler) revokePendingKeys( unverified = append(unverified, fmt.Sprintf("key %q in organization %q at %s: %s", record.KeyName, record.OrganizationName, record.CoderURL, note)) } } + if heldBack { + return kept, false, nil + } conditionChanged := setUnverifiedKeyCondition(provisioner, unverified) conditionChanged = setControlPlaneChangedKeyCondition(provisioner, controlPlaneChanged) || conditionChanged return kept, conditionChanged, nil } +// revokeEarlierTargetKeys deletes the keys of the status.pendingKeys records +// outside target, each with the operator token of its own control plane (see +// revokeKey), and writes the result to status. It keeps the records of the +// key in the Secret and of unpublished keys, which the Secret can hold, +// because the running Deployment can still use them. After a try that leaves +// a key that Coder did not confirm gone, it does not try again for +// provisionerPendingKeyRetryInterval, so an unreachable earlier endpoint does +// not get a request on every reconcile. It reports whether it updated status. +func (r *CoderProvisionerReconciler) revokeEarlierTargetKeys( + ctx context.Context, + provisioner *coderv1alpha1.CoderProvisioner, + target coderv1alpha1.CoderProvisionerKeyRecord, +) (bool, error) { + if value, ok := r.earlierKeyRetryAt.Load(provisioner.UID); ok { + retryAt, ok := value.(time.Time) + if !ok { + return false, fmt.Errorf("assertion failed: earlier key retry time of provisioner %s is %T, not time.Time", provisioner.UID, value) + } + if time.Now().Before(retryAt) { + return false, nil + } + } + appliedKeyID := provisioner.Status.ProvisionerKeyID + inUse := func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { + return record.Unpublished || (record.KeyID != "" && record.KeyID == appliedKeyID) + } + kept, conditionChanged, err := r.revokePendingKeys(ctx, provisioner, provisioner.Status.PendingKeys, target, inUse) + if err != nil { + return false, err + } + if slices.ContainsFunc(kept, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { + return !sameKeySlot(record, target) && !inUse(record) + }) { + r.earlierKeyRetryAt.Store(provisioner.UID, time.Now().Add(provisionerPendingKeyRetryInterval)) + } else { + r.earlierKeyRetryAt.Delete(provisioner.UID) + } + if !conditionChanged && slices.Equal(kept, provisioner.Status.PendingKeys) { + return false, nil + } + provisioner.Status.PendingKeys = kept + if err := r.Status().Update(ctx, provisioner); err != nil { + return false, fmt.Errorf("record deleted provisioner keys of earlier targets in status: %w", err) + } + return true, nil +} + // setUnverifiedKeyCondition sets the UnverifiedKeyKept condition for the // pending entries without key IDs that unverified describes, or removes it // when there are none. It reports whether the conditions changed. diff --git a/internal/controller/coderprovisioner_earliertargets_test.go b/internal/controller/coderprovisioner_earliertargets_test.go index ce83829b..c43c1094 100644 --- a/internal/controller/coderprovisioner_earliertargets_test.go +++ b/internal/controller/coderprovisioner_earliertargets_test.go @@ -10,12 +10,16 @@ import ( "github.com/google/uuid" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" + ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" "github.com/coder/coder-k8s/internal/coderbootstrap" + "github.com/coder/coder-k8s/internal/controller" ) // coderCallLog records the Coder URL of every call, and can fail the deletes @@ -61,7 +65,9 @@ func (c *coderCallLog) Entitlements(ctx context.Context, coderURL, sessionToken // When the provisioner moves on to a control plane C that is not ready, the // key at A must still be deleted at A with the operator token of A, and C must // get no request. Key B, which the Secret holds, must stay. A failed delete is -// retried at the pending key cadence, not on every reconcile. +// retried at the pending key cadence, not on every reconcile. When the Secret +// write at B failed instead, the unpublished key B, which the Secret can hold, +// must stay until C is ready. func TestCoderProvisionerKey_EarlierTargetRevokedWhileTargetNotReady(t *testing.T) { t.Parallel() @@ -71,10 +77,14 @@ func TestCoderProvisionerKey_EarlierTargetRevokedWhileTargetNotReady(t *testing. notReady func(ctx context.Context, t *testing.T, namespace string) // deleteFails fails every delete of key A at C. deleteFails bool + // unpublishedAtB fails the Secret write at B instead of the delete + // of key A. + unpublishedAtB bool }{ {name: "target not entitled", notReady: markNotEntitled}, {name: "target operator token missing", notReady: deleteOperatorToken}, {name: "delete fails at the earlier target", notReady: markNotEntitled, deleteFails: true}, + {name: "unpublished key at the earlier target", notReady: markNotEntitled, unpublishedAtB: true}, } for _, tt := range tests { @@ -95,6 +105,19 @@ func TestCoderProvisionerKey_EarlierTargetRevokedWhileTargetNotReady(t *testing. provisioner := f.createProvisioner(ctx, t, nil, nil) name := types.NamespacedName{Name: provisioner.Name, Namespace: provisioner.Namespace} + keySecretName := provisioner.Name + "-provisioner-key" + failSecretWrite := false + base, err := ctrlclient.NewWithWatch(cfg, ctrlclient.Options{Scheme: scheme}) + require.NoError(t, err) + f.reconciler.Client = interceptor.NewClient(base, interceptor.Funcs{ + Update: func(ctx context.Context, c ctrlclient.WithWatch, obj ctrlclient.Object, opts ...ctrlclient.UpdateOption) error { + if s, ok := obj.(*corev1.Secret); ok && s.Name == keySecretName && failSecretWrite { + failSecretWrite = false + return apierrors.NewServiceUnavailable("injected Secret write failure") + } + return c.Update(ctx, obj, opts...) + }, + }) moveTo := func(controlPlane string) { latest := f.get(ctx, t, provisioner) latest.Spec.ControlPlaneRef.Name = controlPlane @@ -110,11 +133,18 @@ func TestCoderProvisionerKey_EarlierTargetRevokedWhileTargetNotReady(t *testing. require.Equal(t, keyA.KeyID.String(), f.get(ctx, t, provisioner).Status.ProvisionerKeyID) moveTo("coder-b") - calls.failDelete = keyA.KeyID - reconcile() - atB := f.get(ctx, t, provisioner) - require.Equal(t, keyB.KeyID.String(), atB.Status.ProvisionerKeyID) - require.True(t, pendingKeyRecorded(atB, keyA.KeyID), "the failed delete must keep the record of key A") + if tt.unpublishedAtB { + failSecretWrite = true + _, err := f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: name}) + require.Error(t, err, "the injected Secret write failure must fail the attempt at B") + require.True(t, pendingKeyRecorded(f.get(ctx, t, provisioner), keyB.KeyID), "status must record the unpublished key B") + } else { + calls.failDelete = keyA.KeyID + reconcile() + atB := f.get(ctx, t, provisioner) + require.Equal(t, keyB.KeyID.String(), atB.Status.ProvisionerKeyID) + require.True(t, pendingKeyRecorded(atB, keyA.KeyID), "the failed delete must keep the record of key A") + } if !tt.deleteFails { calls.failDelete = uuid.Nil @@ -131,12 +161,21 @@ func TestCoderProvisionerKey_EarlierTargetRevokedWhileTargetNotReady(t *testing. require.NotContains(t, calls.urls, apiURL("coder-c"), "control plane C must get no request while it is not ready") require.NotContains(t, keyDeletes(f.bootstrapClient), keyDelete{coderURL: apiURL("coder-b"), token: "operator-token-b", keyID: keyB.KeyID.String()}, - "nothing may delete key B, which the Secret holds") + "nothing may delete key B, which the Secret holds or can hold") final := f.get(ctx, t, provisioner) + if tt.unpublishedAtB { + require.Empty(t, keyDeletes(f.bootstrapClient)) + require.True(t, pendingKeyRecorded(final, keyB.KeyID), "the record of the unpublished key B must stay") + return + } require.Equal(t, keyB.KeyID.String(), final.Status.ProvisionerKeyID) if tt.deleteFails { require.Equal(t, 1, calls.failedDeletes, "a failed delete must wait for the pending key cadence") require.True(t, pendingKeyRecorded(final, keyA.KeyID), "a failed delete must keep the record of key A") + controller.ElapseEarlierKeyRetryInterval(f.reconciler, final.UID) + reconcile() + require.Equal(t, 2, calls.failedDeletes, "the delete must be tried again after the pending key cadence") + require.NotContains(t, calls.urls, apiURL("coder-c")) return } require.Equal(t, []keyDelete{{coderURL: apiURL("coder"), token: testOperatorToken, keyID: keyA.KeyID.String()}}, keyDeletes(f.bootstrapClient), diff --git a/internal/controller/coderprovisioner_pendingkeys_test.go b/internal/controller/coderprovisioner_pendingkeys_test.go index 8be61ffa..cd906d05 100644 --- a/internal/controller/coderprovisioner_pendingkeys_test.go +++ b/internal/controller/coderprovisioner_pendingkeys_test.go @@ -22,6 +22,7 @@ import ( coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" "github.com/coder/coder-k8s/internal/coderbootstrap" + "github.com/coder/coder-k8s/internal/controller" ) // keyRecordingClient checks that status records every key create before the @@ -623,6 +624,7 @@ func TestCoderProvisionerKey_IDLessRecordKeepsOtherProvisionersKey(t *testing.T) require.Contains(t, report.Message, urlC) store.failLookupAt = "" + controller.ElapseEarlierKeyRetryInterval(f.reconciler, f.get(ctx, t, p).UID) f.reconcile(ctx, t, p) require.Empty(t, f.get(ctx, t, p).Status.PendingKeys) require.Nil(t, unverifiedReport()) diff --git a/internal/controller/export_test.go b/internal/controller/export_test.go index 1d29d470..260b2deb 100644 --- a/internal/controller/export_test.go +++ b/internal/controller/export_test.go @@ -44,3 +44,9 @@ func HasSentCredentials(r *CoderMCPServerReconciler, uid types.UID) bool { }) return found } + +// ElapseEarlierKeyRetryInterval lets the next reconcile of the provisioner uid +// retry the keys of earlier targets, as after provisionerPendingKeyRetryInterval. +func ElapseEarlierKeyRetryInterval(r *CoderProvisionerReconciler, uid types.UID) { + r.earlierKeyRetryAt.Delete(uid) +} From 671b4ab355ee9bac36dee12e39abfa0d3ef5b260 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 23:37:58 +0000 Subject: [PATCH 3/7] fix: report and throttle earlier target keys across both cleanup passes (#416) Change-Id: I4d64a8d50d711e7b0e88c6dec948126e54d0db62 Signed-off-by: Thomas Kosiewski --- .../controller/coderprovisioner_controller.go | 175 ++++++++++++------ .../coderprovisioner_earliertargets_test.go | 111 +++++++++-- 2 files changed, 215 insertions(+), 71 deletions(-) diff --git a/internal/controller/coderprovisioner_controller.go b/internal/controller/coderprovisioner_controller.go index 2052c43c..25505fbb 100644 --- a/internal/controller/coderprovisioner_controller.go +++ b/internal/controller/coderprovisioner_controller.go @@ -230,17 +230,18 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req Unpublished: true, } // Each earlier target record names its own URL and control plane, so its - // cleanup does not wait for the checks of the current target below. - earlierKeysUpdated, err := r.revokeEarlierTargetKeys(ctx, provisioner, target) + // cleanup does not wait for the checks of the current target below. The + // key in the Secret and unpublished keys, which the Secret can hold, wait + // for a replacement below, because the running Deployment can still use + // them. Every status write below records the result. + appliedKeyID := provisioner.Status.ProvisionerKeyID + inUse := func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { + return record.Unpublished || (record.KeyID != "" && record.KeyID == appliedKeyID) + } + earlier, err := r.revokeEarlierTargetKeys(ctx, provisioner, target, inUse) if err != nil { return ctrl.Result{}, err } - if earlierKeysUpdated { - statusSnapshot = provisioner.Status.DeepCopy() - if statusSnapshot == nil { - return ctrl.Result{}, fmt.Errorf("assertion failed: status snapshot must not be nil after earlier target key status update") - } - } sessionToken, operatorAccessResult, operatorAccessReason, operatorAccessMessage, err := r.readOperatorSessionToken(ctx, provisioner, controlPlane) if err != nil { @@ -413,16 +414,26 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req return sameKeySlot(record, target) && record.KeyID == "" }) pending = append(pending, target) - // Without a replacement, revokeEarlierTargetKeys above handles every - // record here. The replacement below deletes the key that the target - // holds. - kept, conditionChanged, err := r.revokePendingKeys(ctx, provisioner, pending, target, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { + // revokeEarlierTargetKeys above handles the records that are not in + // use. The replacement below deletes the key that the target holds. + replacedByName := func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { return existingKey != nil && isKeyID(record.KeyID, existingKey.ID) + } + kept, reports, err := r.revokePendingKeys(ctx, provisioner, pending, target, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { + return !inUse(record) || replacedByName(record) }) if err != nil { return ctrl.Result{}, err } - if conditionChanged || !slices.Equal(kept, provisioner.Status.PendingKeys) { + // The publish step below drops the records of the key found by name. + conditionChanged := false + if earlier.tried { + reports = slices.DeleteFunc(mergeKeptKeyReports(kept, inUse, earlier.reports, reports), func(report keptKeyReport) bool { + return replacedByName(report.record) + }) + conditionChanged = setKeptKeyConditions(provisioner, reports) + } + if conditionChanged || !slices.Equal(kept, statusSnapshot.PendingKeys) { provisioner.Status.PendingKeys = kept if err := r.Status().Update(ctx, provisioner); err != nil { return ctrl.Result{}, fmt.Errorf("record pending provisioner keys in status: %w", err) @@ -1026,44 +1037,56 @@ func (r *CoderProvisionerReconciler) revokeKey( return keyUnverified, nil } +// keptKeyReport is the line that names a kept pending record outside the +// target slot in the ControlPlaneChangedKeyKept condition, or, with a note, in +// the UnverifiedKeyKept condition. +type keptKeyReport struct { + record coderv1alpha1.CoderProvisionerKeyRecord + controlPlaneChanged bool + note string +} + // revokePendingKeys deletes the keys of the records in pending outside the // target slot, and returns the records to keep: the target slot, the records // that keep reports, and the records whose key Coder did not confirm gone. It -// sets the UnverifiedKeyKept and ControlPlaneChangedKeyKept conditions for the -// records it tried, and reports whether the conditions changed. When keep -// holds back a record outside the target slot, the conditions could not name -// that record, so they stay as they are. +// also returns the reports of the kept records outside the target slot. For a +// record that keep reports, it sends no request to Coder. func (r *CoderProvisionerReconciler) revokePendingKeys( ctx context.Context, provisioner *coderv1alpha1.CoderProvisioner, pending []coderv1alpha1.CoderProvisionerKeyRecord, target coderv1alpha1.CoderProvisionerKeyRecord, keep func(coderv1alpha1.CoderProvisionerKeyRecord) bool, -) ([]coderv1alpha1.CoderProvisionerKeyRecord, bool, error) { +) ([]coderv1alpha1.CoderProvisionerKeyRecord, []keptKeyReport, error) { var kept []coderv1alpha1.CoderProvisionerKeyRecord - var unverified, controlPlaneChanged []string - heldBack := false + var reports []keptKeyReport for _, record := range pending { if sameKeySlot(record, target) { kept = append(kept, record) continue } + cleanup := keyKept if keep(record) { - kept = append(kept, record) - heldBack = true - continue - } - cleanup, err := r.revokeKey(ctx, provisioner, record) - if err != nil { - return nil, false, err - } - if cleanup == keyGone { - continue + controlPlane, ok, err := r.readControlPlane(ctx, provisioner.Namespace, record.ControlPlaneRefName) + if err != nil { + return nil, nil, err + } + if ok && !tokenBoundTo(record, controlPlane) { + cleanup = keyControlPlaneChanged + } + } else { + var err error + cleanup, err = r.revokeKey(ctx, provisioner, record) + if err != nil { + return nil, nil, err + } + if cleanup == keyGone { + continue + } } kept = append(kept, record) if cleanup == keyControlPlaneChanged { - controlPlaneChanged = append(controlPlaneChanged, fmt.Sprintf("key %q in organization %q at %s (control plane %q)", - record.KeyName, record.OrganizationName, record.CoderURL, record.ControlPlaneRefName)) + reports = append(reports, keptKeyReport{record: record, controlPlaneChanged: true}) continue } if _, _, ok := parseKeyIDs(record.OrganizationID, record.KeyID); !ok { @@ -1071,46 +1094,83 @@ func (r *CoderProvisionerReconciler) revokePendingKeys( if cleanup == keyUnverified { note = "Coder holds a key with this name that nothing shows this provisioner created" } - unverified = append(unverified, fmt.Sprintf("key %q in organization %q at %s: %s", record.KeyName, record.OrganizationName, record.CoderURL, note)) + reports = append(reports, keptKeyReport{record: record, note: note}) } } - if heldBack { - return kept, false, nil + return kept, reports, nil +} + +// mergeKeptKeyReports returns, in the order of kept, the reports of the +// records in use from replacement, and of the other records from earlier. +func mergeKeptKeyReports( + kept []coderv1alpha1.CoderProvisionerKeyRecord, + inUse func(coderv1alpha1.CoderProvisionerKeyRecord) bool, + earlier, replacement []keptKeyReport, +) []keptKeyReport { + var merged []keptKeyReport + for _, record := range kept { + from := replacement + if !inUse(record) { + from = earlier + } + if i := slices.IndexFunc(from, func(report keptKeyReport) bool { return report.record == record }); i >= 0 { + merged = append(merged, from[i]) + } + } + return merged +} + +// setKeptKeyConditions sets the UnverifiedKeyKept and +// ControlPlaneChangedKeyKept conditions from reports, and reports whether the +// conditions changed. +func setKeptKeyConditions(provisioner *coderv1alpha1.CoderProvisioner, reports []keptKeyReport) bool { + var unverified, controlPlaneChanged []string + for _, report := range reports { + record := report.record + if report.controlPlaneChanged { + controlPlaneChanged = append(controlPlaneChanged, fmt.Sprintf("key %q in organization %q at %s (control plane %q)", + record.KeyName, record.OrganizationName, record.CoderURL, record.ControlPlaneRefName)) + continue + } + unverified = append(unverified, fmt.Sprintf("key %q in organization %q at %s: %s", record.KeyName, record.OrganizationName, record.CoderURL, report.note)) } conditionChanged := setUnverifiedKeyCondition(provisioner, unverified) - conditionChanged = setControlPlaneChangedKeyCondition(provisioner, controlPlaneChanged) || conditionChanged - return kept, conditionChanged, nil + return setControlPlaneChangedKeyCondition(provisioner, controlPlaneChanged) || conditionChanged +} + +// earlierKeyCleanup is the result of revokeEarlierTargetKeys. +type earlierKeyCleanup struct { + // tried is false while the retry interval of an earlier try runs. + tried bool + // reports are the reports of the kept records outside the target slot. + reports []keptKeyReport } // revokeEarlierTargetKeys deletes the keys of the status.pendingKeys records -// outside target, each with the operator token of its own control plane (see -// revokeKey), and writes the result to status. It keeps the records of the -// key in the Secret and of unpublished keys, which the Secret can hold, -// because the running Deployment can still use them. After a try that leaves -// a key that Coder did not confirm gone, it does not try again for -// provisionerPendingKeyRetryInterval, so an unreachable earlier endpoint does -// not get a request on every reconcile. It reports whether it updated status. +// outside target that are not in use, each with the operator token of its own +// control plane (see revokeKey). It updates status.pendingKeys and the +// conditions of the kept records, and writes nothing to the API server. After +// a try that leaves a key that Coder did not confirm gone, it does not try +// again for provisionerPendingKeyRetryInterval, so an unreachable earlier +// endpoint does not get a request on every reconcile. func (r *CoderProvisionerReconciler) revokeEarlierTargetKeys( ctx context.Context, provisioner *coderv1alpha1.CoderProvisioner, target coderv1alpha1.CoderProvisionerKeyRecord, -) (bool, error) { + inUse func(coderv1alpha1.CoderProvisionerKeyRecord) bool, +) (earlierKeyCleanup, error) { if value, ok := r.earlierKeyRetryAt.Load(provisioner.UID); ok { retryAt, ok := value.(time.Time) if !ok { - return false, fmt.Errorf("assertion failed: earlier key retry time of provisioner %s is %T, not time.Time", provisioner.UID, value) + return earlierKeyCleanup{}, fmt.Errorf("assertion failed: earlier key retry time of provisioner %s is %T, not time.Time", provisioner.UID, value) } if time.Now().Before(retryAt) { - return false, nil + return earlierKeyCleanup{}, nil } } - appliedKeyID := provisioner.Status.ProvisionerKeyID - inUse := func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { - return record.Unpublished || (record.KeyID != "" && record.KeyID == appliedKeyID) - } - kept, conditionChanged, err := r.revokePendingKeys(ctx, provisioner, provisioner.Status.PendingKeys, target, inUse) + kept, reports, err := r.revokePendingKeys(ctx, provisioner, provisioner.Status.PendingKeys, target, inUse) if err != nil { - return false, err + return earlierKeyCleanup{}, err } if slices.ContainsFunc(kept, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { return !sameKeySlot(record, target) && !inUse(record) @@ -1119,14 +1179,9 @@ func (r *CoderProvisionerReconciler) revokeEarlierTargetKeys( } else { r.earlierKeyRetryAt.Delete(provisioner.UID) } - if !conditionChanged && slices.Equal(kept, provisioner.Status.PendingKeys) { - return false, nil - } provisioner.Status.PendingKeys = kept - if err := r.Status().Update(ctx, provisioner); err != nil { - return false, fmt.Errorf("record deleted provisioner keys of earlier targets in status: %w", err) - } - return true, nil + setKeptKeyConditions(provisioner, reports) + return earlierKeyCleanup{tried: true, reports: reports}, nil } // setUnverifiedKeyCondition sets the UnverifiedKeyKept condition for the diff --git a/internal/controller/coderprovisioner_earliertargets_test.go b/internal/controller/coderprovisioner_earliertargets_test.go index c43c1094..22604f81 100644 --- a/internal/controller/coderprovisioner_earliertargets_test.go +++ b/internal/controller/coderprovisioner_earliertargets_test.go @@ -11,6 +11,7 @@ import ( "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" @@ -22,13 +23,14 @@ import ( "github.com/coder/coder-k8s/internal/controller" ) -// coderCallLog records the Coder URL of every call, and can fail the deletes -// of one key. +// coderCallLog records the Coder URL of every call, can fail the deletes of +// one key, and can fail every key lookup by name. type coderCallLog struct { coderbootstrap.Client urls []string failDelete uuid.UUID failedDeletes int + failLookup bool } func (c *coderCallLog) EnsureProvisionerKey(ctx context.Context, req coderbootstrap.EnsureProvisionerKeyRequest) (coderbootstrap.EnsureProvisionerKeyResponse, error) { @@ -52,6 +54,9 @@ func (c *coderCallLog) ProvisionerKeyByID(ctx context.Context, req coderbootstra func (c *coderCallLog) ProvisionerKeyByName(ctx context.Context, req coderbootstrap.ProvisionerKeyByNameRequest) (*codersdk.ProvisionerKey, error) { c.urls = append(c.urls, req.CoderURL) + if c.failLookup { + return nil, errors.New("injected lookup failure") + } return c.Client.ProvisionerKeyByName(ctx, req) } @@ -65,7 +70,8 @@ func (c *coderCallLog) Entitlements(ctx context.Context, coderURL, sessionToken // When the provisioner moves on to a control plane C that is not ready, the // key at A must still be deleted at A with the operator token of A, and C must // get no request. Key B, which the Secret holds, must stay. A failed delete is -// retried at the pending key cadence, not on every reconcile. When the Secret +// retried at the pending key cadence, not on every reconcile, also by the +// rotation to a ready C. When the Secret // write at B failed instead, the unpublished key B, which the Secret can hold, // must stay until C is ready. func TestCoderProvisionerKey_EarlierTargetRevokedWhileTargetNotReady(t *testing.T) { @@ -73,7 +79,8 @@ func TestCoderProvisionerKey_EarlierTargetRevokedWhileTargetNotReady(t *testing. tests := []struct { name string - // notReady makes control plane C fail one of its checks. + // notReady makes control plane C fail one of its checks. When it is + // nil, C is ready. notReady func(ctx context.Context, t *testing.T, namespace string) // deleteFails fails every delete of key A at C. deleteFails bool @@ -84,6 +91,7 @@ func TestCoderProvisionerKey_EarlierTargetRevokedWhileTargetNotReady(t *testing. {name: "target not entitled", notReady: markNotEntitled}, {name: "target operator token missing", notReady: deleteOperatorToken}, {name: "delete fails at the earlier target", notReady: markNotEntitled, deleteFails: true}, + {name: "delete fails at the earlier target, target ready", deleteFails: true}, {name: "unpublished key at the earlier target", notReady: markNotEntitled, unpublishedAtB: true}, } @@ -99,7 +107,8 @@ func TestCoderProvisionerKey_EarlierTargetRevokedWhileTargetNotReady(t *testing. } keyA := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: uuid.New(), KeyID: uuid.New(), KeyName: "provisioner", Key: "material-a"} keyB := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: uuid.New(), KeyID: uuid.New(), KeyName: "provisioner", Key: "material-b"} - f.bootstrapClient.provisionerKeyResponses = []coderbootstrap.EnsureProvisionerKeyResponse{keyA, keyB} + keyC := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: uuid.New(), KeyID: uuid.New(), KeyName: "provisioner", Key: "material-c"} + f.bootstrapClient.provisionerKeyResponses = []coderbootstrap.EnsureProvisionerKeyResponse{keyA, keyB, keyC} calls := &coderCallLog{Client: f.bootstrapClient} f.reconciler.BootstrapClient = calls @@ -152,30 +161,37 @@ func TestCoderProvisionerKey_EarlierTargetRevokedWhileTargetNotReady(t *testing. calls.failedDeletes = 0 calls.urls = nil f.bootstrapClient.deleteKeyByIDRequests = nil - tt.notReady(ctx, t, f.namespace) + if tt.notReady != nil { + tt.notReady(ctx, t, f.namespace) + } moveTo("coder-c") reconcile() if tt.deleteFails { reconcile() } - require.NotContains(t, calls.urls, apiURL("coder-c"), "control plane C must get no request while it is not ready") - require.NotContains(t, keyDeletes(f.bootstrapClient), keyDelete{coderURL: apiURL("coder-b"), token: "operator-token-b", keyID: keyB.KeyID.String()}, - "nothing may delete key B, which the Secret holds or can hold") + if tt.notReady != nil { + require.NotContains(t, calls.urls, apiURL("coder-c"), "control plane C must get no request while it is not ready") + require.NotContains(t, keyDeletes(f.bootstrapClient), keyDelete{coderURL: apiURL("coder-b"), token: "operator-token-b", keyID: keyB.KeyID.String()}, + "nothing may delete key B, which the Secret holds or can hold, while C is not ready") + } final := f.get(ctx, t, provisioner) if tt.unpublishedAtB { require.Empty(t, keyDeletes(f.bootstrapClient)) require.True(t, pendingKeyRecorded(final, keyB.KeyID), "the record of the unpublished key B must stay") return } - require.Equal(t, keyB.KeyID.String(), final.Status.ProvisionerKeyID) + applied := keyB + if tt.notReady == nil { + applied = keyC + } + require.Equal(t, applied.KeyID.String(), final.Status.ProvisionerKeyID) if tt.deleteFails { require.Equal(t, 1, calls.failedDeletes, "a failed delete must wait for the pending key cadence") require.True(t, pendingKeyRecorded(final, keyA.KeyID), "a failed delete must keep the record of key A") controller.ElapseEarlierKeyRetryInterval(f.reconciler, final.UID) reconcile() require.Equal(t, 2, calls.failedDeletes, "the delete must be tried again after the pending key cadence") - require.NotContains(t, calls.urls, apiURL("coder-c")) return } require.Equal(t, []keyDelete{{coderURL: apiURL("coder"), token: testOperatorToken, keyID: keyA.KeyID.String()}}, keyDeletes(f.bootstrapClient), @@ -185,6 +201,79 @@ func TestCoderProvisionerKey_EarlierTargetRevokedWhileTargetNotReady(t *testing. } } +// TestCoderProvisionerKey_EarlierTargetReportsBesideUnpublishedKey checks the +// reports of earlier target records while an unpublished record waits for a +// control plane C that is not ready. A record whose control plane was created +// again must be reported, and the report of a record without key IDs must go +// once Coder has no key with its name. +func TestCoderProvisionerKey_EarlierTargetReportsBesideUnpublishedKey(t *testing.T) { + t.Parallel() + ctx := context.Background() + f := newProvisionerCredentialFixture(ctx, t, "cprov-earlier-reports") + createControlPlaneWithToken(ctx, t, f.namespace, "coder-b", "https://coder-b.example.com", "operator-token-b") + createControlPlaneWithToken(ctx, t, f.namespace, "coder-c", "https://coder-c.example.com", "operator-token-c") + calls := &coderCallLog{Client: f.bootstrapClient} + f.reconciler.BootstrapClient = calls + provisioner := f.createProvisioner(ctx, t, nil, nil) + name := types.NamespacedName{Name: provisioner.Name, Namespace: provisioner.Namespace} + f.reconcile(ctx, t, provisioner) + f.reconcile(ctx, t, provisioner) + calls.failLookup = true + + controlPlaneB := &coderv1alpha1.CoderControlPlane{} + require.NoError(t, k8sClient.Get(ctx, types.NamespacedName{Name: "coder-b", Namespace: f.namespace}, controlPlaneB)) + record := func(keyName string, withIDs bool) coderv1alpha1.CoderProvisionerKeyRecord { + record := coderv1alpha1.CoderProvisionerKeyRecord{ + ControlPlaneRefName: "coder-b", + ControlPlaneUID: string(controlPlaneB.UID), + CoderURL: "http://coder-b." + f.namespace + ".svc.cluster.local:80", + OrganizationName: "default", + KeyName: keyName, + } + if withIDs { + record.OrganizationID = uuid.NewString() + record.KeyID = uuid.NewString() + } + return record + } + recreated := record("recreated", true) + recreated.ControlPlaneUID = uuid.NewString() + unpublished := record("unpublished", true) + unpublished.Unpublished = true + idLess := record("id-less", false) + latest := f.get(ctx, t, provisioner) + latest.Status.PendingKeys = []coderv1alpha1.CoderProvisionerKeyRecord{recreated, unpublished, idLess} + require.NoError(t, k8sClient.Status().Update(ctx, latest)) + markNotEntitled(ctx, t, f.namespace) + latest = f.get(ctx, t, provisioner) + latest.Spec.ControlPlaneRef.Name = "coder-c" + require.NoError(t, k8sClient.Update(ctx, latest)) + reconcile := func() *coderv1alpha1.CoderProvisioner { + _, err := f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: name}) + require.NoError(t, err) + return f.get(ctx, t, provisioner) + } + + got := reconcile() + require.Equal(t, []coderv1alpha1.CoderProvisionerKeyRecord{recreated, unpublished, idLess}, got.Status.PendingKeys) + changed := meta.FindStatusCondition(got.Status.Conditions, coderv1alpha1.CoderProvisionerConditionControlPlaneChangedKeyKept) + require.NotNil(t, changed, "the record whose control plane was created again must be reported") + require.Contains(t, changed.Message, `"recreated"`) + require.NotContains(t, changed.Message, `"unpublished"`) + unverified := meta.FindStatusCondition(got.Status.Conditions, coderv1alpha1.CoderProvisionerConditionUnverifiedKeyKept) + require.NotNil(t, unverified, "the record without key IDs must be reported") + require.Contains(t, unverified.Message, `"id-less"`) + + calls.failLookup = false + controller.ElapseEarlierKeyRetryInterval(f.reconciler, got.UID) + got = reconcile() + require.Equal(t, []coderv1alpha1.CoderProvisionerKeyRecord{recreated, unpublished}, got.Status.PendingKeys, + "the record without key IDs must go once Coder has no key with its name") + require.Nil(t, meta.FindStatusCondition(got.Status.Conditions, coderv1alpha1.CoderProvisionerConditionUnverifiedKeyKept)) + require.NotNil(t, meta.FindStatusCondition(got.Status.Conditions, coderv1alpha1.CoderProvisionerConditionControlPlaneChangedKeyKept)) + require.Empty(t, keyDeletes(f.bootstrapClient), "no recorded key may be deleted: one has another control plane, one is unpublished") +} + func markNotEntitled(ctx context.Context, t *testing.T, namespace string) { t.Helper() controlPlane := &coderv1alpha1.CoderControlPlane{} From 921a70706adb39ffd1b20047d1123166bbfb9879 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 23:39:40 +0000 Subject: [PATCH 4/7] test: report an unpublished earlier target record whose control plane changed (#416) Change-Id: Ib83d7afcd7fc9a5800d42fec3197ab3f7900f05d Signed-off-by: Thomas Kosiewski --- .../coderprovisioner_earliertargets_test.go | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/internal/controller/coderprovisioner_earliertargets_test.go b/internal/controller/coderprovisioner_earliertargets_test.go index 22604f81..233cb33d 100644 --- a/internal/controller/coderprovisioner_earliertargets_test.go +++ b/internal/controller/coderprovisioner_earliertargets_test.go @@ -204,8 +204,8 @@ func TestCoderProvisionerKey_EarlierTargetRevokedWhileTargetNotReady(t *testing. // TestCoderProvisionerKey_EarlierTargetReportsBesideUnpublishedKey checks the // reports of earlier target records while an unpublished record waits for a // control plane C that is not ready. A record whose control plane was created -// again must be reported, and the report of a record without key IDs must go -// once Coder has no key with its name. +// again must be reported, also an unpublished one, and the report of a record +// without key IDs must go once Coder has no key with its name. func TestCoderProvisionerKey_EarlierTargetReportsBesideUnpublishedKey(t *testing.T) { t.Parallel() ctx := context.Background() @@ -240,9 +240,12 @@ func TestCoderProvisionerKey_EarlierTargetReportsBesideUnpublishedKey(t *testing recreated.ControlPlaneUID = uuid.NewString() unpublished := record("unpublished", true) unpublished.Unpublished = true + unpublishedRecreated := record("unpublished-recreated", true) + unpublishedRecreated.Unpublished = true + unpublishedRecreated.ControlPlaneUID = recreated.ControlPlaneUID idLess := record("id-less", false) latest := f.get(ctx, t, provisioner) - latest.Status.PendingKeys = []coderv1alpha1.CoderProvisionerKeyRecord{recreated, unpublished, idLess} + latest.Status.PendingKeys = []coderv1alpha1.CoderProvisionerKeyRecord{recreated, unpublished, unpublishedRecreated, idLess} require.NoError(t, k8sClient.Status().Update(ctx, latest)) markNotEntitled(ctx, t, f.namespace) latest = f.get(ctx, t, provisioner) @@ -255,10 +258,11 @@ func TestCoderProvisionerKey_EarlierTargetReportsBesideUnpublishedKey(t *testing } got := reconcile() - require.Equal(t, []coderv1alpha1.CoderProvisionerKeyRecord{recreated, unpublished, idLess}, got.Status.PendingKeys) + require.Equal(t, []coderv1alpha1.CoderProvisionerKeyRecord{recreated, unpublished, unpublishedRecreated, idLess}, got.Status.PendingKeys) changed := meta.FindStatusCondition(got.Status.Conditions, coderv1alpha1.CoderProvisionerConditionControlPlaneChangedKeyKept) require.NotNil(t, changed, "the record whose control plane was created again must be reported") require.Contains(t, changed.Message, `"recreated"`) + require.Contains(t, changed.Message, `"unpublished-recreated"`) require.NotContains(t, changed.Message, `"unpublished"`) unverified := meta.FindStatusCondition(got.Status.Conditions, coderv1alpha1.CoderProvisionerConditionUnverifiedKeyKept) require.NotNil(t, unverified, "the record without key IDs must be reported") @@ -267,7 +271,7 @@ func TestCoderProvisionerKey_EarlierTargetReportsBesideUnpublishedKey(t *testing calls.failLookup = false controller.ElapseEarlierKeyRetryInterval(f.reconciler, got.UID) got = reconcile() - require.Equal(t, []coderv1alpha1.CoderProvisionerKeyRecord{recreated, unpublished}, got.Status.PendingKeys, + require.Equal(t, []coderv1alpha1.CoderProvisionerKeyRecord{recreated, unpublished, unpublishedRecreated}, got.Status.PendingKeys, "the record without key IDs must go once Coder has no key with its name") require.Nil(t, meta.FindStatusCondition(got.Status.Conditions, coderv1alpha1.CoderProvisionerConditionUnverifiedKeyKept)) require.NotNil(t, meta.FindStatusCondition(got.Status.Conditions, coderv1alpha1.CoderProvisionerConditionControlPlaneChangedKeyKept)) From 63990632d203753682ba7e1abda098bb1816d362 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 23:55:09 +0000 Subject: [PATCH 5/7] fix: persist earlier target key drops at once and pin fail-static credentials (#416) Change-Id: I433401d5e847d6251ed48ae6592d28b7d5e4650c Signed-off-by: Thomas Kosiewski --- docs/tutorials/deploy-coderprovisioner.md | 2 +- .../controller/coderprovisioner_controller.go | 25 ++++++++++++---- .../coderprovisioner_earliertargets_test.go | 29 ++++++++++++++----- 3 files changed, 43 insertions(+), 13 deletions(-) diff --git a/docs/tutorials/deploy-coderprovisioner.md b/docs/tutorials/deploy-coderprovisioner.md index e84b8b29..6a9a0ceb 100644 --- a/docs/tutorials/deploy-coderprovisioner.md +++ b/docs/tutorials/deploy-coderprovisioner.md @@ -82,7 +82,7 @@ kubectl delete coderprovisioner coderprovisioner-sample -n coder The operator then deletes the provisioner keys in Coder that this provisioner created. It finds the key in the provisioner key Secret by the organization and key IDs in `status.organizationID` and `status.provisionerKeyID`, not by the names in the spec. It also deletes the key of each entry in `status.pendingKeys` that has an `organizationID` and a `keyID`. The entries name keys that the operator created but did not write to the Secret, for example because a reconcile failed, and keys that a rotation replaced but that Coder did not confirm as deleted. Each entry names the Coder URL of the key and the control plane whose operator token the operator uses to delete it. An entry without these IDs names a key that the operator started to create before Coder returned its ID. For such an entry, the operator deletes no key, because another provisioner or a person can use the same key name in that organization. It removes the entry when Coder has no key with that name, or when that key is the key in `status.provisionerKeyID`. Otherwise, and while the operator cannot reach Coder for that check, the entry stays, the condition `UnverifiedKeyKept` is `True` and names the entry, and deletion of the provisioner leaves the key in Coder. If no other provisioner or person uses the key, delete it with `coder provisioner keys delete --org `. The operator checks again every 2 minutes, and removes the entry and the condition once the key is gone. When the status has no key, for example because the provisioner never passed its checks, the operator removes its finalizer without a call to Coder. -The same records apply while the provisioner runs. When `spec.controlPlaneRef`, the control plane URL, the organization, the key name, or the tags change, the operator creates a new key and deletes the replaced keys by their recorded organization and key IDs, each at its own Coder URL, so a renamed Coder organization does not hide a replaced key. When a rotation fails after Coder created the new key, the next reconcile writes another new key to the Secret, also when you reverted the change. An entry stays in `status.pendingKeys` until Coder confirms that its key is gone, and the operator tries again every 2 minutes. The operator also deletes the keys of entries for earlier targets while the current target is not ready, for example while its operator token is missing or its Coder deployment is not entitled to external provisioners, and sends nothing to the current target then. An entry with `unpublished: true`, and an entry for the key in the Secret, waits for the current target, because the running Deployment can still use its key. Each entry and `status.controlPlaneUID` also record the UID of the control plane. The operator sends the operator token of a control plane for a record only while the control plane has the recorded UID. When a control plane is deleted and created again under the same name, the new one can run another Coder backend, also at the same URL. The operator then creates a new key for the provisioner, and sends the new token for no key of the old control plane. Such an entry stays, the condition `ControlPlaneChangedKeyKept` is `True` and names it, and deletion of the provisioner leaves that key in Coder. A record without a UID, from an operator version that did not record it, counts as bound to its control plane only at the current URL of that control plane. For status without `status.controlPlaneUID`, the next reconcile looks the applied key up at the current control plane by `status.organizationID` and `status.provisionerKeyID`, so a renamed organization does not hide it. When Coder holds the key there, the operator records the UID of that control plane. When Coder answers that it has no such key, the operator creates a new key. When a change of the spec or the control plane replaces the key of such status, its entry in `status.pendingKeys` records the UID of its control plane while that control plane has the recorded URL. An entry whose control plane was deleted, or that `ControlPlaneChangedKeyKept` names, stays for good: delete its key by hand in Coder, then remove the entry, for example the first one with `kubectl patch coderprovisioner coderprovisioner-sample -n coder --subresource=status --type=json -p '[{"op":"remove","path":"/status/pendingKeys/0"}]'`. +The same records apply while the provisioner runs. When `spec.controlPlaneRef`, the control plane URL, the organization, the key name, or the tags change, the operator creates a new key and deletes the replaced keys by their recorded organization and key IDs, each at its own Coder URL, so a renamed Coder organization does not hide a replaced key. When a rotation fails after Coder created the new key, the next reconcile writes another new key to the Secret, also when you reverted the change. An entry stays in `status.pendingKeys` until Coder confirms that its key is gone, and the operator tries again every 2 minutes. The operator also deletes the keys of entries for earlier targets while the current target is not ready, for example while its operator token is missing or its Coder deployment is not entitled to external provisioners. Each delete goes to the Coder URL of the entry, with the operator token of its control plane. While `CredentialsReady` is `False` or the control plane is missing, the operator sends nothing to Coder, so these keys stay. An entry with `unpublished: true`, and an entry for the key in the Secret, waits for the current target, because the running Deployment can still use its key. Each entry and `status.controlPlaneUID` also record the UID of the control plane. The operator sends the operator token of a control plane for a record only while the control plane has the recorded UID. When a control plane is deleted and created again under the same name, the new one can run another Coder backend, also at the same URL. The operator then creates a new key for the provisioner, and sends the new token for no key of the old control plane. Such an entry stays, the condition `ControlPlaneChangedKeyKept` is `True` and names it, and deletion of the provisioner leaves that key in Coder. A record without a UID, from an operator version that did not record it, counts as bound to its control plane only at the current URL of that control plane. For status without `status.controlPlaneUID`, the next reconcile looks the applied key up at the current control plane by `status.organizationID` and `status.provisionerKeyID`, so a renamed organization does not hide it. When Coder holds the key there, the operator records the UID of that control plane. When Coder answers that it has no such key, the operator creates a new key. When a change of the spec or the control plane replaces the key of such status, its entry in `status.pendingKeys` records the UID of its control plane while that control plane has the recorded URL. An entry whose control plane was deleted, or that `ControlPlaneChangedKeyKept` names, stays for good: delete its key by hand in Coder, then remove the entry, for example the first one with `kubectl patch coderprovisioner coderprovisioner-sample -n coder --subresource=status --type=json -p '[{"op":"remove","path":"/status/pendingKeys/0"}]'`. This cleanup is best-effort. When the operator cannot reach Coder, for example because the control plane or its operator token is gone, or when the delete call fails, the operator still removes its finalizer, and the key stays valid in Coder. Delete the provisioner while the control plane runs. If cleanup could not reach Coder, list the keys with `coder provisioner keys list --org `, and delete the key by hand with `coder provisioner keys delete --org `. diff --git a/internal/controller/coderprovisioner_controller.go b/internal/controller/coderprovisioner_controller.go index 25505fbb..fe6cb10d 100644 --- a/internal/controller/coderprovisioner_controller.go +++ b/internal/controller/coderprovisioner_controller.go @@ -233,15 +233,26 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req // cleanup does not wait for the checks of the current target below. The // key in the Secret and unpublished keys, which the Secret can hold, wait // for a replacement below, because the running Deployment can still use - // them. Every status write below records the result. - appliedKeyID := provisioner.Status.ProvisionerKeyID + // them. + appliedKeyID, _ := uuid.Parse(provisioner.Status.ProvisionerKeyID) inUse := func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { - return record.Unpublished || (record.KeyID != "" && record.KeyID == appliedKeyID) + return record.Unpublished || isKeyID(record.KeyID, appliedKeyID) } earlier, err := r.revokeEarlierTargetKeys(ctx, provisioner, target, inUse) if err != nil { return ctrl.Result{}, err } + // The conditions go with the next status write, so that a replacement + // below can update them first. + if !slices.Equal(provisioner.Status.PendingKeys, statusSnapshot.PendingKeys) { + if err := r.Status().Update(ctx, provisioner); err != nil { + return ctrl.Result{}, fmt.Errorf("record deleted provisioner keys of earlier targets in status: %w", err) + } + statusSnapshot = provisioner.Status.DeepCopy() + if statusSnapshot == nil { + return ctrl.Result{}, fmt.Errorf("assertion failed: status snapshot must not be nil after earlier target key status update") + } + } sessionToken, operatorAccessResult, operatorAccessReason, operatorAccessMessage, err := r.readOperatorSessionToken(ctx, provisioner, controlPlane) if err != nil { @@ -1066,7 +1077,8 @@ func (r *CoderProvisionerReconciler) revokePendingKeys( continue } cleanup := keyKept - if keep(record) { + heldBack := keep(record) + if heldBack { controlPlane, ok, err := r.readControlPlane(ctx, provisioner.Namespace, record.ControlPlaneRefName) if err != nil { return nil, nil, err @@ -1091,6 +1103,9 @@ func (r *CoderProvisionerReconciler) revokePendingKeys( } if _, _, ok := parseKeyIDs(record.OrganizationID, record.KeyID); !ok { note := "the operator could not check Coder" + if heldBack { + note = "the operator checks Coder for it at the next key replacement" + } if cleanup == keyUnverified { note = "Coder holds a key with this name that nothing shows this provisioner created" } @@ -1149,7 +1164,7 @@ type earlierKeyCleanup struct { // revokeEarlierTargetKeys deletes the keys of the status.pendingKeys records // outside target that are not in use, each with the operator token of its own // control plane (see revokeKey). It updates status.pendingKeys and the -// conditions of the kept records, and writes nothing to the API server. After +// conditions of the kept records in provisioner only. After // a try that leaves a key that Coder did not confirm gone, it does not try // again for provisionerPendingKeyRetryInterval, so an unreachable earlier // endpoint does not get a request on every reconcile. diff --git a/internal/controller/coderprovisioner_earliertargets_test.go b/internal/controller/coderprovisioner_earliertargets_test.go index 233cb33d..d35c2134 100644 --- a/internal/controller/coderprovisioner_earliertargets_test.go +++ b/internal/controller/coderprovisioner_earliertargets_test.go @@ -81,7 +81,7 @@ func TestCoderProvisionerKey_EarlierTargetRevokedWhileTargetNotReady(t *testing. name string // notReady makes control plane C fail one of its checks. When it is // nil, C is ready. - notReady func(ctx context.Context, t *testing.T, namespace string) + notReady func(ctx context.Context, t *testing.T, f provisionerCredentialFixture, provisioner *coderv1alpha1.CoderProvisioner) // deleteFails fails every delete of key A at C. deleteFails bool // unpublishedAtB fails the Secret write at B instead of the delete @@ -89,6 +89,7 @@ func TestCoderProvisionerKey_EarlierTargetRevokedWhileTargetNotReady(t *testing. unpublishedAtB bool }{ {name: "target not entitled", notReady: markNotEntitled}, + {name: "credentials refused", notReady: refuseCredentials}, {name: "target operator token missing", notReady: deleteOperatorToken}, {name: "delete fails at the earlier target", notReady: markNotEntitled, deleteFails: true}, {name: "delete fails at the earlier target, target ready", deleteFails: true}, @@ -162,7 +163,7 @@ func TestCoderProvisionerKey_EarlierTargetRevokedWhileTargetNotReady(t *testing. calls.urls = nil f.bootstrapClient.deleteKeyByIDRequests = nil if tt.notReady != nil { - tt.notReady(ctx, t, f.namespace) + tt.notReady(ctx, t, f, provisioner) } moveTo("coder-c") reconcile() @@ -176,6 +177,11 @@ func TestCoderProvisionerKey_EarlierTargetRevokedWhileTargetNotReady(t *testing. "nothing may delete key B, which the Secret holds or can hold, while C is not ready") } final := f.get(ctx, t, provisioner) + if tt.name == "credentials refused" { + require.Empty(t, calls.urls, "a failed credential check must send nothing to Coder") + require.True(t, pendingKeyRecorded(final, keyA.KeyID), "the record of key A must stay") + return + } if tt.unpublishedAtB { require.Empty(t, keyDeletes(f.bootstrapClient)) require.True(t, pendingKeyRecorded(final, keyB.KeyID), "the record of the unpublished key B must stay") @@ -247,7 +253,7 @@ func TestCoderProvisionerKey_EarlierTargetReportsBesideUnpublishedKey(t *testing latest := f.get(ctx, t, provisioner) latest.Status.PendingKeys = []coderv1alpha1.CoderProvisionerKeyRecord{recreated, unpublished, unpublishedRecreated, idLess} require.NoError(t, k8sClient.Status().Update(ctx, latest)) - markNotEntitled(ctx, t, f.namespace) + markNotEntitled(ctx, t, f, provisioner) latest = f.get(ctx, t, provisioner) latest.Spec.ControlPlaneRef.Name = "coder-c" require.NoError(t, k8sClient.Update(ctx, latest)) @@ -278,17 +284,26 @@ func TestCoderProvisionerKey_EarlierTargetReportsBesideUnpublishedKey(t *testing require.Empty(t, keyDeletes(f.bootstrapClient), "no recorded key may be deleted: one has another control plane, one is unpublished") } -func markNotEntitled(ctx context.Context, t *testing.T, namespace string) { +func markNotEntitled(ctx context.Context, t *testing.T, f provisionerCredentialFixture, _ *coderv1alpha1.CoderProvisioner) { t.Helper() controlPlane := &coderv1alpha1.CoderControlPlane{} - require.NoError(t, k8sClient.Get(ctx, types.NamespacedName{Name: "coder-c", Namespace: namespace}, controlPlane)) + require.NoError(t, k8sClient.Get(ctx, types.NamespacedName{Name: "coder-c", Namespace: f.namespace}, controlPlane)) now := metav1.NewTime(time.Now()) controlPlane.Status.EntitlementsLastChecked = &now controlPlane.Status.ExternalProvisionerDaemonsEntitlement = string(codersdk.EntitlementNotEntitled) require.NoError(t, k8sClient.Status().Update(ctx, controlPlane)) } -func deleteOperatorToken(ctx context.Context, t *testing.T, namespace string) { +func deleteOperatorToken(ctx context.Context, t *testing.T, f provisionerCredentialFixture, _ *coderv1alpha1.CoderProvisioner) { t.Helper() - require.NoError(t, k8sClient.Delete(ctx, &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "coder-c-operator-token", Namespace: namespace}})) + require.NoError(t, k8sClient.Delete(ctx, &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "coder-c-operator-token", Namespace: f.namespace}})) +} + +// refuseCredentials adds an extraEnv Secret without the credential opt-in. +func refuseCredentials(ctx context.Context, t *testing.T, f provisionerCredentialFixture, provisioner *coderv1alpha1.CoderProvisioner) { + t.Helper() + createSecret(ctx, t, tokenSecret(metav1.ObjectMeta{Name: "extra", Namespace: f.namespace}, "value", testExtraEnvValue)) + latest := f.get(ctx, t, provisioner) + latest.Spec.ExtraEnv = []corev1.EnvVar{secretEnv("EXTRA", "extra", "value")} + require.NoError(t, k8sClient.Update(ctx, latest)) } From 1554c176d1e6f77e5a10207f0bcfdad02e84581c Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Sat, 31 Oct 2026 00:28:39 +0000 Subject: [PATCH 6/7] chore: clarify when earlier target key results are written (#416) Change-Id: Iee9b78897e948fc5e62bda77fd7f992ce1d4421b Signed-off-by: Thomas Kosiewski --- internal/controller/coderprovisioner_controller.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/internal/controller/coderprovisioner_controller.go b/internal/controller/coderprovisioner_controller.go index fe6cb10d..808c53d3 100644 --- a/internal/controller/coderprovisioner_controller.go +++ b/internal/controller/coderprovisioner_controller.go @@ -242,8 +242,9 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req if err != nil { return ctrl.Result{}, err } - // The conditions go with the next status write, so that a replacement - // below can update them first. + // A dropped record is written at once. A change of the reports alone + // waits for the next status write, so that a replacement below can + // update them first. if !slices.Equal(provisioner.Status.PendingKeys, statusSnapshot.PendingKeys) { if err := r.Status().Update(ctx, provisioner); err != nil { return ctrl.Result{}, fmt.Errorf("record deleted provisioner keys of earlier targets in status: %w", err) From b0cb20057ebabbf8527e15ff4ade3e198c4868bf Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Sat, 31 Oct 2026 00:34:17 +0000 Subject: [PATCH 7/7] fix: rebuild earlier target key reports during a replacement while the retry interval runs (#416) Change-Id: I6c50d1eabcdf1ee836f2bdcf86fd52c6584d1832 Signed-off-by: Thomas Kosiewski --- .../controller/coderprovisioner_controller.go | 54 +++++++++---------- .../coderprovisioner_earliertargets_test.go | 27 +++++++++- 2 files changed, 53 insertions(+), 28 deletions(-) diff --git a/internal/controller/coderprovisioner_controller.go b/internal/controller/coderprovisioner_controller.go index 808c53d3..7ab91d43 100644 --- a/internal/controller/coderprovisioner_controller.go +++ b/internal/controller/coderprovisioner_controller.go @@ -81,8 +81,8 @@ type CoderProvisionerReconciler struct { Scheme *runtime.Scheme BootstrapClient coderbootstrap.Client - // earlierKeyRetryAt maps a provisioner UID to the time.Time before which - // revokeEarlierTargetKeys does not try again. + // earlierKeyRetryAt maps a provisioner UID to the earlierKeyRetry of its + // last try in revokeEarlierTargetKeys. earlierKeyRetryAt sync.Map } @@ -238,7 +238,7 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req inUse := func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { return record.Unpublished || isKeyID(record.KeyID, appliedKeyID) } - earlier, err := r.revokeEarlierTargetKeys(ctx, provisioner, target, inUse) + earlierReports, err := r.revokeEarlierTargetKeys(ctx, provisioner, target, inUse) if err != nil { return ctrl.Result{}, err } @@ -438,13 +438,10 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req return ctrl.Result{}, err } // The publish step below drops the records of the key found by name. - conditionChanged := false - if earlier.tried { - reports = slices.DeleteFunc(mergeKeptKeyReports(kept, inUse, earlier.reports, reports), func(report keptKeyReport) bool { - return replacedByName(report.record) - }) - conditionChanged = setKeptKeyConditions(provisioner, reports) - } + reports = slices.DeleteFunc(mergeKeptKeyReports(kept, inUse, earlierReports, reports), func(report keptKeyReport) bool { + return replacedByName(report.record) + }) + conditionChanged := setKeptKeyConditions(provisioner, reports) if conditionChanged || !slices.Equal(kept, statusSnapshot.PendingKeys) { provisioner.Status.PendingKeys = kept if err := r.Status().Update(ctx, provisioner); err != nil { @@ -1154,50 +1151,53 @@ func setKeptKeyConditions(provisioner *coderv1alpha1.CoderProvisioner, reports [ return setControlPlaneChangedKeyCondition(provisioner, controlPlaneChanged) || conditionChanged } -// earlierKeyCleanup is the result of revokeEarlierTargetKeys. -type earlierKeyCleanup struct { - // tried is false while the retry interval of an earlier try runs. - tried bool - // reports are the reports of the kept records outside the target slot. +// earlierKeyRetry is the last try of revokeEarlierTargetKeys that left a key +// that Coder did not confirm gone. +type earlierKeyRetry struct { + // at is the time before which revokeEarlierTargetKeys does not try again. + at time.Time + // reports are the reports of that try. reports []keptKeyReport } // revokeEarlierTargetKeys deletes the keys of the status.pendingKeys records // outside target that are not in use, each with the operator token of its own // control plane (see revokeKey). It updates status.pendingKeys and the -// conditions of the kept records in provisioner only. After -// a try that leaves a key that Coder did not confirm gone, it does not try -// again for provisionerPendingKeyRetryInterval, so an unreachable earlier -// endpoint does not get a request on every reconcile. +// conditions of the kept records in provisioner only, and returns the reports +// of the kept records outside target. After a try that leaves a key that Coder +// did not confirm gone, it does not try again for +// provisionerPendingKeyRetryInterval, so an unreachable earlier endpoint does +// not get a request on every reconcile. Until then it changes nothing and +// returns the reports of that try. func (r *CoderProvisionerReconciler) revokeEarlierTargetKeys( ctx context.Context, provisioner *coderv1alpha1.CoderProvisioner, target coderv1alpha1.CoderProvisionerKeyRecord, inUse func(coderv1alpha1.CoderProvisionerKeyRecord) bool, -) (earlierKeyCleanup, error) { +) ([]keptKeyReport, error) { if value, ok := r.earlierKeyRetryAt.Load(provisioner.UID); ok { - retryAt, ok := value.(time.Time) + retry, ok := value.(earlierKeyRetry) if !ok { - return earlierKeyCleanup{}, fmt.Errorf("assertion failed: earlier key retry time of provisioner %s is %T, not time.Time", provisioner.UID, value) + return nil, fmt.Errorf("assertion failed: earlier key retry of provisioner %s is %T, not earlierKeyRetry", provisioner.UID, value) } - if time.Now().Before(retryAt) { - return earlierKeyCleanup{}, nil + if time.Now().Before(retry.at) { + return retry.reports, nil } } kept, reports, err := r.revokePendingKeys(ctx, provisioner, provisioner.Status.PendingKeys, target, inUse) if err != nil { - return earlierKeyCleanup{}, err + return nil, err } if slices.ContainsFunc(kept, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { return !sameKeySlot(record, target) && !inUse(record) }) { - r.earlierKeyRetryAt.Store(provisioner.UID, time.Now().Add(provisionerPendingKeyRetryInterval)) + r.earlierKeyRetryAt.Store(provisioner.UID, earlierKeyRetry{at: time.Now().Add(provisionerPendingKeyRetryInterval), reports: reports}) } else { r.earlierKeyRetryAt.Delete(provisioner.UID) } provisioner.Status.PendingKeys = kept setKeptKeyConditions(provisioner, reports) - return earlierKeyCleanup{tried: true, reports: reports}, nil + return reports, nil } // setUnverifiedKeyCondition sets the UnverifiedKeyKept condition for the diff --git a/internal/controller/coderprovisioner_earliertargets_test.go b/internal/controller/coderprovisioner_earliertargets_test.go index d35c2134..9f77c262 100644 --- a/internal/controller/coderprovisioner_earliertargets_test.go +++ b/internal/controller/coderprovisioner_earliertargets_test.go @@ -3,6 +3,7 @@ package controller_test import ( "context" "errors" + "slices" "testing" "time" @@ -211,7 +212,9 @@ func TestCoderProvisionerKey_EarlierTargetRevokedWhileTargetNotReady(t *testing. // reports of earlier target records while an unpublished record waits for a // control plane C that is not ready. A record whose control plane was created // again must be reported, also an unpublished one, and the report of a record -// without key IDs must go once Coder has no key with its name. +// without key IDs must go once Coder has no key with its name. When C gets +// ready while the retry interval runs, and its key replacement deletes the key +// of a reported record, the report of that record must go too. func TestCoderProvisionerKey_EarlierTargetReportsBesideUnpublishedKey(t *testing.T) { t.Parallel() ctx := context.Background() @@ -282,6 +285,28 @@ func TestCoderProvisionerKey_EarlierTargetReportsBesideUnpublishedKey(t *testing require.Nil(t, meta.FindStatusCondition(got.Status.Conditions, coderv1alpha1.CoderProvisionerConditionUnverifiedKeyKept)) require.NotNil(t, meta.FindStatusCondition(got.Status.Conditions, coderv1alpha1.CoderProvisionerConditionControlPlaneChangedKeyKept)) require.Empty(t, keyDeletes(f.bootstrapClient), "no recorded key may be deleted: one has another control plane, one is unpublished") + + urlC := "http://coder-c." + f.namespace + ".svc.cluster.local:80" + f.bootstrapClient.provisionerKeyByName = func(req coderbootstrap.ProvisionerKeyByNameRequest) *codersdk.ProvisionerKey { + if req.CoderURL != urlC { + return nil + } + return &codersdk.ProvisionerKey{ID: uuid.MustParse(recreated.KeyID), OrganizationID: uuid.MustParse(recreated.OrganizationID), Name: req.KeyName} + } + controlPlaneC := &coderv1alpha1.CoderControlPlane{} + require.NoError(t, k8sClient.Get(ctx, types.NamespacedName{Name: "coder-c", Namespace: f.namespace}, controlPlaneC)) + controlPlaneC.Status.ExternalProvisionerDaemonsEntitlement = string(codersdk.EntitlementEntitled) + require.NoError(t, k8sClient.Status().Update(ctx, controlPlaneC)) + got = reconcile() + require.Contains(t, keyDeletes(f.bootstrapClient), keyDelete{coderURL: urlC, token: "operator-token-c", keyID: recreated.KeyID}, + "the key replacement at C must delete the key that holds the target name") + require.False(t, slices.ContainsFunc(got.Status.PendingKeys, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { + return record.KeyName == "recreated" + }), "the replacement must drop the record of the deleted key") + changed = meta.FindStatusCondition(got.Status.Conditions, coderv1alpha1.CoderProvisionerConditionControlPlaneChangedKeyKept) + require.NotNil(t, changed) + require.NotContains(t, changed.Message, `"recreated"`, "the report of the deleted key must go") + require.Contains(t, changed.Message, `"unpublished-recreated"`) } func markNotEntitled(ctx context.Context, t *testing.T, f provisionerCredentialFixture, _ *coderv1alpha1.CoderProvisioner) {