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..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 @@ -395,22 +397,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 { diff --git a/internal/controller/coderprovisioner_keyownership_test.go b/internal/controller/coderprovisioner_keyownership_test.go index f4bac049..fd504f65 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,117 @@ func TestCoderProvisionerKey_OrganizationRenamed(t *testing.T) { } } +// TestCoderProvisionerKey_UpgradedStatusKeepsTargetRecords checks status from +// 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) { + 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 + // 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 { + 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: !tt.published, + }) + } + 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