From 40948023c9a6a497fccdbe8caed26a64ce36a993 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 18:26:22 +0000 Subject: [PATCH 01/13] =?UTF-8?q?=F0=9F=A4=96=20test:=20a=20CoderProvision?= =?UTF-8?q?er=20keeps=20other=20keys=20and=20binds=20key=20records=20to=20?= =?UTF-8?q?the=20control=20plane=20UID?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add envtests for #414 and #417. They fail on main: - a provisioner P deletes and replaces the same-named key of provisioner Q at one endpoint and organization (first key, key name change, status without key records); - after a control plane is deleted and created again under the same name with another URL, the operator sends the new control plane's token to the old URL (rotation and deletion, with and without a recorded UID). Change-Id: Ic970e4e8143dff85ce799f4776c4dd8c3a1f21dc Signed-off-by: Thomas Kosiewski --- .../coderprovisioner_keyownership_test.go | 269 ++++++++++++++++++ 1 file changed, 269 insertions(+) create mode 100644 internal/controller/coderprovisioner_keyownership_test.go diff --git a/internal/controller/coderprovisioner_keyownership_test.go b/internal/controller/coderprovisioner_keyownership_test.go new file mode 100644 index 00000000..14bff177 --- /dev/null +++ b/internal/controller/coderprovisioner_keyownership_test.go @@ -0,0 +1,269 @@ +package controller_test + +import ( + "context" + "slices" + "testing" + + "github.com/coder/coder/v2/codersdk" + "github.com/google/uuid" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + "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" + ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" + "github.com/coder/coder-k8s/internal/coderbootstrap" +) + +// TestCoderProvisionerKey_OtherProvisionersKeyKept checks two provisioners P +// and Q with the same key name at one Coder endpoint and organization. Q holds +// the key. Whatever way P reaches that key name, P must not delete or replace +// the key of Q, and must not change its own key, Secret or Deployment. It must +// report the conflict, and get its own key once it has a distinct key name or +// the key of Q is gone. +func TestCoderProvisionerKey_OtherProvisionersKeyKept(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + // holdsKey makes P hold the key p-key before it reaches the key name + // of Q. + holdsKey bool + // dropKeyStatus removes the key fields from the status of P, as + // status that lost its key records. + dropKeyStatus bool + // rename resolves the conflict with a distinct key name. Otherwise + // the key of Q goes. + rename bool + }{ + {name: "first key of P"}, + {name: "key name change of P", holdsKey: true, rename: true}, + {name: "status without key records", holdsKey: true, dropKeyStatus: true, rename: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + f := newProvisionerCredentialFixture(ctx, t, "cprov-samename") + store := &keyStoreClient{fakeBootstrapClient: f.bootstrapClient, keys: map[keySlot]uuid.UUID{}} + f.reconciler.BootstrapClient = store + slotOf := func(keyName string) keySlot { return newKeySlot(internalCoderURL(f), "", keyName) } + + q := f.createKeyProvisioner(ctx, t, "q", "") + q.Spec.Key.Name = "provisioner" + require.NoError(t, k8sClient.Update(ctx, q)) + f.reconcile(ctx, t, q) + f.reconcile(ctx, t, q) + qKeyID := store.keys[slotOf("provisioner")] + require.Equal(t, qKeyID.String(), f.get(ctx, t, q).Status.ProvisionerKeyID, "Q must hold the key named provisioner") + + p := f.createProvisioner(ctx, t, nil, nil) + pName := types.NamespacedName{Name: p.Name, Namespace: p.Namespace} + keySecret := types.NamespacedName{Name: p.Name + "-provisioner-key", Namespace: p.Namespace} + var pKeyID uuid.UUID + var pSecret corev1.Secret + if tt.holdsKey { + latest := f.get(ctx, t, p) + latest.Spec.Key.Name = "p-key" + require.NoError(t, k8sClient.Update(ctx, latest)) + f.reconcile(ctx, t, p) + f.reconcile(ctx, t, p) + pKeyID = store.keys[slotOf("p-key")] + require.Equal(t, pKeyID.String(), f.get(ctx, t, p).Status.ProvisionerKeyID) + require.NoError(t, k8sClient.Get(ctx, keySecret, &pSecret)) + + latest = f.get(ctx, t, p) + latest.Spec.Key.Name = "" + require.NoError(t, k8sClient.Update(ctx, latest)) + if tt.dropKeyStatus { + latest = f.get(ctx, t, p) + latest.Status = coderv1alpha1.CoderProvisionerStatus{SecretRef: latest.Status.SecretRef} + require.NoError(t, k8sClient.Status().Update(ctx, latest)) + } + } else { + f.reconcile(ctx, t, p) + } + + for range 2 { + result, err := f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: pName}) + require.NoError(t, err) + require.Equal(t, qKeyID, store.keys[slotOf("provisioner")], "the key of Q must still exist") + require.Positive(t, result.RequeueAfter, "P must check the key name again") + } + latest := f.get(ctx, t, p) + keyReady := meta.FindStatusCondition(latest.Status.Conditions, coderv1alpha1.CoderProvisionerConditionProvisionerKeyReady) + require.NotNil(t, keyReady, "P must report the conflict") + require.Equal(t, metav1.ConditionFalse, keyReady.Status) + require.Equal(t, "KeyNameInUse", keyReady.Reason) + require.Contains(t, keyReady.Message, `"provisioner"`) + require.Contains(t, keyReady.Message, qKeyID.String()) + require.Nil(t, meta.FindStatusCondition(latest.Status.Conditions, coderv1alpha1.CoderProvisionerConditionUnverifiedKeyKept)) + require.Empty(t, latest.Status.PendingKeys, "P must record no create") + if tt.holdsKey { + require.Equal(t, pKeyID, store.keys[slotOf("p-key")], "the key of P must stay while its new key name is taken") + secret := &corev1.Secret{} + require.NoError(t, k8sClient.Get(ctx, keySecret, secret)) + require.Equal(t, pSecret.Data, secret.Data, "the Secret of P must keep its key") + } else { + _, exists := f.deployment(ctx, t, p) + require.False(t, exists, "P must not start without a key") + } + resourceVersion := latest.ResourceVersion + _, err := f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: pName}) + require.NoError(t, err) + require.Equal(t, resourceVersion, f.get(ctx, t, p).ResourceVersion, "a repeated conflict must not write status") + + keyName := "provisioner" + if tt.rename { + keyName = "p-own" + latest.Spec.Key.Name = keyName + require.NoError(t, k8sClient.Update(ctx, latest)) + } else { + delete(store.keys, slotOf("provisioner")) + } + f.reconcile(ctx, t, p) + latest = f.get(ctx, t, p) + require.Equal(t, store.keys[slotOf(keyName)].String(), latest.Status.ProvisionerKeyID, "P must hold its own key") + keyReady = meta.FindStatusCondition(latest.Status.Conditions, coderv1alpha1.CoderProvisionerConditionProvisionerKeyReady) + require.NotNil(t, keyReady) + require.Equal(t, metav1.ConditionTrue, keyReady.Status) + require.Nil(t, meta.FindStatusCondition(latest.Status.Conditions, coderv1alpha1.CoderProvisionerConditionUnverifiedKeyKept)) + if tt.rename { + require.Equal(t, qKeyID, store.keys[slotOf("provisioner")], "the key of Q must still exist") + } + }) + } +} + +// urlToken is the Coder URL and the operator token of one Coder request. +type urlToken struct{ coderURL, token string } + +// sentTokens lists the Coder URL and token of every Coder request that the +// fake received, and records lookups by name into lookups. +func sentTokens(client *fakeBootstrapClient, lookups []urlToken) []urlToken { + sent := slices.Clone(lookups) + for _, req := range client.provisionerKeyRequests { + sent = append(sent, urlToken{req.CoderURL, req.SessionToken}) + } + for _, req := range client.entitlementsRequests { + sent = append(sent, urlToken{req.CoderURL, req.SessionToken}) + } + for _, req := range keyDeletes(client) { + sent = append(sent, urlToken{req.coderURL, req.token}) + } + return sent +} + +// TestCoderProvisionerKey_ControlPlaneRecreated checks a control plane that is +// deleted and created again under the same name with another URL and another +// operator token. The new token must never reach the URL of the old control +// plane: the record of the old key stays, and the provisioner reports it. A +// record from an operator that did not record control plane UIDs may use the +// token of its control plane only at that control plane's current URL. +func TestCoderProvisionerKey_ControlPlaneRecreated(t *testing.T) { + t.Parallel() + + const ( + oldURL = "http://coder-old.example.com" + newURL = "http://coder-new.example.com" + newToken = "operator-token-new" + ) + tests := []struct { + name string + // legacy removes the control plane UID from status, as an operator + // before UID records wrote it. + legacy bool + // keep keeps the control plane and moves P to another one instead. + keep bool + deleteP bool + }{ + {name: "rotation after the recreate"}, + {name: "deletion after the recreate", deleteP: true}, + {name: "status without control plane UID, rotation after the recreate", legacy: true}, + {name: "status without control plane UID, deletion after the recreate", legacy: true, deleteP: true}, + {name: "status without control plane UID, control plane kept", legacy: true, keep: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + f := newProvisionerCredentialFixture(ctx, t, "cprov-recreated") + setControlPlaneURL(ctx, t, f, oldURL) + keyOld := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: uuid.New(), KeyID: uuid.New(), KeyName: "provisioner", Key: "material-old"} + keyNew := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: uuid.New(), KeyID: uuid.New(), KeyName: "provisioner", Key: "material-new"} + f.bootstrapClient.provisionerKeyResponses = []coderbootstrap.EnsureProvisionerKeyResponse{keyOld, keyNew} + var lookups []urlToken + f.bootstrapClient.provisionerKeyByName = func(req coderbootstrap.ProvisionerKeyByNameRequest) *codersdk.ProvisionerKey { + lookups = append(lookups, urlToken{req.CoderURL, req.SessionToken}) + return nil + } + + p := f.createProvisioner(ctx, t, nil, nil) + pName := types.NamespacedName{Name: p.Name, Namespace: p.Namespace} + f.reconcile(ctx, t, p) + f.reconcile(ctx, t, p) + require.Equal(t, keyOld.KeyID.String(), f.get(ctx, t, p).Status.ProvisionerKeyID) + if tt.legacy { + require.NoError(t, k8sClient.Status().Patch(ctx, f.get(ctx, t, p), + ctrlclient.RawPatch(types.MergePatchType, []byte(`{"status":{"controlPlaneUID":null}}`)))) + } + + if tt.keep { + createControlPlaneWithToken(ctx, t, f.namespace, "coder-b", newURL, newToken) + latest := f.get(ctx, t, p) + latest.Spec.ControlPlaneRef.Name = "coder-b" + require.NoError(t, k8sClient.Update(ctx, latest)) + } else { + require.NoError(t, k8sClient.Delete(ctx, &coderv1alpha1.CoderControlPlane{ObjectMeta: metav1.ObjectMeta{Name: "coder", Namespace: f.namespace}})) + require.NoError(t, k8sClient.Delete(ctx, &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "coder-operator-token", Namespace: f.namespace}})) + createControlPlaneWithToken(ctx, t, f.namespace, "coder", newURL, newToken) + } + + requireNoLeak := func() { + t.Helper() + sent := sentTokens(f.bootstrapClient, lookups) + for _, s := range sent { + require.False(t, s.coderURL == oldURL && s.token == newToken, "the new control plane's token must not reach %s, sent %+v", oldURL, sent) + } + } + if tt.deleteP { + require.NoError(t, k8sClient.Delete(ctx, f.get(ctx, t, p))) + f.reconcile(ctx, t, p) + requireNoLeak() + requireProvisionerGone(ctx, t, pName) + } else { + result, err := f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: pName}) + require.NoError(t, err) + requireNoLeak() + latest := f.get(ctx, t, p) + require.Equal(t, keyNew.KeyID.String(), latest.Status.ProvisionerKeyID) + require.Equal(t, newURL, latest.Status.ControlPlaneURL) + kept := slices.ContainsFunc(latest.Status.PendingKeys, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { + return record.KeyID == keyOld.KeyID.String() && record.CoderURL == oldURL + }) + report := meta.FindStatusCondition(latest.Status.Conditions, "ControlPlaneChangedKeyKept") + if tt.keep { + require.False(t, kept, "the old key must be deleted at its URL") + require.Nil(t, report) + } else { + require.True(t, kept, "the record of the old key must stay") + require.Positive(t, result.RequeueAfter, "P must check the kept record again") + require.NotNil(t, report, "P must report the record that it keeps") + require.Equal(t, metav1.ConditionTrue, report.Status) + require.Contains(t, report.Message, oldURL) + } + } + + if tt.keep { + require.Contains(t, keyDeletes(f.bootstrapClient), keyDelete{coderURL: oldURL, token: testOperatorToken, keyID: keyOld.KeyID.String()}, + "the old key must be deleted at its URL with the token of its control plane") + } + }) + } +} From 6bc81052a64e640a38fdbe48abf26cd5bca522f4 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 18:39:48 +0000 Subject: [PATCH 02/13] =?UTF-8?q?=F0=9F=A4=96=20fix:=20a=20CoderProvisione?= =?UTF-8?q?r=20replaces=20only=20keys=20it=20recorded=20and=20binds=20key?= =?UTF-8?q?=20records=20to=20the=20control=20plane=20UID?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit operator looks up the target key name in Coder. A key with that name whose ID status does not record (status.provisionerKeyID or a status.pendingKeys entry) can belong to another provisioner or a person: the operator neither deletes nor replaces it, changes nothing else, sets ProvisionerKeyReady=False with reason KeyNameInUse, and checks again every 2 minutes. A recorded key is deleted by its ID, then created again. The by-name delete (coderbootstrap DeleteProvisionerKey) is removed. (status.controlPlaneUID) record the UID of the control plane. The operator sends a control plane's operator token to its current Coder API URL, and to another recorded URL only while the control plane has the recorded UID. Otherwise the entry stays and the condition ControlPlaneChangedKeyKept names it. A record without a UID counts as bound only at the current URL of its control plane. Change-Id: I34b7edd839aaa8d2c1e3733983a806694add93d6 Signed-off-by: Thomas Kosiewski --- api/v1alpha1/coderprovisioner_types.go | 21 + .../bases/coder.com_coderprovisioners.yaml | 17 + dist/install.yaml | 17 + docs/reference/api/coderprovisioner.md | 2 + docs/tutorials/deploy-coderprovisioner.md | 4 +- internal/coderbootstrap/client.go | 1 - internal/coderbootstrap/provisionerkeys.go | 37 -- .../coderbootstrap/provisionerkeys_test.go | 115 ------ .../controller/coderprovisioner_controller.go | 378 +++++++----------- .../coderprovisioner_controller_test.go | 43 +- .../coderprovisioner_deletion_test.go | 3 - .../coderprovisioner_keyownership_test.go | 2 +- .../coderprovisioner_keysecret_test.go | 8 +- .../coderprovisioner_pendingkeys_test.go | 86 ++-- .../coderworkspaceproxy_controller_test.go | 21 - 15 files changed, 280 insertions(+), 475 deletions(-) diff --git a/api/v1alpha1/coderprovisioner_types.go b/api/v1alpha1/coderprovisioner_types.go index 7d6655ec..18e95758 100644 --- a/api/v1alpha1/coderprovisioner_types.go +++ b/api/v1alpha1/coderprovisioner_types.go @@ -36,6 +36,14 @@ const ( // operator deletes no key for such an entry. The message names each // entry, and the operator removes the condition when none is left. CoderProvisionerConditionUnverifiedKeyKept = "UnverifiedKeyKept" + // CoderProvisionerConditionControlPlaneChangedKeyKept is True while + // status.pendingKeys holds an entry whose control plane the operator + // cannot show is the one that the entry recorded: the control plane with + // its name has another UID, or the entry has no UID and its coderURL is + // not the current Coder API URL of that control plane. The operator sends + // no operator token for such an entry. The message names each entry, and + // the operator removes the condition when none is left. + CoderProvisionerConditionControlPlaneChangedKeyKept = "ControlPlaneChangedKeyKept" // DefaultProvisionerKeySecretKey is the default data key for provisioner key secrets. DefaultProvisionerKeySecretKey = "key" @@ -110,6 +118,12 @@ type CoderProvisionerStatus struct { ControlPlaneRefName string `json:"controlPlaneRefName,omitempty"` // ControlPlaneURL is the control plane URL last applied to the provisioner key. ControlPlaneURL string `json:"controlPlaneURL,omitempty"` + // ControlPlaneUID is the UID of the control plane last applied to the + // provisioner key. The operator sends the operator token of the control + // plane with controlPlaneRefName for this key only while that control + // plane has this UID. When it is empty, it sends that token only to the + // current Coder API URL of that control plane. + ControlPlaneUID string `json:"controlPlaneUID,omitempty"` // SecretRef references the provisioner key secret data currently in use. SecretRef *SecretKeySelector `json:"secretRef,omitempty"` // PendingKeys lists the provisioner keys that can exist in Coder besides @@ -129,6 +143,13 @@ type CoderProvisionerKeyRecord struct { // ControlPlaneRefName is the CoderControlPlane whose operator token the // operator used for the key. ControlPlaneRefName string `json:"controlPlaneRefName"` + // ControlPlaneUID is the UID of that CoderControlPlane. The operator + // sends its operator token for this entry only while the control plane + // with controlPlaneRefName has this UID. When it is empty, it sends that + // token only when coderURL is the current Coder API URL of that control + // plane. Otherwise it keeps the entry and sets the condition + // ControlPlaneChangedKeyKept. + ControlPlaneUID string `json:"controlPlaneUID,omitempty"` // CoderURL is the Coder API URL that the operator sent the key request to. CoderURL string `json:"coderURL"` // OrganizationName is the Coder organization of the key. diff --git a/config/crd/bases/coder.com_coderprovisioners.yaml b/config/crd/bases/coder.com_coderprovisioners.yaml index 09d2b0b5..d3439c19 100644 --- a/config/crd/bases/coder.com_coderprovisioners.yaml +++ b/config/crd/bases/coder.com_coderprovisioners.yaml @@ -417,6 +417,14 @@ spec: description: ControlPlaneRefName is the control plane ref name last applied to the provisioner key. type: string + controlPlaneUID: + description: |- + ControlPlaneUID is the UID of the control plane last applied to the + provisioner key. The operator sends the operator token of the control + plane with controlPlaneRefName for this key only while that control + plane has this UID. When it is empty, it sends that token only to the + current Coder API URL of that control plane. + type: string controlPlaneURL: description: ControlPlaneURL is the control plane URL last applied to the provisioner key. @@ -457,6 +465,15 @@ spec: ControlPlaneRefName is the CoderControlPlane whose operator token the operator used for the key. type: string + controlPlaneUID: + description: |- + ControlPlaneUID is the UID of that CoderControlPlane. The operator + sends its operator token for this entry only while the control plane + with controlPlaneRefName has this UID. When it is empty, it sends that + token only when coderURL is the current Coder API URL of that control + plane. Otherwise it keeps the entry and sets the condition + ControlPlaneChangedKeyKept. + type: string keyID: description: KeyID is the provisioner key ID that Coder returned. type: string diff --git a/dist/install.yaml b/dist/install.yaml index d353c805..32354e78 100644 --- a/dist/install.yaml +++ b/dist/install.yaml @@ -6179,6 +6179,14 @@ spec: description: ControlPlaneRefName is the control plane ref name last applied to the provisioner key. type: string + controlPlaneUID: + description: |- + ControlPlaneUID is the UID of the control plane last applied to the + provisioner key. The operator sends the operator token of the control + plane with controlPlaneRefName for this key only while that control + plane has this UID. When it is empty, it sends that token only to the + current Coder API URL of that control plane. + type: string controlPlaneURL: description: ControlPlaneURL is the control plane URL last applied to the provisioner key. @@ -6219,6 +6227,15 @@ spec: ControlPlaneRefName is the CoderControlPlane whose operator token the operator used for the key. type: string + controlPlaneUID: + description: |- + ControlPlaneUID is the UID of that CoderControlPlane. The operator + sends its operator token for this entry only while the control plane + with controlPlaneRefName has this UID. When it is empty, it sends that + token only when coderURL is the current Coder API URL of that control + plane. Otherwise it keeps the entry and sets the condition + ControlPlaneChangedKeyKept. + type: string keyID: description: KeyID is the provisioner key ID that Coder returned. type: string diff --git a/docs/reference/api/coderprovisioner.md b/docs/reference/api/coderprovisioner.md index 9394015f..e94c850d 100644 --- a/docs/reference/api/coderprovisioner.md +++ b/docs/reference/api/coderprovisioner.md @@ -40,6 +40,7 @@ | `tagsHash` | string | TagsHash is a deterministic hash of spec.tags last applied to the provisioner key. | | `controlPlaneRefName` | string | ControlPlaneRefName is the control plane ref name last applied to the provisioner key. | | `controlPlaneURL` | string | ControlPlaneURL is the control plane URL last applied to the provisioner key. | +| `controlPlaneUID` | string | ControlPlaneUID is the UID of the control plane last applied to the provisioner key. The operator sends the operator token of the control plane with controlPlaneRefName for this key only while that control plane has this UID. When it is empty, it sends that token only to the current Coder API URL of that control plane. | | `secretRef` | [SecretKeySelector](#secretkeyselector) | SecretRef references the provisioner key secret data currently in use. | | `pendingKeys` | [CoderProvisionerKeyRecord](#coderprovisionerkeyrecord) array | PendingKeys lists the provisioner keys that can exist in Coder besides the key in provisionerKeyID: a key that the operator is about to create or has not yet written to the provisioner key Secret, and replaced keys whose deletion Coder has not confirmed. The operator deletes the key of each entry with organizationID and keyID at its coderURL with the operator token of its control plane, and removes the entry when Coder confirms that the key is gone. | @@ -53,6 +54,7 @@ or was about to create, at one Coder endpoint. | Field | Type | Description | | --- | --- | --- | | `controlPlaneRefName` | string | ControlPlaneRefName is the CoderControlPlane whose operator token the operator used for the key. | +| `controlPlaneUID` | string | ControlPlaneUID is the UID of that CoderControlPlane. The operator sends its operator token for this entry only while the control plane with controlPlaneRefName has this UID. When it is empty, it sends that token only when coderURL is the current Coder API URL of that control plane. Otherwise it keeps the entry and sets the condition ControlPlaneChangedKeyKept. | | `coderURL` | string | CoderURL is the Coder API URL that the operator sent the key request to. | | `organizationName` | string | OrganizationName is the Coder organization of the key. | | `keyName` | string | KeyName is the provisioner key name in Coder. | diff --git a/docs/tutorials/deploy-coderprovisioner.md b/docs/tutorials/deploy-coderprovisioner.md index 47090eb5..b2156b02 100644 --- a/docs/tutorials/deploy-coderprovisioner.md +++ b/docs/tutorials/deploy-coderprovisioner.md @@ -54,6 +54,8 @@ Expected result: the phase is `Ready`, and the conditions include `CredentialsRe The operator creates the provisioner key Secret and owns it. If you set `spec.key.secretName`, name a Secret that does not exist yet: the operator refuses a Secret that it does not own (reason `KeySecretNotOwned`). +Each provisioner needs its own key name in its Coder organization. The key name is `spec.key.name`, or the provisioner name when `spec.key.name` is empty. Before the operator creates or replaces a key, it looks up the key name in Coder. When Coder holds a key with that name, and the status of this provisioner does not record its ID, the operator does not delete or replace that key, because another `CoderProvisioner` (for example one with the same name in another namespace) or a person can use it. It then changes nothing for this provisioner: `ProvisionerKeyReady` is `False` with reason `KeyNameInUse`, and the message names the key and its ID. Set `spec.key.name` to a name that nothing else uses, or delete that key in Coder if nothing uses it. The operator checks again every 2 minutes. The same happens when the operator lost the answer to a key create, because status then has no ID of the new key, and for status without key records, for example after a restore without status. + If you add `spec.extraEnv` entries with a `secretKeyRef`, each Secret needs the label `coder.com/credential=true` and the annotation `coder.com/credential-destinations` with the origin of the control plane `status.url`. Otherwise `CredentialsReady` is `False` and the operator does not change the Deployment. See [Configure credential Secrets for proxies and provisioners](../how-to/workspace-proxy-credentials.md). The operator creates these resources in the `coder` namespace: @@ -80,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, each at its own Coder URL. 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. An entry whose control plane was deleted 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, each at its own Coder URL. 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 to the current Coder API URL of that control plane, and to another URL that a record names only while the control plane has the recorded UID. When a control plane is deleted and created again under the same name with another URL, the operator does not send the new token to the old URL. The 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. 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/coderbootstrap/client.go b/internal/coderbootstrap/client.go index e2385d1b..ea3dbe0f 100644 --- a/internal/coderbootstrap/client.go +++ b/internal/coderbootstrap/client.go @@ -94,7 +94,6 @@ type RegisterWorkspaceProxyResponse struct { type Client interface { EnsureWorkspaceProxy(context.Context, RegisterWorkspaceProxyRequest) (RegisterWorkspaceProxyResponse, error) EnsureProvisionerKey(context.Context, EnsureProvisionerKeyRequest) (EnsureProvisionerKeyResponse, error) - DeleteProvisionerKey(ctx context.Context, coderURL, sessionToken, orgName, keyName string) error DeleteProvisionerKeyByID(context.Context, DeleteProvisionerKeyByIDRequest) error ProvisionerKeyByName(context.Context, ProvisionerKeyByNameRequest) (*codersdk.ProvisionerKey, error) Entitlements(ctx context.Context, coderURL, sessionToken string) (codersdk.Entitlements, error) diff --git a/internal/coderbootstrap/provisionerkeys.go b/internal/coderbootstrap/provisionerkeys.go index 785cfb1c..3033c996 100644 --- a/internal/coderbootstrap/provisionerkeys.go +++ b/internal/coderbootstrap/provisionerkeys.go @@ -104,43 +104,6 @@ func (c *SDKClient) EnsureProvisionerKey(ctx context.Context, req EnsureProvisio }, nil } -// DeleteProvisionerKey deletes a provisioner key by name. -// A missing key is treated as success for idempotency. -func (c *SDKClient) DeleteProvisionerKey(ctx context.Context, coderURL, sessionToken, orgName, keyName string) error { - if err := validateProvisionerKeyInputs(coderURL, sessionToken, keyName); err != nil { - return err - } - - client, err := newAuthenticatedClient(coderURL, sessionToken) - if err != nil { - return err - } - - organizationName := orgName - if organizationName == "" { - organizationName = codersdk.DefaultOrganization - } - - organization, err := resolveOrganizationByName(ctx, client, organizationName) - if err != nil { - return err - } - - _, err = withOptionalRateLimitBypass(ctx, func(requestCtx context.Context) (struct{}, error) { - return struct{}{}, client.DeleteProvisionerKey(requestCtx, organization.ID, keyName) - }) - if err == nil { - return nil - } - - var apiErr *codersdk.Error - if errors.As(err, &apiErr) && apiErr.StatusCode() == http.StatusNotFound { - return nil - } - - return xerrors.Errorf("delete provisioner key %q: %w", keyName, err) -} - // ProvisionerKeyByNameRequest identifies a provisioner key by its name. type ProvisionerKeyByNameRequest struct { CoderURL string diff --git a/internal/coderbootstrap/provisionerkeys_test.go b/internal/coderbootstrap/provisionerkeys_test.go index 57728940..d5abdec1 100644 --- a/internal/coderbootstrap/provisionerkeys_test.go +++ b/internal/coderbootstrap/provisionerkeys_test.go @@ -226,49 +226,6 @@ func TestEnsureProvisionerKey_FallsBackWhenRateLimitBypassRejected(t *testing.T) require.Equal(t, "plaintext-provisioner-key", resp.Key) } -func TestDeleteProvisionerKey_FallsBackWhenRateLimitBypassRejected(t *testing.T) { - t.Parallel() - - const keyName = "delete-me" - orgID := uuid.New() - now := time.Now().UTC().Format(time.RFC3339) - deleteCalls := 0 - bypassRejectedCalls := 0 - - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.Header.Get(codersdk.BypassRatelimitHeader) == "true" { - bypassRejectedCalls++ - writeJSONResponse(t, w, http.StatusPreconditionRequired, map[string]any{"message": "bypass is not allowed"}) - return - } - - switch { - case r.Method == http.MethodGet && r.URL.Path == "/api/v2/organizations/default": - writeJSONResponse(t, w, http.StatusOK, map[string]any{ - "id": orgID.String(), - "name": "default", - "created_at": now, - "updated_at": now, - }) - return - case r.Method == http.MethodDelete && r.URL.Path == "/api/v2/organizations/"+orgID.String()+"/provisionerkeys/"+keyName: - deleteCalls++ - w.WriteHeader(http.StatusNoContent) - return - default: - writeJSONResponse(t, w, http.StatusNotFound, map[string]any{"message": "unexpected route"}) - return - } - })) - defer server.Close() - - client := coderbootstrap.NewSDKClient() - err := client.DeleteProvisionerKey(context.Background(), server.URL, "session-token", "", keyName) - require.NoError(t, err) - require.Equal(t, 1, deleteCalls) - require.Equal(t, 1, bypassRejectedCalls) -} - func TestEnsureProvisionerKey_ValidationErrors(t *testing.T) { t.Parallel() @@ -410,78 +367,6 @@ func TestEntitlements_ValidationErrors(t *testing.T) { require.Contains(t, err.Error(), "session token is required") } -func TestDeleteProvisionerKey_Success(t *testing.T) { - t.Parallel() - - const keyName = "delete-me" - orgID := uuid.New() - now := time.Now().UTC().Format(time.RFC3339) - deleteCalls := 0 - - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - switch { - case r.Method == http.MethodGet && r.URL.Path == "/api/v2/organizations/default": - writeJSONResponse(t, w, http.StatusOK, map[string]any{ - "id": orgID.String(), - "name": "default", - "created_at": now, - "updated_at": now, - }) - return - case r.Method == http.MethodDelete && r.URL.Path == "/api/v2/organizations/"+orgID.String()+"/provisionerkeys/"+keyName: - deleteCalls++ - w.WriteHeader(http.StatusNoContent) - return - default: - writeJSONResponse(t, w, http.StatusNotFound, map[string]any{"message": "unexpected route"}) - return - } - })) - defer server.Close() - - client := coderbootstrap.NewSDKClient() - err := client.DeleteProvisionerKey(context.Background(), server.URL, "session-token", "", keyName) - require.NoError(t, err) - require.Equal(t, 1, deleteCalls) -} - -func TestDeleteProvisionerKey_NotFound(t *testing.T) { - t.Parallel() - - const keyName = "already-deleted" - orgID := uuid.New() - now := time.Now().UTC().Format(time.RFC3339) - deleteCalls := 0 - - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - switch { - case r.Method == http.MethodGet && r.URL.Path == "/api/v2/organizations/default": - writeJSONResponse(t, w, http.StatusOK, map[string]any{ - "id": orgID.String(), - "name": "default", - "created_at": now, - "updated_at": now, - }) - return - case r.Method == http.MethodDelete && r.URL.Path == "/api/v2/organizations/"+orgID.String()+"/provisionerkeys/"+keyName: - deleteCalls++ - writeJSONResponse(t, w, http.StatusNotFound, map[string]any{ - "message": "provisioner key not found", - }) - return - default: - writeJSONResponse(t, w, http.StatusNotFound, map[string]any{"message": "unexpected route"}) - return - } - })) - defer server.Close() - - client := coderbootstrap.NewSDKClient() - err := client.DeleteProvisionerKey(context.Background(), server.URL, "session-token", "", keyName) - require.NoError(t, err) - require.Equal(t, 1, deleteCalls) -} - func writeJSONResponse(t *testing.T, w http.ResponseWriter, statusCode int, payload any) { t.Helper() diff --git a/internal/controller/coderprovisioner_controller.go b/internal/controller/coderprovisioner_controller.go index d8b92780..7141a326 100644 --- a/internal/controller/coderprovisioner_controller.go +++ b/internal/controller/coderprovisioner_controller.go @@ -57,6 +57,7 @@ const ( externalProvisionerEntitlementRetryInterval = 2 * time.Minute provisionerOperatorAccessRetryInterval = 15 * time.Second provisionerPendingKeyRetryInterval = 2 * time.Minute + provisionerKeyNameInUseRetryInterval = 2 * time.Minute // #nosec G101 -- this is a field index key, not a credential. provisionerControlPlaneRefNameFieldIndex = ".spec.controlPlaneRef.name" @@ -266,6 +267,7 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req appliedTagsHash := provisioner.Status.TagsHash appliedControlPlaneRefName := provisioner.Status.ControlPlaneRefName appliedControlPlaneURL := provisioner.Status.ControlPlaneURL + appliedControlPlaneUID := provisioner.Status.ControlPlaneUID // Check whether a usable provisioner key secret already exists. // The secret is considered "usable" only if this provisioner owns it @@ -297,6 +299,7 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req // another slot than the target are deleted here. target := coderv1alpha1.CoderProvisionerKeyRecord{ ControlPlaneRefName: desiredControlPlaneRefName, + ControlPlaneUID: string(controlPlane.UID), CoderURL: coderURL, OrganizationName: organizationName, KeyName: keyName, @@ -309,6 +312,30 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req rotate := !secretUsable || slices.ContainsFunc(provisioner.Status.PendingKeys, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { return record.Unpublished }) + replacing := driftDetected || rotate || metadataMissing + // Coder allows one key per name in an organization. A key with the target + // name that status does not record can belong to another provisioner or + // a person, so the operator neither deletes nor replaces it, and changes + // nothing else until the name is free. + var existingKey *codersdk.ProvisionerKey + if replacing { + existingKey, err = r.BootstrapClient.ProvisionerKeyByName(ctx, coderbootstrap.ProvisionerKeyByNameRequest{ + CoderURL: coderURL, + SessionToken: sessionToken, + OrganizationName: organizationName, + KeyName: keyName, + }) + if err != nil { + setCondition(provisioner, coderv1alpha1.CoderProvisionerConditionProvisionerKeyReady, + metav1.ConditionFalse, "ProvisionerKeyFailed", + fmt.Sprintf("Failed to look up provisioner key %q", keyName)) + _ = r.Status().Update(ctx, provisioner) + return ctrl.Result{}, fmt.Errorf("look up provisioner key %q: %w", keyName, err) + } + if existingKey != nil && !recordsKeyID(provisioner.Status, existingKey.ID) { + return r.refuseKeyNameInUse(ctx, provisioner, coderURL, organizationName, keyName, existingKey.ID) + } + } pending := slices.Clone(provisioner.Status.PendingKeys) var replaced coderv1alpha1.CoderProvisionerKeyRecord if driftDetected { @@ -320,7 +347,7 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req pending = append(pending, replaced) } } - if driftDetected || rotate || metadataMissing { + if replacing { // The newest target record with IDs stays until a create at the // target proves its key gone. Coder allows one key per name, so each // earlier create at the target already proved the older ones gone. @@ -339,7 +366,7 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req pending = append(next, target) } var kept []coderv1alpha1.CoderProvisionerKeyRecord - var unverified []string + var unverified, controlPlaneChanged []string for _, record := range pending { if sameKeySlot(record, target) { kept = append(kept, record) @@ -353,6 +380,11 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req 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)) + continue + } if _, _, ok := parseKeyIDs(record.OrganizationID, record.KeyID); !ok { note := "the operator could not check Coder" if cleanup == keyUnverified { @@ -362,6 +394,7 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req } } conditionChanged := setUnverifiedKeyCondition(provisioner, unverified) + conditionChanged = setControlPlaneChangedKeyCondition(provisioner, controlPlaneChanged) || conditionChanged if conditionChanged || !slices.Equal(kept, provisioner.Status.PendingKeys) { provisioner.Status.PendingKeys = kept if err := r.Status().Update(ctx, provisioner); err != nil { @@ -373,109 +406,30 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req } } - log := ctrl.LoggerFrom(ctx) keyMaterial := "" - if driftDetected { - log.Info("spec drift detected, rotating provisioner key", - "orgDrift", orgDrift, "keyNameDrift", keyNameDrift, "tagsDrift", tagsDrift, - "controlPlaneRefDrift", controlPlaneRefDrift, "controlPlaneURLDrift", controlPlaneURLDrift) - - // A replaced key in another slot was deleted above. One in the target - // slot is deleted here, so that the create below can use its name. - if sameKeySlot(replaced, target) { - if deleteErr := r.BootstrapClient.DeleteProvisionerKey( - ctx, - coderURL, - sessionToken, - replaced.OrganizationName, - replaced.KeyName, - ); deleteErr != nil { - log.Info("failed to delete old provisioner key during drift rotation, creating new key anyway", - "oldKeyName", replaced.KeyName, "error", deleteErr) - } - } - - response, ensureErr := r.BootstrapClient.EnsureProvisionerKey(ctx, coderbootstrap.EnsureProvisionerKeyRequest{ - CoderURL: coderURL, - SessionToken: sessionToken, - OrganizationName: organizationName, - KeyName: keyName, - Tags: provisioner.Spec.Tags, - }) - if ensureErr != nil { - setCondition(provisioner, coderv1alpha1.CoderProvisionerConditionProvisionerKeyReady, - metav1.ConditionFalse, "ProvisionerKeyFailed", - fmt.Sprintf("Failed to ensure provisioner key %q after drift rotation", keyName)) - _ = r.Status().Update(ctx, provisioner) - return ctrl.Result{}, fmt.Errorf("ensure provisioner key %q: %w", keyName, ensureErr) - } - if response.OrganizationID != uuid.Nil { - organizationID = response.OrganizationID.String() - } - if response.KeyID != uuid.Nil { - provisionerKeyID = response.KeyID.String() - } - if response.KeyName != "" { - provisionerKeyName = response.KeyName - } - keyMaterial = response.Key - if keyMaterial == "" { - log.Info("drift-rotated key exists in coderd but returned no plaintext, rotating to recover", - "keyName", keyName) - - if deleteErr := r.BootstrapClient.DeleteProvisionerKey( - ctx, coderURL, sessionToken, organizationName, keyName, - ); deleteErr != nil { + if replacing { + if driftDetected { + ctrl.LoggerFrom(ctx).Info("spec drift detected, rotating provisioner key", + "orgDrift", orgDrift, "keyNameDrift", keyNameDrift, "tagsDrift", tagsDrift, + "controlPlaneRefDrift", controlPlaneRefDrift, "controlPlaneURLDrift", controlPlaneURLDrift) + } + // Coder returns the plaintext of a key only in the create response, + // so a key recorded in status is replaced: deleted by its ID, then + // created again with the desired tags. + if existingKey != nil { + if deleteErr := r.BootstrapClient.DeleteProvisionerKeyByID(ctx, coderbootstrap.DeleteProvisionerKeyByIDRequest{ + CoderURL: coderURL, + SessionToken: sessionToken, + OrganizationID: existingKey.OrganizationID, + KeyID: existingKey.ID, + }); deleteErr != nil { setCondition(provisioner, coderv1alpha1.CoderProvisionerConditionProvisionerKeyReady, metav1.ConditionFalse, "ProvisionerKeyFailed", - fmt.Sprintf("Failed to delete provisioner key %q for drift recovery", keyName)) + fmt.Sprintf("Failed to delete provisioner key %q to replace it", keyName)) _ = r.Status().Update(ctx, provisioner) - return ctrl.Result{}, fmt.Errorf("delete provisioner key %q for drift recovery: %w", keyName, deleteErr) - } - rotated, rotateErr := r.BootstrapClient.EnsureProvisionerKey(ctx, coderbootstrap.EnsureProvisionerKeyRequest{ - CoderURL: coderURL, - SessionToken: sessionToken, - OrganizationName: organizationName, - KeyName: keyName, - Tags: provisioner.Spec.Tags, - }) - if rotateErr != nil { - setCondition(provisioner, coderv1alpha1.CoderProvisionerConditionProvisionerKeyReady, - metav1.ConditionFalse, "ProvisionerKeyFailed", - fmt.Sprintf("Failed to recreate provisioner key %q after drift recovery", keyName)) - _ = r.Status().Update(ctx, provisioner) - return ctrl.Result{}, fmt.Errorf("recreate provisioner key %q after drift recovery: %w", keyName, rotateErr) - } - if rotated.OrganizationID != uuid.Nil { - organizationID = rotated.OrganizationID.String() - } - if rotated.KeyID != uuid.Nil { - provisionerKeyID = rotated.KeyID.String() - } - if rotated.KeyName != "" { - provisionerKeyName = rotated.KeyName - } - keyMaterial = rotated.Key - if keyMaterial == "" { - setCondition(provisioner, coderv1alpha1.CoderProvisionerConditionProvisionerKeyReady, - metav1.ConditionFalse, "ProvisionerKeyFailed", - fmt.Sprintf("Provisioner key %q returned empty material after drift recovery rotation", keyName)) - _ = r.Status().Update(ctx, provisioner) - return ctrl.Result{}, fmt.Errorf("assertion failed: provisioner key %q returned empty material after drift recovery rotation", keyName) + return ctrl.Result{}, fmt.Errorf("delete provisioner key %q to replace it: %w", keyName, deleteErr) } } - appliedOrgName = organizationName - appliedTagsHash = desiredTagsHash - appliedControlPlaneRefName = desiredControlPlaneRefName - appliedControlPlaneURL = controlPlane.Status.URL - setCondition( - provisioner, - coderv1alpha1.CoderProvisionerConditionProvisionerKeyReady, - metav1.ConditionTrue, - "ProvisionerKeyReady", - "Provisioner key is available in coderd", - ) - } else if rotate { response, ensureErr := r.BootstrapClient.EnsureProvisionerKey(ctx, coderbootstrap.EnsureProvisionerKeyRequest{ CoderURL: coderURL, SessionToken: sessionToken, @@ -490,6 +444,10 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req _ = r.Status().Update(ctx, provisioner) return ctrl.Result{}, fmt.Errorf("ensure provisioner key %q: %w", keyName, ensureErr) } + if response.Key == "" { + // Another key took the name after the lookup above. + return r.refuseKeyNameInUse(ctx, provisioner, coderURL, organizationName, keyName, response.KeyID) + } if response.OrganizationID != uuid.Nil { organizationID = response.OrganizationID.String() } @@ -500,144 +458,11 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req provisionerKeyName = response.KeyName } keyMaterial = response.Key - - // If the key already exists in coderd (e.g. the K8s secret was - // deleted), coderd won't return plaintext again. Rotate the key - // by deleting and recreating it to obtain fresh material. - if keyMaterial == "" { - log.Info("provisioner key exists in coderd but its plaintext is not in the secret, rotating key to recover", - "keyName", keyName, "secretName", keySecretName) - - if deleteErr := r.BootstrapClient.DeleteProvisionerKey( - ctx, coderURL, sessionToken, organizationName, keyName, - ); deleteErr != nil { - setCondition(provisioner, coderv1alpha1.CoderProvisionerConditionProvisionerKeyReady, - metav1.ConditionFalse, "ProvisionerKeyFailed", - fmt.Sprintf("Failed to delete stale provisioner key %q for rotation", keyName)) - _ = r.Status().Update(ctx, provisioner) - return ctrl.Result{}, fmt.Errorf("delete stale provisioner key %q for rotation: %w", keyName, deleteErr) - } - rotated, rotateErr := r.BootstrapClient.EnsureProvisionerKey(ctx, coderbootstrap.EnsureProvisionerKeyRequest{ - CoderURL: coderURL, - SessionToken: sessionToken, - OrganizationName: organizationName, - KeyName: keyName, - Tags: provisioner.Spec.Tags, - }) - if rotateErr != nil { - setCondition(provisioner, coderv1alpha1.CoderProvisionerConditionProvisionerKeyReady, - metav1.ConditionFalse, "ProvisionerKeyFailed", - fmt.Sprintf("Failed to recreate provisioner key %q after rotation", keyName)) - _ = r.Status().Update(ctx, provisioner) - return ctrl.Result{}, fmt.Errorf("recreate provisioner key %q after rotation: %w", keyName, rotateErr) - } - if rotated.OrganizationID != uuid.Nil { - organizationID = rotated.OrganizationID.String() - } - if rotated.KeyID != uuid.Nil { - provisionerKeyID = rotated.KeyID.String() - } - if rotated.KeyName != "" { - provisionerKeyName = rotated.KeyName - } - keyMaterial = rotated.Key - if keyMaterial == "" { - setCondition(provisioner, coderv1alpha1.CoderProvisionerConditionProvisionerKeyReady, - metav1.ConditionFalse, "ProvisionerKeyFailed", - fmt.Sprintf("Provisioner key %q returned empty key material after rotation", keyName)) - _ = r.Status().Update(ctx, provisioner) - return ctrl.Result{}, fmt.Errorf("assertion failed: provisioner key %q returned empty key material after rotation", keyName) - } - } appliedOrgName = organizationName appliedTagsHash = desiredTagsHash appliedControlPlaneRefName = desiredControlPlaneRefName appliedControlPlaneURL = controlPlane.Status.URL - setCondition( - provisioner, - coderv1alpha1.CoderProvisionerConditionProvisionerKeyReady, - metav1.ConditionTrue, - "ProvisionerKeyReady", - "Provisioner key is available in coderd", - ) - } else if metadataMissing { - // Secret is usable and no drift detected, but status metadata is empty - // (e.g. upgrade from older version). Call EnsureProvisionerKey to populate - // IDs and key name. If coderd reports an existing key (no plaintext key - // returned), rotate it best-effort so desired tags are applied before - // stamping the metadata baseline. - response, ensureErr := r.BootstrapClient.EnsureProvisionerKey(ctx, coderbootstrap.EnsureProvisionerKeyRequest{ - CoderURL: coderURL, - SessionToken: sessionToken, - OrganizationName: organizationName, - KeyName: keyName, - Tags: provisioner.Spec.Tags, - }) - if ensureErr != nil { - // Return a requeue so metadata population retries. - return ctrl.Result{}, fmt.Errorf("verify provisioner key metadata: %w", ensureErr) - } - if response.OrganizationID != uuid.Nil { - organizationID = response.OrganizationID.String() - } - if response.KeyID != uuid.Nil { - provisionerKeyID = response.KeyID.String() - } - if response.KeyName != "" { - provisionerKeyName = response.KeyName - } - if response.Key != "" { - // Key was freshly created with desired tags; capture material and stamp baseline. - keyMaterial = response.Key - appliedOrgName = organizationName - appliedTagsHash = desiredTagsHash - appliedControlPlaneRefName = desiredControlPlaneRefName - appliedControlPlaneURL = controlPlane.Status.URL - } else { - // Key already exists; tags may be stale. Rotate to ensure desired tags are applied. - log.Info("existing key found during metadata backfill, rotating to ensure desired tags", - "keyName", keyName) - if deleteErr := r.BootstrapClient.DeleteProvisionerKey( - ctx, coderURL, sessionToken, organizationName, keyName, - ); deleteErr != nil { - return ctrl.Result{}, fmt.Errorf("delete provisioner key %q for metadata backfill rotation: %w", keyName, deleteErr) - } - rotated, rotateErr := r.BootstrapClient.EnsureProvisionerKey(ctx, coderbootstrap.EnsureProvisionerKeyRequest{ - CoderURL: coderURL, - SessionToken: sessionToken, - OrganizationName: organizationName, - KeyName: keyName, - Tags: provisioner.Spec.Tags, - }) - if rotateErr != nil { - setCondition(provisioner, coderv1alpha1.CoderProvisionerConditionProvisionerKeyReady, - metav1.ConditionFalse, "ProvisionerKeyFailed", - fmt.Sprintf("Failed to recreate provisioner key %q after metadata backfill rotation", keyName)) - _ = r.Status().Update(ctx, provisioner) - return ctrl.Result{}, fmt.Errorf("recreate provisioner key %q after metadata backfill rotation: %w", keyName, rotateErr) - } - if rotated.OrganizationID != uuid.Nil { - organizationID = rotated.OrganizationID.String() - } - if rotated.KeyID != uuid.Nil { - provisionerKeyID = rotated.KeyID.String() - } - if rotated.KeyName != "" { - provisionerKeyName = rotated.KeyName - } - keyMaterial = rotated.Key - if keyMaterial == "" { - setCondition(provisioner, coderv1alpha1.CoderProvisionerConditionProvisionerKeyReady, - metav1.ConditionFalse, "ProvisionerKeyFailed", - fmt.Sprintf("Provisioner key %q returned empty material after metadata backfill rotation", keyName)) - _ = r.Status().Update(ctx, provisioner) - return ctrl.Result{}, fmt.Errorf("assertion failed: provisioner key %q returned empty material after metadata backfill rotation", keyName) - } - appliedOrgName = organizationName - appliedTagsHash = desiredTagsHash - appliedControlPlaneRefName = desiredControlPlaneRefName - appliedControlPlaneURL = controlPlane.Status.URL - } + appliedControlPlaneUID = string(controlPlane.UID) setCondition( provisioner, coderv1alpha1.CoderProvisionerConditionProvisionerKeyReady, @@ -715,7 +540,7 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req provisioner.Status.PendingKeys[i].Unpublished = false } recordAppliedKeyStatus(provisioner, organizationID, appliedOrgName, provisionerKeyID, provisionerKeyName, - appliedTagsHash, appliedControlPlaneRefName, appliedControlPlaneURL) + appliedTagsHash, appliedControlPlaneRefName, appliedControlPlaneURL, appliedControlPlaneUID) provisioner.Status.SecretRef = &coderv1alpha1.SecretKeySelector{Name: keySecretName, Key: keySecretKey} if !equality.Semantic.DeepEqual(*keyStatusSnapshot, provisioner.Status) { if err := r.Status().Update(ctx, provisioner); err != nil { @@ -825,7 +650,8 @@ func (r *CoderProvisionerReconciler) reconcileDeletion(ctx context.Context, prov } if cleanup != keyGone { log.Info("did not delete a recorded provisioner key during deletion, proceeding with finalizer removal", - "coderURL", record.CoderURL, "keyName", record.KeyName, "keyID", record.KeyID, "unverified", cleanup == keyUnverified) + "coderURL", record.CoderURL, "keyName", record.KeyName, "keyID", record.KeyID, "unverified", cleanup == keyUnverified, + "controlPlaneChanged", cleanup == keyControlPlaneChanged) } } @@ -848,7 +674,7 @@ func coderAPIURL(controlPlane *coderv1alpha1.CoderControlPlane) (string, error) // in Coder. func recordAppliedKeyStatus( provisioner *coderv1alpha1.CoderProvisioner, - organizationID, organizationName, keyID, keyName, tagsHash, controlPlaneRefName, controlPlaneURL string, + organizationID, organizationName, keyID, keyName, tagsHash, controlPlaneRefName, controlPlaneURL, controlPlaneUID string, ) { provisioner.Status.OrganizationID = organizationID provisioner.Status.OrganizationName = organizationName @@ -857,6 +683,45 @@ func recordAppliedKeyStatus( provisioner.Status.TagsHash = tagsHash provisioner.Status.ControlPlaneRefName = controlPlaneRefName provisioner.Status.ControlPlaneURL = controlPlaneURL + provisioner.Status.ControlPlaneUID = controlPlaneUID +} + +// recordsKeyID reports whether status records keyID as a key of this +// provisioner: the applied key or a pending entry. +func recordsKeyID(status coderv1alpha1.CoderProvisionerStatus, keyID uuid.UUID) bool { + if keyID == uuid.Nil { + return false + } + recorded := func(rawKeyID string) bool { + parsed, err := uuid.Parse(rawKeyID) + return err == nil && parsed == keyID + } + return recorded(status.ProvisionerKeyID) || slices.ContainsFunc(status.PendingKeys, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { + return recorded(record.KeyID) + }) +} + +// refuseKeyNameInUse reports that Coder holds a key with the target name that +// status does not record, and stops the reconcile before any key change. It +// checks again later, because nothing signals when that key goes. +func (r *CoderProvisionerReconciler) refuseKeyNameInUse( + ctx context.Context, + provisioner *coderv1alpha1.CoderProvisioner, + coderURL, organizationName, keyName string, + keyID uuid.UUID, +) (ctrl.Result, error) { + ctrl.LoggerFrom(ctx).Info("leaving a provisioner key with the target name that this provisioner did not record", + "coderURL", coderURL, "organization", organizationName, "keyName", keyName, "foundKeyID", keyID) + setCondition(provisioner, coderv1alpha1.CoderProvisionerConditionProvisionerKeyReady, + metav1.ConditionFalse, "KeyNameInUse", + fmt.Sprintf("Coder at %s holds provisioner key %q (ID %s) in organization %q, and status does not record it as a key of this provisioner. "+ + "Another provisioner or a person can use this name, so the operator does not delete or replace that key. "+ + "Set spec.key.name to a name that nothing else uses, or delete that key in Coder if nothing uses it.", + coderURL, keyName, keyID, organizationName)) + if err := r.Status().Update(ctx, provisioner); err != nil { + return ctrl.Result{}, fmt.Errorf("report provisioner key name %q in use: %w", keyName, err) + } + return ctrl.Result{RequeueAfter: provisionerKeyNameInUseRetryInterval}, nil } // revokeUnpublishedKey deletes a key whose plaintext was never written to a @@ -923,6 +788,7 @@ func (r *CoderProvisionerReconciler) appliedKeyRecord( status := provisioner.Status record := coderv1alpha1.CoderProvisionerKeyRecord{ ControlPlaneRefName: cmp.Or(status.ControlPlaneRefName, provisioner.Spec.ControlPlaneRef.Name), + ControlPlaneUID: status.ControlPlaneUID, CoderURL: strings.TrimSpace(status.ControlPlaneURL), OrganizationName: cmp.Or(status.OrganizationName, defaultOrganizationName), KeyName: cmp.Or(status.ProvisionerKeyName, defaultKeyName), @@ -996,8 +862,25 @@ const ( // its name that nothing shows this provisioner created. The operator // leaves that key, and the record stays. keyUnverified + // keyControlPlaneChanged: the control plane of the record may not be the + // one that the record names (see tokenBoundTo), so the operator sends it + // no operator token. The record stays. + keyControlPlaneChanged ) +// tokenBoundTo reports whether the operator may send the operator token of +// controlPlane for record. It may always send it to the current Coder API URL +// of controlPlane. It sends it to another URL that record names only while +// controlPlane has the UID that record names: a control plane deleted and +// created again under the same name must not send its token to the URL of +// the old one. +func tokenBoundTo(record coderv1alpha1.CoderProvisionerKeyRecord, controlPlane *coderv1alpha1.CoderControlPlane) bool { + if current, err := coderAPIURL(controlPlane); err == nil && current == record.CoderURL { + return true + } + return record.ControlPlaneUID != "" && record.ControlPlaneUID == string(controlPlane.UID) +} + // revokeKey deletes the key that record names, at record.CoderURL with the // operator token of the control plane record.ControlPlaneRefName. A record // without key IDs deletes no key. @@ -1012,6 +895,11 @@ func (r *CoderProvisionerReconciler) revokeKey( if err != nil || !ok { return keyKept, err } + if !tokenBoundTo(record, controlPlane) { + log.Info("the control plane of the provisioner key record changed, sending it no operator token", + "recordedUID", record.ControlPlaneUID, "currentUID", controlPlane.UID) + return keyControlPlaneChanged, nil + } sessionToken, tokenResult, tokenReason, tokenMessage, tokenErr := r.readOperatorSessionToken(ctx, provisioner, controlPlane) if tokenErr != nil || tokenResult.RequeueAfter > 0 { log.Info("operator access is not available, keeping the provisioner key record", @@ -1072,6 +960,24 @@ func setUnverifiedKeyCondition(provisioner *coderv1alpha1.CoderProvisioner, unve }) } +// setControlPlaneChangedKeyCondition sets the ControlPlaneChangedKeyKept +// condition for the pending entries that changed describes, or removes it when +// there are none. It reports whether the conditions changed. +func setControlPlaneChangedKeyCondition(provisioner *coderv1alpha1.CoderProvisioner, changed []string) bool { + if len(changed) == 0 { + return meta.RemoveStatusCondition(&provisioner.Status.Conditions, coderv1alpha1.CoderProvisionerConditionControlPlaneChangedKeyKept) + } + return meta.SetStatusCondition(&provisioner.Status.Conditions, metav1.Condition{ + Type: coderv1alpha1.CoderProvisionerConditionControlPlaneChangedKeyKept, + Status: metav1.ConditionTrue, + ObservedGeneration: provisioner.Generation, + Reason: "ControlPlaneUIDMismatch", + Message: "status.pendingKeys holds entries whose control plane now has another UID, or has no recorded UID and another Coder API URL, " + + "so the operator sends no operator token for them: " + strings.Join(changed, "; ") + + ". Delete such a key in Coder by hand, then remove its entry from status.pendingKeys.", + }) +} + func (r *CoderProvisionerReconciler) ensureCleanupFinalizer(ctx context.Context, provisioner *coderv1alpha1.CoderProvisioner) (bool, error) { if controllerutil.ContainsFinalizer(provisioner, coderv1alpha1.ProvisionerKeyCleanupFinalizer) { return false, nil diff --git a/internal/controller/coderprovisioner_controller_test.go b/internal/controller/coderprovisioner_controller_test.go index cc5e1dc9..05c8790f 100644 --- a/internal/controller/coderprovisioner_controller_test.go +++ b/internal/controller/coderprovisioner_controller_test.go @@ -452,7 +452,6 @@ func TestCoderProvisionerReconciler_BasicCreate(t *testing.T) { require.Equal(t, 1, bootstrapClient.provisionerKeyCalls) require.Len(t, bootstrapClient.provisionerKeyRequests, 1) require.Equal(t, "operator-session-token", bootstrapClient.provisionerKeyRequests[0].SessionToken) - require.Equal(t, 0, bootstrapClient.deleteKeyCalls) keySecret := &corev1.Secret{} keySecretName := types.NamespacedName{Name: provisioner.Spec.Key.SecretName, Namespace: provisioner.Namespace} @@ -581,7 +580,6 @@ func TestCoderProvisionerReconciler_Deletion(t *testing.T) { require.False(t, markedForDeletion.DeletionTimestamp.IsZero()) reconcileProvisioner(ctx, t, reconciler, namespacedName) - require.Zero(t, bootstrapClient.deleteKeyCalls) require.Len(t, bootstrapClient.deleteKeyByIDRequests, 1) require.Equal(t, bootstrapClient.provisionerKeyResponses[0].KeyID, bootstrapClient.deleteKeyByIDRequests[0].KeyID) @@ -636,9 +634,8 @@ func TestCoderProvisionerReconciler_DeletionControlPlaneGone(t *testing.T) { reconcileProvisioner(ctx, t, reconciler, namespacedName) - // DeleteProvisionerKey cannot run once operator token source is gone with the + // No key delete can run once the operator token source is gone with the // deleted control plane, but finalizer cleanup must still complete. - require.Equal(t, 0, bootstrapClient.deleteKeyCalls) require.Empty(t, bootstrapClient.deleteKeyByIDRequests) // The finalizer should still be removed. @@ -771,11 +768,11 @@ func TestCoderProvisionerReconciler_RotationOnSecretLoss(t *testing.T) { _ = k8sClient.Delete(context.Background(), provisioner) }) + initial := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: uuid.New(), KeyID: uuid.New(), KeyName: "rotation-key", Key: "initial-key-material"} bootstrapClient := &fakeBootstrapClient{ provisionerKeyResponses: []coderbootstrap.EnsureProvisionerKeyResponse{ - {KeyName: "rotation-key", Key: ""}, - {KeyName: "rotation-key", Key: "rotated-key-material"}, - {KeyName: "rotation-key", Key: "rotated-key-material"}, + initial, + {OrganizationID: initial.OrganizationID, KeyID: uuid.New(), KeyName: "rotation-key", Key: "rotated-key-material"}, }, } reconciler := &controller.CoderProvisionerReconciler{Client: k8sClient, Scheme: scheme, BootstrapClient: bootstrapClient} @@ -783,15 +780,22 @@ func TestCoderProvisionerReconciler_RotationOnSecretLoss(t *testing.T) { reconcileProvisioner(ctx, t, reconciler, request) reconcileProvisioner(ctx, t, reconciler, request) + + // The Secret is lost, and Coder still holds the key, which it returns + // without plaintext. + bootstrapClient.provisionerKeyByName = func(coderbootstrap.ProvisionerKeyByNameRequest) *codersdk.ProvisionerKey { + return &codersdk.ProvisionerKey{ID: initial.KeyID, OrganizationID: initial.OrganizationID, Name: initial.KeyName} + } + keySecretName := types.NamespacedName{Name: provisioner.Spec.Key.SecretName, Namespace: provisioner.Namespace} + require.NoError(t, k8sClient.Delete(ctx, &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: keySecretName.Name, Namespace: keySecretName.Namespace}})) reconcileProvisioner(ctx, t, reconciler, request) require.Equal(t, 2, bootstrapClient.provisionerKeyCalls) - require.Equal(t, 1, bootstrapClient.deleteKeyCalls) - require.Len(t, bootstrapClient.deleteKeyRequests, 1) - require.Equal(t, "rotation-key", bootstrapClient.deleteKeyRequests[0].KeyName) + require.Len(t, bootstrapClient.deleteKeyByIDRequests, 1) + require.Equal(t, initial.KeyID, bootstrapClient.deleteKeyByIDRequests[0].KeyID, "the rotation must delete the recorded key by its ID") keySecret := &corev1.Secret{} - require.NoError(t, k8sClient.Get(ctx, types.NamespacedName{Name: provisioner.Spec.Key.SecretName, Namespace: provisioner.Namespace}, keySecret)) + require.NoError(t, k8sClient.Get(ctx, keySecretName, keySecret)) require.Equal(t, "rotated-key-material", string(keySecret.Data[provisioner.Spec.Key.SecretKey])) } @@ -815,11 +819,11 @@ func TestCoderProvisionerReconciler_TagsDrift(t *testing.T) { _ = k8sClient.Delete(context.Background(), provisioner) }) + initial := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: uuid.New(), KeyID: uuid.New(), KeyName: "tags-drift-key", Key: "initial-key-material"} bootstrapClient := &fakeBootstrapClient{ provisionerKeyResponses: []coderbootstrap.EnsureProvisionerKeyResponse{ - {KeyName: "tags-drift-key", Key: "initial-key-material"}, - {KeyName: "tags-drift-key", Key: ""}, - {KeyName: "tags-drift-key", Key: "rotated-key-material"}, + initial, + {OrganizationID: initial.OrganizationID, KeyID: uuid.New(), KeyName: "tags-drift-key", Key: "rotated-key-material"}, }, } reconciler := &controller.CoderProvisionerReconciler{Client: k8sClient, Scheme: scheme, BootstrapClient: bootstrapClient} @@ -835,14 +839,15 @@ func TestCoderProvisionerReconciler_TagsDrift(t *testing.T) { before.Spec.Tags = map[string]string{"region": "eu-west"} require.NoError(t, k8sClient.Update(ctx, before)) + bootstrapClient.provisionerKeyByName = func(coderbootstrap.ProvisionerKeyByNameRequest) *codersdk.ProvisionerKey { + return &codersdk.ProvisionerKey{ID: initial.KeyID, OrganizationID: initial.OrganizationID, Name: initial.KeyName} + } reconcileProvisioner(ctx, t, reconciler, request) - require.Equal(t, 3, bootstrapClient.provisionerKeyCalls) - require.Equal(t, 2, bootstrapClient.deleteKeyCalls) - require.Len(t, bootstrapClient.deleteKeyRequests, 2) - require.Equal(t, "tags-drift-key", bootstrapClient.deleteKeyRequests[0].KeyName) - require.Equal(t, "tags-drift-key", bootstrapClient.deleteKeyRequests[1].KeyName) + require.Equal(t, 2, bootstrapClient.provisionerKeyCalls) + require.Len(t, bootstrapClient.deleteKeyByIDRequests, 1) + require.Equal(t, initial.KeyID, bootstrapClient.deleteKeyByIDRequests[0].KeyID, "the rotation must delete the old key by its ID") keySecret := &corev1.Secret{} require.NoError(t, k8sClient.Get(ctx, types.NamespacedName{Name: fmt.Sprintf("%s-provisioner-key", provisioner.Name), Namespace: provisioner.Namespace}, keySecret)) diff --git a/internal/controller/coderprovisioner_deletion_test.go b/internal/controller/coderprovisioner_deletion_test.go index 473ded99..881a206d 100644 --- a/internal/controller/coderprovisioner_deletion_test.go +++ b/internal/controller/coderprovisioner_deletion_test.go @@ -72,7 +72,6 @@ func TestCoderProvisionerDeletion_NoRecordedKeySkipsCoder(t *testing.T) { _, err := f.reconciler.Reconcile(ctx, request) require.NoError(t, err) - require.Zero(t, f.bootstrapClient.deleteKeyCalls, "no name-based key deletion") require.Empty(t, f.bootstrapClient.deleteKeyByIDRequests, "no key deletion without a recorded key") requireProvisionerGone(ctx, t, request.NamespacedName) }) @@ -101,7 +100,6 @@ func TestCoderProvisionerDeletion_DeletesRecordedKey(t *testing.T) { require.NoError(t, k8sClient.Delete(ctx, latest)) f.reconcile(ctx, t, provisioner) - require.Zero(t, f.bootstrapClient.deleteKeyCalls, "no name-based key deletion") require.Len(t, f.bootstrapClient.deleteKeyByIDRequests, 1) deleted := f.bootstrapClient.deleteKeyByIDRequests[0] require.Equal(t, recorded.OrganizationID, deleted.OrganizationID) @@ -278,6 +276,5 @@ func TestCoderProvisionerDeletion_TriesCurrentControlPlaneURL(t *testing.T) { require.Equal(t, newKey.KeyID, req.KeyID) require.Equal(t, testOperatorToken, req.SessionToken) } - require.Zero(t, f.bootstrapClient.deleteKeyCalls, "no name-based key deletion") requireProvisionerGone(ctx, t, types.NamespacedName{Name: provisioner.Name, Namespace: provisioner.Namespace}) } diff --git a/internal/controller/coderprovisioner_keyownership_test.go b/internal/controller/coderprovisioner_keyownership_test.go index 14bff177..ee06cb56 100644 --- a/internal/controller/coderprovisioner_keyownership_test.go +++ b/internal/controller/coderprovisioner_keyownership_test.go @@ -247,7 +247,7 @@ func TestCoderProvisionerKey_ControlPlaneRecreated(t *testing.T) { kept := slices.ContainsFunc(latest.Status.PendingKeys, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { return record.KeyID == keyOld.KeyID.String() && record.CoderURL == oldURL }) - report := meta.FindStatusCondition(latest.Status.Conditions, "ControlPlaneChangedKeyKept") + report := meta.FindStatusCondition(latest.Status.Conditions, coderv1alpha1.CoderProvisionerConditionControlPlaneChangedKeyKept) if tt.keep { require.False(t, kept, "the old key must be deleted at its URL") require.Nil(t, report) diff --git a/internal/controller/coderprovisioner_keysecret_test.go b/internal/controller/coderprovisioner_keysecret_test.go index 70125579..103adb74 100644 --- a/internal/controller/coderprovisioner_keysecret_test.go +++ b/internal/controller/coderprovisioner_keysecret_test.go @@ -101,10 +101,9 @@ func TestCoderProvisionerKeySecret_OwnedIsReused(t *testing.T) { t.Parallel() ctx := context.Background() f := newProvisionerCredentialFixture(ctx, t, "cprov-key-owned") - // The key exists in coderd, so coderd returns no plaintext and the - // controller rotates it into the Secret that this provisioner owns. + // Status records no key, so the controller creates one and writes it + // into the Secret that this provisioner owns. f.bootstrapClient.provisionerKeyResponses = []coderbootstrap.EnsureProvisionerKeyResponse{ - {OrganizationID: uuid.New(), KeyID: uuid.New(), KeyName: "provisioner", Key: ""}, {OrganizationID: uuid.New(), KeyID: uuid.New(), KeyName: "provisioner", Key: testRotatedKeyValue}, } provisioner := f.createKeyProvisioner(ctx, t, "provisioner", "") @@ -119,8 +118,7 @@ func TestCoderProvisionerKeySecret_OwnedIsReused(t *testing.T) { latest := f.get(ctx, t, provisioner) requireCredentialsReady(t, latest.Status.Conditions, metav1.ConditionTrue, "CredentialsAccepted") - require.Equal(t, 2, f.bootstrapClient.provisionerKeyCalls) - require.Equal(t, 1, f.bootstrapClient.deleteKeyCalls) + require.Equal(t, 1, f.bootstrapClient.provisionerKeyCalls) after := &corev1.Secret{} require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(secret), after)) diff --git a/internal/controller/coderprovisioner_pendingkeys_test.go b/internal/controller/coderprovisioner_pendingkeys_test.go index eafd1379..8be61ffa 100644 --- a/internal/controller/coderprovisioner_pendingkeys_test.go +++ b/internal/controller/coderprovisioner_pendingkeys_test.go @@ -61,14 +61,11 @@ func (c *keyRecordingClient) DeleteProvisionerKeyByID(ctx context.Context, req c // keyDelete is one key delete request that reached the fake Coder. type keyDelete struct { - coderURL, token, keyName, keyID string + coderURL, token, keyID string } func keyDeletes(client *fakeBootstrapClient) []keyDelete { var deletes []keyDelete - for _, req := range client.deleteKeyRequests { - deletes = append(deletes, keyDelete{coderURL: req.CoderURL, token: req.SessionToken, keyName: req.KeyName}) - } for _, req := range client.deleteKeyByIDRequests { deletes = append(deletes, keyDelete{coderURL: req.CoderURL, token: req.SessionToken, keyID: req.KeyID.String()}) } @@ -135,7 +132,7 @@ func TestCoderProvisionerKey_IntermediateEndpointRevoked(t *testing.T) { f.bootstrapClient.provisionerKeyResponses = []coderbootstrap.EnsureProvisionerKeyResponse{keyA, keyB, keyC} if tt.statusWriteFails { f.bootstrapClient.provisionerKeyByName = func(req coderbootstrap.ProvisionerKeyByNameRequest) *codersdk.ProvisionerKey { - if req.CoderURL != apiURL("coder-b") { + if req.CoderURL != apiURL("coder-b") || f.bootstrapClient.provisionerKeyCalls < 2 { return nil } return &codersdk.ProvisionerKey{ID: keyB.KeyID, OrganizationID: keyB.OrganizationID, Name: keyB.KeyName} @@ -250,6 +247,8 @@ func TestCoderProvisionerKey_IntermediateEndpointRevoked(t *testing.T) { // earlier reconcile created at the target and did not publish. The next // reconcile must replace them with a key that status records as applied, also // when nothing else asks for a rotation, and deletion must delete that key. +// A key whose create answer was lost is not recorded, so the provisioner +// must leave it and report it until it is deleted by hand. func TestCoderProvisionerKey_UnpublishedKeyIsRotated(t *testing.T) { t.Parallel() @@ -257,11 +256,14 @@ func TestCoderProvisionerKey_UnpublishedKeyIsRotated(t *testing.T) { name string // fail makes one reconcile leave an unpublished key at the target. fail func(ctx context.Context, t *testing.T, f provisionerCredentialFixture, provisioner *coderv1alpha1.CoderProvisioner, unpublished coderbootstrap.EnsureProvisionerKeyResponse) + // answerLost: status records no ID of the unpublished key. + answerLost bool }{ { // The create reached Coder, but the operator got no answer. The // spec change that caused the rotation is then reverted. - name: "create answer lost, spec reverted", + name: "create answer lost, spec reverted", + answerLost: true, fail: func(ctx context.Context, t *testing.T, f provisionerCredentialFixture, provisioner *coderv1alpha1.CoderProvisioner, _ coderbootstrap.EnsureProvisionerKeyResponse) { latest := f.get(ctx, t, provisioner) latest.Spec.Tags = map[string]string{"region": "rotated"} @@ -306,17 +308,29 @@ func TestCoderProvisionerKey_UnpublishedKeyIsRotated(t *testing.T) { f := newProvisionerCredentialFixture(ctx, t, "cprov-unpublished") first := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: uuid.New(), KeyID: uuid.New(), KeyName: "provisioner", Key: "material-1"} unpublished := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: first.OrganizationID, KeyID: uuid.New(), KeyName: "provisioner", Key: "material-2"} - // Coder lists the unpublished key, without plaintext, until the - // rotation deletes it by name. - listed := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: unpublished.OrganizationID, KeyID: unpublished.KeyID, KeyName: "provisioner"} final := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: first.OrganizationID, KeyID: uuid.New(), KeyName: "provisioner", Key: "material-3"} - f.bootstrapClient.provisionerKeyResponses = []coderbootstrap.EnsureProvisionerKeyResponse{first, unpublished, listed, final} + f.bootstrapClient.provisionerKeyResponses = []coderbootstrap.EnsureProvisionerKeyResponse{first, unpublished, final} + var inCoder *codersdk.ProvisionerKey + f.bootstrapClient.provisionerKeyByName = func(coderbootstrap.ProvisionerKeyByNameRequest) *codersdk.ProvisionerKey { return inCoder } 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) + inCoder = &codersdk.ProvisionerKey{ID: first.KeyID, OrganizationID: first.OrganizationID, Name: first.KeyName} tt.fail(ctx, t, f, provisioner, unpublished) - deletesBefore := len(f.bootstrapClient.deleteKeyRequests) + inCoder = &codersdk.ProvisionerKey{ID: unpublished.KeyID, OrganizationID: unpublished.OrganizationID, Name: unpublished.KeyName} + if tt.answerLost { + result, err := f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: name}) + require.NoError(t, err) + require.Positive(t, result.RequeueAfter) + keyReady := meta.FindStatusCondition(f.get(ctx, t, provisioner).Status.Conditions, coderv1alpha1.CoderProvisionerConditionProvisionerKeyReady) + require.NotNil(t, keyReady) + require.Equal(t, "KeyNameInUse", keyReady.Reason, "the provisioner must report the key that it cannot show it created") + require.Contains(t, keyReady.Message, unpublished.KeyID.String()) + require.NotContains(t, keyDeletes(f.bootstrapClient), keyDelete{coderURL: internalCoderURL(f), token: testOperatorToken, keyID: unpublished.KeyID.String()}) + inCoder = nil + } f.reconcile(ctx, t, provisioner) latest := f.get(ctx, t, provisioner) @@ -325,9 +339,10 @@ func TestCoderProvisionerKey_UnpublishedKeyIsRotated(t *testing.T) { secret := &corev1.Secret{} require.NoError(t, k8sClient.Get(ctx, types.NamespacedName{Name: provisioner.Name + "-provisioner-key", Namespace: provisioner.Namespace}, secret)) require.Equal(t, "material-3", string(secret.Data[coderv1alpha1.DefaultProvisionerKeySecretKey])) - require.Contains(t, f.bootstrapClient.deleteKeyRequests[deletesBefore:], - deleteKeyRequest{CoderURL: internalCoderURL(f), SessionToken: testOperatorToken, OrganizationName: "default", KeyName: "provisioner"}, - "the rotation must delete the unpublished key") + if !tt.answerLost { + require.Contains(t, keyDeletes(f.bootstrapClient), keyDelete{coderURL: internalCoderURL(f), token: testOperatorToken, keyID: unpublished.KeyID.String()}, + "the rotation must delete the unpublished key") + } require.NoError(t, k8sClient.Delete(ctx, latest)) f.reconcile(ctx, t, provisioner) @@ -489,11 +504,6 @@ func (c *keyStoreClient) EnsureProvisionerKey(_ context.Context, req coderbootst return response, nil } -func (c *keyStoreClient) DeleteProvisionerKey(_ context.Context, coderURL, _, organizationName, keyName string) error { - delete(c.keys, newKeySlot(coderURL, organizationName, keyName)) - return nil -} - func (c *keyStoreClient) DeleteProvisionerKeyByID(_ context.Context, req coderbootstrap.DeleteProvisionerKeyByIDRequest) error { maps.DeleteFunc(c.keys, func(slot keySlot, keyID uuid.UUID) bool { return slot.coderURL == req.CoderURL && slot.organizationID() == req.OrganizationID && keyID == req.KeyID @@ -514,13 +524,13 @@ func (c *keyStoreClient) ProvisionerKeyByName(_ context.Context, req coderbootst } // TestCoderProvisionerKey_IDLessRecordKeepsOtherProvisionersKey checks a -// provisioner P whose target moves from A to endpoint B, where provisioner Q -// holds its key under the same key name, and then to endpoint C. P records -// each create without key IDs, and each create fails before it reaches -// Coder. Neither the cleanup after P moves back to A nor the deletion of P -// may delete the key of Q. After the move back, P must report each entry it -// keeps, and drop an entry and its report once Coder has no key with its -// name. +// provisioner P whose target moves from A to endpoint B and then to endpoint +// C. P records each create without key IDs, and each create fails before it +// reaches Coder. Between the two moves, provisioner Q creates its key at B +// under the same key name. Neither the cleanup after P moves back to A nor +// the deletion of P may delete the key of Q. After the move back, P must +// report each entry it keeps, and drop an entry and its report once Coder has +// no key with its name. func TestCoderProvisionerKey_IDLessRecordKeepsOtherProvisionersKey(t *testing.T) { t.Parallel() @@ -542,16 +552,6 @@ func TestCoderProvisionerKey_IDLessRecordKeepsOtherProvisionersKey(t *testing.T) store := &keyStoreClient{fakeBootstrapClient: f.bootstrapClient, keys: map[keySlot]uuid.UUID{}} f.reconciler.BootstrapClient = store - q := f.createKeyProvisioner(ctx, t, "q", "") - q.Spec.ControlPlaneRef.Name = "coder-b" - q.Spec.Key.Name = "provisioner" - require.NoError(t, k8sClient.Update(ctx, q)) - f.reconcile(ctx, t, q) - f.reconcile(ctx, t, q) - slotB := newKeySlot(urlB, "", "provisioner") - qKeyID := f.get(ctx, t, q).Status.ProvisionerKeyID - require.Equal(t, store.keys[slotB].String(), qKeyID, "Q must hold the key named provisioner at B") - p := f.createProvisioner(ctx, t, nil, nil) pName := types.NamespacedName{Name: p.Name, Namespace: p.Namespace} f.reconcile(ctx, t, p) @@ -570,6 +570,8 @@ func TestCoderProvisionerKey_IDLessRecordKeepsOtherProvisionersKey(t *testing.T) return meta.FindStatusCondition(f.get(ctx, t, p).Status.Conditions, coderv1alpha1.CoderProvisionerConditionUnverifiedKeyKept) } + slotB := newKeySlot(urlB, "", "provisioner") + var qKeyID string store.failEnsureAt = []string{urlB, urlC} for _, controlPlane := range []string{"coder-b", "coder-c"} { moveTo(controlPlane) @@ -578,6 +580,18 @@ func TestCoderProvisionerKey_IDLessRecordKeepsOtherProvisionersKey(t *testing.T) require.True(t, slices.ContainsFunc(f.get(ctx, t, p).Status.PendingKeys, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { return record.CoderURL == apiURL(controlPlane) && record.KeyName == "provisioner" && record.KeyID == "" }), "P must record its create at %s without key IDs", controlPlane) + if controlPlane != "coder-b" { + continue + } + store.failEnsureAt = []string{urlC} + q := f.createKeyProvisioner(ctx, t, "q", "") + q.Spec.ControlPlaneRef.Name = "coder-b" + q.Spec.Key.Name = "provisioner" + require.NoError(t, k8sClient.Update(ctx, q)) + f.reconcile(ctx, t, q) + f.reconcile(ctx, t, q) + qKeyID = f.get(ctx, t, q).Status.ProvisionerKeyID + require.Equal(t, store.keys[slotB].String(), qKeyID, "Q must hold the key named provisioner at B") } if deleteP { diff --git a/internal/controller/coderworkspaceproxy_controller_test.go b/internal/controller/coderworkspaceproxy_controller_test.go index 93f7674e..f78a62be 100644 --- a/internal/controller/coderworkspaceproxy_controller_test.go +++ b/internal/controller/coderworkspaceproxy_controller_test.go @@ -29,9 +29,6 @@ type fakeBootstrapClient struct { provisionerKeyErr error provisionerKeyCalls int provisionerKeyRequests []coderbootstrap.EnsureProvisionerKeyRequest - deleteKeyErr error - deleteKeyCalls int - deleteKeyRequests []deleteKeyRequest deleteKeyByIDErr error deleteKeyByIDRequests []coderbootstrap.DeleteProvisionerKeyByIDRequest // provisionerKeyByName answers ProvisionerKeyByName. When it is nil, @@ -45,13 +42,6 @@ type fakeBootstrapClient struct { entitlementsRequests []entitlementsRequest } -type deleteKeyRequest struct { - CoderURL string - SessionToken string - OrganizationName string - KeyName string -} - type entitlementsRequest struct { CoderURL string SessionToken string @@ -80,17 +70,6 @@ func (f *fakeBootstrapClient) EnsureProvisionerKey(_ context.Context, req coderb return f.provisionerKeyResponses[idx], nil } -func (f *fakeBootstrapClient) DeleteProvisionerKey(_ context.Context, coderURL, sessionToken, orgName, keyName string) error { - f.deleteKeyCalls++ - f.deleteKeyRequests = append(f.deleteKeyRequests, deleteKeyRequest{ - CoderURL: coderURL, - SessionToken: sessionToken, - OrganizationName: orgName, - KeyName: keyName, - }) - return f.deleteKeyErr -} - func (f *fakeBootstrapClient) DeleteProvisionerKeyByID(_ context.Context, req coderbootstrap.DeleteProvisionerKeyByIDRequest) error { f.deleteKeyByIDRequests = append(f.deleteKeyByIDRequests, req) return f.deleteKeyByIDErr From 5fcce533572f2efe8d7f0cd3f839aa29151fa8e3 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 18:54:07 +0000 Subject: [PATCH 03/13] =?UTF-8?q?=F0=9F=A4=96=20fix:=20restore=20Provision?= =?UTF-8?q?erKeyReady=20after=20a=20key=20name=20conflict=20is=20reverted?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A revert to the applied key name needs no rotation, so the reconcile never cleared KeyNameInUse. Set ProvisionerKeyReady=True whenever the reconcile passes the key step. Align the controlPlaneUID field docs with the rule in tokenBoundTo: a control plane's current Coder API URL always gets its token, another recorded URL only while the recorded UID matches. Change-Id: I6e8eb9d01717d6feca66368d812f40f7d1028abd Signed-off-by: Thomas Kosiewski --- api/v1alpha1/coderprovisioner_types.go | 26 +++++++++---------- .../bases/coder.com_coderprovisioners.yaml | 15 +++++------ dist/install.yaml | 15 +++++------ docs/reference/api/coderprovisioner.md | 4 +-- .../controller/coderprovisioner_controller.go | 14 +++++----- .../coderprovisioner_keyownership_test.go | 24 ++++++++++------- 6 files changed, 49 insertions(+), 49 deletions(-) diff --git a/api/v1alpha1/coderprovisioner_types.go b/api/v1alpha1/coderprovisioner_types.go index 18e95758..e0f82bc1 100644 --- a/api/v1alpha1/coderprovisioner_types.go +++ b/api/v1alpha1/coderprovisioner_types.go @@ -37,12 +37,11 @@ const ( // entry, and the operator removes the condition when none is left. CoderProvisionerConditionUnverifiedKeyKept = "UnverifiedKeyKept" // CoderProvisionerConditionControlPlaneChangedKeyKept is True while - // status.pendingKeys holds an entry whose control plane the operator - // cannot show is the one that the entry recorded: the control plane with - // its name has another UID, or the entry has no UID and its coderURL is - // not the current Coder API URL of that control plane. The operator sends - // no operator token for such an entry. The message names each entry, and - // the operator removes the condition when none is left. + // status.pendingKeys holds an entry whose coderURL is not the current + // Coder API URL of its control plane, and that control plane has another + // UID than the entry records, or the entry records no UID. The operator + // sends no operator token for such an entry. The message names each + // entry, and the operator removes the condition when none is left. CoderProvisionerConditionControlPlaneChangedKeyKept = "ControlPlaneChangedKeyKept" // DefaultProvisionerKeySecretKey is the default data key for provisioner key secrets. @@ -120,9 +119,9 @@ type CoderProvisionerStatus struct { ControlPlaneURL string `json:"controlPlaneURL,omitempty"` // ControlPlaneUID is the UID of the control plane last applied to the // provisioner key. The operator sends the operator token of the control - // plane with controlPlaneRefName for this key only while that control - // plane has this UID. When it is empty, it sends that token only to the - // current Coder API URL of that control plane. + // plane with controlPlaneRefName for this key to the current Coder API URL + // of that control plane, and to another URL only while that control plane + // has this UID. ControlPlaneUID string `json:"controlPlaneUID,omitempty"` // SecretRef references the provisioner key secret data currently in use. SecretRef *SecretKeySelector `json:"secretRef,omitempty"` @@ -144,11 +143,10 @@ type CoderProvisionerKeyRecord struct { // operator used for the key. ControlPlaneRefName string `json:"controlPlaneRefName"` // ControlPlaneUID is the UID of that CoderControlPlane. The operator - // sends its operator token for this entry only while the control plane - // with controlPlaneRefName has this UID. When it is empty, it sends that - // token only when coderURL is the current Coder API URL of that control - // plane. Otherwise it keeps the entry and sets the condition - // ControlPlaneChangedKeyKept. + // sends its operator token for this entry when coderURL is the current + // Coder API URL of the control plane with controlPlaneRefName, or while + // that control plane has this UID. Otherwise it keeps the entry and sets + // the condition ControlPlaneChangedKeyKept. ControlPlaneUID string `json:"controlPlaneUID,omitempty"` // CoderURL is the Coder API URL that the operator sent the key request to. CoderURL string `json:"coderURL"` diff --git a/config/crd/bases/coder.com_coderprovisioners.yaml b/config/crd/bases/coder.com_coderprovisioners.yaml index d3439c19..48762ada 100644 --- a/config/crd/bases/coder.com_coderprovisioners.yaml +++ b/config/crd/bases/coder.com_coderprovisioners.yaml @@ -421,9 +421,9 @@ spec: description: |- ControlPlaneUID is the UID of the control plane last applied to the provisioner key. The operator sends the operator token of the control - plane with controlPlaneRefName for this key only while that control - plane has this UID. When it is empty, it sends that token only to the - current Coder API URL of that control plane. + plane with controlPlaneRefName for this key to the current Coder API URL + of that control plane, and to another URL only while that control plane + has this UID. type: string controlPlaneURL: description: ControlPlaneURL is the control plane URL last applied @@ -468,11 +468,10 @@ spec: controlPlaneUID: description: |- ControlPlaneUID is the UID of that CoderControlPlane. The operator - sends its operator token for this entry only while the control plane - with controlPlaneRefName has this UID. When it is empty, it sends that - token only when coderURL is the current Coder API URL of that control - plane. Otherwise it keeps the entry and sets the condition - ControlPlaneChangedKeyKept. + sends its operator token for this entry when coderURL is the current + Coder API URL of the control plane with controlPlaneRefName, or while + that control plane has this UID. Otherwise it keeps the entry and sets + the condition ControlPlaneChangedKeyKept. type: string keyID: description: KeyID is the provisioner key ID that Coder returned. diff --git a/dist/install.yaml b/dist/install.yaml index 32354e78..ef1fa313 100644 --- a/dist/install.yaml +++ b/dist/install.yaml @@ -6183,9 +6183,9 @@ spec: description: |- ControlPlaneUID is the UID of the control plane last applied to the provisioner key. The operator sends the operator token of the control - plane with controlPlaneRefName for this key only while that control - plane has this UID. When it is empty, it sends that token only to the - current Coder API URL of that control plane. + plane with controlPlaneRefName for this key to the current Coder API URL + of that control plane, and to another URL only while that control plane + has this UID. type: string controlPlaneURL: description: ControlPlaneURL is the control plane URL last applied @@ -6230,11 +6230,10 @@ spec: controlPlaneUID: description: |- ControlPlaneUID is the UID of that CoderControlPlane. The operator - sends its operator token for this entry only while the control plane - with controlPlaneRefName has this UID. When it is empty, it sends that - token only when coderURL is the current Coder API URL of that control - plane. Otherwise it keeps the entry and sets the condition - ControlPlaneChangedKeyKept. + sends its operator token for this entry when coderURL is the current + Coder API URL of the control plane with controlPlaneRefName, or while + that control plane has this UID. Otherwise it keeps the entry and sets + the condition ControlPlaneChangedKeyKept. type: string keyID: description: KeyID is the provisioner key ID that Coder returned. diff --git a/docs/reference/api/coderprovisioner.md b/docs/reference/api/coderprovisioner.md index e94c850d..a97e15e9 100644 --- a/docs/reference/api/coderprovisioner.md +++ b/docs/reference/api/coderprovisioner.md @@ -40,7 +40,7 @@ | `tagsHash` | string | TagsHash is a deterministic hash of spec.tags last applied to the provisioner key. | | `controlPlaneRefName` | string | ControlPlaneRefName is the control plane ref name last applied to the provisioner key. | | `controlPlaneURL` | string | ControlPlaneURL is the control plane URL last applied to the provisioner key. | -| `controlPlaneUID` | string | ControlPlaneUID is the UID of the control plane last applied to the provisioner key. The operator sends the operator token of the control plane with controlPlaneRefName for this key only while that control plane has this UID. When it is empty, it sends that token only to the current Coder API URL of that control plane. | +| `controlPlaneUID` | string | ControlPlaneUID is the UID of the control plane last applied to the provisioner key. The operator sends the operator token of the control plane with controlPlaneRefName for this key to the current Coder API URL of that control plane, and to another URL only while that control plane has this UID. | | `secretRef` | [SecretKeySelector](#secretkeyselector) | SecretRef references the provisioner key secret data currently in use. | | `pendingKeys` | [CoderProvisionerKeyRecord](#coderprovisionerkeyrecord) array | PendingKeys lists the provisioner keys that can exist in Coder besides the key in provisionerKeyID: a key that the operator is about to create or has not yet written to the provisioner key Secret, and replaced keys whose deletion Coder has not confirmed. The operator deletes the key of each entry with organizationID and keyID at its coderURL with the operator token of its control plane, and removes the entry when Coder confirms that the key is gone. | @@ -54,7 +54,7 @@ or was about to create, at one Coder endpoint. | Field | Type | Description | | --- | --- | --- | | `controlPlaneRefName` | string | ControlPlaneRefName is the CoderControlPlane whose operator token the operator used for the key. | -| `controlPlaneUID` | string | ControlPlaneUID is the UID of that CoderControlPlane. The operator sends its operator token for this entry only while the control plane with controlPlaneRefName has this UID. When it is empty, it sends that token only when coderURL is the current Coder API URL of that control plane. Otherwise it keeps the entry and sets the condition ControlPlaneChangedKeyKept. | +| `controlPlaneUID` | string | ControlPlaneUID is the UID of that CoderControlPlane. The operator sends its operator token for this entry when coderURL is the current Coder API URL of the control plane with controlPlaneRefName, or while that control plane has this UID. Otherwise it keeps the entry and sets the condition ControlPlaneChangedKeyKept. | | `coderURL` | string | CoderURL is the Coder API URL that the operator sent the key request to. | | `organizationName` | string | OrganizationName is the Coder organization of the key. | | `keyName` | string | KeyName is the provisioner key name in Coder. | diff --git a/internal/controller/coderprovisioner_controller.go b/internal/controller/coderprovisioner_controller.go index 7141a326..e7c4a3b8 100644 --- a/internal/controller/coderprovisioner_controller.go +++ b/internal/controller/coderprovisioner_controller.go @@ -463,14 +463,14 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req appliedControlPlaneRefName = desiredControlPlaneRefName appliedControlPlaneURL = controlPlane.Status.URL appliedControlPlaneUID = string(controlPlane.UID) - setCondition( - provisioner, - coderv1alpha1.CoderProvisionerConditionProvisionerKeyReady, - metav1.ConditionTrue, - "ProvisionerKeyReady", - "Provisioner key is available in coderd", - ) } + setCondition( + provisioner, + coderv1alpha1.CoderProvisionerConditionProvisionerKeyReady, + metav1.ConditionTrue, + "ProvisionerKeyReady", + "Provisioner key is available in coderd", + ) if keyMaterial != "" { // A new key exists in Coder. Record its IDs in its pending record diff --git a/internal/controller/coderprovisioner_keyownership_test.go b/internal/controller/coderprovisioner_keyownership_test.go index ee06cb56..3ea2992d 100644 --- a/internal/controller/coderprovisioner_keyownership_test.go +++ b/internal/controller/coderprovisioner_keyownership_test.go @@ -1,6 +1,7 @@ package controller_test import ( + "cmp" "context" "slices" "testing" @@ -24,7 +25,7 @@ import ( // the key. Whatever way P reaches that key name, P must not delete or replace // the key of Q, and must not change its own key, Secret or Deployment. It must // report the conflict, and get its own key once it has a distinct key name or -// the key of Q is gone. +// the key of Q is gone, or keep its key after a revert to its key name. func TestCoderProvisionerKey_OtherProvisionersKeyKept(t *testing.T) { t.Parallel() @@ -36,13 +37,14 @@ func TestCoderProvisionerKey_OtherProvisionersKeyKept(t *testing.T) { // dropKeyStatus removes the key fields from the status of P, as // status that lost its key records. dropKeyStatus bool - // rename resolves the conflict with a distinct key name. Otherwise - // the key of Q goes. - rename bool + // resolveName resolves the conflict with this key name for P. + // Otherwise the key of Q goes. + resolveName string }{ {name: "first key of P"}, - {name: "key name change of P", holdsKey: true, rename: true}, - {name: "status without key records", holdsKey: true, dropKeyStatus: true, rename: true}, + {name: "key name change of P", holdsKey: true, resolveName: "p-own"}, + {name: "key name change of P, reverted", holdsKey: true, resolveName: "p-key"}, + {name: "status without key records", holdsKey: true, dropKeyStatus: true, resolveName: "p-own"}, } for _, tt := range tests { @@ -118,9 +120,8 @@ func TestCoderProvisionerKey_OtherProvisionersKeyKept(t *testing.T) { require.NoError(t, err) require.Equal(t, resourceVersion, f.get(ctx, t, p).ResourceVersion, "a repeated conflict must not write status") - keyName := "provisioner" - if tt.rename { - keyName = "p-own" + keyName := cmp.Or(tt.resolveName, "provisioner") + if tt.resolveName != "" { latest.Spec.Key.Name = keyName require.NoError(t, k8sClient.Update(ctx, latest)) } else { @@ -133,9 +134,12 @@ func TestCoderProvisionerKey_OtherProvisionersKeyKept(t *testing.T) { require.NotNil(t, keyReady) require.Equal(t, metav1.ConditionTrue, keyReady.Status) require.Nil(t, meta.FindStatusCondition(latest.Status.Conditions, coderv1alpha1.CoderProvisionerConditionUnverifiedKeyKept)) - if tt.rename { + if tt.resolveName != "" { require.Equal(t, qKeyID, store.keys[slotOf("provisioner")], "the key of Q must still exist") } + if tt.resolveName == "p-key" { + require.Equal(t, pKeyID.String(), latest.Status.ProvisionerKeyID, "a revert must keep the key of P") + } }) } } From cd33036074dc841087a52b0ee10a6fb7207dfab2 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 19:08:57 +0000 Subject: [PATCH 04/13] =?UTF-8?q?=F0=9F=A4=96=20fix:=20require=20the=20rec?= =?UTF-8?q?orded=20control=20plane=20UID=20and=20rotate=20when=20it=20chan?= =?UTF-8?q?ges?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex review of #420: - A control plane created again under the same name keeps its in-cluster Coder API URL with TLS, and can serve another Coder backend there. A record with a UID now gets its control plane's token only while the UID matches, also at the current URL: an answer from another backend does not show that the old key is gone. Records without a UID keep the current-URL rule. - A changed control plane UID is now drift, so the provisioner creates a new key instead of keeping one that the new backend can reject. - Records with different control plane UIDs name different key slots, so the replaced key of the old control plane stays recorded and reported. A record of the key that the target replacement deletes is dropped with the target records. Change-Id: I87c3908df4a3f1e32896367da5018d32c413fc5f Signed-off-by: Thomas Kosiewski --- api/v1alpha1/coderprovisioner_types.go | 26 ++++---- .../bases/coder.com_coderprovisioners.yaml | 16 +++-- dist/install.yaml | 16 +++-- docs/reference/api/coderprovisioner.md | 4 +- docs/tutorials/deploy-coderprovisioner.md | 2 +- .../controller/coderprovisioner_controller.go | 64 +++++++++++-------- .../coderprovisioner_keyownership_test.go | 59 +++++++++++++---- 7 files changed, 119 insertions(+), 68 deletions(-) diff --git a/api/v1alpha1/coderprovisioner_types.go b/api/v1alpha1/coderprovisioner_types.go index e0f82bc1..bca2a08e 100644 --- a/api/v1alpha1/coderprovisioner_types.go +++ b/api/v1alpha1/coderprovisioner_types.go @@ -37,11 +37,11 @@ const ( // entry, and the operator removes the condition when none is left. CoderProvisionerConditionUnverifiedKeyKept = "UnverifiedKeyKept" // CoderProvisionerConditionControlPlaneChangedKeyKept is True while - // status.pendingKeys holds an entry whose coderURL is not the current - // Coder API URL of its control plane, and that control plane has another - // UID than the entry records, or the entry records no UID. The operator - // sends no operator token for such an entry. The message names each - // entry, and the operator removes the condition when none is left. + // status.pendingKeys holds an entry whose control plane has another UID + // than the entry records, or an entry without a UID whose coderURL is not + // the current Coder API URL of its control plane. The operator sends no + // operator token for such an entry. The message names each entry, and the + // operator removes the condition when none is left. CoderProvisionerConditionControlPlaneChangedKeyKept = "ControlPlaneChangedKeyKept" // DefaultProvisionerKeySecretKey is the default data key for provisioner key secrets. @@ -119,9 +119,10 @@ type CoderProvisionerStatus struct { ControlPlaneURL string `json:"controlPlaneURL,omitempty"` // ControlPlaneUID is the UID of the control plane last applied to the // provisioner key. The operator sends the operator token of the control - // plane with controlPlaneRefName for this key to the current Coder API URL - // of that control plane, and to another URL only while that control plane - // has this UID. + // plane with controlPlaneRefName for this key only while that control + // plane has this UID, and rotates the key when it has another UID. When it + // is empty, it sends that token only to the current Coder API URL of that + // control plane. ControlPlaneUID string `json:"controlPlaneUID,omitempty"` // SecretRef references the provisioner key secret data currently in use. SecretRef *SecretKeySelector `json:"secretRef,omitempty"` @@ -143,10 +144,11 @@ type CoderProvisionerKeyRecord struct { // operator used for the key. ControlPlaneRefName string `json:"controlPlaneRefName"` // ControlPlaneUID is the UID of that CoderControlPlane. The operator - // sends its operator token for this entry when coderURL is the current - // Coder API URL of the control plane with controlPlaneRefName, or while - // that control plane has this UID. Otherwise it keeps the entry and sets - // the condition ControlPlaneChangedKeyKept. + // sends its operator token for this entry only while the control plane + // with controlPlaneRefName has this UID. When it is empty, it sends that + // token only when coderURL is the current Coder API URL of that control + // plane. Otherwise it keeps the entry and sets the condition + // ControlPlaneChangedKeyKept. ControlPlaneUID string `json:"controlPlaneUID,omitempty"` // CoderURL is the Coder API URL that the operator sent the key request to. CoderURL string `json:"coderURL"` diff --git a/config/crd/bases/coder.com_coderprovisioners.yaml b/config/crd/bases/coder.com_coderprovisioners.yaml index 48762ada..55a11064 100644 --- a/config/crd/bases/coder.com_coderprovisioners.yaml +++ b/config/crd/bases/coder.com_coderprovisioners.yaml @@ -421,9 +421,10 @@ spec: description: |- ControlPlaneUID is the UID of the control plane last applied to the provisioner key. The operator sends the operator token of the control - plane with controlPlaneRefName for this key to the current Coder API URL - of that control plane, and to another URL only while that control plane - has this UID. + plane with controlPlaneRefName for this key only while that control + plane has this UID, and rotates the key when it has another UID. When it + is empty, it sends that token only to the current Coder API URL of that + control plane. type: string controlPlaneURL: description: ControlPlaneURL is the control plane URL last applied @@ -468,10 +469,11 @@ spec: controlPlaneUID: description: |- ControlPlaneUID is the UID of that CoderControlPlane. The operator - sends its operator token for this entry when coderURL is the current - Coder API URL of the control plane with controlPlaneRefName, or while - that control plane has this UID. Otherwise it keeps the entry and sets - the condition ControlPlaneChangedKeyKept. + sends its operator token for this entry only while the control plane + with controlPlaneRefName has this UID. When it is empty, it sends that + token only when coderURL is the current Coder API URL of that control + plane. Otherwise it keeps the entry and sets the condition + ControlPlaneChangedKeyKept. type: string keyID: description: KeyID is the provisioner key ID that Coder returned. diff --git a/dist/install.yaml b/dist/install.yaml index ef1fa313..ace41043 100644 --- a/dist/install.yaml +++ b/dist/install.yaml @@ -6183,9 +6183,10 @@ spec: description: |- ControlPlaneUID is the UID of the control plane last applied to the provisioner key. The operator sends the operator token of the control - plane with controlPlaneRefName for this key to the current Coder API URL - of that control plane, and to another URL only while that control plane - has this UID. + plane with controlPlaneRefName for this key only while that control + plane has this UID, and rotates the key when it has another UID. When it + is empty, it sends that token only to the current Coder API URL of that + control plane. type: string controlPlaneURL: description: ControlPlaneURL is the control plane URL last applied @@ -6230,10 +6231,11 @@ spec: controlPlaneUID: description: |- ControlPlaneUID is the UID of that CoderControlPlane. The operator - sends its operator token for this entry when coderURL is the current - Coder API URL of the control plane with controlPlaneRefName, or while - that control plane has this UID. Otherwise it keeps the entry and sets - the condition ControlPlaneChangedKeyKept. + sends its operator token for this entry only while the control plane + with controlPlaneRefName has this UID. When it is empty, it sends that + token only when coderURL is the current Coder API URL of that control + plane. Otherwise it keeps the entry and sets the condition + ControlPlaneChangedKeyKept. type: string keyID: description: KeyID is the provisioner key ID that Coder returned. diff --git a/docs/reference/api/coderprovisioner.md b/docs/reference/api/coderprovisioner.md index a97e15e9..9369a55f 100644 --- a/docs/reference/api/coderprovisioner.md +++ b/docs/reference/api/coderprovisioner.md @@ -40,7 +40,7 @@ | `tagsHash` | string | TagsHash is a deterministic hash of spec.tags last applied to the provisioner key. | | `controlPlaneRefName` | string | ControlPlaneRefName is the control plane ref name last applied to the provisioner key. | | `controlPlaneURL` | string | ControlPlaneURL is the control plane URL last applied to the provisioner key. | -| `controlPlaneUID` | string | ControlPlaneUID is the UID of the control plane last applied to the provisioner key. The operator sends the operator token of the control plane with controlPlaneRefName for this key to the current Coder API URL of that control plane, and to another URL only while that control plane has this UID. | +| `controlPlaneUID` | string | ControlPlaneUID is the UID of the control plane last applied to the provisioner key. The operator sends the operator token of the control plane with controlPlaneRefName for this key only while that control plane has this UID, and rotates the key when it has another UID. When it is empty, it sends that token only to the current Coder API URL of that control plane. | | `secretRef` | [SecretKeySelector](#secretkeyselector) | SecretRef references the provisioner key secret data currently in use. | | `pendingKeys` | [CoderProvisionerKeyRecord](#coderprovisionerkeyrecord) array | PendingKeys lists the provisioner keys that can exist in Coder besides the key in provisionerKeyID: a key that the operator is about to create or has not yet written to the provisioner key Secret, and replaced keys whose deletion Coder has not confirmed. The operator deletes the key of each entry with organizationID and keyID at its coderURL with the operator token of its control plane, and removes the entry when Coder confirms that the key is gone. | @@ -54,7 +54,7 @@ or was about to create, at one Coder endpoint. | Field | Type | Description | | --- | --- | --- | | `controlPlaneRefName` | string | ControlPlaneRefName is the CoderControlPlane whose operator token the operator used for the key. | -| `controlPlaneUID` | string | ControlPlaneUID is the UID of that CoderControlPlane. The operator sends its operator token for this entry when coderURL is the current Coder API URL of the control plane with controlPlaneRefName, or while that control plane has this UID. Otherwise it keeps the entry and sets the condition ControlPlaneChangedKeyKept. | +| `controlPlaneUID` | string | ControlPlaneUID is the UID of that CoderControlPlane. The operator sends its operator token for this entry only while the control plane with controlPlaneRefName has this UID. When it is empty, it sends that token only when coderURL is the current Coder API URL of that control plane. Otherwise it keeps the entry and sets the condition ControlPlaneChangedKeyKept. | | `coderURL` | string | CoderURL is the Coder API URL that the operator sent the key request to. | | `organizationName` | string | OrganizationName is the Coder organization of the key. | | `keyName` | string | KeyName is the provisioner key name in Coder. | diff --git a/docs/tutorials/deploy-coderprovisioner.md b/docs/tutorials/deploy-coderprovisioner.md index b2156b02..b0b4b2db 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, each at its own Coder URL. 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 to the current Coder API URL of that control plane, and to another URL that a record names only while the control plane has the recorded UID. When a control plane is deleted and created again under the same name with another URL, the operator does not send the new token to the old URL. The 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. 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, each at its own Coder URL. 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. 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 e7c4a3b8..b7b93a32 100644 --- a/internal/controller/coderprovisioner_controller.go +++ b/internal/controller/coderprovisioner_controller.go @@ -262,7 +262,10 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req tagsDrift := status.TagsHash != "" && status.TagsHash != desiredTagsHash controlPlaneRefDrift := status.ControlPlaneRefName != "" && status.ControlPlaneRefName != desiredControlPlaneRefName controlPlaneURLDrift := status.ControlPlaneURL != "" && status.ControlPlaneURL != controlPlane.Status.URL - driftDetected := orgDrift || keyNameDrift || tagsDrift || controlPlaneRefDrift || controlPlaneURLDrift + // A control plane deleted and created again under the same name can run + // another Coder backend at the same URL, where the applied key is unknown. + controlPlaneUIDDrift := status.ControlPlaneUID != "" && status.ControlPlaneUID != string(controlPlane.UID) + driftDetected := orgDrift || keyNameDrift || tagsDrift || controlPlaneRefDrift || controlPlaneURLDrift || controlPlaneUIDDrift appliedOrgName := provisioner.Status.OrganizationName appliedTagsHash := provisioner.Status.TagsHash appliedControlPlaneRefName := provisioner.Status.ControlPlaneRefName @@ -368,7 +371,8 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req var kept []coderv1alpha1.CoderProvisionerKeyRecord var unverified, controlPlaneChanged []string for _, record := range pending { - if sameKeySlot(record, target) { + // The replacement below deletes the key that the target holds. + if sameKeySlot(record, target) || (existingKey != nil && isKeyID(record.KeyID, existingKey.ID)) { kept = append(kept, record) continue } @@ -411,7 +415,8 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req if driftDetected { ctrl.LoggerFrom(ctx).Info("spec drift detected, rotating provisioner key", "orgDrift", orgDrift, "keyNameDrift", keyNameDrift, "tagsDrift", tagsDrift, - "controlPlaneRefDrift", controlPlaneRefDrift, "controlPlaneURLDrift", controlPlaneURLDrift) + "controlPlaneRefDrift", controlPlaneRefDrift, "controlPlaneURLDrift", controlPlaneURLDrift, + "controlPlaneUIDDrift", controlPlaneUIDDrift) } // Coder returns the plaintext of a key only in the create response, // so a key recorded in status is replaced: deleted by its ID, then @@ -531,10 +536,11 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req } // The Secret holds the key at the target, so the applied fields take over // its record. Coder allows one key per name, so the create also proved - // that the keys of the other target records are gone. No other key is in - // the Secret now. + // that the keys of the other target records are gone, and so did the + // delete of the key that the create replaced, wherever it was recorded. + // No other key is in the Secret now. provisioner.Status.PendingKeys = slices.DeleteFunc(provisioner.Status.PendingKeys, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { - return sameKeySlot(record, target) + return sameKeySlot(record, target) || (existingKey != nil && isKeyID(record.KeyID, existingKey.ID)) }) for i := range provisioner.Status.PendingKeys { provisioner.Status.PendingKeys[i].Unpublished = false @@ -689,18 +695,17 @@ func recordAppliedKeyStatus( // recordsKeyID reports whether status records keyID as a key of this // provisioner: the applied key or a pending entry. func recordsKeyID(status coderv1alpha1.CoderProvisionerStatus, keyID uuid.UUID) bool { - if keyID == uuid.Nil { - return false - } - recorded := func(rawKeyID string) bool { - parsed, err := uuid.Parse(rawKeyID) - return err == nil && parsed == keyID - } - return recorded(status.ProvisionerKeyID) || slices.ContainsFunc(status.PendingKeys, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { - return recorded(record.KeyID) + return isKeyID(status.ProvisionerKeyID, keyID) || slices.ContainsFunc(status.PendingKeys, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { + return isKeyID(record.KeyID, keyID) }) } +// isKeyID reports whether rawKeyID is the key ID keyID. +func isKeyID(rawKeyID string, keyID uuid.UUID) bool { + parsed, err := uuid.Parse(rawKeyID) + return err == nil && keyID != uuid.Nil && parsed == keyID +} + // refuseKeyNameInUse reports that Coder holds a key with the target name that // status does not record, and stops the reconcile before any key change. It // checks again later, because nothing signals when that key goes. @@ -770,10 +775,15 @@ func parseKeyIDs(rawOrganizationID, rawKeyID string) (organizationID, keyID uuid } // sameKeySlot reports whether two records name the same key name in the same -// organization at the same Coder URL. Coder allows one key per name in an -// organization, so both records refer to the same key. +// organization at the same Coder URL of the same control plane. Coder allows +// one key per name in an organization, so both records refer to the same key. +// A control plane created again under the same name can serve another Coder +// backend at the same URL, so records with different control plane UIDs name +// different slots. A record without a UID is from an operator that did not +// record it, and matches any UID. func sameKeySlot(a, b coderv1alpha1.CoderProvisionerKeyRecord) bool { - return a.CoderURL == b.CoderURL && a.OrganizationName == b.OrganizationName && a.KeyName == b.KeyName + sameControlPlane := a.ControlPlaneUID == "" || b.ControlPlaneUID == "" || a.ControlPlaneUID == b.ControlPlaneUID + return sameControlPlane && a.CoderURL == b.CoderURL && a.OrganizationName == b.OrganizationName && a.KeyName == b.KeyName } // appliedKeyRecord returns a record of the key that the applied status fields @@ -869,16 +879,18 @@ const ( ) // tokenBoundTo reports whether the operator may send the operator token of -// controlPlane for record. It may always send it to the current Coder API URL -// of controlPlane. It sends it to another URL that record names only while -// controlPlane has the UID that record names: a control plane deleted and -// created again under the same name must not send its token to the URL of -// the old one. +// controlPlane for record: only while controlPlane has the UID that record +// names. A control plane deleted and created again under the same name must +// not get its token sent to the URL of the old one, and its answer does not +// show whether the key of the old one is gone, also at the same URL. A record +// without a UID, from an operator that did not record it, is bound only at +// the current Coder API URL of controlPlane. func tokenBoundTo(record coderv1alpha1.CoderProvisionerKeyRecord, controlPlane *coderv1alpha1.CoderControlPlane) bool { - if current, err := coderAPIURL(controlPlane); err == nil && current == record.CoderURL { - return true + if record.ControlPlaneUID != "" { + return record.ControlPlaneUID == string(controlPlane.UID) } - return record.ControlPlaneUID != "" && record.ControlPlaneUID == string(controlPlane.UID) + current, err := coderAPIURL(controlPlane) + return err == nil && current == record.CoderURL } // revokeKey deletes the key that record names, at record.CoderURL with the diff --git a/internal/controller/coderprovisioner_keyownership_test.go b/internal/controller/coderprovisioner_keyownership_test.go index 3ea2992d..ca2692c8 100644 --- a/internal/controller/coderprovisioner_keyownership_test.go +++ b/internal/controller/coderprovisioner_keyownership_test.go @@ -164,11 +164,13 @@ func sentTokens(client *fakeBootstrapClient, lookups []urlToken) []urlToken { } // TestCoderProvisionerKey_ControlPlaneRecreated checks a control plane that is -// deleted and created again under the same name with another URL and another -// operator token. The new token must never reach the URL of the old control -// plane: the record of the old key stays, and the provisioner reports it. A -// record from an operator that did not record control plane UIDs may use the -// token of its control plane only at that control plane's current URL. +// deleted and created again under the same name with another operator token, +// at another URL or at the same URL with another Coder backend. The new token +// must never reach the URL of the old control plane, and must never be used +// for the key of the old one: the record of the old key stays, and the +// provisioner reports it. The provisioner must create a new key. A record +// from an operator that did not record control plane UIDs may use the token +// of its control plane only at that control plane's current URL. func TestCoderProvisionerKey_ControlPlaneRecreated(t *testing.T) { t.Parallel() @@ -183,11 +185,19 @@ func TestCoderProvisionerKey_ControlPlaneRecreated(t *testing.T) { // before UID records wrote it. legacy bool // keep keeps the control plane and moves P to another one instead. - keep bool - deleteP bool + keep bool + // sameURL creates the control plane again at its old URL. + sameURL bool + // sameBackend: the new control plane serves the Coder backend of the + // old one, which still holds the old key. + sameBackend bool + deleteP bool }{ {name: "rotation after the recreate"}, {name: "deletion after the recreate", deleteP: true}, + {name: "rotation after the recreate at the same URL", sameURL: true}, + {name: "deletion after the recreate at the same URL", sameURL: true, deleteP: true}, + {name: "rotation after the recreate at the same URL and backend", sameURL: true, sameBackend: true}, {name: "status without control plane UID, rotation after the recreate", legacy: true}, {name: "status without control plane UID, deletion after the recreate", legacy: true, deleteP: true}, {name: "status without control plane UID, control plane kept", legacy: true, keep: true}, @@ -203,9 +213,13 @@ func TestCoderProvisionerKey_ControlPlaneRecreated(t *testing.T) { keyNew := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: uuid.New(), KeyID: uuid.New(), KeyName: "provisioner", Key: "material-new"} f.bootstrapClient.provisionerKeyResponses = []coderbootstrap.EnsureProvisionerKeyResponse{keyOld, keyNew} var lookups []urlToken + var recreated bool f.bootstrapClient.provisionerKeyByName = func(req coderbootstrap.ProvisionerKeyByNameRequest) *codersdk.ProvisionerKey { lookups = append(lookups, urlToken{req.CoderURL, req.SessionToken}) - return nil + if !tt.sameBackend || !recreated || req.CoderURL != oldURL || len(f.bootstrapClient.deleteKeyByIDRequests) > 0 { + return nil + } + return &codersdk.ProvisionerKey{ID: keyOld.KeyID, OrganizationID: keyOld.OrganizationID, Name: keyOld.KeyName} } p := f.createProvisioner(ctx, t, nil, nil) @@ -218,6 +232,10 @@ func TestCoderProvisionerKey_ControlPlaneRecreated(t *testing.T) { ctrlclient.RawPatch(types.MergePatchType, []byte(`{"status":{"controlPlaneUID":null}}`)))) } + currentURL := newURL + if tt.sameURL { + currentURL = oldURL + } if tt.keep { createControlPlaneWithToken(ctx, t, f.namespace, "coder-b", newURL, newToken) latest := f.get(ctx, t, p) @@ -226,11 +244,20 @@ func TestCoderProvisionerKey_ControlPlaneRecreated(t *testing.T) { } else { require.NoError(t, k8sClient.Delete(ctx, &coderv1alpha1.CoderControlPlane{ObjectMeta: metav1.ObjectMeta{Name: "coder", Namespace: f.namespace}})) require.NoError(t, k8sClient.Delete(ctx, &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "coder-operator-token", Namespace: f.namespace}})) - createControlPlaneWithToken(ctx, t, f.namespace, "coder", newURL, newToken) + createControlPlaneWithToken(ctx, t, f.namespace, "coder", currentURL, newToken) } + recreated = true requireNoLeak := func() { t.Helper() + // A record without a UID is bound at the current URL, so + // only a recorded UID keeps the new token from the old key. + for _, d := range keyDeletes(f.bootstrapClient) { + require.False(t, !tt.legacy && !tt.sameBackend && d.keyID == keyOld.KeyID.String() && d.token == newToken, "the new control plane's token must not be used for the old key, sent %+v", keyDeletes(f.bootstrapClient)) + } + if tt.sameURL { + return + } sent := sentTokens(f.bootstrapClient, lookups) for _, s := range sent { require.False(t, s.coderURL == oldURL && s.token == newToken, "the new control plane's token must not reach %s, sent %+v", oldURL, sent) @@ -246,16 +273,22 @@ func TestCoderProvisionerKey_ControlPlaneRecreated(t *testing.T) { require.NoError(t, err) requireNoLeak() latest := f.get(ctx, t, p) - require.Equal(t, keyNew.KeyID.String(), latest.Status.ProvisionerKeyID) - require.Equal(t, newURL, latest.Status.ControlPlaneURL) + require.Equal(t, keyNew.KeyID.String(), latest.Status.ProvisionerKeyID, "P must create a new key") + require.Equal(t, currentURL, latest.Status.ControlPlaneURL) kept := slices.ContainsFunc(latest.Status.PendingKeys, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { return record.KeyID == keyOld.KeyID.String() && record.CoderURL == oldURL }) report := meta.FindStatusCondition(latest.Status.Conditions, coderv1alpha1.CoderProvisionerConditionControlPlaneChangedKeyKept) - if tt.keep { + switch { + case tt.sameBackend: + require.Contains(t, keyDeletes(f.bootstrapClient), keyDelete{coderURL: oldURL, token: newToken, keyID: keyOld.KeyID.String()}, + "the rotation must delete the old key that the current backend holds") + require.False(t, kept, "a key that the current backend deleted must not stay recorded") + require.Nil(t, report) + case tt.keep: require.False(t, kept, "the old key must be deleted at its URL") require.Nil(t, report) - } else { + default: require.True(t, kept, "the record of the old key must stay") require.Positive(t, result.RequeueAfter, "P must check the kept record again") require.NotNil(t, report, "P must report the record that it keeps") From 94bdf890157f603990c2097e21e8ebd81cc4d2b4 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 19:22:49 +0000 Subject: [PATCH 05/13] =?UTF-8?q?=F0=9F=A4=96=20fix:=20verify=20the=20appl?= =?UTF-8?q?ied=20key=20of=20status=20without=20a=20control=20plane=20UID?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex review of #420: after an upgrade, status.controlPlaneUID is empty, so a later recreate of the control plane under the same name and URL went undetected. A reconcile of such status now looks the applied key up at the current control plane. When Coder holds it there, the operator records that control plane's UID, so a later recreate is drift. When it does not, the provisioner creates a new key. Change-Id: Iaa1eb2ee78c71263b6775524e3bbef63dbff9af9 Signed-off-by: Thomas Kosiewski --- .../controller/coderprovisioner_controller.go | 13 +- .../coderprovisioner_keyownership_test.go | 113 +++++++++++------- 2 files changed, 83 insertions(+), 43 deletions(-) diff --git a/internal/controller/coderprovisioner_controller.go b/internal/controller/coderprovisioner_controller.go index b7b93a32..b4943888 100644 --- a/internal/controller/coderprovisioner_controller.go +++ b/internal/controller/coderprovisioner_controller.go @@ -316,12 +316,17 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req return record.Unpublished }) replacing := driftDetected || rotate || metadataMissing + // Status from an operator that did not record the control plane UID does + // not show that the applied key belongs to the Coder backend of the + // current control plane. The applied key is kept, and bound to that + // control plane, only when the lookup below finds it there. + verifyApplied := !replacing && status.ControlPlaneUID == "" // Coder allows one key per name in an organization. A key with the target // name that status does not record can belong to another provisioner or // a person, so the operator neither deletes nor replaces it, and changes // nothing else until the name is free. var existingKey *codersdk.ProvisionerKey - if replacing { + if replacing || verifyApplied { existingKey, err = r.BootstrapClient.ProvisionerKeyByName(ctx, coderbootstrap.ProvisionerKeyByNameRequest{ CoderURL: coderURL, SessionToken: sessionToken, @@ -335,6 +340,12 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req _ = r.Status().Update(ctx, provisioner) return ctrl.Result{}, fmt.Errorf("look up provisioner key %q: %w", keyName, err) } + if verifyApplied && existingKey != nil && isKeyID(status.ProvisionerKeyID, existingKey.ID) { + appliedControlPlaneUID = string(controlPlane.UID) + existingKey = nil + } else { + replacing = true + } if existingKey != nil && !recordsKeyID(provisioner.Status, existingKey.ID) { return r.refuseKeyNameInUse(ctx, provisioner, coderURL, organizationName, keyName, existingKey.ID) } diff --git a/internal/controller/coderprovisioner_keyownership_test.go b/internal/controller/coderprovisioner_keyownership_test.go index ca2692c8..4b8bdfe3 100644 --- a/internal/controller/coderprovisioner_keyownership_test.go +++ b/internal/controller/coderprovisioner_keyownership_test.go @@ -170,7 +170,8 @@ func sentTokens(client *fakeBootstrapClient, lookups []urlToken) []urlToken { // for the key of the old one: the record of the old key stays, and the // provisioner reports it. The provisioner must create a new key. A record // from an operator that did not record control plane UIDs may use the token -// of its control plane only at that control plane's current URL. +// of its control plane only at that control plane's current URL, and its +// applied key stays only when the current backend holds it. func TestCoderProvisionerKey_ControlPlaneRecreated(t *testing.T) { t.Parallel() @@ -179,11 +180,20 @@ func TestCoderProvisionerKey_ControlPlaneRecreated(t *testing.T) { newURL = "http://coder-new.example.com" newToken = "operator-token-new" ) + // What a rotation after the change leaves of the old key. + const ( + oldKeyKept = "kept" // its record stays, and the provisioner reports it + oldKeyDeleted = "deleted" // deleted where Coder holds it, no record left + oldKeyDropped = "dropped" // the current backend has no such key, no record left + ) tests := []struct { name string // legacy removes the control plane UID from status, as an operator // before UID records wrote it. legacy bool + // verifyFirst reconciles once before the recreate, while the backend + // holds the old key. + verifyFirst bool // keep keeps the control plane and moves P to another one instead. keep bool // sameURL creates the control plane again at its old URL. @@ -192,15 +202,19 @@ func TestCoderProvisionerKey_ControlPlaneRecreated(t *testing.T) { // old one, which still holds the old key. sameBackend bool deleteP bool + // oldKey is what a rotation leaves of the old key. + oldKey string }{ - {name: "rotation after the recreate"}, + {name: "rotation after the recreate", oldKey: oldKeyKept}, {name: "deletion after the recreate", deleteP: true}, - {name: "rotation after the recreate at the same URL", sameURL: true}, + {name: "rotation after the recreate at the same URL", sameURL: true, oldKey: oldKeyKept}, {name: "deletion after the recreate at the same URL", sameURL: true, deleteP: true}, - {name: "rotation after the recreate at the same URL and backend", sameURL: true, sameBackend: true}, - {name: "status without control plane UID, rotation after the recreate", legacy: true}, + {name: "rotation after the recreate at the same URL and backend", sameURL: true, sameBackend: true, oldKey: oldKeyDeleted}, + {name: "status without control plane UID, rotation after the recreate", legacy: true, oldKey: oldKeyKept}, {name: "status without control plane UID, deletion after the recreate", legacy: true, deleteP: true}, - {name: "status without control plane UID, control plane kept", legacy: true, keep: true}, + {name: "status without control plane UID, recreate at the same URL", legacy: true, sameURL: true, oldKey: oldKeyDropped}, + {name: "status without control plane UID, verified before the recreate at the same URL", legacy: true, verifyFirst: true, sameURL: true, oldKey: oldKeyKept}, + {name: "status without control plane UID, control plane kept", legacy: true, keep: true, oldKey: oldKeyDeleted}, } for _, tt := range tests { @@ -213,10 +227,11 @@ func TestCoderProvisionerKey_ControlPlaneRecreated(t *testing.T) { keyNew := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: uuid.New(), KeyID: uuid.New(), KeyName: "provisioner", Key: "material-new"} f.bootstrapClient.provisionerKeyResponses = []coderbootstrap.EnsureProvisionerKeyResponse{keyOld, keyNew} var lookups []urlToken - var recreated bool + // oldKeyInCoder: the backend at oldURL holds the old key. + oldKeyInCoder := false f.bootstrapClient.provisionerKeyByName = func(req coderbootstrap.ProvisionerKeyByNameRequest) *codersdk.ProvisionerKey { lookups = append(lookups, urlToken{req.CoderURL, req.SessionToken}) - if !tt.sameBackend || !recreated || req.CoderURL != oldURL || len(f.bootstrapClient.deleteKeyByIDRequests) > 0 { + if !oldKeyInCoder || req.CoderURL != oldURL || len(f.bootstrapClient.deleteKeyByIDRequests) > 0 { return nil } return &codersdk.ProvisionerKey{ID: keyOld.KeyID, OrganizationID: keyOld.OrganizationID, Name: keyOld.KeyName} @@ -231,6 +246,14 @@ func TestCoderProvisionerKey_ControlPlaneRecreated(t *testing.T) { require.NoError(t, k8sClient.Status().Patch(ctx, f.get(ctx, t, p), ctrlclient.RawPatch(types.MergePatchType, []byte(`{"status":{"controlPlaneUID":null}}`)))) } + if tt.verifyFirst { + oldKeyInCoder = true + f.reconcile(ctx, t, p) + latest := f.get(ctx, t, p) + require.Equal(t, keyOld.KeyID.String(), latest.Status.ProvisionerKeyID, "a key that the backend holds must stay") + require.NotEmpty(t, latest.Status.ControlPlaneUID, "the operator must bind the verified key to its control plane") + } + oldKeyInCoder = tt.sameBackend currentURL := newURL if tt.sameURL { @@ -246,14 +269,15 @@ func TestCoderProvisionerKey_ControlPlaneRecreated(t *testing.T) { require.NoError(t, k8sClient.Delete(ctx, &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "coder-operator-token", Namespace: f.namespace}})) createControlPlaneWithToken(ctx, t, f.namespace, "coder", currentURL, newToken) } - recreated = true requireNoLeak := func() { t.Helper() - // A record without a UID is bound at the current URL, so - // only a recorded UID keeps the new token from the old key. + // A record without a UID is bound at the current URL, so only + // a recorded UID keeps the new token from the old key. + bound := !tt.legacy || tt.verifyFirst for _, d := range keyDeletes(f.bootstrapClient) { - require.False(t, !tt.legacy && !tt.sameBackend && d.keyID == keyOld.KeyID.String() && d.token == newToken, "the new control plane's token must not be used for the old key, sent %+v", keyDeletes(f.bootstrapClient)) + require.False(t, bound && !tt.sameBackend && d.keyID == keyOld.KeyID.String() && d.token == newToken, + "the new control plane's token must not be used for the old key, sent %+v", keyDeletes(f.bootstrapClient)) } if tt.sameURL { return @@ -268,38 +292,43 @@ func TestCoderProvisionerKey_ControlPlaneRecreated(t *testing.T) { f.reconcile(ctx, t, p) requireNoLeak() requireProvisionerGone(ctx, t, pName) - } else { - result, err := f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: pName}) - require.NoError(t, err) - requireNoLeak() - latest := f.get(ctx, t, p) - require.Equal(t, keyNew.KeyID.String(), latest.Status.ProvisionerKeyID, "P must create a new key") - require.Equal(t, currentURL, latest.Status.ControlPlaneURL) - kept := slices.ContainsFunc(latest.Status.PendingKeys, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { - return record.KeyID == keyOld.KeyID.String() && record.CoderURL == oldURL - }) - report := meta.FindStatusCondition(latest.Status.Conditions, coderv1alpha1.CoderProvisionerConditionControlPlaneChangedKeyKept) - switch { - case tt.sameBackend: - require.Contains(t, keyDeletes(f.bootstrapClient), keyDelete{coderURL: oldURL, token: newToken, keyID: keyOld.KeyID.String()}, - "the rotation must delete the old key that the current backend holds") - require.False(t, kept, "a key that the current backend deleted must not stay recorded") - require.Nil(t, report) - case tt.keep: - require.False(t, kept, "the old key must be deleted at its URL") - require.Nil(t, report) - default: - require.True(t, kept, "the record of the old key must stay") - require.Positive(t, result.RequeueAfter, "P must check the kept record again") - require.NotNil(t, report, "P must report the record that it keeps") - require.Equal(t, metav1.ConditionTrue, report.Status) - require.Contains(t, report.Message, oldURL) - } + return } - if tt.keep { - require.Contains(t, keyDeletes(f.bootstrapClient), keyDelete{coderURL: oldURL, token: testOperatorToken, keyID: keyOld.KeyID.String()}, - "the old key must be deleted at its URL with the token of its control plane") + result, err := f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: pName}) + require.NoError(t, err) + requireNoLeak() + latest := f.get(ctx, t, p) + require.Equal(t, keyNew.KeyID.String(), latest.Status.ProvisionerKeyID, "P must create a new key") + require.Equal(t, currentURL, latest.Status.ControlPlaneURL) + kept := slices.ContainsFunc(latest.Status.PendingKeys, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { + return record.KeyID == keyOld.KeyID.String() && record.CoderURL == oldURL + }) + report := meta.FindStatusCondition(latest.Status.Conditions, coderv1alpha1.CoderProvisionerConditionControlPlaneChangedKeyKept) + oldKeyDeletes := slices.DeleteFunc(keyDeletes(f.bootstrapClient), func(d keyDelete) bool { return d.keyID != keyOld.KeyID.String() }) + switch tt.oldKey { + case oldKeyKept: + require.True(t, kept, "the record of the old key must stay") + require.Positive(t, result.RequeueAfter, "P must check the kept record again") + require.NotNil(t, report, "P must report the record that it keeps") + require.Equal(t, metav1.ConditionTrue, report.Status) + require.Contains(t, report.Message, oldURL) + require.Empty(t, oldKeyDeletes) + case oldKeyDeleted: + token := newToken + if tt.keep { + token = testOperatorToken + } + require.Equal(t, []keyDelete{{coderURL: oldURL, token: token, keyID: keyOld.KeyID.String()}}, oldKeyDeletes, + "the old key must be deleted once, where Coder holds it, with the token of the control plane that serves it") + require.False(t, kept, "a deleted key must not stay recorded") + require.Nil(t, report) + case oldKeyDropped: + require.Empty(t, oldKeyDeletes) + require.False(t, kept) + require.Nil(t, report) + default: + t.Fatalf("assertion failed: unknown oldKey %q", tt.oldKey) } }) } From 9b59a917913f32c8f3c63ebb79f6a0209c94c331 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 19:23:27 +0000 Subject: [PATCH 06/13] =?UTF-8?q?=F0=9F=A4=96=20docs:=20describe=20the=20a?= =?UTF-8?q?pplied=20key=20check=20for=20status=20without=20a=20control=20p?= =?UTF-8?q?lane=20UID?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Change-Id: I9f73b49b2eec2f1cf18b29857c9f2c3a9cd63e9b Signed-off-by: Thomas Kosiewski --- api/v1alpha1/coderprovisioner_types.go | 3 ++- config/crd/bases/coder.com_coderprovisioners.yaml | 3 ++- dist/install.yaml | 3 ++- docs/reference/api/coderprovisioner.md | 2 +- docs/tutorials/deploy-coderprovisioner.md | 2 +- 5 files changed, 8 insertions(+), 5 deletions(-) diff --git a/api/v1alpha1/coderprovisioner_types.go b/api/v1alpha1/coderprovisioner_types.go index bca2a08e..9e9515cc 100644 --- a/api/v1alpha1/coderprovisioner_types.go +++ b/api/v1alpha1/coderprovisioner_types.go @@ -122,7 +122,8 @@ type CoderProvisionerStatus struct { // plane with controlPlaneRefName for this key only while that control // plane has this UID, and rotates the key when it has another UID. When it // is empty, it sends that token only to the current Coder API URL of that - // control plane. + // control plane, and the next reconcile looks the key up there: it sets + // this field when Coder holds the key, and creates a new key otherwise. ControlPlaneUID string `json:"controlPlaneUID,omitempty"` // SecretRef references the provisioner key secret data currently in use. SecretRef *SecretKeySelector `json:"secretRef,omitempty"` diff --git a/config/crd/bases/coder.com_coderprovisioners.yaml b/config/crd/bases/coder.com_coderprovisioners.yaml index 55a11064..7ca29e5b 100644 --- a/config/crd/bases/coder.com_coderprovisioners.yaml +++ b/config/crd/bases/coder.com_coderprovisioners.yaml @@ -424,7 +424,8 @@ spec: plane with controlPlaneRefName for this key only while that control plane has this UID, and rotates the key when it has another UID. When it is empty, it sends that token only to the current Coder API URL of that - control plane. + control plane, and the next reconcile looks the key up there: it sets + this field when Coder holds the key, and creates a new key otherwise. type: string controlPlaneURL: description: ControlPlaneURL is the control plane URL last applied diff --git a/dist/install.yaml b/dist/install.yaml index ace41043..00e6c2d8 100644 --- a/dist/install.yaml +++ b/dist/install.yaml @@ -6186,7 +6186,8 @@ spec: plane with controlPlaneRefName for this key only while that control plane has this UID, and rotates the key when it has another UID. When it is empty, it sends that token only to the current Coder API URL of that - control plane. + control plane, and the next reconcile looks the key up there: it sets + this field when Coder holds the key, and creates a new key otherwise. type: string controlPlaneURL: description: ControlPlaneURL is the control plane URL last applied diff --git a/docs/reference/api/coderprovisioner.md b/docs/reference/api/coderprovisioner.md index 9369a55f..8b9e1a6a 100644 --- a/docs/reference/api/coderprovisioner.md +++ b/docs/reference/api/coderprovisioner.md @@ -40,7 +40,7 @@ | `tagsHash` | string | TagsHash is a deterministic hash of spec.tags last applied to the provisioner key. | | `controlPlaneRefName` | string | ControlPlaneRefName is the control plane ref name last applied to the provisioner key. | | `controlPlaneURL` | string | ControlPlaneURL is the control plane URL last applied to the provisioner key. | -| `controlPlaneUID` | string | ControlPlaneUID is the UID of the control plane last applied to the provisioner key. The operator sends the operator token of the control plane with controlPlaneRefName for this key only while that control plane has this UID, and rotates the key when it has another UID. When it is empty, it sends that token only to the current Coder API URL of that control plane. | +| `controlPlaneUID` | string | ControlPlaneUID is the UID of the control plane last applied to the provisioner key. The operator sends the operator token of the control plane with controlPlaneRefName for this key only while that control plane has this UID, and rotates the key when it has another UID. When it is empty, it sends that token only to the current Coder API URL of that control plane, and the next reconcile looks the key up there: it sets this field when Coder holds the key, and creates a new key otherwise. | | `secretRef` | [SecretKeySelector](#secretkeyselector) | SecretRef references the provisioner key secret data currently in use. | | `pendingKeys` | [CoderProvisionerKeyRecord](#coderprovisionerkeyrecord) array | PendingKeys lists the provisioner keys that can exist in Coder besides the key in provisionerKeyID: a key that the operator is about to create or has not yet written to the provisioner key Secret, and replaced keys whose deletion Coder has not confirmed. The operator deletes the key of each entry with organizationID and keyID at its coderURL with the operator token of its control plane, and removes the entry when Coder confirms that the key is gone. | diff --git a/docs/tutorials/deploy-coderprovisioner.md b/docs/tutorials/deploy-coderprovisioner.md index b0b4b2db..c91fea93 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, each at its own Coder URL. 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. 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, each at its own Coder URL. 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. When Coder holds the key there, the operator records the UID of that control plane. Otherwise it creates a new key. 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 `. From 0149387c4791ab50415ee1754ea9871dd5765dcd Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 19:43:44 +0000 Subject: [PATCH 07/13] =?UTF-8?q?=F0=9F=A4=96=20fix:=20delete=20keys=20of?= =?UTF-8?q?=20earlier=20targets=20while=20the=20target=20key=20name=20is?= =?UTF-8?q?=20in=20use?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex security review of #420: the KeyNameInUse refusal returned before the pending cleanup, so whoever holds the target name kept every key recorded at an earlier target valid. The refusal now runs the same cleanup for the published records outside the target slot, each with the token of its own control plane. Unpublished records stay, because the Secret can hold their key, and the target is still not changed. Change-Id: I1d99dff9a50609f222012531ca33ae7d18e6f6ca Signed-off-by: Thomas Kosiewski --- docs/tutorials/deploy-coderprovisioner.md | 2 +- .../controller/coderprovisioner_controller.go | 90 ++++++++++++------- .../coderprovisioner_keyownership_test.go | 21 +++++ 3 files changed, 82 insertions(+), 31 deletions(-) diff --git a/docs/tutorials/deploy-coderprovisioner.md b/docs/tutorials/deploy-coderprovisioner.md index c91fea93..0562efc2 100644 --- a/docs/tutorials/deploy-coderprovisioner.md +++ b/docs/tutorials/deploy-coderprovisioner.md @@ -54,7 +54,7 @@ Expected result: the phase is `Ready`, and the conditions include `CredentialsRe The operator creates the provisioner key Secret and owns it. If you set `spec.key.secretName`, name a Secret that does not exist yet: the operator refuses a Secret that it does not own (reason `KeySecretNotOwned`). -Each provisioner needs its own key name in its Coder organization. The key name is `spec.key.name`, or the provisioner name when `spec.key.name` is empty. Before the operator creates or replaces a key, it looks up the key name in Coder. When Coder holds a key with that name, and the status of this provisioner does not record its ID, the operator does not delete or replace that key, because another `CoderProvisioner` (for example one with the same name in another namespace) or a person can use it. It then changes nothing for this provisioner: `ProvisionerKeyReady` is `False` with reason `KeyNameInUse`, and the message names the key and its ID. Set `spec.key.name` to a name that nothing else uses, or delete that key in Coder if nothing uses it. The operator checks again every 2 minutes. The same happens when the operator lost the answer to a key create, because status then has no ID of the new key, and for status without key records, for example after a restore without status. +Each provisioner needs its own key name in its Coder organization. The key name is `spec.key.name`, or the provisioner name when `spec.key.name` is empty. Before the operator creates or replaces a key, it looks up the key name in Coder. When Coder holds a key with that name, and the status of this provisioner does not record its ID, the operator does not delete or replace that key, because another `CoderProvisioner` (for example one with the same name in another namespace) or a person can use it. It keeps the current key, Secret and Deployment of this provisioner, and only deletes the replaced keys that `status.pendingKeys` records for earlier targets: `ProvisionerKeyReady` is `False` with reason `KeyNameInUse`, and the message names the key and its ID. Set `spec.key.name` to a name that nothing else uses, or delete that key in Coder if nothing uses it. The operator checks again every 2 minutes. The same happens when the operator lost the answer to a key create, because status then has no ID of the new key, and for status without key records, for example after a restore without status. If you add `spec.extraEnv` entries with a `secretKeyRef`, each Secret needs the label `coder.com/credential=true` and the annotation `coder.com/credential-destinations` with the origin of the control plane `status.url`. Otherwise `CredentialsReady` is `False` and the operator does not change the Deployment. See [Configure credential Secrets for proxies and provisioners](../how-to/workspace-proxy-credentials.md). diff --git a/internal/controller/coderprovisioner_controller.go b/internal/controller/coderprovisioner_controller.go index b4943888..1615b0fc 100644 --- a/internal/controller/coderprovisioner_controller.go +++ b/internal/controller/coderprovisioner_controller.go @@ -347,6 +347,15 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req replacing = true } 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) } } @@ -379,37 +388,13 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req } pending = append(next, target) } - var kept []coderv1alpha1.CoderProvisionerKeyRecord - var unverified, controlPlaneChanged []string - for _, record := range pending { - // The replacement below deletes the key that the target holds. - if sameKeySlot(record, target) || (existingKey != nil && isKeyID(record.KeyID, existingKey.ID)) { - kept = append(kept, record) - continue - } - cleanup, revokeErr := r.revokeKey(ctx, provisioner, record) - if revokeErr != nil { - return ctrl.Result{}, revokeErr - } - 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)) - continue - } - if _, _, ok := parseKeyIDs(record.OrganizationID, record.KeyID); !ok { - note := "the operator could not check Coder" - 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)) - } + // 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 } - conditionChanged := setUnverifiedKeyCondition(provisioner, unverified) - conditionChanged = setControlPlaneChangedKeyCondition(provisioner, controlPlaneChanged) || conditionChanged if conditionChanged || !slices.Equal(kept, provisioner.Status.PendingKeys) { provisioner.Status.PendingKeys = kept if err := r.Status().Update(ctx, provisioner); err != nil { @@ -966,6 +951,51 @@ func (r *CoderProvisionerReconciler) revokeKey( return keyUnverified, nil } +// 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. +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) { + var kept []coderv1alpha1.CoderProvisionerKeyRecord + var unverified, controlPlaneChanged []string + for _, record := range pending { + if sameKeySlot(record, target) || keep(record) { + kept = append(kept, record) + continue + } + cleanup, err := r.revokeKey(ctx, provisioner, record) + if err != nil { + return nil, false, 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)) + continue + } + if _, _, ok := parseKeyIDs(record.OrganizationID, record.KeyID); !ok { + note := "the operator could not check Coder" + 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)) + } + } + conditionChanged := setUnverifiedKeyCondition(provisioner, unverified) + conditionChanged = setControlPlaneChangedKeyCondition(provisioner, controlPlaneChanged) || conditionChanged + return kept, conditionChanged, 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_keyownership_test.go b/internal/controller/coderprovisioner_keyownership_test.go index 4b8bdfe3..c9cb894e 100644 --- a/internal/controller/coderprovisioner_keyownership_test.go +++ b/internal/controller/coderprovisioner_keyownership_test.go @@ -37,6 +37,9 @@ func TestCoderProvisionerKey_OtherProvisionersKeyKept(t *testing.T) { // dropKeyStatus removes the key fields from the status of P, as // status that lost its key records. dropKeyStatus bool + // staleRecord records a replaced key of P at another key name, whose + // delete Coder has not confirmed yet. + staleRecord bool // resolveName resolves the conflict with this key name for P. // Otherwise the key of Q goes. resolveName string @@ -44,6 +47,7 @@ func TestCoderProvisionerKey_OtherProvisionersKeyKept(t *testing.T) { {name: "first key of P"}, {name: "key name change of P", holdsKey: true, resolveName: "p-own"}, {name: "key name change of P, reverted", holdsKey: true, resolveName: "p-key"}, + {name: "key name change of P with a replaced key left", holdsKey: true, staleRecord: true, resolveName: "p-own"}, {name: "status without key records", holdsKey: true, dropKeyStatus: true, resolveName: "p-own"}, } @@ -91,6 +95,22 @@ func TestCoderProvisionerKey_OtherProvisionersKeyKept(t *testing.T) { f.reconcile(ctx, t, p) } + if tt.staleRecord { + stale := slotOf("p-stale") + store.keys[stale] = uuid.New() + latest := f.get(ctx, t, p) + latest.Status.PendingKeys = append(latest.Status.PendingKeys, coderv1alpha1.CoderProvisionerKeyRecord{ + ControlPlaneRefName: f.controlPlane.Name, + ControlPlaneUID: string(f.controlPlane.UID), + CoderURL: internalCoderURL(f), + OrganizationName: "default", + KeyName: "p-stale", + OrganizationID: stale.organizationID().String(), + KeyID: store.keys[stale].String(), + }) + require.NoError(t, k8sClient.Status().Update(ctx, latest)) + } + for range 2 { result, err := f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: pName}) require.NoError(t, err) @@ -106,6 +126,7 @@ func TestCoderProvisionerKey_OtherProvisionersKeyKept(t *testing.T) { require.Contains(t, keyReady.Message, qKeyID.String()) require.Nil(t, meta.FindStatusCondition(latest.Status.Conditions, coderv1alpha1.CoderProvisionerConditionUnverifiedKeyKept)) require.Empty(t, latest.Status.PendingKeys, "P must record no create") + require.NotContains(t, store.keys, slotOf("p-stale"), "a key recorded at an earlier target must not wait for the conflict") if tt.holdsKey { require.Equal(t, pKeyID, store.keys[slotOf("p-key")], "the key of P must stay while its new key name is taken") secret := &corev1.Secret{} From c86c1f1c844c15b5afac27b13dac83f75bcb2ecf Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 20:25:33 +0000 Subject: [PATCH 08/13] =?UTF-8?q?=F0=9F=A4=96=20test:=20deleting=20a=20pro?= =?UTF-8?q?visioner=20deletes=20its=20key=20after=20an=20organization=20re?= =?UTF-8?q?name=20of=20status=20without=20a=20control=20plane=20UID?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Change-Id: I58615bff7826db6ca2c6b122bb1b4d3e2c7d8052 Signed-off-by: Thomas Kosiewski --- .../coderprovisioner_keyownership_test.go | 115 ++++++++++++++++++ 1 file changed, 115 insertions(+) diff --git a/internal/controller/coderprovisioner_keyownership_test.go b/internal/controller/coderprovisioner_keyownership_test.go index c9cb894e..ff19be4b 100644 --- a/internal/controller/coderprovisioner_keyownership_test.go +++ b/internal/controller/coderprovisioner_keyownership_test.go @@ -3,7 +3,13 @@ package controller_test import ( "cmp" "context" + "encoding/json" + "maps" + "net/http" + "net/http/httptest" "slices" + "strings" + "sync" "testing" "github.com/coder/coder/v2/codersdk" @@ -354,3 +360,112 @@ func TestCoderProvisionerKey_ControlPlaneRecreated(t *testing.T) { }) } } + +// renamedOrgCoder is a Coder at an httptest server, reached through the real +// SDK client, with one organization named team. Key creates answer from the +// fake and add the key to the server. +type renamedOrgCoder struct { + *coderbootstrap.SDKClient + fake *fakeBootstrapClient + mu sync.Mutex + keys map[uuid.UUID]codersdk.ProvisionerKey + // team is the ID of the organization named team. + team uuid.UUID +} + +func (c *renamedOrgCoder) EnsureProvisionerKey(ctx context.Context, req coderbootstrap.EnsureProvisionerKeyRequest) (coderbootstrap.EnsureProvisionerKeyResponse, error) { + response, err := c.fake.EnsureProvisionerKey(ctx, req) + if err == nil { + c.mu.Lock() + c.keys[response.KeyID] = codersdk.ProvisionerKey{ID: response.KeyID, OrganizationID: response.OrganizationID, Name: response.KeyName} + c.mu.Unlock() + } + return response, err +} + +func (c *renamedOrgCoder) Entitlements(ctx context.Context, coderURL, sessionToken string) (codersdk.Entitlements, error) { + return c.fake.Entitlements(ctx, coderURL, sessionToken) +} + +func (c *renamedOrgCoder) serveHTTP(t *testing.T, w http.ResponseWriter, r *http.Request) { + c.mu.Lock() + defer c.mu.Unlock() + w.Header().Set("Content-Type", "application/json") + if r.Method == http.MethodGet && r.URL.Path == "/api/v2/organizations/team" { + _ = json.NewEncoder(w).Encode(codersdk.Organization{MinimalOrganization: codersdk.MinimalOrganization{ID: c.team, Name: "team"}}) + return + } + parts := strings.Split(strings.Trim(r.URL.Path, "/"), "/") + 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 { + keys := []codersdk.ProvisionerKey{} + for _, key := range c.keys { + if key.OrganizationID == orgID { + keys = append(keys, key) + } + } + _ = json.NewEncoder(w).Encode(keys) + return + } + if r.Method == http.MethodDelete && len(parts) == 6 { + maps.DeleteFunc(c.keys, func(_ uuid.UUID, key codersdk.ProvisionerKey) bool { + return key.OrganizationID == orgID && key.Name == parts[5] + }) + w.WriteHeader(http.StatusNoContent) + return + } + } + } + t.Errorf("unexpected request %s %s", r.Method, r.URL.Path) + w.WriteHeader(http.StatusNotFound) +} + +// TestCoderProvisionerKey_StatusWithoutUIDOrganizationRenamed checks status +// without a control plane UID whose applied key is in organization A, named +// team. A is renamed, and another organization takes the name team, at the +// same control plane. Coder still holds the key in A, so the deletion of the +// provisioner must delete it there by its recorded IDs. +func TestCoderProvisionerKey_StatusWithoutUIDOrganizationRenamed(t *testing.T) { + t.Parallel() + ctx := context.Background() + f := newProvisionerCredentialFixture(ctx, t, "cprov-orgrename") + orgA, orgB := uuid.New(), uuid.New() + keyA, keyB := 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: orgB, KeyID: keyB, KeyName: "provisioner", Key: "material-b"}, + } + 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) + require.NoError(t, k8sClient.Status().Patch(ctx, f.get(ctx, t, p), + ctrlclient.RawPatch(types.MergePatchType, []byte(`{"status":{"controlPlaneUID":null}}`)))) + + coder.mu.Lock() + coder.team = orgB + coder.mu.Unlock() + f.reconcile(ctx, t, p) + require.NoError(t, k8sClient.Delete(ctx, f.get(ctx, t, p))) + f.reconcile(ctx, t, p) + requireProvisionerGone(ctx, t, types.NamespacedName{Name: p.Name, Namespace: p.Namespace}) + + coder.mu.Lock() + defer coder.mu.Unlock() + require.NotContains(t, coder.keys, keyA, "deleting the provisioner must revoke its recorded key in the renamed organization by ID") + require.Empty(t, coder.keys, "deleting the provisioner must leave none of its keys in Coder") +} From 1126867b4c59d45f68ef514c369300a8a2caef72 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 20:32:14 +0000 Subject: [PATCH 09/13] =?UTF-8?q?=F0=9F=A4=96=20fix:=20look=20up=20the=20a?= =?UTF-8?q?pplied=20key=20of=20status=20without=20a=20control=20plane=20UI?= =?UTF-8?q?D=20by=20its=20recorded=20IDs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An organization can be renamed and its name reused. The lookup by organization name then missed the applied key, the provisioner replaced it without a record, and deletion left the key valid in Coder. The lookup now lists the recorded organization ID and matches the recorded key ID. The key is replaced only when Coder answers that it has no such key there. Change-Id: I05a8ac8173d4d36c159aac4aef9bd99f4b3c499b Signed-off-by: Thomas Kosiewski --- api/v1alpha1/coderprovisioner_types.go | 5 +- .../bases/coder.com_coderprovisioners.yaml | 5 +- dist/install.yaml | 5 +- docs/reference/api/coderprovisioner.md | 2 +- docs/tutorials/deploy-coderprovisioner.md | 2 +- internal/coderbootstrap/client.go | 1 + internal/coderbootstrap/provisionerkeys.go | 94 +++++++++++++------ .../provisionerkeys_deletebyid_test.go | 67 +++++++++++++ .../controller/coderprovisioner_controller.go | 38 ++++++-- .../coderprovisioner_keyownership_test.go | 15 ++- .../coderworkspaceproxy_controller_test.go | 10 ++ 11 files changed, 194 insertions(+), 50 deletions(-) diff --git a/api/v1alpha1/coderprovisioner_types.go b/api/v1alpha1/coderprovisioner_types.go index 9e9515cc..4357f03f 100644 --- a/api/v1alpha1/coderprovisioner_types.go +++ b/api/v1alpha1/coderprovisioner_types.go @@ -122,8 +122,9 @@ type CoderProvisionerStatus struct { // plane with controlPlaneRefName for this key only while that control // plane has this UID, and rotates the key when it has another UID. When it // is empty, it sends that token only to the current Coder API URL of that - // control plane, and the next reconcile looks the key up there: it sets - // this field when Coder holds the key, and creates a new key otherwise. + // control plane, and the next reconcile looks the key up there by + // organizationID and provisionerKeyID: it sets this field when Coder holds + // the key, and creates a new key when Coder answers that it has none. ControlPlaneUID string `json:"controlPlaneUID,omitempty"` // SecretRef references the provisioner key secret data currently in use. SecretRef *SecretKeySelector `json:"secretRef,omitempty"` diff --git a/config/crd/bases/coder.com_coderprovisioners.yaml b/config/crd/bases/coder.com_coderprovisioners.yaml index 7ca29e5b..14d03f26 100644 --- a/config/crd/bases/coder.com_coderprovisioners.yaml +++ b/config/crd/bases/coder.com_coderprovisioners.yaml @@ -424,8 +424,9 @@ spec: plane with controlPlaneRefName for this key only while that control plane has this UID, and rotates the key when it has another UID. When it is empty, it sends that token only to the current Coder API URL of that - control plane, and the next reconcile looks the key up there: it sets - this field when Coder holds the key, and creates a new key otherwise. + control plane, and the next reconcile looks the key up there by + organizationID and provisionerKeyID: it sets this field when Coder holds + the key, and creates a new key when Coder answers that it has none. type: string controlPlaneURL: description: ControlPlaneURL is the control plane URL last applied diff --git a/dist/install.yaml b/dist/install.yaml index 9f1d1ada..f451ccda 100644 --- a/dist/install.yaml +++ b/dist/install.yaml @@ -6192,8 +6192,9 @@ spec: plane with controlPlaneRefName for this key only while that control plane has this UID, and rotates the key when it has another UID. When it is empty, it sends that token only to the current Coder API URL of that - control plane, and the next reconcile looks the key up there: it sets - this field when Coder holds the key, and creates a new key otherwise. + control plane, and the next reconcile looks the key up there by + organizationID and provisionerKeyID: it sets this field when Coder holds + the key, and creates a new key when Coder answers that it has none. type: string controlPlaneURL: description: ControlPlaneURL is the control plane URL last applied diff --git a/docs/reference/api/coderprovisioner.md b/docs/reference/api/coderprovisioner.md index 8b9e1a6a..8b4496f8 100644 --- a/docs/reference/api/coderprovisioner.md +++ b/docs/reference/api/coderprovisioner.md @@ -40,7 +40,7 @@ | `tagsHash` | string | TagsHash is a deterministic hash of spec.tags last applied to the provisioner key. | | `controlPlaneRefName` | string | ControlPlaneRefName is the control plane ref name last applied to the provisioner key. | | `controlPlaneURL` | string | ControlPlaneURL is the control plane URL last applied to the provisioner key. | -| `controlPlaneUID` | string | ControlPlaneUID is the UID of the control plane last applied to the provisioner key. The operator sends the operator token of the control plane with controlPlaneRefName for this key only while that control plane has this UID, and rotates the key when it has another UID. When it is empty, it sends that token only to the current Coder API URL of that control plane, and the next reconcile looks the key up there: it sets this field when Coder holds the key, and creates a new key otherwise. | +| `controlPlaneUID` | string | ControlPlaneUID is the UID of the control plane last applied to the provisioner key. The operator sends the operator token of the control plane with controlPlaneRefName for this key only while that control plane has this UID, and rotates the key when it has another UID. When it is empty, it sends that token only to the current Coder API URL of that control plane, and the next reconcile looks the key up there by organizationID and provisionerKeyID: it sets this field when Coder holds the key, and creates a new key when Coder answers that it has none. | | `secretRef` | [SecretKeySelector](#secretkeyselector) | SecretRef references the provisioner key secret data currently in use. | | `pendingKeys` | [CoderProvisionerKeyRecord](#coderprovisionerkeyrecord) array | PendingKeys lists the provisioner keys that can exist in Coder besides the key in provisionerKeyID: a key that the operator is about to create or has not yet written to the provisioner key Secret, and replaced keys whose deletion Coder has not confirmed. The operator deletes the key of each entry with organizationID and keyID at its coderURL with the operator token of its control plane, and removes the entry when Coder confirms that the key is gone. | diff --git a/docs/tutorials/deploy-coderprovisioner.md b/docs/tutorials/deploy-coderprovisioner.md index 0562efc2..fc397d0a 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, each at its own Coder URL. 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. When Coder holds the key there, the operator records the UID of that control plane. Otherwise it creates a new key. 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, each at its own Coder URL. 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. 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/coderbootstrap/client.go b/internal/coderbootstrap/client.go index ea3dbe0f..70b5445f 100644 --- a/internal/coderbootstrap/client.go +++ b/internal/coderbootstrap/client.go @@ -95,6 +95,7 @@ type Client interface { EnsureWorkspaceProxy(context.Context, RegisterWorkspaceProxyRequest) (RegisterWorkspaceProxyResponse, error) EnsureProvisionerKey(context.Context, EnsureProvisionerKeyRequest) (EnsureProvisionerKeyResponse, error) DeleteProvisionerKeyByID(context.Context, DeleteProvisionerKeyByIDRequest) error + ProvisionerKeyByID(context.Context, ProvisionerKeyByIDRequest) (*codersdk.ProvisionerKey, error) ProvisionerKeyByName(context.Context, ProvisionerKeyByNameRequest) (*codersdk.ProvisionerKey, error) Entitlements(ctx context.Context, coderURL, sessionToken string) (codersdk.Entitlements, error) } diff --git a/internal/coderbootstrap/provisionerkeys.go b/internal/coderbootstrap/provisionerkeys.go index 3033c996..2b9f3e39 100644 --- a/internal/coderbootstrap/provisionerkeys.go +++ b/internal/coderbootstrap/provisionerkeys.go @@ -148,6 +148,27 @@ func (c *SDKClient) ProvisionerKeyByName(ctx context.Context, req ProvisionerKey return key, nil } +// ProvisionerKeyByIDRequest identifies one provisioner key by the IDs that +// the caller recorded when it applied the key. +type ProvisionerKeyByIDRequest struct { + CoderURL string + SessionToken string + OrganizationID uuid.UUID + KeyID uuid.UUID +} + +// ProvisionerKeyByID returns the provisioner key with req.KeyID in the +// organization req.OrganizationID. It returns nil when Coder answers that the +// organization does not exist, or lists no key with that ID in it. +func (c *SDKClient) ProvisionerKeyByID(ctx context.Context, req ProvisionerKeyByIDRequest) (*codersdk.ProvisionerKey, error) { + client, err := newProvisionerKeyByIDClient(req) + if err != nil { + return nil, err + } + + return findProvisionerKeyByID(ctx, client, req.OrganizationID, req.KeyID) +} + // DeleteProvisionerKeyByIDRequest identifies one provisioner key by the IDs // that the caller recorded when it applied the key. type DeleteProvisionerKeyByIDRequest struct { @@ -164,56 +185,71 @@ type DeleteProvisionerKeyByIDRequest struct { // and the delete are two requests: a key deleted and created again with the // same name between them is deleted too. func (c *SDKClient) DeleteProvisionerKeyByID(ctx context.Context, req DeleteProvisionerKeyByIDRequest) error { + client, err := newProvisionerKeyByIDClient(ProvisionerKeyByIDRequest(req)) + if err != nil { + return err + } + + match, err := findProvisionerKeyByID(ctx, client, req.OrganizationID, req.KeyID) + if err != nil { + return err + } + if match == nil { + return nil + } + + _, err = withOptionalRateLimitBypass(ctx, func(requestCtx context.Context) (struct{}, error) { + return struct{}{}, client.DeleteProvisionerKey(requestCtx, req.OrganizationID, match.Name) + }) + if err != nil && !isNotFound(err) { + return xerrors.Errorf("delete provisioner key %q: %w", match.Name, err) + } + + return nil +} + +func newProvisionerKeyByIDClient(req ProvisionerKeyByIDRequest) (*codersdk.Client, error) { if req.CoderURL == "" { - return xerrors.New("coder URL is required") + return nil, xerrors.New("coder URL is required") } if req.SessionToken == "" { - return xerrors.New("session token is required") + return nil, xerrors.New("session token is required") } if req.OrganizationID == uuid.Nil { - return xerrors.New("organization ID is required") + return nil, xerrors.New("organization ID is required") } if req.KeyID == uuid.Nil { - return xerrors.New("provisioner key ID is required") + return nil, xerrors.New("provisioner key ID is required") } - client, err := newAuthenticatedClient(req.CoderURL, req.SessionToken) - if err != nil { - return err - } + return newAuthenticatedClient(req.CoderURL, req.SessionToken) +} +// findProvisionerKeyByID returns the key with keyID in the organization +// organizationID, or nil when Coder answers that the organization does not +// exist or lists no key with that ID. +func findProvisionerKeyByID(ctx context.Context, client *codersdk.Client, organizationID, keyID uuid.UUID) (*codersdk.ProvisionerKey, error) { keys, err := withOptionalRateLimitBypass(ctx, func(requestCtx context.Context) ([]codersdk.ProvisionerKey, error) { - return client.ListProvisionerKeys(requestCtx, req.OrganizationID) + return client.ListProvisionerKeys(requestCtx, organizationID) }) if isNotFound(err) { - return nil + return nil, nil } if err != nil { - return xerrors.Errorf("list provisioner keys for organization %q: %w", req.OrganizationID, err) + return nil, xerrors.Errorf("list provisioner keys for organization %q: %w", organizationID, err) } - var match *codersdk.ProvisionerKey for i := range keys { - if keys[i].ID == req.KeyID { - match = &keys[i] - break + if keys[i].ID != keyID { + continue } - } - if match == nil { - return nil - } - if match.Name == "" { - return xerrors.Errorf("assertion failed: provisioner key %q has an empty name", req.KeyID) - } - - _, err = withOptionalRateLimitBypass(ctx, func(requestCtx context.Context) (struct{}, error) { - return struct{}{}, client.DeleteProvisionerKey(requestCtx, req.OrganizationID, match.Name) - }) - if err != nil && !isNotFound(err) { - return xerrors.Errorf("delete provisioner key %q: %w", match.Name, err) + if keys[i].Name == "" { + return nil, xerrors.Errorf("assertion failed: provisioner key %q has an empty name", keyID) + } + return &keys[i], nil } - return nil + return nil, nil } // coderNotFoundMessage is the message of the 404 that Coder answers for a diff --git a/internal/coderbootstrap/provisionerkeys_deletebyid_test.go b/internal/coderbootstrap/provisionerkeys_deletebyid_test.go index 48f72653..87e97a5c 100644 --- a/internal/coderbootstrap/provisionerkeys_deletebyid_test.go +++ b/internal/coderbootstrap/provisionerkeys_deletebyid_test.go @@ -90,6 +90,73 @@ func TestDeleteProvisionerKeyByID(t *testing.T) { } } +func TestProvisionerKeyByID(t *testing.T) { + t.Parallel() + + orgID := uuid.New() + recordedKeyID := uuid.New() + now := time.Now().UTC().Format(time.RFC3339) + keysPath := "/api/v2/organizations/" + orgID.String() + "/provisionerkeys" + + tests := []struct { + name string + listStatus int + listBody map[string]any + listedID uuid.UUID + wantKey bool + wantErr bool + }{ + {name: "recorded key exists", listStatus: http.StatusOK, listedID: recordedKeyID, wantKey: true}, + {name: "same name, other ID", listStatus: http.StatusOK, listedID: uuid.New()}, + {name: "organization gone", listStatus: http.StatusNotFound, listBody: coderNotFound}, + {name: "404 not from Coder", listStatus: http.StatusNotFound, listBody: map[string]any{"message": "Route not found."}, wantErr: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet || r.URL.Path != keysPath { + t.Errorf("unexpected request %s %s", r.Method, r.URL.Path) + w.WriteHeader(http.StatusNotFound) + return + } + if tt.listStatus != http.StatusOK { + writeJSONResponse(t, w, tt.listStatus, tt.listBody) + return + } + writeJSONResponse(t, w, http.StatusOK, []map[string]any{{ + "id": tt.listedID.String(), + "created_at": now, + "organization": orgID.String(), + "name": "shared-name", + "tags": map[string]string{}, + }}) + })) + defer server.Close() + + key, err := coderbootstrap.NewSDKClient().ProvisionerKeyByID(context.Background(), coderbootstrap.ProvisionerKeyByIDRequest{ + CoderURL: server.URL, + SessionToken: "session-token", + OrganizationID: orgID, + KeyID: recordedKeyID, + }) + if tt.wantErr { + require.Error(t, err, "only Coder's own 404 proves that the key is gone") + return + } + require.NoError(t, err) + if !tt.wantKey { + require.Nil(t, key) + return + } + require.NotNil(t, key) + require.Equal(t, recordedKeyID, key.ID) + }) + } +} + func TestDeleteProvisionerKeyByID_ValidationErrors(t *testing.T) { t.Parallel() diff --git a/internal/controller/coderprovisioner_controller.go b/internal/controller/coderprovisioner_controller.go index 1615b0fc..752c427c 100644 --- a/internal/controller/coderprovisioner_controller.go +++ b/internal/controller/coderprovisioner_controller.go @@ -318,15 +318,39 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req replacing := driftDetected || rotate || metadataMissing // Status from an operator that did not record the control plane UID does // not show that the applied key belongs to the Coder backend of the - // current control plane. The applied key is kept, and bound to that - // control plane, only when the lookup below finds it there. - verifyApplied := !replacing && status.ControlPlaneUID == "" + // current control plane, which has the recorded name and URL here. The + // applied key is kept, and bound to that control plane, only when Coder + // there lists it under its recorded IDs. An organization can be renamed + // and its name reused, so the lookup does not use the name. + if !replacing && status.ControlPlaneUID == "" { + var applied *codersdk.ProvisionerKey + if appliedOrgID, appliedKeyID, ok := parseKeyIDs(status.OrganizationID, status.ProvisionerKeyID); ok { + applied, err = r.BootstrapClient.ProvisionerKeyByID(ctx, coderbootstrap.ProvisionerKeyByIDRequest{ + CoderURL: coderURL, + SessionToken: sessionToken, + OrganizationID: appliedOrgID, + KeyID: appliedKeyID, + }) + if err != nil { + setCondition(provisioner, coderv1alpha1.CoderProvisionerConditionProvisionerKeyReady, + metav1.ConditionFalse, "ProvisionerKeyFailed", + fmt.Sprintf("Failed to look up provisioner key %q", provisionerKeyName)) + _ = r.Status().Update(ctx, provisioner) + return ctrl.Result{}, fmt.Errorf("look up the applied provisioner key %q: %w", provisionerKeyName, err) + } + } + if applied != nil { + appliedControlPlaneUID = string(controlPlane.UID) + } else { + replacing = true + } + } // Coder allows one key per name in an organization. A key with the target // name that status does not record can belong to another provisioner or // a person, so the operator neither deletes nor replaces it, and changes // nothing else until the name is free. var existingKey *codersdk.ProvisionerKey - if replacing || verifyApplied { + if replacing { existingKey, err = r.BootstrapClient.ProvisionerKeyByName(ctx, coderbootstrap.ProvisionerKeyByNameRequest{ CoderURL: coderURL, SessionToken: sessionToken, @@ -340,12 +364,6 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req _ = r.Status().Update(ctx, provisioner) return ctrl.Result{}, fmt.Errorf("look up provisioner key %q: %w", keyName, err) } - if verifyApplied && existingKey != nil && isKeyID(status.ProvisionerKeyID, existingKey.ID) { - appliedControlPlaneUID = string(controlPlane.UID) - existingKey = nil - } else { - replacing = true - } 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. diff --git a/internal/controller/coderprovisioner_keyownership_test.go b/internal/controller/coderprovisioner_keyownership_test.go index ff19be4b..4cbcfab6 100644 --- a/internal/controller/coderprovisioner_keyownership_test.go +++ b/internal/controller/coderprovisioner_keyownership_test.go @@ -256,13 +256,22 @@ func TestCoderProvisionerKey_ControlPlaneRecreated(t *testing.T) { var lookups []urlToken // oldKeyInCoder: the backend at oldURL holds the old key. oldKeyInCoder := false - f.bootstrapClient.provisionerKeyByName = func(req coderbootstrap.ProvisionerKeyByNameRequest) *codersdk.ProvisionerKey { - lookups = append(lookups, urlToken{req.CoderURL, req.SessionToken}) - if !oldKeyInCoder || req.CoderURL != oldURL || len(f.bootstrapClient.deleteKeyByIDRequests) > 0 { + oldKeyAt := func(coderURL, token string) *codersdk.ProvisionerKey { + lookups = append(lookups, urlToken{coderURL, token}) + if !oldKeyInCoder || coderURL != oldURL || len(f.bootstrapClient.deleteKeyByIDRequests) > 0 { return nil } return &codersdk.ProvisionerKey{ID: keyOld.KeyID, OrganizationID: keyOld.OrganizationID, Name: keyOld.KeyName} } + f.bootstrapClient.provisionerKeyByName = func(req coderbootstrap.ProvisionerKeyByNameRequest) *codersdk.ProvisionerKey { + return oldKeyAt(req.CoderURL, req.SessionToken) + } + f.bootstrapClient.provisionerKeyByID = func(req coderbootstrap.ProvisionerKeyByIDRequest) *codersdk.ProvisionerKey { + if req.OrganizationID != keyOld.OrganizationID || req.KeyID != keyOld.KeyID { + return nil + } + return oldKeyAt(req.CoderURL, req.SessionToken) + } p := f.createProvisioner(ctx, t, nil, nil) pName := types.NamespacedName{Name: p.Name, Namespace: p.Namespace} diff --git a/internal/controller/coderworkspaceproxy_controller_test.go b/internal/controller/coderworkspaceproxy_controller_test.go index f78a62be..f7fb80d0 100644 --- a/internal/controller/coderworkspaceproxy_controller_test.go +++ b/internal/controller/coderworkspaceproxy_controller_test.go @@ -34,6 +34,9 @@ type fakeBootstrapClient struct { // provisionerKeyByName answers ProvisionerKeyByName. When it is nil, // Coder has no key with the name. provisionerKeyByName func(coderbootstrap.ProvisionerKeyByNameRequest) *codersdk.ProvisionerKey + // provisionerKeyByID answers ProvisionerKeyByID. When it is nil, Coder + // has no key with the ID. + provisionerKeyByID func(coderbootstrap.ProvisionerKeyByIDRequest) *codersdk.ProvisionerKey // Entitlements support. entitlementsResponse codersdk.Entitlements @@ -82,6 +85,13 @@ func (f *fakeBootstrapClient) ProvisionerKeyByName(_ context.Context, req coderb return f.provisionerKeyByName(req), nil } +func (f *fakeBootstrapClient) ProvisionerKeyByID(_ context.Context, req coderbootstrap.ProvisionerKeyByIDRequest) (*codersdk.ProvisionerKey, error) { + if f.provisionerKeyByID == nil { + return nil, nil + } + return f.provisionerKeyByID(req), nil +} + func (f *fakeBootstrapClient) Entitlements(_ context.Context, coderURL, sessionToken string) (codersdk.Entitlements, error) { f.entitlementsCalls++ f.entitlementsRequests = append(f.entitlementsRequests, entitlementsRequest{ From c410f57297e0ee80afc601379d8bcba6168ff3b0 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 20:32:31 +0000 Subject: [PATCH 10/13] =?UTF-8?q?=F0=9F=A4=96=20test:=20a=20replaced=20key?= =?UTF-8?q?=20of=20status=20without=20a=20control=20plane=20UID=20gets=20n?= =?UTF-8?q?o=20token=20of=20a=20recreated=20control=20plane?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Change-Id: I18cf5c99489f5e11e7fba9683f38176bfacf93e8 Signed-off-by: Thomas Kosiewski --- .../coderprovisioner_keyownership_test.go | 57 +++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/internal/controller/coderprovisioner_keyownership_test.go b/internal/controller/coderprovisioner_keyownership_test.go index 4cbcfab6..fc8bd305 100644 --- a/internal/controller/coderprovisioner_keyownership_test.go +++ b/internal/controller/coderprovisioner_keyownership_test.go @@ -4,6 +4,7 @@ import ( "cmp" "context" "encoding/json" + "errors" "maps" "net/http" "net/http/httptest" @@ -478,3 +479,59 @@ func TestCoderProvisionerKey_StatusWithoutUIDOrganizationRenamed(t *testing.T) { require.NotContains(t, coder.keys, keyA, "deleting the provisioner must revoke its recorded key in the renamed organization by ID") 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 +// coder is then deleted and created again at the same URL with another +// operator token. The new token must not be used for the replaced key: its +// entry stays, and P reports it. +func TestCoderProvisionerKey_StatusWithoutUIDReplacedKeyBound(t *testing.T) { + t.Parallel() + + const ( + oldURL = "http://coder-old.example.com" + newToken = "operator-token-new" + ) + ctx := context.Background() + f := newProvisionerCredentialFixture(ctx, t, "cprov-bound") + setControlPlaneURL(ctx, t, f, oldURL) + keyOld := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: uuid.New(), KeyID: uuid.New(), KeyName: "provisioner", Key: "material-old"} + keyNew := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: uuid.New(), KeyID: uuid.New(), KeyName: "provisioner", Key: "material-new"} + f.bootstrapClient.provisionerKeyResponses = []coderbootstrap.EnsureProvisionerKeyResponse{keyOld, keyNew} + p := f.createProvisioner(ctx, t, nil, nil) + pName := types.NamespacedName{Name: p.Name, Namespace: p.Namespace} + f.reconcile(ctx, t, p) + f.reconcile(ctx, t, p) + require.Equal(t, keyOld.KeyID.String(), f.get(ctx, t, p).Status.ProvisionerKeyID) + require.NoError(t, k8sClient.Status().Patch(ctx, f.get(ctx, t, p), + ctrlclient.RawPatch(types.MergePatchType, []byte(`{"status":{"controlPlaneUID":null}}`)))) + + createControlPlaneWithToken(ctx, t, f.namespace, "coder-b", "http://coder-b.example.com", "operator-token-b") + latest := f.get(ctx, t, p) + latest.Spec.ControlPlaneRef.Name = "coder-b" + require.NoError(t, k8sClient.Update(ctx, latest)) + f.bootstrapClient.deleteKeyByIDErr = errors.New("injected delete failure") + _, err := f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: pName}) + require.NoError(t, err) + require.Equal(t, keyNew.KeyID.String(), f.get(ctx, t, p).Status.ProvisionerKeyID, "P must move its key to coder-b") + + require.NoError(t, k8sClient.Delete(ctx, &coderv1alpha1.CoderControlPlane{ObjectMeta: metav1.ObjectMeta{Name: "coder", Namespace: f.namespace}})) + require.NoError(t, k8sClient.Delete(ctx, &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "coder-operator-token", Namespace: f.namespace}})) + createControlPlaneWithToken(ctx, t, f.namespace, "coder", oldURL, newToken) + f.bootstrapClient.deleteKeyByIDErr = nil + _, err = f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: pName}) + require.NoError(t, err) + + for _, d := range keyDeletes(f.bootstrapClient) { + require.False(t, d.keyID == keyOld.KeyID.String() && d.token == newToken, + "the new control plane's token must not be used for the old key, sent %+v", keyDeletes(f.bootstrapClient)) + } + latest = f.get(ctx, t, p) + require.True(t, slices.ContainsFunc(latest.Status.PendingKeys, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { + return record.KeyID == keyOld.KeyID.String() + }), "the record of the replaced key must stay") + report := meta.FindStatusCondition(latest.Status.Conditions, coderv1alpha1.CoderProvisionerConditionControlPlaneChangedKeyKept) + require.NotNil(t, report, "P must report the record that it keeps") + require.Equal(t, metav1.ConditionTrue, report.Status) +} From 7087cb85e0e06ac7b7e532d3122c42fbf2fdb9aa Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 20:33:44 +0000 Subject: [PATCH 11/13] =?UTF-8?q?=F0=9F=A4=96=20fix:=20bind=20the=20replac?= =?UTF-8?q?ed=20key=20record=20of=20status=20without=20a=20control=20plane?= =?UTF-8?q?=20UID=20to=20its=20control=20plane?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a spec or control plane change replaces the key of status without a control plane UID, the entry of the replaced key now records the UID of the control plane that it names, while that control plane has the recorded URL. A control plane created again under that name before Coder confirms the delete then gets no operator token for the entry, which stays and is reported with ControlPlaneChangedKeyKept. Change-Id: Icd5366d5e86857b5e559fa539a33170ecfce3fdb Signed-off-by: Thomas Kosiewski --- api/v1alpha1/coderprovisioner_types.go | 4 ++- .../bases/coder.com_coderprovisioners.yaml | 4 ++- dist/install.yaml | 4 ++- docs/reference/api/coderprovisioner.md | 2 +- docs/tutorials/deploy-coderprovisioner.md | 2 +- .../controller/coderprovisioner_controller.go | 26 +++++++++++++++++++ 6 files changed, 37 insertions(+), 5 deletions(-) diff --git a/api/v1alpha1/coderprovisioner_types.go b/api/v1alpha1/coderprovisioner_types.go index 4357f03f..952878d4 100644 --- a/api/v1alpha1/coderprovisioner_types.go +++ b/api/v1alpha1/coderprovisioner_types.go @@ -150,7 +150,9 @@ type CoderProvisionerKeyRecord struct { // with controlPlaneRefName has this UID. When it is empty, it sends that // token only when coderURL is the current Coder API URL of that control // plane. Otherwise it keeps the entry and sets the condition - // ControlPlaneChangedKeyKept. + // ControlPlaneChangedKeyKept. When a rotation replaces the key of status + // without status.controlPlaneUID, the entry of that key records the UID + // while coderURL is the current Coder API URL of that control plane. ControlPlaneUID string `json:"controlPlaneUID,omitempty"` // CoderURL is the Coder API URL that the operator sent the key request to. CoderURL string `json:"coderURL"` diff --git a/config/crd/bases/coder.com_coderprovisioners.yaml b/config/crd/bases/coder.com_coderprovisioners.yaml index 14d03f26..77bf93d8 100644 --- a/config/crd/bases/coder.com_coderprovisioners.yaml +++ b/config/crd/bases/coder.com_coderprovisioners.yaml @@ -475,7 +475,9 @@ spec: with controlPlaneRefName has this UID. When it is empty, it sends that token only when coderURL is the current Coder API URL of that control plane. Otherwise it keeps the entry and sets the condition - ControlPlaneChangedKeyKept. + ControlPlaneChangedKeyKept. When a rotation replaces the key of status + without status.controlPlaneUID, the entry of that key records the UID + while coderURL is the current Coder API URL of that control plane. type: string keyID: description: KeyID is the provisioner key ID that Coder returned. diff --git a/dist/install.yaml b/dist/install.yaml index f451ccda..9a0c3df5 100644 --- a/dist/install.yaml +++ b/dist/install.yaml @@ -6243,7 +6243,9 @@ spec: with controlPlaneRefName has this UID. When it is empty, it sends that token only when coderURL is the current Coder API URL of that control plane. Otherwise it keeps the entry and sets the condition - ControlPlaneChangedKeyKept. + ControlPlaneChangedKeyKept. When a rotation replaces the key of status + without status.controlPlaneUID, the entry of that key records the UID + while coderURL is the current Coder API URL of that control plane. type: string keyID: description: KeyID is the provisioner key ID that Coder returned. diff --git a/docs/reference/api/coderprovisioner.md b/docs/reference/api/coderprovisioner.md index 8b4496f8..655ae2b9 100644 --- a/docs/reference/api/coderprovisioner.md +++ b/docs/reference/api/coderprovisioner.md @@ -54,7 +54,7 @@ or was about to create, at one Coder endpoint. | Field | Type | Description | | --- | --- | --- | | `controlPlaneRefName` | string | ControlPlaneRefName is the CoderControlPlane whose operator token the operator used for the key. | -| `controlPlaneUID` | string | ControlPlaneUID is the UID of that CoderControlPlane. The operator sends its operator token for this entry only while the control plane with controlPlaneRefName has this UID. When it is empty, it sends that token only when coderURL is the current Coder API URL of that control plane. Otherwise it keeps the entry and sets the condition ControlPlaneChangedKeyKept. | +| `controlPlaneUID` | string | ControlPlaneUID is the UID of that CoderControlPlane. The operator sends its operator token for this entry only while the control plane with controlPlaneRefName has this UID. When it is empty, it sends that token only when coderURL is the current Coder API URL of that control plane. Otherwise it keeps the entry and sets the condition ControlPlaneChangedKeyKept. When a rotation replaces the key of status without status.controlPlaneUID, the entry of that key records the UID while coderURL is the current Coder API URL of that control plane. | | `coderURL` | string | CoderURL is the Coder API URL that the operator sent the key request to. | | `organizationName` | string | OrganizationName is the Coder organization of the key. | | `keyName` | string | KeyName is the provisioner key name in Coder. | diff --git a/docs/tutorials/deploy-coderprovisioner.md b/docs/tutorials/deploy-coderprovisioner.md index fc397d0a..5c413bc1 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, each at its own Coder URL. 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. 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, each at its own Coder URL. 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"}]'`. 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 752c427c..97193e51 100644 --- a/internal/controller/coderprovisioner_controller.go +++ b/internal/controller/coderprovisioner_controller.go @@ -384,6 +384,10 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req if err != nil { return ctrl.Result{}, err } + replaced, err = r.bindToControlPlane(ctx, provisioner.Namespace, replaced) + if err != nil { + return ctrl.Result{}, err + } if !sameKeySlot(replaced, target) && !slices.Contains(pending, replaced) { pending = append(pending, replaced) } @@ -872,6 +876,28 @@ func (r *CoderProvisionerReconciler) readControlPlane(ctx context.Context, names return controlPlane, true, nil } +// bindToControlPlane records in record, when it has no control plane UID, the +// UID of the control plane that it names, while that control plane has the +// recorded Coder API URL (see tokenBoundTo). A control plane created again +// under that name later then gets no operator token for record. +func (r *CoderProvisionerReconciler) bindToControlPlane( + ctx context.Context, + namespace string, + record coderv1alpha1.CoderProvisionerKeyRecord, +) (coderv1alpha1.CoderProvisionerKeyRecord, error) { + if record.ControlPlaneUID != "" { + return record, nil + } + controlPlane, ok, err := r.readControlPlane(ctx, namespace, record.ControlPlaneRefName) + if err != nil || !ok { + return record, err + } + if tokenBoundTo(record, controlPlane) { + record.ControlPlaneUID = string(controlPlane.UID) + } + return record, nil +} + // keyCleanup is the result of the cleanup of one recorded provisioner key. type keyCleanup int From cf7d2a536a196597eb9eb3c7b0f68dfe9f2dd120 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 20:38:08 +0000 Subject: [PATCH 12/13] =?UTF-8?q?=F0=9F=A4=96=20test:=20a=20rotation=20aft?= =?UTF-8?q?er=20an=20organization=20rename=20deletes=20the=20replaced=20ke?= =?UTF-8?q?y=20by=20its=20recorded=20IDs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Change-Id: I7b7e9c64b40486603df107bf9d905790eaab2371 Signed-off-by: Thomas Kosiewski --- .../coderprovisioner_keyownership_test.go | 123 ++++++++++++------ 1 file changed, 81 insertions(+), 42 deletions(-) diff --git a/internal/controller/coderprovisioner_keyownership_test.go b/internal/controller/coderprovisioner_keyownership_test.go index fc8bd305..f4bac049 100644 --- a/internal/controller/coderprovisioner_keyownership_test.go +++ b/internal/controller/coderprovisioner_keyownership_test.go @@ -431,53 +431,92 @@ func (c *renamedOrgCoder) serveHTTP(t *testing.T, w http.ResponseWriter, r *http w.WriteHeader(http.StatusNotFound) } -// TestCoderProvisionerKey_StatusWithoutUIDOrganizationRenamed checks status -// without a control plane UID whose applied key is in organization A, named -// team. A is renamed, and another organization takes the name team, at the -// same control plane. Coder still holds the key in A, so the deletion of the -// provisioner must delete it there by its recorded IDs. -func TestCoderProvisionerKey_StatusWithoutUIDOrganizationRenamed(t *testing.T) { +// TestCoderProvisionerKey_OrganizationRenamed checks a provisioner whose +// applied key is in organization A, named team. A is renamed, and another +// organization takes the name team, at the same control plane. A lookup of +// the name team then does not find the key. Coder still holds the key in A, +// so a rotation must delete it there by its recorded IDs, and so must the +// deletion of the provisioner. +func TestCoderProvisionerKey_OrganizationRenamed(t *testing.T) { t.Parallel() - ctx := context.Background() - f := newProvisionerCredentialFixture(ctx, t, "cprov-orgrename") - orgA, orgB := uuid.New(), uuid.New() - keyA, keyB := 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: orgB, KeyID: keyB, KeyName: "provisioner", Key: "material-b"}, + + tests := []struct { + name string + // legacy removes the control plane UID from status, as an operator + // before UID records wrote it. + legacy bool + // loseSecret deletes the provisioner key Secret after the rename. + loseSecret bool + // changeTags changes spec.tags after the rename. + changeTags bool + }{ + {name: "status without control plane UID", legacy: true}, + {name: "Secret loss", loseSecret: true}, + {name: "status without control plane UID, Secret loss", legacy: true, loseSecret: true}, + {name: "tags change", changeTags: true}, } - 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) - require.NoError(t, k8sClient.Status().Patch(ctx, f.get(ctx, t, p), - ctrlclient.RawPatch(types.MergePatchType, []byte(`{"status":{"controlPlaneUID":null}}`)))) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + f := newProvisionerCredentialFixture(ctx, t, "cprov-orgrename") + orgA, orgB := uuid.New(), uuid.New() + keyA, keyB := 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: orgB, KeyID: keyB, KeyName: "provisioner", Key: "material-b"}, + } + f.reconciler.BootstrapClient = coder - coder.mu.Lock() - coder.team = orgB - coder.mu.Unlock() - f.reconcile(ctx, t, p) - require.NoError(t, k8sClient.Delete(ctx, f.get(ctx, t, p))) - f.reconcile(ctx, t, p) - requireProvisionerGone(ctx, t, types.NamespacedName{Name: p.Name, Namespace: p.Namespace}) + 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) + if tt.legacy { + require.NoError(t, k8sClient.Status().Patch(ctx, f.get(ctx, t, p), + ctrlclient.RawPatch(types.MergePatchType, []byte(`{"status":{"controlPlaneUID":null}}`)))) + } - coder.mu.Lock() - defer coder.mu.Unlock() - require.NotContains(t, coder.keys, keyA, "deleting the provisioner must revoke its recorded key in the renamed organization by ID") - require.Empty(t, coder.keys, "deleting the provisioner must leave none of its keys in Coder") + coder.mu.Lock() + coder.team = orgB + coder.mu.Unlock() + if tt.loseSecret { + require.NoError(t, k8sClient.Delete(ctx, &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: p.Name + "-provisioner-key", Namespace: p.Namespace}})) + } + if tt.changeTags { + latest := f.get(ctx, t, p) + latest.Spec.Tags = map[string]string{"scope": "organization"} + require.NoError(t, k8sClient.Update(ctx, latest)) + } + f.reconcile(ctx, t, p) + if tt.loseSecret || tt.changeTags { + require.Equal(t, keyB.String(), f.get(ctx, t, p).Status.ProvisionerKeyID, "the rotation must create a new key") + coder.mu.Lock() + require.NotContains(t, coder.keys, keyA, "the rotation must delete the replaced key in the renamed organization by ID") + coder.mu.Unlock() + } + require.NoError(t, k8sClient.Delete(ctx, f.get(ctx, t, p))) + f.reconcile(ctx, t, p) + requireProvisionerGone(ctx, t, types.NamespacedName{Name: p.Name, Namespace: p.Namespace}) + + coder.mu.Lock() + defer coder.mu.Unlock() + require.NotContains(t, coder.keys, keyA, "deleting the provisioner must revoke its recorded key in the renamed organization by ID") + require.Empty(t, coder.keys, "deleting the provisioner must leave none of its keys in Coder") + }) + } } // TestCoderProvisionerKey_StatusWithoutUIDReplacedKeyBound checks status From 90bb3a2686135fa8cfb1517e946923c4be54eda2 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 20:42:18 +0000 Subject: [PATCH 13/13] =?UTF-8?q?=F0=9F=A4=96=20fix:=20a=20replacement=20d?= =?UTF-8?q?eletes=20the=20keys=20that=20status=20records=20at=20the=20targ?= =?UTF-8?q?et=20by=20their=20recorded=20IDs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An organization can be renamed and its name reused, so the lookup of the target key name can miss a key that status records at the target. A Secret loss, a metadata backfill or a tags change then created a key in the other organization and dropped the record of the old key, which stayed valid in Coder. The replacement now also deletes the applied key and the target records with key IDs by those IDs before the create. Change-Id: I45de872317bee52bd2f0f8fbf2ea6c54ea13f1e2 Signed-off-by: Thomas Kosiewski --- docs/tutorials/deploy-coderprovisioner.md | 2 +- .../controller/coderprovisioner_controller.go | 54 ++++++++++++++----- 2 files changed, 41 insertions(+), 15 deletions(-) diff --git a/docs/tutorials/deploy-coderprovisioner.md b/docs/tutorials/deploy-coderprovisioner.md index 5c413bc1..9dcc043f 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, each at its own Coder URL. 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. 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 97193e51..cbaf2dd9 100644 --- a/internal/controller/coderprovisioner_controller.go +++ b/internal/controller/coderprovisioner_controller.go @@ -322,6 +322,7 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req // applied key is kept, and bound to that control plane, only when Coder // there lists it under its recorded IDs. An organization can be renamed // and its name reused, so the lookup does not use the name. + appliedGone := false if !replacing && status.ControlPlaneUID == "" { var applied *codersdk.ProvisionerKey if appliedOrgID, appliedKeyID, ok := parseKeyIDs(status.OrganizationID, status.ProvisionerKeyID); ok { @@ -343,6 +344,7 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req appliedControlPlaneUID = string(controlPlane.UID) } else { replacing = true + appliedGone = true } } // Coder allows one key per name in an organization. A key with the target @@ -379,7 +381,7 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req } pending := slices.Clone(provisioner.Status.PendingKeys) var replaced coderv1alpha1.CoderProvisionerKeyRecord - if driftDetected { + if replacing { replaced, err = r.appliedKeyRecord(ctx, provisioner, organizationName, keyName) if err != nil { return ctrl.Result{}, err @@ -388,14 +390,14 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req if err != nil { return ctrl.Result{}, err } - if !sameKeySlot(replaced, target) && !slices.Contains(pending, replaced) { - pending = append(pending, replaced) - } + } + if driftDetected && !sameKeySlot(replaced, target) && !slices.Contains(pending, replaced) { + pending = append(pending, replaced) } if replacing { - // The newest target record with IDs stays until a create at the - // target proves its key gone. Coder allows one key per name, so each - // earlier create at the target already proved the older ones gone. + // 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 != "" { @@ -438,14 +440,39 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req } // Coder returns the plaintext of a key only in the create response, // so a key recorded in status is replaced: deleted by its ID, then - // created again with the desired tags. + // created again with the desired tags. An organization can be renamed + // and its name reused, so the lookup by name above can miss a key that + // status records at the target. Such keys are deleted by their + // recorded IDs too, before the create overwrites or drops the record. + var replacedKeys []coderbootstrap.DeleteProvisionerKeyByIDRequest if existingKey != nil { - if deleteErr := r.BootstrapClient.DeleteProvisionerKeyByID(ctx, coderbootstrap.DeleteProvisionerKeyByIDRequest{ + replacedKeys = append(replacedKeys, coderbootstrap.DeleteProvisionerKeyByIDRequest{ CoderURL: coderURL, SessionToken: sessionToken, OrganizationID: existingKey.OrganizationID, KeyID: existingKey.ID, - }); deleteErr != nil { + }) + } + recorded := slices.Clone(provisioner.Status.PendingKeys) + if !appliedGone { + recorded = append(recorded, replaced) + } + for _, record := range recorded { + recordOrgID, recordKeyID, ok := parseKeyIDs(record.OrganizationID, record.KeyID) + if !ok || !sameKeySlot(record, target) || slices.ContainsFunc(replacedKeys, func(req coderbootstrap.DeleteProvisionerKeyByIDRequest) bool { + return req.KeyID == recordKeyID + }) { + continue + } + replacedKeys = append(replacedKeys, coderbootstrap.DeleteProvisionerKeyByIDRequest{ + CoderURL: coderURL, + SessionToken: sessionToken, + OrganizationID: recordOrgID, + KeyID: recordKeyID, + }) + } + for _, replacedKey := range replacedKeys { + if deleteErr := r.BootstrapClient.DeleteProvisionerKeyByID(ctx, replacedKey); deleteErr != nil { setCondition(provisioner, coderv1alpha1.CoderProvisionerConditionProvisionerKeyReady, metav1.ConditionFalse, "ProvisionerKeyFailed", fmt.Sprintf("Failed to delete provisioner key %q to replace it", keyName)) @@ -553,10 +580,9 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req return ctrl.Result{}, fmt.Errorf("assertion failed: key status snapshot must not be nil") } // The Secret holds the key at the target, so the applied fields take over - // its record. Coder allows one key per name, so the create also proved - // that the keys of the other target records are gone, and so did the - // delete of the key that the create replaced, wherever it was recorded. - // No other key is in the Secret now. + // its record. The replacement deleted the keys of the other target + // records by ID, and the key that the name lookup found, wherever it was + // recorded. No other key is in the Secret now. provisioner.Status.PendingKeys = slices.DeleteFunc(provisioner.Status.PendingKeys, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { return sameKeySlot(record, target) || (existingKey != nil && isKeyID(record.KeyID, existingKey.ID)) })