From 27074ea8c5866aff52af6a4a89ca5477a59a64f7 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 21:44:19 +0000 Subject: [PATCH 1/4] test: show that a CoderProvisioner drops older live target key records (#424) Change-Id: Iaa18cc18a5f4885026329506558d91f959884569 Signed-off-by: Thomas Kosiewski --- .../coderprovisioner_keyownership_test.go | 114 ++++++++++++++++++ 1 file changed, 114 insertions(+) diff --git a/internal/controller/coderprovisioner_keyownership_test.go b/internal/controller/coderprovisioner_keyownership_test.go index f4bac049..84eb2511 100644 --- a/internal/controller/coderprovisioner_keyownership_test.go +++ b/internal/controller/coderprovisioner_keyownership_test.go @@ -381,6 +381,8 @@ type renamedOrgCoder struct { keys map[uuid.UUID]codersdk.ProvisionerKey // team is the ID of the organization named team. team uuid.UUID + // failList is the ID of an organization whose key list fails. + failList uuid.UUID } func (c *renamedOrgCoder) EnsureProvisionerKey(ctx context.Context, req coderbootstrap.EnsureProvisionerKeyRequest) (coderbootstrap.EnsureProvisionerKeyResponse, error) { @@ -409,6 +411,11 @@ func (c *renamedOrgCoder) serveHTTP(t *testing.T, w http.ResponseWriter, r *http if len(parts) >= 5 && parts[0] == "api" && parts[1] == "v2" && parts[2] == "organizations" && parts[4] == "provisionerkeys" { if orgID, err := uuid.Parse(parts[3]); err == nil { if r.Method == http.MethodGet && len(parts) == 5 { + if orgID == c.failList { + w.WriteHeader(http.StatusServiceUnavailable) + _ = json.NewEncoder(w).Encode(codersdk.Response{Message: "injected list failure"}) + return + } keys := []codersdk.ProvisionerKey{} for _, key := range c.keys { if key.OrganizationID == orgID { @@ -519,6 +526,113 @@ func TestCoderProvisionerKey_OrganizationRenamed(t *testing.T) { } } +// TestCoderProvisionerKey_UpgradedStatusKeepsTargetRecords checks status from +// an older operator with two unpublished records of the target key under +// different organization IDs: the organization named team was renamed and +// its name reused twice. Coder still holds both keys. The rotation must +// delete each key by its recorded IDs before it drops the record, and keep a +// record whose key Coder did not confirm gone. +func TestCoderProvisionerKey_UpgradedStatusKeepsTargetRecords(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + // failOlderList fails the key list in the organization of the older + // record until the first reconcile after the upgrade returns. + failOlderList bool + }{ + {name: "both keys deleted"}, + {name: "failed delete keeps the record", failOlderList: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + f := newProvisionerCredentialFixture(ctx, t, "cprov-upgraded") + orgA, orgB, orgC, orgD := uuid.New(), uuid.New(), uuid.New(), uuid.New() + keyA, olderKey, newerKey, keyD := uuid.New(), uuid.New(), uuid.New(), uuid.New() + coder := &renamedOrgCoder{ + SDKClient: coderbootstrap.NewSDKClient(), + fake: f.bootstrapClient, + keys: map[uuid.UUID]codersdk.ProvisionerKey{}, + team: orgA, + } + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { coder.serveHTTP(t, w, r) })) + t.Cleanup(server.Close) + setControlPlaneURL(ctx, t, f, server.URL) + f.bootstrapClient.provisionerKeyResponses = []coderbootstrap.EnsureProvisionerKeyResponse{ + {OrganizationID: orgA, KeyID: keyA, KeyName: "provisioner", Key: "material-a"}, + {OrganizationID: orgD, KeyID: keyD, KeyName: "provisioner", Key: "material-d"}, + } + f.reconciler.BootstrapClient = coder + + p := f.createProvisioner(ctx, t, nil, nil) + p.Spec.OrganizationName = "team" + require.NoError(t, k8sClient.Update(ctx, p)) + f.reconcile(ctx, t, p) + f.reconcile(ctx, t, p) + require.Equal(t, keyA.String(), f.get(ctx, t, p).Status.ProvisionerKeyID) + + older := codersdk.ProvisionerKey{ID: olderKey, OrganizationID: orgB, Name: "provisioner"} + newer := codersdk.ProvisionerKey{ID: newerKey, OrganizationID: orgC, Name: "provisioner"} + coder.mu.Lock() + coder.keys[olderKey] = older + coder.keys[newerKey] = newer + coder.team = orgD + if tt.failOlderList { + coder.failList = orgB + } + coder.mu.Unlock() + latest := f.get(ctx, t, p) + for _, key := range []codersdk.ProvisionerKey{older, newer} { + latest.Status.PendingKeys = append(latest.Status.PendingKeys, coderv1alpha1.CoderProvisionerKeyRecord{ + ControlPlaneRefName: f.controlPlane.Name, + CoderURL: server.URL, + OrganizationName: "team", + KeyName: "provisioner", + OrganizationID: key.OrganizationID.String(), + KeyID: key.ID.String(), + Unpublished: true, + }) + } + require.NoError(t, k8sClient.Status().Update(ctx, latest)) + + if tt.failOlderList { + _, err := f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{Name: p.Name, Namespace: p.Namespace}}) + require.Error(t, err, "the rotation must stop when Coder does not confirm a recorded key gone") + recorded := f.get(ctx, t, p).Status.PendingKeys + for _, key := range []uuid.UUID{olderKey, newerKey} { + require.True(t, slices.ContainsFunc(recorded, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { + return record.KeyID == key.String() + }), "status must keep the record of live key %s", key) + } + coder.mu.Lock() + require.Contains(t, coder.keys, olderKey) + coder.failList = uuid.Nil + coder.mu.Unlock() + } + + f.reconcile(ctx, t, p) + rotated := f.get(ctx, t, p) + require.Equal(t, keyD.String(), rotated.Status.ProvisionerKeyID, "the rotation must create a new key") + require.Empty(t, rotated.Status.PendingKeys) + coder.mu.Lock() + require.NotContains(t, coder.keys, olderKey, "the rotation must delete the older recorded key by ID") + require.NotContains(t, coder.keys, newerKey, "the rotation must delete the newer recorded key by ID") + require.NotContains(t, coder.keys, keyA, "the rotation must delete the replaced key by ID") + coder.mu.Unlock() + + require.NoError(t, k8sClient.Delete(ctx, rotated)) + f.reconcile(ctx, t, p) + requireProvisionerGone(ctx, t, types.NamespacedName{Name: p.Name, Namespace: p.Namespace}) + coder.mu.Lock() + defer coder.mu.Unlock() + require.Empty(t, coder.keys, "deleting the provisioner must leave none of its keys in Coder") + }) + } +} + // TestCoderProvisionerKey_StatusWithoutUIDReplacedKeyBound checks status // without a control plane UID when P moves from the control plane coder to // coder-b, and Coder fails the delete of the replaced key. The control plane From 9aa56d3f5f4da432d0aaeb1d91bfcd420c73198a Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 21:45:38 +0000 Subject: [PATCH 2/4] fix: keep every CoderProvisioner target key record with IDs until Coder confirms its key gone (#424) Change-Id: I0b46d9be08ccaca81a9fea4bdb4324b12d30c854 Signed-off-by: Thomas Kosiewski --- docs/tutorials/deploy-coderprovisioner.md | 5 ++++ .../controller/coderprovisioner_controller.go | 24 +++++++------------ 2 files changed, 13 insertions(+), 16 deletions(-) diff --git a/docs/tutorials/deploy-coderprovisioner.md b/docs/tutorials/deploy-coderprovisioner.md index 9dcc043f..5de36a7d 100644 --- a/docs/tutorials/deploy-coderprovisioner.md +++ b/docs/tutorials/deploy-coderprovisioner.md @@ -86,4 +86,9 @@ The same records apply while the provisioner runs. When `spec.controlPlaneRef`, 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 `. +Two limits remain: + +- A record without a control plane UID, from an operator version that did not record it, cannot show which Coder backend holds its key. When its URL now reaches another Coder backend, that backend answers that it has no such key, and the operator removes the record. If that URL later reaches the old backend again, the key is still valid there. Delete such a key by hand. +- Coder deletes a provisioner key only by name. The operator lists the keys of the recorded organization, finds the recorded key ID, and then deletes the key with that name. When someone deletes that key and creates another key with the same name between these two calls, the operator deletes the new key. + To remove all the other resources, do the steps in [the control plane cleanup](getting-started.md#5-clean-up-optional). diff --git a/internal/controller/coderprovisioner_controller.go b/internal/controller/coderprovisioner_controller.go index cbaf2dd9..26243149 100644 --- a/internal/controller/coderprovisioner_controller.go +++ b/internal/controller/coderprovisioner_controller.go @@ -395,22 +395,14 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req pending = append(pending, replaced) } if replacing { - // The newest target record with IDs stays until the replacement below - // deletes its key by ID. Each earlier replacement at the target - // already deleted the older ones by ID before its create. - newest := -1 - for i, record := range pending { - if sameKeySlot(record, target) && record.KeyID != "" { - newest = i - } - } - var next []coderv1alpha1.CoderProvisionerKeyRecord - for i, record := range pending { - if !sameKeySlot(record, target) || i == newest { - next = append(next, record) - } - } - pending = append(next, target) + // Every target record with IDs stays until the replacement below + // deletes its key by ID. Status from an older operator can hold more + // than one, under organization IDs that a reused organization name + // once had, and each key can still be valid. + pending = slices.DeleteFunc(pending, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { + 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 { From 9a75a8f14c6b6e1f9b1ca3e8bb00232f2afd90b9 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 21:51:42 +0000 Subject: [PATCH 3/4] test: show that a published target key record is dropped without a delete (#424) Change-Id: I524273d9b6e00e8fd83a9810c172f2c3be163793 Signed-off-by: Thomas Kosiewski --- internal/controller/coderprovisioner_keyownership_test.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/internal/controller/coderprovisioner_keyownership_test.go b/internal/controller/coderprovisioner_keyownership_test.go index 84eb2511..6ea2f168 100644 --- a/internal/controller/coderprovisioner_keyownership_test.go +++ b/internal/controller/coderprovisioner_keyownership_test.go @@ -540,9 +540,13 @@ func TestCoderProvisionerKey_UpgradedStatusKeepsTargetRecords(t *testing.T) { // failOlderList fails the key list in the organization of the older // record until the first reconcile after the upgrade returns. failOlderList bool + // published records the keys as published, so that nothing but the + // records themselves calls for a rotation. + published bool }{ {name: "both keys deleted"}, {name: "failed delete keeps the record", failOlderList: true}, + {name: "published records", published: true}, } for _, tt := range tests { @@ -593,7 +597,7 @@ func TestCoderProvisionerKey_UpgradedStatusKeepsTargetRecords(t *testing.T) { KeyName: "provisioner", OrganizationID: key.OrganizationID.String(), KeyID: key.ID.String(), - Unpublished: true, + Unpublished: !tt.published, }) } require.NoError(t, k8sClient.Status().Update(ctx, latest)) From 12c9c2f334a2ff737b49a045fd85d606065e7394 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 21:52:22 +0000 Subject: [PATCH 4/4] fix: rotate when a published target key record names a key other than the applied key (#424) Change-Id: If519b7e06e051ef7efa2cbc01658dc401e53eec4 Signed-off-by: Thomas Kosiewski --- internal/controller/coderprovisioner_controller.go | 6 ++++-- internal/controller/coderprovisioner_keyownership_test.go | 6 +++--- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/internal/controller/coderprovisioner_controller.go b/internal/controller/coderprovisioner_controller.go index 26243149..95134ab7 100644 --- a/internal/controller/coderprovisioner_controller.go +++ b/internal/controller/coderprovisioner_controller.go @@ -311,9 +311,11 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req // 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, - // so a rotation replaces it with a key that status records. + // so a rotation replaces it with a key that status records. A published + // target record with IDs other than the applied key names a key that is + // not in the Secret and can still be valid, so a rotation deletes it by ID. rotate := !secretUsable || slices.ContainsFunc(provisioner.Status.PendingKeys, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { - return record.Unpublished + return record.Unpublished || (sameKeySlot(record, target) && record.KeyID != "" && record.KeyID != status.ProvisionerKeyID) }) replacing := driftDetected || rotate || metadataMissing // Status from an operator that did not record the control plane UID does diff --git a/internal/controller/coderprovisioner_keyownership_test.go b/internal/controller/coderprovisioner_keyownership_test.go index 6ea2f168..fd504f65 100644 --- a/internal/controller/coderprovisioner_keyownership_test.go +++ b/internal/controller/coderprovisioner_keyownership_test.go @@ -527,9 +527,9 @@ func TestCoderProvisionerKey_OrganizationRenamed(t *testing.T) { } // TestCoderProvisionerKey_UpgradedStatusKeepsTargetRecords checks status from -// an older operator with two unpublished records of the target key under -// different organization IDs: the organization named team was renamed and -// its name reused twice. Coder still holds both keys. The rotation must +// an older operator with two records of the target key under different +// organization IDs: the organization named team was renamed and its name +// reused twice. Coder still holds both keys. The rotation must // delete each key by its recorded IDs before it drops the record, and keep a // record whose key Coder did not confirm gone. func TestCoderProvisionerKey_UpgradedStatusKeepsTargetRecords(t *testing.T) {