From b1625aa63fe132f4c1b522699b20f3d28b1cd9a7 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 14:49:58 +0000 Subject: [PATCH 01/10] tests: show that a CoderProvisioner key at an intermediate endpoint is never revoked (#259) Change-Id: Iac0ef846266c12f7aa807a17c973a568476634bc Signed-off-by: Thomas Kosiewski --- api/v1alpha1/coderprovisioner_types.go | 27 +++ api/v1alpha1/zz_generated.deepcopy.go | 21 ++ .../bases/coder.com_coderprovisioners.yaml | 45 ++++ dist/install.yaml | 45 ++++ docs/reference/api/coderprovisioner.md | 15 ++ .../coderprovisioner_pendingkeys_test.go | 202 ++++++++++++++++++ 6 files changed, 355 insertions(+) create mode 100644 internal/controller/coderprovisioner_pendingkeys_test.go diff --git a/api/v1alpha1/coderprovisioner_types.go b/api/v1alpha1/coderprovisioner_types.go index 13e56ac53..90cabe17b 100644 --- a/api/v1alpha1/coderprovisioner_types.go +++ b/api/v1alpha1/coderprovisioner_types.go @@ -105,6 +105,33 @@ type CoderProvisionerStatus struct { ControlPlaneURL string `json:"controlPlaneURL,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 + // 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 each entry + // at its coderURL with the operator token of its control plane, and + // removes the entry when Coder confirms that the key is gone. + // +listType=atomic + PendingKeys []CoderProvisionerKeyRecord `json:"pendingKeys,omitempty"` +} + +// CoderProvisionerKeyRecord names a provisioner key that the operator created, +// or was about to create, at one Coder endpoint. +type CoderProvisionerKeyRecord struct { + // ControlPlaneRefName is the CoderControlPlane whose operator token the + // operator used for the key. + ControlPlaneRefName string `json:"controlPlaneRefName"` + // 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. + OrganizationName string `json:"organizationName"` + // KeyName is the provisioner key name in Coder. + KeyName string `json:"keyName"` + // OrganizationID is the organization ID that Coder returned for the key. + // Without organizationID and keyID, the operator deletes the key by name. + OrganizationID string `json:"organizationID,omitempty"` + // KeyID is the provisioner key ID that Coder returned. + KeyID string `json:"keyID,omitempty"` } // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 4c37b03bc..b4b3529fd 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -948,6 +948,22 @@ func (in *CoderProvisioner) DeepCopyObject() runtime.Object { return nil } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *CoderProvisionerKeyRecord) DeepCopyInto(out *CoderProvisionerKeyRecord) { + *out = *in + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new CoderProvisionerKeyRecord. +func (in *CoderProvisionerKeyRecord) DeepCopy() *CoderProvisionerKeyRecord { + if in == nil { + return nil + } + out := new(CoderProvisionerKeyRecord) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *CoderProvisionerKeySpec) DeepCopyInto(out *CoderProvisionerKeySpec) { *out = *in @@ -1065,6 +1081,11 @@ func (in *CoderProvisionerStatus) DeepCopyInto(out *CoderProvisionerStatus) { *out = new(SecretKeySelector) **out = **in } + if in.PendingKeys != nil { + in, out := &in.PendingKeys, &out.PendingKeys + *out = make([]CoderProvisionerKeyRecord, len(*in)) + copy(*out, *in) + } return } diff --git a/config/crd/bases/coder.com_coderprovisioners.yaml b/config/crd/bases/coder.com_coderprovisioners.yaml index e5e78be9f..adb36cca9 100644 --- a/config/crd/bases/coder.com_coderprovisioners.yaml +++ b/config/crd/bases/coder.com_coderprovisioners.yaml @@ -434,6 +434,51 @@ spec: description: OrganizationName is the organization name last applied to the provisioner key. type: string + pendingKeys: + description: |- + 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 each entry + at its coderURL with the operator token of its control plane, and + removes the entry when Coder confirms that the key is gone. + items: + description: |- + CoderProvisionerKeyRecord names a provisioner key that the operator created, + or was about to create, at one Coder endpoint. + properties: + coderURL: + description: CoderURL is the Coder API URL that the operator + sent the key request to. + type: string + controlPlaneRefName: + description: |- + ControlPlaneRefName is the CoderControlPlane whose operator token the + operator used for the key. + type: string + keyID: + description: KeyID is the provisioner key ID that Coder returned. + type: string + keyName: + description: KeyName is the provisioner key name in Coder. + type: string + organizationID: + description: |- + OrganizationID is the organization ID that Coder returned for the key. + Without organizationID and keyID, the operator deletes the key by name. + type: string + organizationName: + description: OrganizationName is the Coder organization of the + key. + type: string + required: + - coderURL + - controlPlaneRefName + - keyName + - organizationName + type: object + type: array + x-kubernetes-list-type: atomic phase: description: Phase is a high-level readiness indicator. type: string diff --git a/dist/install.yaml b/dist/install.yaml index 7172ee67e..11ba572bd 100644 --- a/dist/install.yaml +++ b/dist/install.yaml @@ -6190,6 +6190,51 @@ spec: description: OrganizationName is the organization name last applied to the provisioner key. type: string + pendingKeys: + description: |- + 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 each entry + at its coderURL with the operator token of its control plane, and + removes the entry when Coder confirms that the key is gone. + items: + description: |- + CoderProvisionerKeyRecord names a provisioner key that the operator created, + or was about to create, at one Coder endpoint. + properties: + coderURL: + description: CoderURL is the Coder API URL that the operator + sent the key request to. + type: string + controlPlaneRefName: + description: |- + ControlPlaneRefName is the CoderControlPlane whose operator token the + operator used for the key. + type: string + keyID: + description: KeyID is the provisioner key ID that Coder returned. + type: string + keyName: + description: KeyName is the provisioner key name in Coder. + type: string + organizationID: + description: |- + OrganizationID is the organization ID that Coder returned for the key. + Without organizationID and keyID, the operator deletes the key by name. + type: string + organizationName: + description: OrganizationName is the Coder organization of the + key. + type: string + required: + - coderURL + - controlPlaneRefName + - keyName + - organizationName + type: object + type: array + x-kubernetes-list-type: atomic phase: description: Phase is a high-level readiness indicator. type: string diff --git a/docs/reference/api/coderprovisioner.md b/docs/reference/api/coderprovisioner.md index 438762b4f..bb568e61c 100644 --- a/docs/reference/api/coderprovisioner.md +++ b/docs/reference/api/coderprovisioner.md @@ -41,9 +41,24 @@ | `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. | | `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 each entry at its coderURL with the operator token of its control plane, and removes the entry when Coder confirms that the key is gone. | ## Referenced types +### CoderProvisionerKeyRecord + +CoderProvisionerKeyRecord names a provisioner key that the operator created, +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. | +| `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. | +| `organizationID` | string | OrganizationID is the organization ID that Coder returned for the key. Without organizationID and keyID, the operator deletes the key by name. | +| `keyID` | string | KeyID is the provisioner key ID that Coder returned. | + ### CoderProvisionerKeySpec CoderProvisionerKeySpec configures provisioner key naming and storage. diff --git a/internal/controller/coderprovisioner_pendingkeys_test.go b/internal/controller/coderprovisioner_pendingkeys_test.go new file mode 100644 index 000000000..9961a19ed --- /dev/null +++ b/internal/controller/coderprovisioner_pendingkeys_test.go @@ -0,0 +1,202 @@ +package controller_test + +import ( + "context" + "errors" + "slices" + "testing" + + "github.com/google/uuid" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" + ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" + "github.com/coder/coder-k8s/internal/coderbootstrap" +) + +// keyRecordingClient checks that status records every key create before the +// create reaches Coder, and can fail the revoke of one key. +type keyRecordingClient struct { + *fakeBootstrapClient + t *testing.T + provisioner types.NamespacedName + // onCreated runs after Coder returned a new key. + onCreated func(coderbootstrap.EnsureProvisionerKeyResponse) + // failRevoke makes the delete of this key ID fail. + failRevoke uuid.UUID +} + +func (c *keyRecordingClient) EnsureProvisionerKey(ctx context.Context, req coderbootstrap.EnsureProvisionerKeyRequest) (coderbootstrap.EnsureProvisionerKeyResponse, error) { + latest := &coderv1alpha1.CoderProvisioner{} + require.NoError(c.t, k8sClient.Get(ctx, c.provisioner, latest)) + recorded := slices.ContainsFunc(latest.Status.PendingKeys, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { + return record.CoderURL == req.CoderURL && record.OrganizationName == req.OrganizationName && record.KeyName == req.KeyName + }) + if !recorded { + c.t.Errorf("key %q at %s can be created without a record in status.pendingKeys %+v", req.KeyName, req.CoderURL, latest.Status.PendingKeys) + } + response, err := c.fakeBootstrapClient.EnsureProvisionerKey(ctx, req) + if err == nil && response.Key != "" && c.onCreated != nil { + c.onCreated(response) + } + return response, err +} + +func (c *keyRecordingClient) DeleteProvisionerKeyByID(ctx context.Context, req coderbootstrap.DeleteProvisionerKeyByIDRequest) error { + if req.KeyID == c.failRevoke { + return errors.New("injected revoke failure") + } + return c.fakeBootstrapClient.DeleteProvisionerKeyByID(ctx, req) +} + +// keyDelete is one key delete request that reached the fake Coder. +type keyDelete struct { + coderURL, token, keyName, 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()}) + } + return deletes +} + +func createControlPlaneWithToken(ctx context.Context, t *testing.T, namespace, name, url, token string) { + t.Helper() + createTestControlPlane(ctx, t, namespace, name, url) + secret := &corev1.Secret{} + require.NoError(t, k8sClient.Get(ctx, types.NamespacedName{Name: name + "-operator-token", Namespace: namespace}, secret)) + secret.Data[coderv1alpha1.DefaultTokenSecretKey] = []byte(token) + require.NoError(t, k8sClient.Update(ctx, secret)) +} + +// TestCoderProvisionerKey_IntermediateEndpointRevoked checks a control plane +// target that changes twice, A to B to C, while the attempt at B fails after +// Coder created the key at B. Both the next rotation and deletion must delete +// the key at B, with the operator token of control plane B, and must send no +// token to the endpoint of another control plane. +func TestCoderProvisionerKey_IntermediateEndpointRevoked(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + // statusWriteFails makes the status write that records key B fail, + // and the revoke of the unpublished key B fail too, as after a crash + // right after the create. Otherwise the Secret write fails. + statusWriteFails bool + // deleteAtC deletes the provisioner instead of reconciling at C. + deleteAtC bool + }{ + {name: "secret write fails at B, rotation at C"}, + {name: "secret write fails at B, deletion at C", deleteAtC: true}, + {name: "key B unrecorded by ID, rotation at C", statusWriteFails: true}, + {name: "key B unrecorded by ID, deletion at C", statusWriteFails: true, deleteAtC: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + f := newProvisionerCredentialFixture(ctx, t, "cprov-intermediate") + createControlPlaneWithToken(ctx, t, f.namespace, "coder-b", "https://coder-b.example.com", "operator-token-b") + createControlPlaneWithToken(ctx, t, f.namespace, "coder-c", "https://coder-c.example.com", "operator-token-c") + apiURL := func(controlPlane string) string { + return "http://" + controlPlane + "." + f.namespace + ".svc.cluster.local:80" + } + tokenAt := map[string]string{ + apiURL("coder"): testOperatorToken, + apiURL("coder-b"): "operator-token-b", + apiURL("coder-c"): "operator-token-c", + } + keyA := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: uuid.New(), KeyID: uuid.New(), KeyName: "provisioner", Key: "material-a"} + keyB := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: uuid.New(), KeyID: uuid.New(), KeyName: "provisioner", Key: "material-b"} + keyC := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: uuid.New(), KeyID: uuid.New(), KeyName: "provisioner", Key: "material-c"} + f.bootstrapClient.provisionerKeyResponses = []coderbootstrap.EnsureProvisionerKeyResponse{keyA, keyB, keyC} + + provisioner := f.createProvisioner(ctx, t, nil, nil) + name := types.NamespacedName{Name: provisioner.Name, Namespace: provisioner.Namespace} + keySecretName := provisioner.Name + "-provisioner-key" + client := &keyRecordingClient{fakeBootstrapClient: f.bootstrapClient, t: t, provisioner: name} + f.reconciler.BootstrapClient = client + var failSecretWrite, failStatusWrite bool + base, err := ctrlclient.NewWithWatch(cfg, ctrlclient.Options{Scheme: scheme}) + require.NoError(t, err) + f.reconciler.Client = interceptor.NewClient(base, interceptor.Funcs{ + SubResourceUpdate: func(ctx context.Context, c ctrlclient.Client, sub string, obj ctrlclient.Object, opts ...ctrlclient.SubResourceUpdateOption) error { + if _, ok := obj.(*coderv1alpha1.CoderProvisioner); ok && failStatusWrite { + failStatusWrite = false + return apierrors.NewServiceUnavailable("injected status write failure") + } + return c.SubResource(sub).Update(ctx, obj, opts...) + }, + Update: func(ctx context.Context, c ctrlclient.WithWatch, obj ctrlclient.Object, opts ...ctrlclient.UpdateOption) error { + if s, ok := obj.(*corev1.Secret); ok && s.Name == keySecretName && failSecretWrite { + failSecretWrite = false + return apierrors.NewServiceUnavailable("injected Secret write failure") + } + return c.Update(ctx, obj, opts...) + }, + }) + moveTo := func(controlPlane string) { + latest := f.get(ctx, t, provisioner) + latest.Spec.ControlPlaneRef.Name = controlPlane + require.NoError(t, k8sClient.Update(ctx, latest)) + } + + f.reconcile(ctx, t, provisioner) + f.reconcile(ctx, t, provisioner) + require.Equal(t, keyA.KeyID.String(), f.get(ctx, t, provisioner).Status.ProvisionerKeyID) + + moveTo("coder-b") + if tt.statusWriteFails { + client.failRevoke = keyB.KeyID + client.onCreated = func(created coderbootstrap.EnsureProvisionerKeyResponse) { + failStatusWrite = created.KeyID == keyB.KeyID + } + } else { + failSecretWrite = true + } + _, err = f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: name}) + require.Error(t, err, "the injected failure must fail the attempt at B") + require.Equal(t, 2, f.bootstrapClient.provisionerKeyCalls, "Coder must have created key B") + client.failRevoke = uuid.Nil + client.onCreated = nil + + moveTo("coder-c") + if tt.deleteAtC { + require.NoError(t, k8sClient.Delete(ctx, f.get(ctx, t, provisioner))) + } + f.reconcile(ctx, t, provisioner) + + deletes := keyDeletes(f.bootstrapClient) + revokedAtB := slices.ContainsFunc(deletes, func(d keyDelete) bool { + return d.coderURL == apiURL("coder-b") && d.token == "operator-token-b" && + (d.keyID == keyB.KeyID.String() || (d.keyID == "" && d.keyName == keyB.KeyName)) + }) + require.True(t, revokedAtB, "key B must be deleted at B with the operator token of B, got %+v", deletes) + for _, d := range deletes { + want, known := tokenAt[d.coderURL] + require.True(t, known, "delete sent to %s, which no record names", d.coderURL) + require.Equal(t, want, d.token, "delete at %s must use the operator token of that control plane", d.coderURL) + } + + if tt.deleteAtC { + requireProvisionerGone(ctx, t, name) + return + } + final := f.get(ctx, t, provisioner) + require.Equal(t, keyC.KeyID.String(), final.Status.ProvisionerKeyID) + require.Equal(t, "https://coder-c.example.com", final.Status.ControlPlaneURL) + require.Empty(t, final.Status.PendingKeys, "every replaced key must be confirmed deleted") + }) + } +} From e9c111e1e9f70a4745f34e5a69092cf14582e70f Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 15:04:18 +0000 Subject: [PATCH 02/10] fix: record every CoderProvisioner key with its endpoint and token source, and revoke it there (#259) Change-Id: I27dde188ca1aa83839b622858523babd03874997 Signed-off-by: Thomas Kosiewski --- docs/tutorials/deploy-coderprovisioner.md | 4 +- internal/coderbootstrap/provisionerkeys.go | 20 +- .../provisionerkeys_deletebyid_test.go | 13 +- .../coderbootstrap/provisionerkeys_test.go | 90 +++-- .../controller/coderprovisioner_controller.go | 330 ++++++++++++------ .../coderprovisioner_deletion_test.go | 55 +-- .../coderprovisioner_pendingkeys_test.go | 50 +++ 7 files changed, 371 insertions(+), 191 deletions(-) diff --git a/docs/tutorials/deploy-coderprovisioner.md b/docs/tutorials/deploy-coderprovisioner.md index 3c4efc744..503fb020c 100644 --- a/docs/tutorials/deploy-coderprovisioner.md +++ b/docs/tutorials/deploy-coderprovisioner.md @@ -78,7 +78,9 @@ kubectl get secret coderprovisioner-sample-provisioner-key -n coder kubectl delete coderprovisioner coderprovisioner-sample -n coder ``` -The operator then deletes the provisioner key in Coder that this provisioner created. It finds the key by the organization and key IDs in `status.organizationID` and `status.provisionerKeyID`, not by the names in the spec. 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 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 each key in `status.pendingKeys`. These are 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 `keyID` is deleted by name. 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. An entry stays in `status.pendingKeys` until Coder confirms that its key is gone. 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/provisionerkeys.go b/internal/coderbootstrap/provisionerkeys.go index 7bfab48eb..c0e936336 100644 --- a/internal/coderbootstrap/provisionerkeys.go +++ b/internal/coderbootstrap/provisionerkeys.go @@ -104,7 +104,7 @@ func (c *SDKClient) EnsureProvisionerKey(ctx context.Context, req EnsureProvisio } // DeleteProvisionerKey deletes a provisioner key by name. -// A missing key is treated as success for idempotency. +// A missing key or organization 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 @@ -121,6 +121,9 @@ func (c *SDKClient) DeleteProvisionerKey(ctx context.Context, coderURL, sessionT } organization, err := resolveOrganizationByName(ctx, client, organizationName) + if isNotFound(err) { + return nil + } if err != nil { return err } @@ -128,12 +131,7 @@ func (c *SDKClient) DeleteProvisionerKey(ctx context.Context, coderURL, sessionT _, 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 { + if err == nil || isNotFound(err) { return nil } @@ -208,9 +206,15 @@ func (c *SDKClient) DeleteProvisionerKeyByID(ctx context.Context, req DeleteProv return nil } +// coderNotFoundMessage is the message of the 404 that Coder answers for a +// missing organization or provisioner key (coderd/httpapi/httpapi.go:149 and +// coderd/httpmw/provisionerkey.go:45-46 in v2.37.3). Another 404, for example +// from a proxy or for a wrong base URL, does not prove that the key is gone. +const coderNotFoundMessage = "Resource not found or you do not have access to this resource" + func isNotFound(err error) bool { var apiErr *codersdk.Error - return errors.As(err, &apiErr) && apiErr.StatusCode() == http.StatusNotFound + return errors.As(err, &apiErr) && apiErr.StatusCode() == http.StatusNotFound && apiErr.Message == coderNotFoundMessage } // Entitlements returns deployment entitlements for the given coderd instance. diff --git a/internal/coderbootstrap/provisionerkeys_deletebyid_test.go b/internal/coderbootstrap/provisionerkeys_deletebyid_test.go index 9339fe242..f1f84f6b5 100644 --- a/internal/coderbootstrap/provisionerkeys_deletebyid_test.go +++ b/internal/coderbootstrap/provisionerkeys_deletebyid_test.go @@ -25,12 +25,15 @@ func TestDeleteProvisionerKeyByID(t *testing.T) { tests := []struct { name string listStatus int + listBody map[string]any listedID uuid.UUID wantDeletes []string + wantErr bool }{ {name: "recorded key exists", listStatus: http.StatusOK, listedID: recordedKeyID, wantDeletes: []string{keysPath + "/shared-name"}}, {name: "same name, other ID", listStatus: http.StatusOK, listedID: uuid.New()}, - {name: "organization gone", listStatus: http.StatusNotFound}, + {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 { @@ -43,7 +46,7 @@ func TestDeleteProvisionerKeyByID(t *testing.T) { switch { case r.Method == http.MethodGet && r.URL.Path == keysPath: if tt.listStatus != http.StatusOK { - writeJSONResponse(t, w, tt.listStatus, map[string]any{"message": "not found"}) + writeJSONResponse(t, w, tt.listStatus, tt.listBody) return } writeJSONResponse(t, w, http.StatusOK, []map[string]any{{ @@ -71,7 +74,11 @@ func TestDeleteProvisionerKeyByID(t *testing.T) { OrganizationID: orgID, KeyID: recordedKeyID, }) - require.NoError(t, err) + if tt.wantErr { + require.Error(t, err, "only Coder's own 404 proves that the key is gone") + } else { + require.NoError(t, err) + } mu.Lock() defer mu.Unlock() require.Equal(t, tt.wantDeletes, deletes) diff --git a/internal/coderbootstrap/provisionerkeys_test.go b/internal/coderbootstrap/provisionerkeys_test.go index 57728940a..24b632018 100644 --- a/internal/coderbootstrap/provisionerkeys_test.go +++ b/internal/coderbootstrap/provisionerkeys_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "sync" "sync/atomic" "testing" "time" @@ -445,41 +446,68 @@ func TestDeleteProvisionerKey_Success(t *testing.T) { require.Equal(t, 1, deleteCalls) } +// coderNotFound is the body of the 404 that Coder answers for a missing +// organization or provisioner key. +var coderNotFound = map[string]any{"message": "Resource not found or you do not have access to this resource"} + 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) + tests := []struct { + name string + orgGone bool + deleteBody map[string]any + wantDeletes int + wantErr bool + }{ + {name: "key gone", deleteBody: coderNotFound, wantDeletes: 1}, + {name: "organization gone", orgGone: true}, + {name: "404 not from Coder", deleteBody: map[string]any{"message": "Route not found."}, wantDeletes: 1, wantErr: true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + orgID := uuid.New() + now := time.Now().UTC().Format(time.RFC3339) + var mu sync.Mutex + 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": + if tt.orgGone { + writeJSONResponse(t, w, http.StatusNotFound, coderNotFound) + return + } + writeJSONResponse(t, w, http.StatusOK, map[string]any{ + "id": orgID.String(), + "name": "default", + "created_at": now, + "updated_at": now, + }) + case r.Method == http.MethodDelete && r.URL.Path == "/api/v2/organizations/"+orgID.String()+"/provisionerkeys/"+keyName: + mu.Lock() + deleteCalls++ + mu.Unlock() + writeJSONResponse(t, w, http.StatusNotFound, tt.deleteBody) + default: + writeJSONResponse(t, w, http.StatusNotFound, map[string]any{"message": "unexpected route"}) + } + })) + defer server.Close() + + err := coderbootstrap.NewSDKClient().DeleteProvisionerKey(context.Background(), server.URL, "session-token", "", keyName) + if tt.wantErr { + require.Error(t, err, "only Coder's own 404 proves that the key is gone") + } else { + require.NoError(t, err) + } + mu.Lock() + defer mu.Unlock() + require.Equal(t, tt.wantDeletes, deleteCalls) + }) + } } func writeJSONResponse(t *testing.T, w http.ResponseWriter, statusCode int, payload any) { diff --git a/internal/controller/coderprovisioner_controller.go b/internal/controller/coderprovisioner_controller.go index a9ca11001..71290596b 100644 --- a/internal/controller/coderprovisioner_controller.go +++ b/internal/controller/coderprovisioner_controller.go @@ -2,6 +2,7 @@ package controller import ( + "cmp" "context" "crypto/rand" "encoding/binary" @@ -286,6 +287,61 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req provisionerKeyName = keyName } + metadataMissing := status.OrganizationName == "" || status.TagsHash == "" || status.ControlPlaneRefName == "" || status.ControlPlaneURL == "" + + // Every key that can exist in Coder has a record in status: the applied + // fields for the key in the Secret, status.pendingKeys for the others. A + // key to create is recorded by name before the create, because Coder + // returns its ID only in the create response. Keys recorded in another + // slot than the target are deleted here. + target := coderv1alpha1.CoderProvisionerKeyRecord{ + ControlPlaneRefName: desiredControlPlaneRefName, + CoderURL: coderURL, + OrganizationName: organizationName, + KeyName: keyName, + } + pending := slices.Clone(provisioner.Status.PendingKeys) + var replaced coderv1alpha1.CoderProvisionerKeyRecord + if driftDetected { + replaced, err = r.appliedKeyRecord(ctx, provisioner, organizationName, keyName) + if err != nil { + return ctrl.Result{}, err + } + if !sameKeySlot(replaced, target) && !slices.Contains(pending, replaced) { + pending = append(pending, replaced) + } + } + if driftDetected || !secretUsable || metadataMissing { + pending = slices.DeleteFunc(pending, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { + return sameKeySlot(record, target) + }) + pending = append(pending, target) + } + var kept []coderv1alpha1.CoderProvisionerKeyRecord + for _, record := range pending { + if sameKeySlot(record, target) { + kept = append(kept, record) + continue + } + revoked, revokeErr := r.revokeKey(ctx, provisioner, record) + if revokeErr != nil { + return ctrl.Result{}, revokeErr + } + if !revoked { + kept = append(kept, record) + } + } + if !slices.Equal(kept, provisioner.Status.PendingKeys) { + provisioner.Status.PendingKeys = kept + if err := r.Status().Update(ctx, provisioner); err != nil { + return ctrl.Result{}, fmt.Errorf("record pending provisioner keys in status: %w", err) + } + statusSnapshot = provisioner.Status.DeepCopy() + if statusSnapshot == nil { + return ctrl.Result{}, fmt.Errorf("assertion failed: status snapshot must not be nil after pending key status update") + } + } + log := ctrl.LoggerFrom(ctx) keyMaterial := "" if driftDetected { @@ -293,25 +349,19 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req "orgDrift", orgDrift, "keyNameDrift", keyNameDrift, "tagsDrift", tagsDrift, "controlPlaneRefDrift", controlPlaneRefDrift, "controlPlaneURLDrift", controlPlaneURLDrift) - oldOrg := provisioner.Status.OrganizationName - if oldOrg == "" { - oldOrg = organizationName - } - oldKeyName := provisioner.Status.ProvisionerKeyName - if oldKeyName == "" { - oldKeyName = keyName - } - oldControlPlaneURL := recordedCoderAPIURL(provisioner.Status.ControlPlaneURL, controlPlane, coderURL) - - if deleteErr := r.BootstrapClient.DeleteProvisionerKey( - ctx, - oldControlPlaneURL, - sessionToken, - oldOrg, - oldKeyName, - ); deleteErr != nil { - log.Info("failed to delete old provisioner key during drift rotation, creating new key anyway", - "oldKeyName", oldKeyName, "error", deleteErr) + // 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{ @@ -479,7 +529,7 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req "ProvisionerKeyReady", "Provisioner key is available in coderd", ) - } else if status.OrganizationName == "" || status.TagsHash == "" || status.ControlPlaneRefName == "" || status.ControlPlaneURL == "" { + } 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 @@ -567,17 +617,22 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req } if keyMaterial != "" { - // A new key exists in Coder. Record its IDs in status before its - // plaintext reaches the Secret, so that deletion can find the key to - // revoke (at status.controlPlaneURL or, during a control plane URL - // change, at the current control plane URL). Only the IDs: the applied spec values stay as they are - // until the Secret holds the key, so a failed Secret write is detected - // as drift (or missing metadata) and rotated again on the next try. - // A failed status write (for example a conflict because the - // provisioner is being deleted) revokes the new key before anyone can - // read it. - provisioner.Status.OrganizationID = organizationID - provisioner.Status.ProvisionerKeyID = provisionerKeyID + // A new key exists in Coder. Record its IDs in its pending record + // before its plaintext reaches the Secret, so that deletion and a + // later rotation delete it by ID at the target endpoint. The applied + // status fields stay as they are until the Secret holds the key, so a + // failed Secret write is detected as drift (or missing metadata) and + // rotated again on the next try. A failed status write (for example a + // conflict because the provisioner is being deleted) revokes the new + // key before anyone can read it. + i := slices.IndexFunc(provisioner.Status.PendingKeys, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { + return sameKeySlot(record, target) + }) + if i < 0 { + return ctrl.Result{}, fmt.Errorf("assertion failed: status has no pending record of the new provisioner key %q", keyName) + } + provisioner.Status.PendingKeys[i].OrganizationID = organizationID + provisioner.Status.PendingKeys[i].KeyID = provisionerKeyID if err := r.Status().Update(ctx, provisioner); err != nil { r.revokeUnpublishedKey(ctx, coderURL, sessionToken, organizationID, provisionerKeyID) return ctrl.Result{}, fmt.Errorf("record new provisioner key in status: %w", err) @@ -620,6 +675,20 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req if keyStatusSnapshot == nil { 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. When this reconcile created no key, a target record with + // IDs names a key whose Secret write succeeded but whose status write + // after it failed. + if keyMaterial == "" { + for _, record := range provisioner.Status.PendingKeys { + if _, _, ok := parseKeyIDs(record.OrganizationID, record.KeyID); ok && sameKeySlot(record, target) { + organizationID, provisionerKeyID = record.OrganizationID, record.KeyID + } + } + } + provisioner.Status.PendingKeys = slices.DeleteFunc(provisioner.Status.PendingKeys, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { + return sameKeySlot(record, target) + }) recordAppliedKeyStatus(provisioner, organizationID, appliedOrgName, provisionerKeyID, provisionerKeyName, appliedTagsHash, appliedControlPlaneRefName, appliedControlPlaneURL) provisioner.Status.SecretRef = &coderv1alpha1.SecretKeySelector{Name: keySecretName, Key: keySecretKey} @@ -687,78 +756,37 @@ func (r *CoderProvisionerReconciler) reconcileDeletion(ctx context.Context, prov log := ctrl.LoggerFrom(ctx) - // Delete only the key that status records for this resource. A provisioner - // that never applied a key (refused, not entitled, never ready) must not - // delete a Coder key that happens to have the name from its spec. - organizationID, keyID, recorded := recordedProvisionerKey(provisioner.Status) - if !recorded { + // Delete only the keys that status records for this resource. A + // provisioner that never applied a key (refused, not entitled, never + // ready) must not delete a Coder key that happens to have the name from + // its spec. + records := slices.Clone(provisioner.Status.PendingKeys) + if _, _, ok := parseKeyIDs(provisioner.Status.OrganizationID, provisioner.Status.ProvisionerKeyID); ok { + applied, err := r.appliedKeyRecord(ctx, provisioner, provisioner.Status.OrganizationName, provisioner.Status.ProvisionerKeyName) + if err != nil { + return ctrl.Result{}, err + } + records = append(records, applied) + } + if len(records) == 0 { log.Info("status records no Coder provisioner key, removing the finalizer without a Coder call") return r.removeCleanupFinalizer(ctx, provisioner) } - keyName := provisioner.Status.ProvisionerKeyName - // Best-effort remote key cleanup: if the referenced control plane, - // its URL, operator access token, or any other prerequisite is - // unavailable, log a warning and proceed to finalizer removal so the - // CR does not get stuck in Terminating. This is common during - // namespace teardown, when the control plane was never ready, or - // when operator access bootstrap has not completed. - controlPlaneURL := provisioner.Status.ControlPlaneURL - controlPlaneName := strings.TrimSpace(provisioner.Spec.ControlPlaneRef.Name) - var controlPlane *coderv1alpha1.CoderControlPlane - if controlPlaneName != "" { - controlPlane = &coderv1alpha1.CoderControlPlane{} - namespacedName := types.NamespacedName{Name: controlPlaneName, Namespace: provisioner.Namespace} - if err := r.Get(ctx, namespacedName, controlPlane); err != nil { - log.Info("unable to read referenced CoderControlPlane during deletion, skipping remote key cleanup", - "controlPlaneRef", controlPlaneName, "error", err) - controlPlane = nil - } else if controlPlane.Name != controlPlaneName || controlPlane.Namespace != provisioner.Namespace { - return ctrl.Result{}, fmt.Errorf("assertion failed: fetched control plane %s/%s does not match expected %s/%s", - controlPlane.Namespace, controlPlane.Name, provisioner.Namespace, controlPlaneName) + // Best-effort remote key cleanup: when a key's control plane, its + // operator access token, or Coder is unavailable, the key stays and the + // finalizer is still removed, so the CR does not get stuck in + // Terminating. This is common during namespace teardown, when the + // control plane was never ready, or when operator access bootstrap has + // not completed. + for _, record := range records { + revoked, err := r.revokeKey(ctx, provisioner, record) + if err != nil { + return ctrl.Result{}, err } - } - if controlPlaneURL == "" && controlPlane != nil { - controlPlaneURL = strings.TrimSpace(controlPlane.Status.URL) - } - - if controlPlaneURL != "" && controlPlane != nil { - sessionToken, tokenResult, tokenReason, tokenMessage, tokenErr := r.readOperatorSessionToken(ctx, provisioner, controlPlane) - switch { - case tokenErr != nil: - log.Info("unable to resolve operator access token during deletion, skipping remote key cleanup", - "error", tokenErr) - case tokenResult.RequeueAfter > 0: - log.Info("operator access is not ready during deletion, skipping remote key cleanup", - "reason", tokenReason, "message", tokenMessage) - default: - currentURL, urlErr := coderAPIURL(controlPlane) - if urlErr != nil { - log.Info("unable to derive the Coder API URL during deletion, skipping remote key cleanup", "error", urlErr) - break - } - // During a control plane URL change, status can hold the new key's - // IDs while status.controlPlaneURL still names the old endpoint. - // Try the current endpoint too: the delete matches the key ID only. - coderURLs := []string{recordedCoderAPIURL(provisioner.Status.ControlPlaneURL, controlPlane, currentURL)} - if currentURL != coderURLs[0] { - coderURLs = append(coderURLs, currentURL) - } - for _, coderURL := range coderURLs { - if deleteErr := r.BootstrapClient.DeleteProvisionerKeyByID(ctx, coderbootstrap.DeleteProvisionerKeyByIDRequest{ - CoderURL: coderURL, - SessionToken: sessionToken, - OrganizationID: organizationID, - KeyID: keyID, - }); deleteErr != nil { - // Treat key deletion failures as best-effort so the - // finalizer is still removed. Transient errors, auth - // issues, or org-lookup failures should not block CR - // cleanup. - log.Info("failed to delete remote provisioner key during deletion, proceeding with finalizer removal", - "keyName", keyName, "keyID", keyID, "error", deleteErr) - } - } + if !revoked { + log.Info("could not delete a recorded provisioner key during deletion, proceeding with finalizer removal", + "coderURL", record.CoderURL, "keyName", record.KeyName, "keyID", record.KeyID) } } @@ -805,7 +833,8 @@ func recordAppliedKeyStatus( } // revokeUnpublishedKey deletes a key whose plaintext was never written to a -// Secret. It is best-effort: the key is unknown outside this reconcile. +// Secret. It is best-effort: when it fails, the pending record of the key +// still names it, by name. func (r *CoderProvisionerReconciler) revokeUnpublishedKey(ctx context.Context, coderURL, sessionToken, organizationID, keyID string) { log := ctrl.LoggerFrom(ctx) orgID, orgErr := uuid.Parse(organizationID) @@ -833,15 +862,14 @@ func (r *CoderProvisionerReconciler) removeCleanupFinalizer(ctx context.Context, return ctrl.Result{}, nil } -// recordedProvisionerKey returns the organization and key IDs that status -// records for the key this provisioner created. ok is false when status does -// not record valid IDs. -func recordedProvisionerKey(status coderv1alpha1.CoderProvisionerStatus) (organizationID, keyID uuid.UUID, ok bool) { - organizationID, err := uuid.Parse(status.OrganizationID) +// parseKeyIDs parses the organization and key IDs of a recorded provisioner +// key. ok is false when the record has no valid IDs. +func parseKeyIDs(rawOrganizationID, rawKeyID string) (organizationID, keyID uuid.UUID, ok bool) { + organizationID, err := uuid.Parse(rawOrganizationID) if err != nil || organizationID == uuid.Nil { return uuid.Nil, uuid.Nil, false } - keyID, err = uuid.Parse(status.ProvisionerKeyID) + keyID, err = uuid.Parse(rawKeyID) if err != nil || keyID == uuid.Nil { return uuid.Nil, uuid.Nil, false } @@ -849,6 +877,98 @@ func recordedProvisionerKey(status coderv1alpha1.CoderProvisionerStatus) (organi return organizationID, keyID, true } +// 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. +func sameKeySlot(a, b coderv1alpha1.CoderProvisionerKeyRecord) bool { + return a.CoderURL == b.CoderURL && a.OrganizationName == b.OrganizationName && a.KeyName == b.KeyName +} + +// appliedKeyRecord returns a record of the key that the applied status fields +// describe, at the Coder API URL for status.controlPlaneURL. When the control +// plane that applied the key cannot be read, the record keeps +// status.controlPlaneURL. +func (r *CoderProvisionerReconciler) appliedKeyRecord( + ctx context.Context, + provisioner *coderv1alpha1.CoderProvisioner, + defaultOrganizationName, defaultKeyName string, +) (coderv1alpha1.CoderProvisionerKeyRecord, error) { + status := provisioner.Status + record := coderv1alpha1.CoderProvisionerKeyRecord{ + ControlPlaneRefName: cmp.Or(status.ControlPlaneRefName, provisioner.Spec.ControlPlaneRef.Name), + CoderURL: strings.TrimSpace(status.ControlPlaneURL), + OrganizationName: cmp.Or(status.OrganizationName, defaultOrganizationName), + KeyName: cmp.Or(status.ProvisionerKeyName, defaultKeyName), + OrganizationID: status.OrganizationID, + KeyID: status.ProvisionerKeyID, + } + controlPlane, ok, err := r.readControlPlane(ctx, provisioner.Namespace, record.ControlPlaneRefName) + if err != nil || !ok { + return record, err + } + apiURL, err := coderAPIURL(controlPlane) + if err != nil { + ctrl.LoggerFrom(ctx).Info("unable to derive the Coder API URL of the applied provisioner key", "error", err) + return record, nil + } + record.CoderURL = recordedCoderAPIURL(status.ControlPlaneURL, controlPlane, apiURL) + return record, nil +} + +// readControlPlane reads a control plane in namespace. ok is false when it +// cannot be read. +func (r *CoderProvisionerReconciler) readControlPlane(ctx context.Context, namespace, name string) (*coderv1alpha1.CoderControlPlane, bool, error) { + controlPlane := &coderv1alpha1.CoderControlPlane{} + if err := r.Get(ctx, types.NamespacedName{Name: name, Namespace: namespace}, controlPlane); err != nil { + ctrl.LoggerFrom(ctx).Info("unable to read CoderControlPlane", "controlPlaneRef", name, "error", err) + return nil, false, nil + } + if controlPlane.Name != name || controlPlane.Namespace != namespace { + return nil, false, fmt.Errorf("assertion failed: fetched control plane %s/%s does not match expected %s/%s", + controlPlane.Namespace, controlPlane.Name, namespace, name) + } + return controlPlane, true, nil +} + +// 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 is deleted by name. revoked reports whether Coder confirmed +// that the key is gone. +func (r *CoderProvisionerReconciler) revokeKey( + ctx context.Context, + provisioner *coderv1alpha1.CoderProvisioner, + record coderv1alpha1.CoderProvisionerKeyRecord, +) (revoked bool, err error) { + log := ctrl.LoggerFrom(ctx).WithValues("coderURL", record.CoderURL, "controlPlaneRef", record.ControlPlaneRefName, + "keyName", record.KeyName, "keyID", record.KeyID) + controlPlane, ok, err := r.readControlPlane(ctx, provisioner.Namespace, record.ControlPlaneRefName) + if err != nil || !ok { + return false, err + } + 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", + "reason", tokenReason, "message", tokenMessage, "error", tokenErr) + return false, nil + } + + if organizationID, keyID, ok := parseKeyIDs(record.OrganizationID, record.KeyID); ok { + err = r.BootstrapClient.DeleteProvisionerKeyByID(ctx, coderbootstrap.DeleteProvisionerKeyByIDRequest{ + CoderURL: record.CoderURL, + SessionToken: sessionToken, + OrganizationID: organizationID, + KeyID: keyID, + }) + } else { + err = r.BootstrapClient.DeleteProvisionerKey(ctx, record.CoderURL, sessionToken, record.OrganizationName, record.KeyName) + } + if err != nil { + log.Info("failed to delete the recorded provisioner key, keeping its record", "error", err) + return false, nil + } + return true, nil +} + 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_deletion_test.go b/internal/controller/coderprovisioner_deletion_test.go index 510ae65c9..5465fb80a 100644 --- a/internal/controller/coderprovisioner_deletion_test.go +++ b/internal/controller/coderprovisioner_deletion_test.go @@ -3,6 +3,7 @@ package controller_test import ( "context" "errors" + "slices" "testing" "time" @@ -143,10 +144,10 @@ func TestCoderProvisionerKey_RecordedBeforePublished(t *testing.T) { base, err := ctrlclient.NewWithWatch(cfg, ctrlclient.Options{Scheme: scheme}) require.NoError(t, err) - var statusAtSecretWrite string + var recordedAtSecretWrite bool ordered := interceptor.NewClient(base, interceptor.Funcs{ SubResourceUpdate: func(ctx context.Context, c ctrlclient.Client, sub string, obj ctrlclient.Object, opts ...ctrlclient.SubResourceUpdateOption) error { - if p, ok := obj.(*coderv1alpha1.CoderProvisioner); ok && failStatus && p.Status.ProvisionerKeyID == created.KeyID.String() { + if p, ok := obj.(*coderv1alpha1.CoderProvisioner); ok && failStatus && pendingKeyRecorded(p, created.KeyID) { return apierrors.NewConflict(coderv1alpha1.GroupVersion.WithResource("coderprovisioners").GroupResource(), p.Name, errors.New("injected")) } return c.SubResource(sub).Update(ctx, obj, opts...) @@ -155,7 +156,7 @@ func TestCoderProvisionerKey_RecordedBeforePublished(t *testing.T) { if s, ok := obj.(*corev1.Secret); ok && s.Name == keySecretName.Name { latest := &coderv1alpha1.CoderProvisioner{} require.NoError(t, c.Get(ctx, types.NamespacedName{Name: provisioner.Name, Namespace: provisioner.Namespace}, latest)) - statusAtSecretWrite = latest.Status.ProvisionerKeyID + recordedAtSecretWrite = pendingKeyRecorded(latest, created.KeyID) } return c.Create(ctx, obj, opts...) }, @@ -169,7 +170,7 @@ func TestCoderProvisionerKey_RecordedBeforePublished(t *testing.T) { if !failStatus { require.NoError(t, err) - require.Equal(t, created.KeyID.String(), statusAtSecretWrite, "status must record the key before the Secret holds it") + require.True(t, recordedAtSecretWrite, "status must record the key before the Secret holds it") require.Empty(t, f.bootstrapClient.deleteKeyByIDRequests) return } @@ -224,7 +225,7 @@ func TestCoderProvisionerKey_FailedSecretUpdateRotatesAgain(t *testing.T) { _, err = f.reconciler.Reconcile(ctx, request) require.Error(t, err, "the injected Secret update failure must fail the reconcile") afterFailure := f.get(ctx, t, provisioner) - require.Equal(t, f.bootstrapClient.provisionerKeyResponses[1].KeyID.String(), afterFailure.Status.ProvisionerKeyID, + require.True(t, pendingKeyRecorded(afterFailure, f.bootstrapClient.provisionerKeyResponses[1].KeyID), "status must record the new key before the Secret write") _, err = f.reconciler.Reconcile(ctx, request) @@ -234,44 +235,12 @@ func TestCoderProvisionerKey_FailedSecretUpdateRotatesAgain(t *testing.T) { "the retry must rotate again and publish a live key") final := f.get(ctx, t, provisioner) require.Equal(t, f.bootstrapClient.provisionerKeyResponses[2].KeyID.String(), final.Status.ProvisionerKeyID) + require.Empty(t, final.Status.PendingKeys, "the published key moves to the applied status fields") } -// TestCoderProvisionerDeletion_TriesCurrentControlPlaneURL checks the window -// of a control plane URL change: status holds the new key's IDs but still the -// old status.controlPlaneURL. Deletion must also try the current URL. -func TestCoderProvisionerDeletion_TriesCurrentControlPlaneURL(t *testing.T) { - t.Parallel() - ctx := context.Background() - f := newProvisionerCredentialFixture(ctx, t, "cprov-delete-urldrift") - provisioner := f.createProvisioner(ctx, t, nil, nil) - f.reconcile(ctx, t, provisioner) - f.reconcile(ctx, t, provisioner) - - newKey := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: uuid.New(), KeyID: uuid.New()} - latest := f.get(ctx, t, provisioner) - require.Equal(t, testCoderOrigin, latest.Status.ControlPlaneURL) - latest.Status.OrganizationID = newKey.OrganizationID.String() - latest.Status.ProvisionerKeyID = newKey.KeyID.String() - require.NoError(t, k8sClient.Status().Update(ctx, latest)) - - const newURL = "https://coder-new.example.com" - controlPlane := &coderv1alpha1.CoderControlPlane{} - require.NoError(t, k8sClient.Get(ctx, types.NamespacedName{Name: f.controlPlane.Name, Namespace: f.namespace}, controlPlane)) - controlPlane.Status.URL = newURL - require.NoError(t, k8sClient.Status().Update(ctx, controlPlane)) - - require.NoError(t, k8sClient.Delete(ctx, f.get(ctx, t, provisioner))) - f.reconcile(ctx, t, provisioner) - - require.Len(t, f.bootstrapClient.deleteKeyByIDRequests, 2) - // The recorded URL names an earlier endpoint and is used as recorded. The - // current endpoint is HTTPS, so the call uses the internal HTTP URL. - require.Equal(t, testCoderOrigin, f.bootstrapClient.deleteKeyByIDRequests[0].CoderURL) - require.Equal(t, internalCoderURL(f), f.bootstrapClient.deleteKeyByIDRequests[1].CoderURL) - for _, req := range f.bootstrapClient.deleteKeyByIDRequests { - require.Equal(t, newKey.OrganizationID, req.OrganizationID) - require.Equal(t, newKey.KeyID, req.KeyID) - } - require.Zero(t, f.bootstrapClient.deleteKeyCalls, "no name-based key deletion") - requireProvisionerGone(ctx, t, types.NamespacedName{Name: provisioner.Name, Namespace: provisioner.Namespace}) +// pendingKeyRecorded reports whether status.pendingKeys records the key ID. +func pendingKeyRecorded(provisioner *coderv1alpha1.CoderProvisioner, keyID uuid.UUID) bool { + return slices.ContainsFunc(provisioner.Status.PendingKeys, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { + return record.KeyID == keyID.String() + }) } diff --git a/internal/controller/coderprovisioner_pendingkeys_test.go b/internal/controller/coderprovisioner_pendingkeys_test.go index 9961a19ed..78d82e783 100644 --- a/internal/controller/coderprovisioner_pendingkeys_test.go +++ b/internal/controller/coderprovisioner_pendingkeys_test.go @@ -10,6 +10,7 @@ import ( "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" @@ -200,3 +201,52 @@ func TestCoderProvisionerKey_IntermediateEndpointRevoked(t *testing.T) { }) } } + +// TestCoderProvisionerKey_FailedStatusWriteAfterSecretKeepsKey checks a +// rotation whose Secret write succeeds while the status write after it fails. +// The next reconcile must record the key in the Secret as the applied key, so +// that deletion still deletes it. +func TestCoderProvisionerKey_FailedStatusWriteAfterSecretKeepsKey(t *testing.T) { + t.Parallel() + ctx := context.Background() + f := newProvisionerCredentialFixture(ctx, t, "cprov-adopt") + first := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: uuid.New(), KeyID: uuid.New(), KeyName: "provisioner", Key: "material-1"} + second := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: uuid.New(), KeyID: uuid.New(), KeyName: "provisioner", Key: "material-2"} + f.bootstrapClient.provisionerKeyResponses = []coderbootstrap.EnsureProvisionerKeyResponse{first, second} + 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) + + keySecret := types.NamespacedName{Name: provisioner.Name + "-provisioner-key", Namespace: provisioner.Namespace} + require.NoError(t, k8sClient.Delete(ctx, &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: keySecret.Name, Namespace: keySecret.Namespace}})) + failStatusWrite := true + base, err := ctrlclient.NewWithWatch(cfg, ctrlclient.Options{Scheme: scheme}) + require.NoError(t, err) + f.reconciler.Client = interceptor.NewClient(base, interceptor.Funcs{ + SubResourceUpdate: func(ctx context.Context, c ctrlclient.Client, sub string, obj ctrlclient.Object, opts ...ctrlclient.SubResourceUpdateOption) error { + if p, ok := obj.(*coderv1alpha1.CoderProvisioner); ok && failStatusWrite && p.Status.ProvisionerKeyID == second.KeyID.String() { + failStatusWrite = false + return apierrors.NewServiceUnavailable("injected status write failure") + } + return c.SubResource(sub).Update(ctx, obj, opts...) + }, + }) + _, err = f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: name}) + require.Error(t, err, "the injected status write failure must fail the rotation") + secret := &corev1.Secret{} + require.NoError(t, k8sClient.Get(ctx, keySecret, secret)) + require.Equal(t, "material-2", string(secret.Data[coderv1alpha1.DefaultProvisionerKeySecretKey])) + + f.reconcile(ctx, t, provisioner) + latest := f.get(ctx, t, provisioner) + require.Equal(t, second.KeyID.String(), latest.Status.ProvisionerKeyID, "status must name the key in the Secret") + require.Empty(t, latest.Status.PendingKeys) + + require.NoError(t, k8sClient.Delete(ctx, latest)) + f.reconcile(ctx, t, provisioner) + require.True(t, slices.ContainsFunc(f.bootstrapClient.deleteKeyByIDRequests, func(req coderbootstrap.DeleteProvisionerKeyByIDRequest) bool { + return req.KeyID == second.KeyID + }), "deletion must delete the key in the Secret, got %+v", f.bootstrapClient.deleteKeyByIDRequests) + requireProvisionerGone(ctx, t, name) +} From 27c324ffed114faed9e5c35f4f76a4c41b8e803c Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 15:27:49 +0000 Subject: [PATCH 03/10] fix: rotate a key left unpublished at the target, keep the applied key on by-name deletes, and map an old HTTPS URL to the internal one (#259) Change-Id: I98b10ef7c5fd0e0b25a9022bfb80e6e40fdae5d9 Signed-off-by: Thomas Kosiewski --- api/v1alpha1/coderprovisioner_types.go | 3 +- .../bases/coder.com_coderprovisioners.yaml | 3 +- dist/install.yaml | 3 +- docs/reference/api/coderprovisioner.md | 2 +- docs/tutorials/deploy-coderprovisioner.md | 2 +- internal/coderbootstrap/client.go | 1 + internal/coderbootstrap/provisionerkeys.go | 63 +++++++- .../provisionerkeys_deletebyid_test.go | 82 ++++++++++ .../coderbootstrap/provisionerkeys_test.go | 90 ++++------- .../controller/coderprovisioner_controller.go | 73 ++++----- .../coderprovisioner_controller_test.go | 11 +- .../coderprovisioner_pendingkeys_test.go | 153 ++++++++++++------ .../coderworkspaceproxy_controller_test.go | 6 + 13 files changed, 334 insertions(+), 158 deletions(-) diff --git a/api/v1alpha1/coderprovisioner_types.go b/api/v1alpha1/coderprovisioner_types.go index 90cabe17b..48600f999 100644 --- a/api/v1alpha1/coderprovisioner_types.go +++ b/api/v1alpha1/coderprovisioner_types.go @@ -128,7 +128,8 @@ type CoderProvisionerKeyRecord struct { // KeyName is the provisioner key name in Coder. KeyName string `json:"keyName"` // OrganizationID is the organization ID that Coder returned for the key. - // Without organizationID and keyID, the operator deletes the key by name. + // Without organizationID and keyID, the operator deletes the key by name, + // unless that name holds the key in provisionerKeyID. OrganizationID string `json:"organizationID,omitempty"` // KeyID is the provisioner key ID that Coder returned. KeyID string `json:"keyID,omitempty"` diff --git a/config/crd/bases/coder.com_coderprovisioners.yaml b/config/crd/bases/coder.com_coderprovisioners.yaml index adb36cca9..f79bed4f0 100644 --- a/config/crd/bases/coder.com_coderprovisioners.yaml +++ b/config/crd/bases/coder.com_coderprovisioners.yaml @@ -465,7 +465,8 @@ spec: organizationID: description: |- OrganizationID is the organization ID that Coder returned for the key. - Without organizationID and keyID, the operator deletes the key by name. + Without organizationID and keyID, the operator deletes the key by name, + unless that name holds the key in provisionerKeyID. type: string organizationName: description: OrganizationName is the Coder organization of the diff --git a/dist/install.yaml b/dist/install.yaml index 11ba572bd..6af0e01c4 100644 --- a/dist/install.yaml +++ b/dist/install.yaml @@ -6221,7 +6221,8 @@ spec: organizationID: description: |- OrganizationID is the organization ID that Coder returned for the key. - Without organizationID and keyID, the operator deletes the key by name. + Without organizationID and keyID, the operator deletes the key by name, + unless that name holds the key in provisionerKeyID. type: string organizationName: description: OrganizationName is the Coder organization of the diff --git a/docs/reference/api/coderprovisioner.md b/docs/reference/api/coderprovisioner.md index bb568e61c..fb81ad8ca 100644 --- a/docs/reference/api/coderprovisioner.md +++ b/docs/reference/api/coderprovisioner.md @@ -56,7 +56,7 @@ or was about to create, at one Coder endpoint. | `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. | -| `organizationID` | string | OrganizationID is the organization ID that Coder returned for the key. Without organizationID and keyID, the operator deletes the key by name. | +| `organizationID` | string | OrganizationID is the organization ID that Coder returned for the key. Without organizationID and keyID, the operator deletes the key by name, unless that name holds the key in provisionerKeyID. | | `keyID` | string | KeyID is the provisioner key ID that Coder returned. | ### CoderProvisionerKeySpec diff --git a/docs/tutorials/deploy-coderprovisioner.md b/docs/tutorials/deploy-coderprovisioner.md index 503fb020c..adc1761ac 100644 --- a/docs/tutorials/deploy-coderprovisioner.md +++ b/docs/tutorials/deploy-coderprovisioner.md @@ -78,7 +78,7 @@ kubectl get secret coderprovisioner-sample-provisioner-key -n coder 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 each key in `status.pendingKeys`. These are 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 `keyID` is deleted by name. 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 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 each key in `status.pendingKeys`. These are 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 `keyID` names a key that the operator started to create before Coder returned its ID. The operator deletes the key with that name, unless it is the key in `status.provisionerKeyID`. 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. An entry stays in `status.pendingKeys` until Coder confirms that its key is gone. diff --git a/internal/coderbootstrap/client.go b/internal/coderbootstrap/client.go index 9b99bc0cb..9b27ac739 100644 --- a/internal/coderbootstrap/client.go +++ b/internal/coderbootstrap/client.go @@ -96,6 +96,7 @@ type Client interface { EnsureProvisionerKey(context.Context, EnsureProvisionerKeyRequest) (EnsureProvisionerKeyResponse, error) DeleteProvisionerKey(ctx context.Context, coderURL, sessionToken, orgName, keyName string) error DeleteProvisionerKeyByID(context.Context, DeleteProvisionerKeyByIDRequest) error + DeleteProvisionerKeyByName(context.Context, DeleteProvisionerKeyByNameRequest) error Entitlements(ctx context.Context, coderURL, sessionToken string) (codersdk.Entitlements, error) } diff --git a/internal/coderbootstrap/provisionerkeys.go b/internal/coderbootstrap/provisionerkeys.go index c0e936336..e99b650d6 100644 --- a/internal/coderbootstrap/provisionerkeys.go +++ b/internal/coderbootstrap/provisionerkeys.go @@ -1,6 +1,7 @@ package coderbootstrap import ( + "cmp" "context" "errors" "net/http" @@ -104,7 +105,7 @@ func (c *SDKClient) EnsureProvisionerKey(ctx context.Context, req EnsureProvisio } // DeleteProvisionerKey deletes a provisioner key by name. -// A missing key or organization is treated as success for idempotency. +// 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 @@ -121,9 +122,6 @@ func (c *SDKClient) DeleteProvisionerKey(ctx context.Context, coderURL, sessionT } organization, err := resolveOrganizationByName(ctx, client, organizationName) - if isNotFound(err) { - return nil - } if err != nil { return err } @@ -131,13 +129,68 @@ func (c *SDKClient) DeleteProvisionerKey(ctx context.Context, coderURL, sessionT _, err = withOptionalRateLimitBypass(ctx, func(requestCtx context.Context) (struct{}, error) { return struct{}{}, client.DeleteProvisionerKey(requestCtx, organization.ID, keyName) }) - if err == nil || isNotFound(err) { + 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) } +// DeleteProvisionerKeyByNameRequest identifies a provisioner key by the name +// that the caller recorded before it sent the create, when Coder had not yet +// returned the key ID. +type DeleteProvisionerKeyByNameRequest struct { + CoderURL string + SessionToken string + OrganizationName string + KeyName string + // KeepKeyID is the ID of a key that the delete leaves in place, for + // example the key that the caller uses now. + KeepKeyID uuid.UUID +} + +// DeleteProvisionerKeyByName deletes the provisioner key named req.KeyName in +// the organization req.OrganizationName, unless its ID is req.KeepKeyID. It +// does nothing when no key has that name. +func (c *SDKClient) DeleteProvisionerKeyByName(ctx context.Context, req DeleteProvisionerKeyByNameRequest) error { + if err := validateProvisionerKeyInputs(req.CoderURL, req.SessionToken, req.KeyName); err != nil { + return err + } + + client, err := newAuthenticatedClient(req.CoderURL, req.SessionToken) + if err != nil { + return err + } + + organization, err := resolveOrganizationByName(ctx, client, cmp.Or(req.OrganizationName, codersdk.DefaultOrganization)) + if err != nil { + return err + } + key, err := withOptionalRateLimitBypass(ctx, func(requestCtx context.Context) (*codersdk.ProvisionerKey, error) { + return findOrganizationProvisionerKey(requestCtx, client, organization.ID, req.KeyName) + }) + if err != nil { + return err + } + if key == nil || (req.KeepKeyID != uuid.Nil && key.ID == req.KeepKeyID) { + return nil + } + + _, err = withOptionalRateLimitBypass(ctx, func(requestCtx context.Context) (struct{}, error) { + return struct{}{}, client.DeleteProvisionerKey(requestCtx, organization.ID, key.Name) + }) + if err != nil && !isNotFound(err) { + return xerrors.Errorf("delete provisioner key %q: %w", key.Name, err) + } + + return nil +} + // DeleteProvisionerKeyByIDRequest identifies one provisioner key by the IDs // that the caller recorded when it applied the key. type DeleteProvisionerKeyByIDRequest struct { diff --git a/internal/coderbootstrap/provisionerkeys_deletebyid_test.go b/internal/coderbootstrap/provisionerkeys_deletebyid_test.go index f1f84f6b5..88c6bbd05 100644 --- a/internal/coderbootstrap/provisionerkeys_deletebyid_test.go +++ b/internal/coderbootstrap/provisionerkeys_deletebyid_test.go @@ -14,6 +14,10 @@ import ( "github.com/coder/coder-k8s/internal/coderbootstrap" ) +// coderNotFound is the body of the 404 that Coder answers for a missing +// organization or provisioner key. +var coderNotFound = map[string]any{"message": "Resource not found or you do not have access to this resource"} + func TestDeleteProvisionerKeyByID(t *testing.T) { t.Parallel() @@ -115,3 +119,81 @@ func TestDeleteProvisionerKeyByID_ValidationErrors(t *testing.T) { }) } } + +func TestDeleteProvisionerKeyByName(t *testing.T) { + t.Parallel() + + orgID := uuid.New() + keptKeyID := uuid.New() + now := time.Now().UTC().Format(time.RFC3339) + keysPath := "/api/v2/organizations/" + orgID.String() + "/provisionerkeys" + + tests := []struct { + name string + listedID uuid.UUID + listedName string + deleteBody map[string]any + wantDeletes []string + wantErr bool + }{ + {name: "key exists", listedID: uuid.New(), listedName: "provisioner", wantDeletes: []string{keysPath + "/provisioner"}}, + {name: "no key with the name", listedID: uuid.New(), listedName: "other"}, + {name: "key is the one to keep", listedID: keptKeyID, listedName: "provisioner"}, + {name: "key gone before the delete", listedID: uuid.New(), listedName: "provisioner", deleteBody: coderNotFound, wantDeletes: []string{keysPath + "/provisioner"}}, + { + name: "404 not from Coder", listedID: uuid.New(), listedName: "provisioner", + deleteBody: map[string]any{"message": "Route not found."}, wantDeletes: []string{keysPath + "/provisioner"}, wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + var mu sync.Mutex + var deletes []string + 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}) + case r.Method == http.MethodGet && r.URL.Path == keysPath: + writeJSONResponse(t, w, http.StatusOK, []map[string]any{{ + "id": tt.listedID.String(), + "created_at": now, + "organization": orgID.String(), + "name": tt.listedName, + "tags": map[string]string{}, + }}) + case r.Method == http.MethodDelete: + mu.Lock() + deletes = append(deletes, r.URL.Path) + mu.Unlock() + if tt.deleteBody != nil { + writeJSONResponse(t, w, http.StatusNotFound, tt.deleteBody) + return + } + w.WriteHeader(http.StatusNoContent) + default: + t.Errorf("unexpected request %s %s", r.Method, r.URL.Path) + w.WriteHeader(http.StatusNotFound) + } + })) + defer server.Close() + + err := coderbootstrap.NewSDKClient().DeleteProvisionerKeyByName(context.Background(), coderbootstrap.DeleteProvisionerKeyByNameRequest{ + CoderURL: server.URL, + SessionToken: "session-token", + KeyName: "provisioner", + KeepKeyID: keptKeyID, + }) + if tt.wantErr { + require.Error(t, err, "only Coder's own 404 proves that the key is gone") + } else { + require.NoError(t, err) + } + mu.Lock() + defer mu.Unlock() + require.Equal(t, tt.wantDeletes, deletes) + }) + } +} diff --git a/internal/coderbootstrap/provisionerkeys_test.go b/internal/coderbootstrap/provisionerkeys_test.go index 24b632018..57728940a 100644 --- a/internal/coderbootstrap/provisionerkeys_test.go +++ b/internal/coderbootstrap/provisionerkeys_test.go @@ -5,7 +5,6 @@ import ( "encoding/json" "net/http" "net/http/httptest" - "sync" "sync/atomic" "testing" "time" @@ -446,68 +445,41 @@ func TestDeleteProvisionerKey_Success(t *testing.T) { require.Equal(t, 1, deleteCalls) } -// coderNotFound is the body of the 404 that Coder answers for a missing -// organization or provisioner key. -var coderNotFound = map[string]any{"message": "Resource not found or you do not have access to this resource"} - func TestDeleteProvisionerKey_NotFound(t *testing.T) { t.Parallel() const keyName = "already-deleted" - tests := []struct { - name string - orgGone bool - deleteBody map[string]any - wantDeletes int - wantErr bool - }{ - {name: "key gone", deleteBody: coderNotFound, wantDeletes: 1}, - {name: "organization gone", orgGone: true}, - {name: "404 not from Coder", deleteBody: map[string]any{"message": "Route not found."}, wantDeletes: 1, wantErr: true}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - orgID := uuid.New() - now := time.Now().UTC().Format(time.RFC3339) - var mu sync.Mutex - 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": - if tt.orgGone { - writeJSONResponse(t, w, http.StatusNotFound, coderNotFound) - return - } - writeJSONResponse(t, w, http.StatusOK, map[string]any{ - "id": orgID.String(), - "name": "default", - "created_at": now, - "updated_at": now, - }) - case r.Method == http.MethodDelete && r.URL.Path == "/api/v2/organizations/"+orgID.String()+"/provisionerkeys/"+keyName: - mu.Lock() - deleteCalls++ - mu.Unlock() - writeJSONResponse(t, w, http.StatusNotFound, tt.deleteBody) - default: - writeJSONResponse(t, w, http.StatusNotFound, map[string]any{"message": "unexpected route"}) - } - })) - defer server.Close() - - err := coderbootstrap.NewSDKClient().DeleteProvisionerKey(context.Background(), server.URL, "session-token", "", keyName) - if tt.wantErr { - require.Error(t, err, "only Coder's own 404 proves that the key is gone") - } else { - require.NoError(t, err) - } - mu.Lock() - defer mu.Unlock() - require.Equal(t, tt.wantDeletes, deleteCalls) - }) - } + 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) { diff --git a/internal/controller/coderprovisioner_controller.go b/internal/controller/coderprovisioner_controller.go index 71290596b..7dd65a978 100644 --- a/internal/controller/coderprovisioner_controller.go +++ b/internal/controller/coderprovisioner_controller.go @@ -300,6 +300,14 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req OrganizationName: organizationName, KeyName: keyName, } + // A record in the target slot names a key that an earlier reconcile + // created there and did not publish. The Secret can hold that key while + // the applied fields name another one, or Coder can hold it without any + // plaintext. A rotation replaces it with a key that status records. + targetPending := slices.ContainsFunc(provisioner.Status.PendingKeys, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { + return sameKeySlot(record, target) + }) + rotate := !secretUsable || targetPending pending := slices.Clone(provisioner.Status.PendingKeys) var replaced coderv1alpha1.CoderProvisionerKeyRecord if driftDetected { @@ -311,7 +319,7 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req pending = append(pending, replaced) } } - if driftDetected || !secretUsable || metadataMissing { + if driftDetected || rotate || metadataMissing { pending = slices.DeleteFunc(pending, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { return sameKeySlot(record, target) }) @@ -444,7 +452,7 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req "ProvisionerKeyReady", "Provisioner key is available in coderd", ) - } else if !secretUsable { + } else if rotate { response, ensureErr := r.BootstrapClient.EnsureProvisionerKey(ctx, coderbootstrap.EnsureProvisionerKeyRequest{ CoderURL: coderURL, SessionToken: sessionToken, @@ -474,7 +482,7 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req // 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 secret is missing, rotating key to recover", + 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( @@ -676,16 +684,7 @@ 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. When this reconcile created no key, a target record with - // IDs names a key whose Secret write succeeded but whose status write - // after it failed. - if keyMaterial == "" { - for _, record := range provisioner.Status.PendingKeys { - if _, _, ok := parseKeyIDs(record.OrganizationID, record.KeyID); ok && sameKeySlot(record, target) { - organizationID, provisionerKeyID = record.OrganizationID, record.KeyID - } - } - } + // its record. provisioner.Status.PendingKeys = slices.DeleteFunc(provisioner.Status.PendingKeys, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { return sameKeySlot(record, target) }) @@ -757,9 +756,9 @@ func (r *CoderProvisionerReconciler) reconcileDeletion(ctx context.Context, prov log := ctrl.LoggerFrom(ctx) // Delete only the keys that status records for this resource. A - // provisioner that never applied a key (refused, not entitled, never - // ready) must not delete a Coder key that happens to have the name from - // its spec. + // provisioner that never sent a key create (refused, not entitled, never + // ready) has no record, and must not delete a Coder key that happens to + // have the name from its spec. records := slices.Clone(provisioner.Status.PendingKeys) if _, _, ok := parseKeyIDs(provisioner.Status.OrganizationID, provisioner.Status.ProvisionerKeyID); ok { applied, err := r.appliedKeyRecord(ctx, provisioner, provisioner.Status.OrganizationName, provisioner.Status.ProvisionerKeyName) @@ -805,18 +804,6 @@ func coderAPIURL(controlPlane *coderv1alpha1.CoderControlPlane) (string, error) return coderURL.String(), nil } -// recordedCoderAPIURL maps a status.url that status.controlPlaneURL recorded -// to the URL for Coder API calls. An empty recorded URL, or one equal to the -// control plane's current status.url, maps to currentAPIURL. Any other URL -// names an earlier endpoint and is used as recorded. -func recordedCoderAPIURL(recordedURL string, controlPlane *coderv1alpha1.CoderControlPlane, currentAPIURL string) string { - recordedURL = strings.TrimSpace(recordedURL) - if recordedURL == "" || recordedURL == strings.TrimSpace(controlPlane.Status.URL) { - return currentAPIURL - } - return recordedURL -} - // recordAppliedKeyStatus sets the status fields that describe the key applied // in Coder. func recordAppliedKeyStatus( @@ -833,8 +820,8 @@ func recordAppliedKeyStatus( } // revokeUnpublishedKey deletes a key whose plaintext was never written to a -// Secret. It is best-effort: when it fails, the pending record of the key -// still names it, by name. +// Secret. It is best-effort: when it fails, the pending record without key +// IDs still names the key. func (r *CoderProvisionerReconciler) revokeUnpublishedKey(ctx context.Context, coderURL, sessionToken, organizationID, keyID string) { log := ctrl.LoggerFrom(ctx) orgID, orgErr := uuid.Parse(organizationID) @@ -885,9 +872,9 @@ func sameKeySlot(a, b coderv1alpha1.CoderProvisionerKeyRecord) bool { } // appliedKeyRecord returns a record of the key that the applied status fields -// describe, at the Coder API URL for status.controlPlaneURL. When the control -// plane that applied the key cannot be read, the record keeps -// status.controlPlaneURL. +// describe, at the Coder API URL that status.controlPlaneURL maps to for its +// control plane (see coderAPIURL). When that control plane cannot be read, +// the record keeps status.controlPlaneURL. func (r *CoderProvisionerReconciler) appliedKeyRecord( ctx context.Context, provisioner *coderv1alpha1.CoderProvisioner, @@ -906,12 +893,14 @@ func (r *CoderProvisionerReconciler) appliedKeyRecord( if err != nil || !ok { return record, err } - apiURL, err := coderAPIURL(controlPlane) + applied := controlPlane.DeepCopy() + applied.Status.URL = cmp.Or(record.CoderURL, controlPlane.Status.URL) + apiURL, err := coderAPIURL(applied) if err != nil { ctrl.LoggerFrom(ctx).Info("unable to derive the Coder API URL of the applied provisioner key", "error", err) return record, nil } - record.CoderURL = recordedCoderAPIURL(status.ControlPlaneURL, controlPlane, apiURL) + record.CoderURL = apiURL return record, nil } @@ -932,8 +921,8 @@ func (r *CoderProvisionerReconciler) readControlPlane(ctx context.Context, names // 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 is deleted by name. revoked reports whether Coder confirmed -// that the key is gone. +// without key IDs is deleted by name, unless that name holds the applied key. +// revoked reports whether Coder confirmed that the key is gone. func (r *CoderProvisionerReconciler) revokeKey( ctx context.Context, provisioner *coderv1alpha1.CoderProvisioner, @@ -960,7 +949,15 @@ func (r *CoderProvisionerReconciler) revokeKey( KeyID: keyID, }) } else { - err = r.BootstrapClient.DeleteProvisionerKey(ctx, record.CoderURL, sessionToken, record.OrganizationName, record.KeyName) + // Keep the applied key: another URL can reach the same Coder. + _, appliedKeyID, _ := parseKeyIDs(provisioner.Status.OrganizationID, provisioner.Status.ProvisionerKeyID) + err = r.BootstrapClient.DeleteProvisionerKeyByName(ctx, coderbootstrap.DeleteProvisionerKeyByNameRequest{ + CoderURL: record.CoderURL, + SessionToken: sessionToken, + OrganizationName: record.OrganizationName, + KeyName: record.KeyName, + KeepKeyID: appliedKeyID, + }) } if err != nil { log.Info("failed to delete the recorded provisioner key, keeping its record", "error", err) diff --git a/internal/controller/coderprovisioner_controller_test.go b/internal/controller/coderprovisioner_controller_test.go index 9387e6164..cc5e1dc9e 100644 --- a/internal/controller/coderprovisioner_controller_test.go +++ b/internal/controller/coderprovisioner_controller_test.go @@ -876,10 +876,11 @@ func TestCoderProvisionerReconciler_KeyNameDrift(t *testing.T) { _ = k8sClient.Delete(context.Background(), provisioner) }) + keyV1 := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: uuid.New(), KeyID: uuid.New(), KeyName: "key-v1", Key: "key-v1-material"} bootstrapClient := &fakeBootstrapClient{ provisionerKeyResponses: []coderbootstrap.EnsureProvisionerKeyResponse{ - {KeyName: "key-v1", Key: "key-v1-material"}, - {KeyName: "key-v2", Key: "key-v2-material"}, + keyV1, + {OrganizationID: keyV1.OrganizationID, KeyID: uuid.New(), KeyName: "key-v2", Key: "key-v2-material"}, }, } reconciler := &controller.CoderProvisionerReconciler{Client: k8sClient, Scheme: scheme, BootstrapClient: bootstrapClient} @@ -895,10 +896,8 @@ func TestCoderProvisionerReconciler_KeyNameDrift(t *testing.T) { reconcileProvisioner(ctx, t, reconciler, request) - require.GreaterOrEqual(t, bootstrapClient.deleteKeyCalls, 1) - require.NotEmpty(t, bootstrapClient.deleteKeyRequests) - lastDelete := bootstrapClient.deleteKeyRequests[len(bootstrapClient.deleteKeyRequests)-1] - require.Equal(t, "key-v1", lastDelete.KeyName) + require.Len(t, bootstrapClient.deleteKeyByIDRequests, 1) + require.Equal(t, keyV1.KeyID, bootstrapClient.deleteKeyByIDRequests[0].KeyID, "the key named key-v1 must be deleted") require.GreaterOrEqual(t, len(bootstrapClient.provisionerKeyRequests), 2) lastEnsure := bootstrapClient.provisionerKeyRequests[len(bootstrapClient.provisionerKeyRequests)-1] diff --git a/internal/controller/coderprovisioner_pendingkeys_test.go b/internal/controller/coderprovisioner_pendingkeys_test.go index 78d82e783..d4799ae6f 100644 --- a/internal/controller/coderprovisioner_pendingkeys_test.go +++ b/internal/controller/coderprovisioner_pendingkeys_test.go @@ -65,6 +65,9 @@ func keyDeletes(client *fakeBootstrapClient) []keyDelete { for _, req := range client.deleteKeyRequests { deletes = append(deletes, keyDelete{coderURL: req.CoderURL, token: req.SessionToken, keyName: req.KeyName}) } + for _, req := range client.deleteKeyByNameRequests { + 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()}) } @@ -96,11 +99,15 @@ func TestCoderProvisionerKey_IntermediateEndpointRevoked(t *testing.T) { statusWriteFails bool // deleteAtC deletes the provisioner instead of reconciling at C. deleteAtC bool + // revokeFailsOnce fails the first delete of key B at C. The record + // must stay until a later reconcile deletes the key. + revokeFailsOnce bool }{ {name: "secret write fails at B, rotation at C"}, {name: "secret write fails at B, deletion at C", deleteAtC: true}, {name: "key B unrecorded by ID, rotation at C", statusWriteFails: true}, {name: "key B unrecorded by ID, deletion at C", statusWriteFails: true, deleteAtC: true}, + {name: "revoke of key B fails once, rotation at C", revokeFailsOnce: true}, } for _, tt := range tests { @@ -176,7 +183,15 @@ func TestCoderProvisionerKey_IntermediateEndpointRevoked(t *testing.T) { if tt.deleteAtC { require.NoError(t, k8sClient.Delete(ctx, f.get(ctx, t, provisioner))) } + if tt.revokeFailsOnce { + client.failRevoke = keyB.KeyID + } f.reconcile(ctx, t, provisioner) + if tt.revokeFailsOnce { + require.True(t, pendingKeyRecorded(f.get(ctx, t, provisioner), keyB.KeyID), "a failed delete must keep the record of key B") + client.failRevoke = uuid.Nil + f.reconcile(ctx, t, provisioner) + } deletes := keyDeletes(f.bootstrapClient) revokedAtB := slices.ContainsFunc(deletes, func(d keyDelete) bool { @@ -184,6 +199,8 @@ func TestCoderProvisionerKey_IntermediateEndpointRevoked(t *testing.T) { (d.keyID == keyB.KeyID.String() || (d.keyID == "" && d.keyName == keyB.KeyName)) }) require.True(t, revokedAtB, "key B must be deleted at B with the operator token of B, got %+v", deletes) + require.Contains(t, deletes, keyDelete{coderURL: apiURL("coder"), token: testOperatorToken, keyID: keyA.KeyID.String()}, + "key A must be deleted at A with the operator token of A") for _, d := range deletes { want, known := tokenAt[d.coderURL] require.True(t, known, "delete sent to %s, which no record names", d.coderURL) @@ -194,6 +211,8 @@ func TestCoderProvisionerKey_IntermediateEndpointRevoked(t *testing.T) { requireProvisionerGone(ctx, t, name) return } + require.False(t, slices.ContainsFunc(deletes, func(d keyDelete) bool { return d.coderURL == apiURL("coder-c") }), + "nothing may delete key C, which the Secret holds") final := f.get(ctx, t, provisioner) require.Equal(t, keyC.KeyID.String(), final.Status.ProvisionerKeyID) require.Equal(t, "https://coder-c.example.com", final.Status.ControlPlaneURL) @@ -202,51 +221,95 @@ func TestCoderProvisionerKey_IntermediateEndpointRevoked(t *testing.T) { } } -// TestCoderProvisionerKey_FailedStatusWriteAfterSecretKeepsKey checks a -// rotation whose Secret write succeeds while the status write after it fails. -// The next reconcile must record the key in the Secret as the applied key, so -// that deletion still deletes it. -func TestCoderProvisionerKey_FailedStatusWriteAfterSecretKeepsKey(t *testing.T) { +// TestCoderProvisionerKey_UnpublishedKeyIsRotated checks keys that an +// 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. +func TestCoderProvisionerKey_UnpublishedKeyIsRotated(t *testing.T) { t.Parallel() - ctx := context.Background() - f := newProvisionerCredentialFixture(ctx, t, "cprov-adopt") - first := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: uuid.New(), KeyID: uuid.New(), KeyName: "provisioner", Key: "material-1"} - second := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: uuid.New(), KeyID: uuid.New(), KeyName: "provisioner", Key: "material-2"} - f.bootstrapClient.provisionerKeyResponses = []coderbootstrap.EnsureProvisionerKeyResponse{first, second} - 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) - - keySecret := types.NamespacedName{Name: provisioner.Name + "-provisioner-key", Namespace: provisioner.Namespace} - require.NoError(t, k8sClient.Delete(ctx, &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: keySecret.Name, Namespace: keySecret.Namespace}})) - failStatusWrite := true - base, err := ctrlclient.NewWithWatch(cfg, ctrlclient.Options{Scheme: scheme}) - require.NoError(t, err) - f.reconciler.Client = interceptor.NewClient(base, interceptor.Funcs{ - SubResourceUpdate: func(ctx context.Context, c ctrlclient.Client, sub string, obj ctrlclient.Object, opts ...ctrlclient.SubResourceUpdateOption) error { - if p, ok := obj.(*coderv1alpha1.CoderProvisioner); ok && failStatusWrite && p.Status.ProvisionerKeyID == second.KeyID.String() { - failStatusWrite = false - return apierrors.NewServiceUnavailable("injected status write failure") - } - return c.SubResource(sub).Update(ctx, obj, opts...) + + tests := []struct { + 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) + }{ + { + // 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", + 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"} + require.NoError(t, k8sClient.Update(ctx, latest)) + f.bootstrapClient.provisionerKeyErr = errors.New("injected lost create answer") + _, err := f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{Name: provisioner.Name, Namespace: provisioner.Namespace}}) + require.Error(t, err) + f.bootstrapClient.provisionerKeyErr = nil + latest = f.get(ctx, t, provisioner) + latest.Spec.Tags = nil + require.NoError(t, k8sClient.Update(ctx, latest)) + }, }, - }) - _, err = f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: name}) - require.Error(t, err, "the injected status write failure must fail the rotation") - secret := &corev1.Secret{} - require.NoError(t, k8sClient.Get(ctx, keySecret, secret)) - require.Equal(t, "material-2", string(secret.Data[coderv1alpha1.DefaultProvisionerKeySecretKey])) - - f.reconcile(ctx, t, provisioner) - latest := f.get(ctx, t, provisioner) - require.Equal(t, second.KeyID.String(), latest.Status.ProvisionerKeyID, "status must name the key in the Secret") - require.Empty(t, latest.Status.PendingKeys) - - require.NoError(t, k8sClient.Delete(ctx, latest)) - f.reconcile(ctx, t, provisioner) - require.True(t, slices.ContainsFunc(f.bootstrapClient.deleteKeyByIDRequests, func(req coderbootstrap.DeleteProvisionerKeyByIDRequest) bool { - return req.KeyID == second.KeyID - }), "deletion must delete the key in the Secret, got %+v", f.bootstrapClient.deleteKeyByIDRequests) - requireProvisionerGone(ctx, t, name) + { + // The Secret holds the new key, but the status write after it fails. + name: "status write after the Secret write fails", + fail: func(ctx context.Context, t *testing.T, f provisionerCredentialFixture, provisioner *coderv1alpha1.CoderProvisioner, unpublished coderbootstrap.EnsureProvisionerKeyResponse) { + require.NoError(t, k8sClient.Delete(ctx, &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: provisioner.Name + "-provisioner-key", Namespace: provisioner.Namespace}})) + base, err := ctrlclient.NewWithWatch(cfg, ctrlclient.Options{Scheme: scheme}) + require.NoError(t, err) + failStatusWrite := true + f.reconciler.Client = interceptor.NewClient(base, interceptor.Funcs{ + SubResourceUpdate: func(ctx context.Context, c ctrlclient.Client, sub string, obj ctrlclient.Object, opts ...ctrlclient.SubResourceUpdateOption) error { + if p, ok := obj.(*coderv1alpha1.CoderProvisioner); ok && failStatusWrite && p.Status.ProvisionerKeyID == unpublished.KeyID.String() { + failStatusWrite = false + return apierrors.NewServiceUnavailable("injected status write failure") + } + return c.SubResource(sub).Update(ctx, obj, opts...) + }, + }) + _, err = f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{Name: provisioner.Name, Namespace: provisioner.Namespace}}) + require.Error(t, err) + f.reconciler.Client = k8sClient + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + 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} + provisioner := f.createProvisioner(ctx, t, nil, nil) + f.reconcile(ctx, t, provisioner) + f.reconcile(ctx, t, provisioner) + + tt.fail(ctx, t, f, provisioner, unpublished) + deletesBefore := len(f.bootstrapClient.deleteKeyRequests) + f.reconcile(ctx, t, provisioner) + + latest := f.get(ctx, t, provisioner) + require.Equal(t, final.KeyID.String(), latest.Status.ProvisionerKeyID, "status must name the key in the Secret") + require.Empty(t, latest.Status.PendingKeys) + 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") + + require.NoError(t, k8sClient.Delete(ctx, latest)) + f.reconcile(ctx, t, provisioner) + require.True(t, slices.ContainsFunc(f.bootstrapClient.deleteKeyByIDRequests, func(req coderbootstrap.DeleteProvisionerKeyByIDRequest) bool { + return req.KeyID == final.KeyID + }), "deletion must delete the key in the Secret, got %+v", f.bootstrapClient.deleteKeyByIDRequests) + requireProvisionerGone(ctx, t, types.NamespacedName{Name: provisioner.Name, Namespace: provisioner.Namespace}) + }) + } } diff --git a/internal/controller/coderworkspaceproxy_controller_test.go b/internal/controller/coderworkspaceproxy_controller_test.go index 66b03f112..dfb4da071 100644 --- a/internal/controller/coderworkspaceproxy_controller_test.go +++ b/internal/controller/coderworkspaceproxy_controller_test.go @@ -34,6 +34,7 @@ type fakeBootstrapClient struct { deleteKeyRequests []deleteKeyRequest deleteKeyByIDErr error deleteKeyByIDRequests []coderbootstrap.DeleteProvisionerKeyByIDRequest + deleteKeyByNameRequests []coderbootstrap.DeleteProvisionerKeyByNameRequest // Entitlements support. entitlementsResponse codersdk.Entitlements @@ -93,6 +94,11 @@ func (f *fakeBootstrapClient) DeleteProvisionerKeyByID(_ context.Context, req co return f.deleteKeyByIDErr } +func (f *fakeBootstrapClient) DeleteProvisionerKeyByName(_ context.Context, req coderbootstrap.DeleteProvisionerKeyByNameRequest) error { + f.deleteKeyByNameRequests = append(f.deleteKeyByNameRequests, req) + return nil +} + func (f *fakeBootstrapClient) Entitlements(_ context.Context, coderURL, sessionToken string) (codersdk.Entitlements, error) { f.entitlementsCalls++ f.entitlementsRequests = append(f.entitlementsRequests, entitlementsRequest{ From 54c982fed324209d75d91ee2b601e45e3f53139b Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 15:42:45 +0000 Subject: [PATCH 04/10] fix: keep the IDs of an unpublished key until a create proves it gone, retry unconfirmed deletes, and count a deleted organization as gone (#259) Change-Id: I1b838ed30fd0452da76e6649fc2d2146e3f60143 Signed-off-by: Thomas Kosiewski --- docs/tutorials/deploy-coderprovisioner.md | 2 +- internal/coderbootstrap/provisionerkeys.go | 5 +- .../provisionerkeys_deletebyid_test.go | 4 ++ .../controller/coderprovisioner_controller.go | 31 +++++++++---- .../coderprovisioner_pendingkeys_test.go | 46 +++++++++++++++++-- 5 files changed, 74 insertions(+), 14 deletions(-) diff --git a/docs/tutorials/deploy-coderprovisioner.md b/docs/tutorials/deploy-coderprovisioner.md index adc1761ac..c5c284a67 100644 --- a/docs/tutorials/deploy-coderprovisioner.md +++ b/docs/tutorials/deploy-coderprovisioner.md @@ -80,7 +80,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 each key in `status.pendingKeys`. These are 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 `keyID` names a key that the operator started to create before Coder returned its ID. The operator deletes the key with that name, unless it is the key in `status.provisionerKeyID`. 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. An entry stays in `status.pendingKeys` until Coder confirms that its key is gone. +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. 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"}]'`. 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/provisionerkeys.go b/internal/coderbootstrap/provisionerkeys.go index e99b650d6..7c78d9661 100644 --- a/internal/coderbootstrap/provisionerkeys.go +++ b/internal/coderbootstrap/provisionerkeys.go @@ -156,7 +156,7 @@ type DeleteProvisionerKeyByNameRequest struct { // DeleteProvisionerKeyByName deletes the provisioner key named req.KeyName in // the organization req.OrganizationName, unless its ID is req.KeepKeyID. It -// does nothing when no key has that name. +// does nothing when the organization or a key with that name does not exist. func (c *SDKClient) DeleteProvisionerKeyByName(ctx context.Context, req DeleteProvisionerKeyByNameRequest) error { if err := validateProvisionerKeyInputs(req.CoderURL, req.SessionToken, req.KeyName); err != nil { return err @@ -168,6 +168,9 @@ func (c *SDKClient) DeleteProvisionerKeyByName(ctx context.Context, req DeletePr } organization, err := resolveOrganizationByName(ctx, client, cmp.Or(req.OrganizationName, codersdk.DefaultOrganization)) + if isNotFound(err) { + return nil + } if err != nil { return err } diff --git a/internal/coderbootstrap/provisionerkeys_deletebyid_test.go b/internal/coderbootstrap/provisionerkeys_deletebyid_test.go index 88c6bbd05..bc3311afc 100644 --- a/internal/coderbootstrap/provisionerkeys_deletebyid_test.go +++ b/internal/coderbootstrap/provisionerkeys_deletebyid_test.go @@ -130,12 +130,14 @@ func TestDeleteProvisionerKeyByName(t *testing.T) { tests := []struct { name string + orgGone bool listedID uuid.UUID listedName string deleteBody map[string]any wantDeletes []string wantErr bool }{ + {name: "organization gone", orgGone: true}, {name: "key exists", listedID: uuid.New(), listedName: "provisioner", wantDeletes: []string{keysPath + "/provisioner"}}, {name: "no key with the name", listedID: uuid.New(), listedName: "other"}, {name: "key is the one to keep", listedID: keptKeyID, listedName: "provisioner"}, @@ -154,6 +156,8 @@ func TestDeleteProvisionerKeyByName(t *testing.T) { var deletes []string 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" && tt.orgGone: + writeJSONResponse(t, w, http.StatusNotFound, coderNotFound) 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}) case r.Method == http.MethodGet && r.URL.Path == keysPath: diff --git a/internal/controller/coderprovisioner_controller.go b/internal/controller/coderprovisioner_controller.go index 7dd65a978..89c13ce23 100644 --- a/internal/controller/coderprovisioner_controller.go +++ b/internal/controller/coderprovisioner_controller.go @@ -56,6 +56,7 @@ const ( externalProvisionerEntitlementRetryInterval = 2 * time.Minute provisionerOperatorAccessRetryInterval = 15 * time.Second + provisionerPendingKeyRetryInterval = 2 * time.Minute // #nosec G101 -- this is a field index key, not a credential. provisionerControlPlaneRefNameFieldIndex = ".spec.controlPlaneRef.name" @@ -320,10 +321,22 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req } } if driftDetected || rotate || metadataMissing { - pending = slices.DeleteFunc(pending, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { - return sameKeySlot(record, target) - }) - pending = append(pending, target) + // 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. + newest := -1 + for i, record := range pending { + if sameKeySlot(record, target) && record.KeyID != "" { + newest = i + } + } + var next []coderv1alpha1.CoderProvisionerKeyRecord + for i, record := range pending { + if !sameKeySlot(record, target) || i == newest { + next = append(next, record) + } + } + pending = append(next, target) } var kept []coderv1alpha1.CoderProvisionerKeyRecord for _, record := range pending { @@ -633,9 +646,7 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req // rotated again on the next try. A failed status write (for example a // conflict because the provisioner is being deleted) revokes the new // key before anyone can read it. - i := slices.IndexFunc(provisioner.Status.PendingKeys, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { - return sameKeySlot(record, target) - }) + i := slices.Index(provisioner.Status.PendingKeys, target) if i < 0 { return ctrl.Result{}, fmt.Errorf("assertion failed: status has no pending record of the new provisioner key %q", keyName) } @@ -684,7 +695,8 @@ 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. + // its record. Coder allows one key per name, so the create also proved + // that the keys of the other target records are gone. provisioner.Status.PendingKeys = slices.DeleteFunc(provisioner.Status.PendingKeys, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { return sameKeySlot(record, target) }) @@ -745,6 +757,9 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req return ctrl.Result{}, err } + if len(provisioner.Status.PendingKeys) > 0 { + return ctrl.Result{RequeueAfter: provisionerPendingKeyRetryInterval}, nil + } return ctrl.Result{}, nil } diff --git a/internal/controller/coderprovisioner_pendingkeys_test.go b/internal/controller/coderprovisioner_pendingkeys_test.go index d4799ae6f..021459e03 100644 --- a/internal/controller/coderprovisioner_pendingkeys_test.go +++ b/internal/controller/coderprovisioner_pendingkeys_test.go @@ -185,13 +185,13 @@ func TestCoderProvisionerKey_IntermediateEndpointRevoked(t *testing.T) { } if tt.revokeFailsOnce { client.failRevoke = keyB.KeyID - } - f.reconcile(ctx, t, provisioner) - if tt.revokeFailsOnce { + result, err := f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: name}) + require.NoError(t, err) + require.Positive(t, result.RequeueAfter, "a failed delete must schedule another attempt") require.True(t, pendingKeyRecorded(f.get(ctx, t, provisioner), keyB.KeyID), "a failed delete must keep the record of key B") client.failRevoke = uuid.Nil - f.reconcile(ctx, t, provisioner) } + f.reconcile(ctx, t, provisioner) deletes := keyDeletes(f.bootstrapClient) revokedAtB := slices.ContainsFunc(deletes, func(d keyDelete) bool { @@ -313,3 +313,41 @@ func TestCoderProvisionerKey_UnpublishedKeyIsRotated(t *testing.T) { }) } } + +// TestCoderProvisionerKey_FailedRotationKeepsKeyIDs checks that a rotation +// that fails before its create keeps the IDs of the unpublished key at the +// target, so that a later delete still finds that key by ID. +func TestCoderProvisionerKey_FailedRotationKeepsKeyIDs(t *testing.T) { + t.Parallel() + ctx := context.Background() + f := newProvisionerCredentialFixture(ctx, t, "cprov-keep-ids") + 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"} + f.bootstrapClient.provisionerKeyResponses = []coderbootstrap.EnsureProvisionerKeyResponse{first, unpublished} + 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) + + keySecretName := provisioner.Name + "-provisioner-key" + require.NoError(t, k8sClient.Delete(ctx, &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: keySecretName, Namespace: provisioner.Namespace}})) + base, err := ctrlclient.NewWithWatch(cfg, ctrlclient.Options{Scheme: scheme}) + require.NoError(t, err) + f.reconciler.Client = interceptor.NewClient(base, interceptor.Funcs{ + Create: func(ctx context.Context, c ctrlclient.WithWatch, obj ctrlclient.Object, opts ...ctrlclient.CreateOption) error { + if s, ok := obj.(*corev1.Secret); ok && s.Name == keySecretName { + return apierrors.NewServiceUnavailable("injected Secret write failure") + } + return c.Create(ctx, obj, opts...) + }, + }) + _, err = f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: name}) + require.Error(t, err, "the injected Secret write failure must fail the rotation") + require.True(t, pendingKeyRecorded(f.get(ctx, t, provisioner), unpublished.KeyID)) + + f.bootstrapClient.provisionerKeyErr = errors.New("injected Coder failure") + _, err = f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: name}) + require.Error(t, err) + require.True(t, pendingKeyRecorded(f.get(ctx, t, provisioner), unpublished.KeyID), + "a failed rotation must keep the IDs of the unpublished key") +} From 77c3441fbc4c3928564e8b43658cc9fe0255dee4 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 15:51:36 +0000 Subject: [PATCH 05/10] fix: count Coder's 404 on the key list as gone in the by-name delete (#259) Change-Id: If242534c4c40750c96650d00ada817dc5042c736 Signed-off-by: Thomas Kosiewski --- internal/coderbootstrap/provisionerkeys.go | 3 +++ internal/coderbootstrap/provisionerkeys_deletebyid_test.go | 4 ++++ 2 files changed, 7 insertions(+) diff --git a/internal/coderbootstrap/provisionerkeys.go b/internal/coderbootstrap/provisionerkeys.go index 7c78d9661..07dfcd398 100644 --- a/internal/coderbootstrap/provisionerkeys.go +++ b/internal/coderbootstrap/provisionerkeys.go @@ -177,6 +177,9 @@ func (c *SDKClient) DeleteProvisionerKeyByName(ctx context.Context, req DeletePr key, err := withOptionalRateLimitBypass(ctx, func(requestCtx context.Context) (*codersdk.ProvisionerKey, error) { return findOrganizationProvisionerKey(requestCtx, client, organization.ID, req.KeyName) }) + if isNotFound(err) { + return nil + } if err != nil { return err } diff --git a/internal/coderbootstrap/provisionerkeys_deletebyid_test.go b/internal/coderbootstrap/provisionerkeys_deletebyid_test.go index bc3311afc..7abaedf74 100644 --- a/internal/coderbootstrap/provisionerkeys_deletebyid_test.go +++ b/internal/coderbootstrap/provisionerkeys_deletebyid_test.go @@ -131,6 +131,7 @@ func TestDeleteProvisionerKeyByName(t *testing.T) { tests := []struct { name string orgGone bool + listGone bool listedID uuid.UUID listedName string deleteBody map[string]any @@ -138,6 +139,7 @@ func TestDeleteProvisionerKeyByName(t *testing.T) { wantErr bool }{ {name: "organization gone", orgGone: true}, + {name: "organization gone before the list", listGone: true}, {name: "key exists", listedID: uuid.New(), listedName: "provisioner", wantDeletes: []string{keysPath + "/provisioner"}}, {name: "no key with the name", listedID: uuid.New(), listedName: "other"}, {name: "key is the one to keep", listedID: keptKeyID, listedName: "provisioner"}, @@ -160,6 +162,8 @@ func TestDeleteProvisionerKeyByName(t *testing.T) { writeJSONResponse(t, w, http.StatusNotFound, coderNotFound) 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}) + case r.Method == http.MethodGet && r.URL.Path == keysPath && tt.listGone: + writeJSONResponse(t, w, http.StatusNotFound, coderNotFound) case r.Method == http.MethodGet && r.URL.Path == keysPath: writeJSONResponse(t, w, http.StatusOK, []map[string]any{{ "id": tt.listedID.String(), From b25997ec3628d42a497a07c2248816c73245a08a Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 16:05:32 +0000 Subject: [PATCH 06/10] fix: replace a Secret key that a failed rotation left unpublished, and keep the current-URL fallback for older status (#259) Change-Id: I28210e78dede73870e4cfdc4e73afed26558139b Signed-off-by: Thomas Kosiewski --- api/v1alpha1/coderprovisioner_types.go | 5 ++ .../bases/coder.com_coderprovisioners.yaml | 7 ++ dist/install.yaml | 7 ++ docs/reference/api/coderprovisioner.md | 1 + docs/tutorials/deploy-coderprovisioner.md | 2 +- .../controller/coderprovisioner_controller.go | 56 +++++++++++--- .../coderprovisioner_deletion_test.go | 37 ++++++++++ .../coderprovisioner_pendingkeys_test.go | 74 +++++++++++++++++++ 8 files changed, 179 insertions(+), 10 deletions(-) diff --git a/api/v1alpha1/coderprovisioner_types.go b/api/v1alpha1/coderprovisioner_types.go index 48600f999..7302ca097 100644 --- a/api/v1alpha1/coderprovisioner_types.go +++ b/api/v1alpha1/coderprovisioner_types.go @@ -133,6 +133,11 @@ type CoderProvisionerKeyRecord struct { OrganizationID string `json:"organizationID,omitempty"` // KeyID is the provisioner key ID that Coder returned. KeyID string `json:"keyID,omitempty"` + // Unpublished is true from before the create until the operator records + // a key as applied. While it is true, the Secret can hold this key, or a + // key that a rotation deleted, so the operator replaces the key in the + // Secret. + Unpublished bool `json:"unpublished,omitempty"` } // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object diff --git a/config/crd/bases/coder.com_coderprovisioners.yaml b/config/crd/bases/coder.com_coderprovisioners.yaml index f79bed4f0..c2c42e854 100644 --- a/config/crd/bases/coder.com_coderprovisioners.yaml +++ b/config/crd/bases/coder.com_coderprovisioners.yaml @@ -472,6 +472,13 @@ spec: description: OrganizationName is the Coder organization of the key. type: string + unpublished: + description: |- + Unpublished is true from before the create until the operator records + a key as applied. While it is true, the Secret can hold this key, or a + key that a rotation deleted, so the operator replaces the key in the + Secret. + type: boolean required: - coderURL - controlPlaneRefName diff --git a/dist/install.yaml b/dist/install.yaml index 6af0e01c4..ac1a59e76 100644 --- a/dist/install.yaml +++ b/dist/install.yaml @@ -6228,6 +6228,13 @@ spec: description: OrganizationName is the Coder organization of the key. type: string + unpublished: + description: |- + Unpublished is true from before the create until the operator records + a key as applied. While it is true, the Secret can hold this key, or a + key that a rotation deleted, so the operator replaces the key in the + Secret. + type: boolean required: - coderURL - controlPlaneRefName diff --git a/docs/reference/api/coderprovisioner.md b/docs/reference/api/coderprovisioner.md index fb81ad8ca..41b059b17 100644 --- a/docs/reference/api/coderprovisioner.md +++ b/docs/reference/api/coderprovisioner.md @@ -58,6 +58,7 @@ or was about to create, at one Coder endpoint. | `keyName` | string | KeyName is the provisioner key name in Coder. | | `organizationID` | string | OrganizationID is the organization ID that Coder returned for the key. Without organizationID and keyID, the operator deletes the key by name, unless that name holds the key in provisionerKeyID. | | `keyID` | string | KeyID is the provisioner key ID that Coder returned. | +| `unpublished` | boolean | Unpublished is true from before the create until the operator records a key as applied. While it is true, the Secret can hold this key, or a key that a rotation deleted, so the operator replaces the key in the Secret. | ### CoderProvisionerKeySpec diff --git a/docs/tutorials/deploy-coderprovisioner.md b/docs/tutorials/deploy-coderprovisioner.md index c5c284a67..ef79271e4 100644 --- a/docs/tutorials/deploy-coderprovisioner.md +++ b/docs/tutorials/deploy-coderprovisioner.md @@ -80,7 +80,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 each key in `status.pendingKeys`. These are 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 `keyID` names a key that the operator started to create before Coder returned its ID. The operator deletes the key with that name, unless it is the key in `status.provisionerKeyID`. 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. 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. 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"}]'`. 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 89c13ce23..968d47b83 100644 --- a/internal/controller/coderprovisioner_controller.go +++ b/internal/controller/coderprovisioner_controller.go @@ -300,15 +300,15 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req CoderURL: coderURL, OrganizationName: organizationName, KeyName: keyName, - } - // A record in the target slot names a key that an earlier reconcile - // created there and did not publish. The Secret can hold that key while - // the applied fields name another one, or Coder can hold it without any - // plaintext. A rotation replaces it with a key that status records. - targetPending := slices.ContainsFunc(provisioner.Status.PendingKeys, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { - return sameKeySlot(record, target) + Unpublished: true, + } + // An unpublished record names a key that an earlier reconcile created, or + // started to create, for the Secret and did not record as applied. The + // Secret can hold that key, or a key that the rotation already deleted, + // so a rotation replaces it with a key that status records. + rotate := !secretUsable || slices.ContainsFunc(provisioner.Status.PendingKeys, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { + return record.Unpublished }) - rotate := !secretUsable || targetPending pending := slices.Clone(provisioner.Status.PendingKeys) var replaced coderv1alpha1.CoderProvisionerKeyRecord if driftDetected { @@ -696,10 +696,14 @@ 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. + // that the keys of the other target records are gone. 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) }) + for i := range provisioner.Status.PendingKeys { + provisioner.Status.PendingKeys[i].Unpublished = false + } recordAppliedKeyStatus(provisioner, organizationID, appliedOrgName, provisionerKeyID, provisionerKeyName, appliedTagsHash, appliedControlPlaneRefName, appliedControlPlaneURL) provisioner.Status.SecretRef = &coderv1alpha1.SecretKeySelector{Name: keySecretName, Key: keySecretKey} @@ -781,6 +785,17 @@ func (r *CoderProvisionerReconciler) reconcileDeletion(ctx context.Context, prov return ctrl.Result{}, err } records = append(records, applied) + // Before status.pendingKeys, the operator wrote the IDs of a new key + // before status.controlPlaneURL, so status from an older operator + // can name a key at the current endpoint under the old URL. The + // delete matches the key ID only. + current, err := r.currentEndpointRecord(ctx, provisioner, applied) + if err != nil { + return ctrl.Result{}, err + } + if current.CoderURL != applied.CoderURL { + records = append(records, current) + } } if len(records) == 0 { log.Info("status records no Coder provisioner key, removing the finalizer without a Coder call") @@ -919,6 +934,29 @@ func (r *CoderProvisionerReconciler) appliedKeyRecord( return record, nil } +// currentEndpointRecord returns applied at the current Coder API URL of the +// control plane that spec.controlPlaneRef names, with that control plane's +// operator token. It returns applied when that control plane cannot be read. +func (r *CoderProvisionerReconciler) currentEndpointRecord( + ctx context.Context, + provisioner *coderv1alpha1.CoderProvisioner, + applied coderv1alpha1.CoderProvisionerKeyRecord, +) (coderv1alpha1.CoderProvisionerKeyRecord, error) { + controlPlane, ok, err := r.readControlPlane(ctx, provisioner.Namespace, provisioner.Spec.ControlPlaneRef.Name) + if err != nil || !ok { + return applied, err + } + apiURL, urlErr := coderAPIURL(controlPlane) + if urlErr != nil { + ctrl.LoggerFrom(ctx).Info("unable to derive the current Coder API URL during deletion", "error", urlErr) + return applied, nil + } + current := applied + current.ControlPlaneRefName = controlPlane.Name + current.CoderURL = apiURL + return current, nil +} + // readControlPlane reads a control plane in namespace. ok is false when it // cannot be read. func (r *CoderProvisionerReconciler) readControlPlane(ctx context.Context, namespace, name string) (*coderv1alpha1.CoderControlPlane, bool, error) { diff --git a/internal/controller/coderprovisioner_deletion_test.go b/internal/controller/coderprovisioner_deletion_test.go index 5465fb80a..473ded997 100644 --- a/internal/controller/coderprovisioner_deletion_test.go +++ b/internal/controller/coderprovisioner_deletion_test.go @@ -244,3 +244,40 @@ func pendingKeyRecorded(provisioner *coderv1alpha1.CoderProvisioner, keyID uuid. return record.KeyID == keyID.String() }) } + +// TestCoderProvisionerDeletion_TriesCurrentControlPlaneURL checks status that +// an older operator wrote during a control plane URL change: the new key's +// IDs under the old status.controlPlaneURL. Deletion must also try the +// current URL. +func TestCoderProvisionerDeletion_TriesCurrentControlPlaneURL(t *testing.T) { + t.Parallel() + ctx := context.Background() + f := newProvisionerCredentialFixture(ctx, t, "cprov-delete-urldrift") + const oldURL, newURL = "http://coder-old.example.com", "http://coder-new.example.com" + setControlPlaneURL(ctx, t, f, oldURL) + provisioner := f.createProvisioner(ctx, t, nil, nil) + f.reconcile(ctx, t, provisioner) + f.reconcile(ctx, t, provisioner) + + newKey := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: uuid.New(), KeyID: uuid.New()} + latest := f.get(ctx, t, provisioner) + require.Equal(t, oldURL, latest.Status.ControlPlaneURL) + latest.Status.OrganizationID = newKey.OrganizationID.String() + latest.Status.ProvisionerKeyID = newKey.KeyID.String() + require.NoError(t, k8sClient.Status().Update(ctx, latest)) + setControlPlaneURL(ctx, t, f, newURL) + + require.NoError(t, k8sClient.Delete(ctx, f.get(ctx, t, provisioner))) + f.reconcile(ctx, t, provisioner) + + require.Len(t, f.bootstrapClient.deleteKeyByIDRequests, 2) + require.Equal(t, oldURL, f.bootstrapClient.deleteKeyByIDRequests[0].CoderURL) + require.Equal(t, newURL, f.bootstrapClient.deleteKeyByIDRequests[1].CoderURL) + for _, req := range f.bootstrapClient.deleteKeyByIDRequests { + require.Equal(t, newKey.OrganizationID, req.OrganizationID) + 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_pendingkeys_test.go b/internal/controller/coderprovisioner_pendingkeys_test.go index 021459e03..fb4ff541c 100644 --- a/internal/controller/coderprovisioner_pendingkeys_test.go +++ b/internal/controller/coderprovisioner_pendingkeys_test.go @@ -351,3 +351,77 @@ func TestCoderProvisionerKey_FailedRotationKeepsKeyIDs(t *testing.T) { require.True(t, pendingKeyRecorded(f.get(ctx, t, provisioner), unpublished.KeyID), "a failed rotation must keep the IDs of the unpublished key") } + +// setControlPlaneURL sets status.url of the fixture's control plane. +func setControlPlaneURL(ctx context.Context, t *testing.T, f provisionerCredentialFixture, url string) { + t.Helper() + controlPlane := &coderv1alpha1.CoderControlPlane{} + require.NoError(t, k8sClient.Get(ctx, types.NamespacedName{Name: f.controlPlane.Name, Namespace: f.namespace}, controlPlane)) + controlPlane.Status.URL = url + require.NoError(t, k8sClient.Status().Update(ctx, controlPlane)) +} + +// TestCoderProvisionerKey_RevertAfterFailedRotation checks a rotation from +// endpoint A to B that fails after its create, followed by a revert to A. +// The Secret then holds key B, which is valid only at B, or key A, which the +// rotation deleted. The next reconcile must delete key B and write a new key +// for A. +func TestCoderProvisionerKey_RevertAfterFailedRotation(t *testing.T) { + t.Parallel() + + for _, secretWritten := range []bool{true, false} { + name := "status write after the Secret write fails" + if !secretWritten { + name = "Secret write fails" + } + t.Run(name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + f := newProvisionerCredentialFixture(ctx, t, "cprov-revert") + first := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: uuid.New(), KeyID: uuid.New(), KeyName: "provisioner", Key: "material-1"} + atB := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: uuid.New(), KeyID: uuid.New(), KeyName: "provisioner", Key: "material-b"} + final := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: first.OrganizationID, KeyID: uuid.New(), KeyName: "provisioner", Key: "material-3"} + f.bootstrapClient.provisionerKeyResponses = []coderbootstrap.EnsureProvisionerKeyResponse{first, atB, final} + provisioner := f.createProvisioner(ctx, t, nil, nil) + name := types.NamespacedName{Name: provisioner.Name, Namespace: provisioner.Namespace} + keySecret := types.NamespacedName{Name: provisioner.Name + "-provisioner-key", Namespace: provisioner.Namespace} + f.reconcile(ctx, t, provisioner) + f.reconcile(ctx, t, provisioner) + + const urlB = "http://coder-b.example.com" + setControlPlaneURL(ctx, t, f, urlB) + base, err := ctrlclient.NewWithWatch(cfg, ctrlclient.Options{Scheme: scheme}) + require.NoError(t, err) + f.reconciler.Client = interceptor.NewClient(base, interceptor.Funcs{ + SubResourceUpdate: func(ctx context.Context, c ctrlclient.Client, sub string, obj ctrlclient.Object, opts ...ctrlclient.SubResourceUpdateOption) error { + if p, ok := obj.(*coderv1alpha1.CoderProvisioner); ok && secretWritten && p.Status.ProvisionerKeyID == atB.KeyID.String() { + return apierrors.NewServiceUnavailable("injected status write failure") + } + return c.SubResource(sub).Update(ctx, obj, opts...) + }, + Update: func(ctx context.Context, c ctrlclient.WithWatch, obj ctrlclient.Object, opts ...ctrlclient.UpdateOption) error { + if s, ok := obj.(*corev1.Secret); ok && !secretWritten && s.Name == keySecret.Name { + return apierrors.NewServiceUnavailable("injected Secret write failure") + } + return c.Update(ctx, obj, opts...) + }, + }) + _, err = f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: name}) + require.Error(t, err, "the injected failure must fail the rotation to B") + + f.reconciler.Client = k8sClient + setControlPlaneURL(ctx, t, f, testCoderOrigin) + f.reconcile(ctx, t, provisioner) + + require.Contains(t, keyDeletes(f.bootstrapClient), keyDelete{coderURL: urlB, token: testOperatorToken, keyID: atB.KeyID.String()}, + "key B must be deleted at B") + secret := &corev1.Secret{} + require.NoError(t, k8sClient.Get(ctx, keySecret, secret)) + require.Equal(t, "material-3", string(secret.Data[coderv1alpha1.DefaultProvisionerKeySecretKey]), "the Secret must hold a new key for A") + latest := f.get(ctx, t, provisioner) + require.Equal(t, final.KeyID.String(), latest.Status.ProvisionerKeyID) + require.Equal(t, testCoderOrigin, latest.Status.ControlPlaneURL) + require.Empty(t, latest.Status.PendingKeys) + }) + } +} From 488e3f09006593e31bd9894b133e9f51d9edee65 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 16:45:59 +0000 Subject: [PATCH 07/10] tests: show that an ID-less CoderProvisioner key record deletes another provisioner's same-named key (#259) Change-Id: Id70605da9dff77139df4da0367fc88be2dcdd155 Signed-off-by: Thomas Kosiewski --- .../coderprovisioner_pendingkeys_test.go | 123 ++++++++++++++++++ 1 file changed, 123 insertions(+) diff --git a/internal/controller/coderprovisioner_pendingkeys_test.go b/internal/controller/coderprovisioner_pendingkeys_test.go index fb4ff541c..6e6856a41 100644 --- a/internal/controller/coderprovisioner_pendingkeys_test.go +++ b/internal/controller/coderprovisioner_pendingkeys_test.go @@ -1,8 +1,10 @@ package controller_test import ( + "cmp" "context" "errors" + "maps" "slices" "testing" @@ -425,3 +427,124 @@ func TestCoderProvisionerKey_RevertAfterFailedRotation(t *testing.T) { }) } } + +// keySlot is the place of a provisioner key in Coder: Coder allows one key +// per name in an organization. +type keySlot struct{ coderURL, organizationName, keyName string } + +// keyStoreClient is a Coder that keeps provisioner keys, one per slot. +// failEnsureAt fails each key create at that URL before it reaches Coder. +type keyStoreClient struct { + *fakeBootstrapClient + keys map[keySlot]uuid.UUID + failEnsureAt string +} + +func newKeySlot(coderURL, organizationName, keyName string) keySlot { + return keySlot{coderURL: coderURL, organizationName: cmp.Or(organizationName, "default"), keyName: keyName} +} + +func (s keySlot) organizationID() uuid.UUID { + return uuid.NewSHA1(uuid.NameSpaceURL, []byte(s.coderURL+"/"+s.organizationName)) +} + +func (c *keyStoreClient) EnsureProvisionerKey(_ context.Context, req coderbootstrap.EnsureProvisionerKeyRequest) (coderbootstrap.EnsureProvisionerKeyResponse, error) { + if req.CoderURL == c.failEnsureAt { + return coderbootstrap.EnsureProvisionerKeyResponse{}, errors.New("injected connection refused") + } + slot := newKeySlot(req.CoderURL, req.OrganizationName, req.KeyName) + response := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: slot.organizationID(), KeyName: req.KeyName} + if keyID, ok := c.keys[slot]; ok { + response.KeyID = keyID + return response, nil + } + response.KeyID = uuid.New() + response.Key = "material-" + response.KeyID.String() + c.keys[slot] = response.KeyID + 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 + }) + return nil +} + +func (c *keyStoreClient) DeleteProvisionerKeyByName(_ context.Context, req coderbootstrap.DeleteProvisionerKeyByNameRequest) error { + slot := newKeySlot(req.CoderURL, req.OrganizationName, req.KeyName) + if keyID, ok := c.keys[slot]; ok && keyID != req.KeepKeyID { + delete(c.keys, slot) + } + return nil +} + +// TestCoderProvisionerKey_IDLessRecordKeepsOtherProvisionersKey checks a +// provisioner P whose target moves to endpoint B, where provisioner Q holds +// its key under the same key name. P records the create at B without key IDs, +// and the create fails before it reaches Coder. Neither the cleanup after P +// moves back nor the deletion of P may delete the key of Q. +func TestCoderProvisionerKey_IDLessRecordKeepsOtherProvisionersKey(t *testing.T) { + t.Parallel() + + for _, deleteP := range []bool{false, true} { + name := "P moves back to A" + if deleteP { + name = "P is deleted" + } + t.Run(name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + f := newProvisionerCredentialFixture(ctx, t, "cprov-idless") + createControlPlaneWithToken(ctx, t, f.namespace, "coder-b", "https://coder-b.example.com", "operator-token-b") + urlB := "http://coder-b." + f.namespace + ".svc.cluster.local:80" + 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) + f.reconcile(ctx, t, p) + moveTo := func(controlPlane string) { + latest := f.get(ctx, t, p) + latest.Spec.ControlPlaneRef.Name = controlPlane + require.NoError(t, k8sClient.Update(ctx, latest)) + } + + store.failEnsureAt = urlB + moveTo("coder-b") + _, err := f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: pName}) + require.Error(t, err, "the create at B must fail") + require.True(t, slices.ContainsFunc(f.get(ctx, t, p).Status.PendingKeys, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { + return record.CoderURL == urlB && record.KeyName == "provisioner" && record.KeyID == "" + }), "P must record its create at B without key IDs") + + if deleteP { + require.NoError(t, k8sClient.Delete(ctx, f.get(ctx, t, p))) + f.reconcile(ctx, t, p) + requireProvisionerGone(ctx, t, pName) + } else { + moveTo("coder") + _, err = f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: pName}) + require.NoError(t, err) + } + + require.Equal(t, qKeyID, store.keys[slotB].String(), "the key of Q at B must still exist") + }) + } +} From ee49817738b827e41e75ac9ac7c40afec596403f Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 16:54:41 +0000 Subject: [PATCH 08/10] fix: never delete a key by name for a CoderProvisioner key record without IDs, and report a same-named key with UnverifiedKeyKept (#259) Change-Id: I85db43792ec3049f32ceb5ccc289bd2751f68221 Signed-off-by: Thomas Kosiewski --- api/v1alpha1/coderprovisioner_types.go | 19 ++- .../bases/coder.com_coderprovisioners.yaml | 13 +- dist/install.yaml | 13 +- docs/reference/api/coderprovisioner.md | 4 +- docs/tutorials/deploy-coderprovisioner.md | 2 +- internal/coderbootstrap/client.go | 2 +- internal/coderbootstrap/provisionerkeys.go | 42 +++---- .../provisionerkeys_deletebyid_test.go | 64 ++++------ .../controller/coderprovisioner_controller.go | 111 +++++++++++++----- .../coderprovisioner_pendingkeys_test.go | 80 +++++++++---- .../coderworkspaceproxy_controller_test.go | 12 +- 11 files changed, 221 insertions(+), 141 deletions(-) diff --git a/api/v1alpha1/coderprovisioner_types.go b/api/v1alpha1/coderprovisioner_types.go index 7302ca097..4e7752c36 100644 --- a/api/v1alpha1/coderprovisioner_types.go +++ b/api/v1alpha1/coderprovisioner_types.go @@ -29,6 +29,12 @@ const ( // referenced credential Secret and setting passed the operator's checks. // When it is False, the operator keeps the last applied Deployment. CoderProvisionerConditionCredentialsReady = "CredentialsReady" + // CoderProvisionerConditionUnverifiedKeyKept is True while an entry in + // status.pendingKeys has no key ID and Coder holds a key with its name. + // Nothing shows that this provisioner created that key, so the operator + // does not delete it. The operator removes the condition when no such + // entry is left. + CoderProvisionerConditionUnverifiedKeyKept = "UnverifiedKeyKept" // DefaultProvisionerKeySecretKey is the default data key for provisioner key secrets. DefaultProvisionerKeySecretKey = "key" @@ -108,9 +114,10 @@ type CoderProvisionerStatus struct { // 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 each entry - // at its coderURL with the operator token of its control plane, and - // removes the entry when Coder confirms that the key is gone. + // whose deletion Coder has not confirmed. The operator deletes the key of + // each entry with keyID at its coderURL with the operator token of its + // control plane, and removes the entry when Coder confirms that the key + // is gone. // +listType=atomic PendingKeys []CoderProvisionerKeyRecord `json:"pendingKeys,omitempty"` } @@ -128,8 +135,10 @@ type CoderProvisionerKeyRecord struct { // KeyName is the provisioner key name in Coder. KeyName string `json:"keyName"` // OrganizationID is the organization ID that Coder returned for the key. - // Without organizationID and keyID, the operator deletes the key by name, - // unless that name holds the key in provisionerKeyID. + // Without organizationID and keyID, the operator deletes no key. It + // removes the entry when Coder has no key with keyName, or when that key + // is the key in provisionerKeyID. Otherwise it keeps the entry and sets + // the condition UnverifiedKeyKept. OrganizationID string `json:"organizationID,omitempty"` // KeyID is the provisioner key ID that Coder returned. KeyID string `json:"keyID,omitempty"` diff --git a/config/crd/bases/coder.com_coderprovisioners.yaml b/config/crd/bases/coder.com_coderprovisioners.yaml index c2c42e854..d2f33ff8e 100644 --- a/config/crd/bases/coder.com_coderprovisioners.yaml +++ b/config/crd/bases/coder.com_coderprovisioners.yaml @@ -439,9 +439,10 @@ spec: 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 each entry - at its coderURL with the operator token of its control plane, and - removes the entry when Coder confirms that the key is gone. + whose deletion Coder has not confirmed. The operator deletes the key of + each entry with keyID at its coderURL with the operator token of its + control plane, and removes the entry when Coder confirms that the key + is gone. items: description: |- CoderProvisionerKeyRecord names a provisioner key that the operator created, @@ -465,8 +466,10 @@ spec: organizationID: description: |- OrganizationID is the organization ID that Coder returned for the key. - Without organizationID and keyID, the operator deletes the key by name, - unless that name holds the key in provisionerKeyID. + Without organizationID and keyID, the operator deletes no key. It + removes the entry when Coder has no key with keyName, or when that key + is the key in provisionerKeyID. Otherwise it keeps the entry and sets + the condition UnverifiedKeyKept. type: string organizationName: description: OrganizationName is the Coder organization of the diff --git a/dist/install.yaml b/dist/install.yaml index ac1a59e76..a94d972be 100644 --- a/dist/install.yaml +++ b/dist/install.yaml @@ -6195,9 +6195,10 @@ spec: 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 each entry - at its coderURL with the operator token of its control plane, and - removes the entry when Coder confirms that the key is gone. + whose deletion Coder has not confirmed. The operator deletes the key of + each entry with keyID at its coderURL with the operator token of its + control plane, and removes the entry when Coder confirms that the key + is gone. items: description: |- CoderProvisionerKeyRecord names a provisioner key that the operator created, @@ -6221,8 +6222,10 @@ spec: organizationID: description: |- OrganizationID is the organization ID that Coder returned for the key. - Without organizationID and keyID, the operator deletes the key by name, - unless that name holds the key in provisionerKeyID. + Without organizationID and keyID, the operator deletes no key. It + removes the entry when Coder has no key with keyName, or when that key + is the key in provisionerKeyID. Otherwise it keeps the entry and sets + the condition UnverifiedKeyKept. type: string organizationName: description: OrganizationName is the Coder organization of the diff --git a/docs/reference/api/coderprovisioner.md b/docs/reference/api/coderprovisioner.md index 41b059b17..fe206989f 100644 --- a/docs/reference/api/coderprovisioner.md +++ b/docs/reference/api/coderprovisioner.md @@ -41,7 +41,7 @@ | `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. | | `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 each entry at its coderURL with the operator token of its control plane, and removes the entry when Coder confirms that the key is gone. | +| `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 keyID at its coderURL with the operator token of its control plane, and removes the entry when Coder confirms that the key is gone. | ## Referenced types @@ -56,7 +56,7 @@ or was about to create, at one Coder endpoint. | `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. | -| `organizationID` | string | OrganizationID is the organization ID that Coder returned for the key. Without organizationID and keyID, the operator deletes the key by name, unless that name holds the key in provisionerKeyID. | +| `organizationID` | string | OrganizationID is the organization ID that Coder returned for the key. Without organizationID and keyID, the operator deletes no key. It removes the entry when Coder has no key with keyName, or when that key is the key in provisionerKeyID. Otherwise it keeps the entry and sets the condition UnverifiedKeyKept. | | `keyID` | string | KeyID is the provisioner key ID that Coder returned. | | `unpublished` | boolean | Unpublished is true from before the create until the operator records a key as applied. While it is true, the Secret can hold this key, or a key that a rotation deleted, so the operator replaces the key in the Secret. | diff --git a/docs/tutorials/deploy-coderprovisioner.md b/docs/tutorials/deploy-coderprovisioner.md index ef79271e4..7e4854d5f 100644 --- a/docs/tutorials/deploy-coderprovisioner.md +++ b/docs/tutorials/deploy-coderprovisioner.md @@ -78,7 +78,7 @@ kubectl get secret coderprovisioner-sample-provisioner-key -n coder 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 each key in `status.pendingKeys`. These are 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 `keyID` names a key that the operator started to create before Coder returned its ID. The operator deletes the key with that name, unless it is the key in `status.provisionerKeyID`. 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 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 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 `keyID` 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 the entry stays, the condition `UnverifiedKeyKept` is `True` and names the key, and deletion of the provisioner leaves that 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"}]'`. diff --git a/internal/coderbootstrap/client.go b/internal/coderbootstrap/client.go index 9b27ac739..e2385d1ba 100644 --- a/internal/coderbootstrap/client.go +++ b/internal/coderbootstrap/client.go @@ -96,7 +96,7 @@ type Client interface { EnsureProvisionerKey(context.Context, EnsureProvisionerKeyRequest) (EnsureProvisionerKeyResponse, error) DeleteProvisionerKey(ctx context.Context, coderURL, sessionToken, orgName, keyName string) error DeleteProvisionerKeyByID(context.Context, DeleteProvisionerKeyByIDRequest) error - DeleteProvisionerKeyByName(context.Context, DeleteProvisionerKeyByNameRequest) 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 07dfcd398..785cfb1c1 100644 --- a/internal/coderbootstrap/provisionerkeys.go +++ b/internal/coderbootstrap/provisionerkeys.go @@ -141,60 +141,48 @@ func (c *SDKClient) DeleteProvisionerKey(ctx context.Context, coderURL, sessionT return xerrors.Errorf("delete provisioner key %q: %w", keyName, err) } -// DeleteProvisionerKeyByNameRequest identifies a provisioner key by the name -// that the caller recorded before it sent the create, when Coder had not yet -// returned the key ID. -type DeleteProvisionerKeyByNameRequest struct { +// ProvisionerKeyByNameRequest identifies a provisioner key by its name. +type ProvisionerKeyByNameRequest struct { CoderURL string SessionToken string OrganizationName string KeyName string - // KeepKeyID is the ID of a key that the delete leaves in place, for - // example the key that the caller uses now. - KeepKeyID uuid.UUID } -// DeleteProvisionerKeyByName deletes the provisioner key named req.KeyName in -// the organization req.OrganizationName, unless its ID is req.KeepKeyID. It -// does nothing when the organization or a key with that name does not exist. -func (c *SDKClient) DeleteProvisionerKeyByName(ctx context.Context, req DeleteProvisionerKeyByNameRequest) error { +// ProvisionerKeyByName returns the provisioner key named req.KeyName in the +// organization req.OrganizationName. It returns nil when Coder answers that +// the organization or a key with that name does not exist. +func (c *SDKClient) ProvisionerKeyByName(ctx context.Context, req ProvisionerKeyByNameRequest) (*codersdk.ProvisionerKey, error) { if err := validateProvisionerKeyInputs(req.CoderURL, req.SessionToken, req.KeyName); err != nil { - return err + return nil, err } client, err := newAuthenticatedClient(req.CoderURL, req.SessionToken) if err != nil { - return err + return nil, err } organization, err := resolveOrganizationByName(ctx, client, cmp.Or(req.OrganizationName, codersdk.DefaultOrganization)) if isNotFound(err) { - return nil + return nil, nil } if err != nil { - return err + return nil, err } key, err := withOptionalRateLimitBypass(ctx, func(requestCtx context.Context) (*codersdk.ProvisionerKey, error) { return findOrganizationProvisionerKey(requestCtx, client, organization.ID, req.KeyName) }) if isNotFound(err) { - return nil + return nil, nil } if err != nil { - return err + return nil, err } - if key == nil || (req.KeepKeyID != uuid.Nil && key.ID == req.KeepKeyID) { - return nil + if key != nil && key.ID == uuid.Nil { + return nil, xerrors.Errorf("assertion failed: provisioner key %q returned an empty ID", req.KeyName) } - _, err = withOptionalRateLimitBypass(ctx, func(requestCtx context.Context) (struct{}, error) { - return struct{}{}, client.DeleteProvisionerKey(requestCtx, organization.ID, key.Name) - }) - if err != nil && !isNotFound(err) { - return xerrors.Errorf("delete provisioner key %q: %w", key.Name, err) - } - - return nil + return key, nil } // DeleteProvisionerKeyByIDRequest identifies one provisioner key by the IDs diff --git a/internal/coderbootstrap/provisionerkeys_deletebyid_test.go b/internal/coderbootstrap/provisionerkeys_deletebyid_test.go index 7abaedf74..48f726531 100644 --- a/internal/coderbootstrap/provisionerkeys_deletebyid_test.go +++ b/internal/coderbootstrap/provisionerkeys_deletebyid_test.go @@ -120,67 +120,49 @@ func TestDeleteProvisionerKeyByID_ValidationErrors(t *testing.T) { } } -func TestDeleteProvisionerKeyByName(t *testing.T) { +func TestProvisionerKeyByName(t *testing.T) { t.Parallel() orgID := uuid.New() - keptKeyID := uuid.New() + listedID := uuid.New() now := time.Now().UTC().Format(time.RFC3339) keysPath := "/api/v2/organizations/" + orgID.String() + "/provisionerkeys" tests := []struct { - name string - orgGone bool - listGone bool - listedID uuid.UUID - listedName string - deleteBody map[string]any - wantDeletes []string - wantErr bool + name string + orgGone bool + listBody map[string]any + listedName string + wantKeyID uuid.UUID + wantErr bool }{ {name: "organization gone", orgGone: true}, - {name: "organization gone before the list", listGone: true}, - {name: "key exists", listedID: uuid.New(), listedName: "provisioner", wantDeletes: []string{keysPath + "/provisioner"}}, - {name: "no key with the name", listedID: uuid.New(), listedName: "other"}, - {name: "key is the one to keep", listedID: keptKeyID, listedName: "provisioner"}, - {name: "key gone before the delete", listedID: uuid.New(), listedName: "provisioner", deleteBody: coderNotFound, wantDeletes: []string{keysPath + "/provisioner"}}, - { - name: "404 not from Coder", listedID: uuid.New(), listedName: "provisioner", - deleteBody: map[string]any{"message": "Route not found."}, wantDeletes: []string{keysPath + "/provisioner"}, wantErr: true, - }, + {name: "organization gone before the list", listBody: coderNotFound}, + {name: "key exists", listedName: "provisioner", wantKeyID: listedID}, + {name: "no key with the name", listedName: "other"}, + {name: "404 not from Coder", listBody: map[string]any{"message": "Route not found."}, wantErr: true}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - var mu sync.Mutex - var deletes []string 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" && tt.orgGone: writeJSONResponse(t, w, http.StatusNotFound, coderNotFound) 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}) - case r.Method == http.MethodGet && r.URL.Path == keysPath && tt.listGone: - writeJSONResponse(t, w, http.StatusNotFound, coderNotFound) + case r.Method == http.MethodGet && r.URL.Path == keysPath && tt.listBody != nil: + writeJSONResponse(t, w, http.StatusNotFound, tt.listBody) case r.Method == http.MethodGet && r.URL.Path == keysPath: writeJSONResponse(t, w, http.StatusOK, []map[string]any{{ - "id": tt.listedID.String(), + "id": listedID.String(), "created_at": now, "organization": orgID.String(), "name": tt.listedName, "tags": map[string]string{}, }}) - case r.Method == http.MethodDelete: - mu.Lock() - deletes = append(deletes, r.URL.Path) - mu.Unlock() - if tt.deleteBody != nil { - writeJSONResponse(t, w, http.StatusNotFound, tt.deleteBody) - return - } - w.WriteHeader(http.StatusNoContent) default: t.Errorf("unexpected request %s %s", r.Method, r.URL.Path) w.WriteHeader(http.StatusNotFound) @@ -188,20 +170,22 @@ func TestDeleteProvisionerKeyByName(t *testing.T) { })) defer server.Close() - err := coderbootstrap.NewSDKClient().DeleteProvisionerKeyByName(context.Background(), coderbootstrap.DeleteProvisionerKeyByNameRequest{ + key, err := coderbootstrap.NewSDKClient().ProvisionerKeyByName(context.Background(), coderbootstrap.ProvisionerKeyByNameRequest{ CoderURL: server.URL, SessionToken: "session-token", KeyName: "provisioner", - KeepKeyID: keptKeyID, }) if tt.wantErr { require.Error(t, err, "only Coder's own 404 proves that the key is gone") - } else { - require.NoError(t, err) + return } - mu.Lock() - defer mu.Unlock() - require.Equal(t, tt.wantDeletes, deletes) + require.NoError(t, err) + if tt.wantKeyID == uuid.Nil { + require.Nil(t, key) + return + } + require.NotNil(t, key) + require.Equal(t, tt.wantKeyID, key.ID) }) } } diff --git a/internal/controller/coderprovisioner_controller.go b/internal/controller/coderprovisioner_controller.go index 968d47b83..f48dfc6fb 100644 --- a/internal/controller/coderprovisioner_controller.go +++ b/internal/controller/coderprovisioner_controller.go @@ -293,8 +293,8 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req // Every key that can exist in Coder has a record in status: the applied // fields for the key in the Secret, status.pendingKeys for the others. A // key to create is recorded by name before the create, because Coder - // returns its ID only in the create response. Keys recorded in another - // slot than the target are deleted here. + // returns its ID only in the create response. Keys recorded with IDs in + // another slot than the target are deleted here. target := coderv1alpha1.CoderProvisionerKeyRecord{ ControlPlaneRefName: desiredControlPlaneRefName, CoderURL: coderURL, @@ -338,21 +338,25 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req } pending = append(next, target) } - var kept []coderv1alpha1.CoderProvisionerKeyRecord + var kept, unverified []coderv1alpha1.CoderProvisionerKeyRecord for _, record := range pending { if sameKeySlot(record, target) { kept = append(kept, record) continue } - revoked, revokeErr := r.revokeKey(ctx, provisioner, record) + cleanup, revokeErr := r.revokeKey(ctx, provisioner, record) if revokeErr != nil { return ctrl.Result{}, revokeErr } - if !revoked { + if cleanup != keyGone { kept = append(kept, record) } + if cleanup == keyUnverified { + unverified = append(unverified, record) + } } - if !slices.Equal(kept, provisioner.Status.PendingKeys) { + conditionChanged := setUnverifiedKeyCondition(provisioner, unverified) + if conditionChanged || !slices.Equal(kept, provisioner.Status.PendingKeys) { provisioner.Status.PendingKeys = kept if err := r.Status().Update(ctx, provisioner); err != nil { return ctrl.Result{}, fmt.Errorf("record pending provisioner keys in status: %w", err) @@ -809,13 +813,13 @@ func (r *CoderProvisionerReconciler) reconcileDeletion(ctx context.Context, prov // control plane was never ready, or when operator access bootstrap has // not completed. for _, record := range records { - revoked, err := r.revokeKey(ctx, provisioner, record) + cleanup, err := r.revokeKey(ctx, provisioner, record) if err != nil { return ctrl.Result{}, err } - if !revoked { - log.Info("could not delete a recorded provisioner key during deletion, proceeding with finalizer removal", - "coderURL", record.CoderURL, "keyName", record.KeyName, "keyID", record.KeyID) + 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) } } @@ -972,51 +976,98 @@ func (r *CoderProvisionerReconciler) readControlPlane(ctx context.Context, names return controlPlane, true, nil } +// keyCleanup is the result of the cleanup of one recorded provisioner key. +type keyCleanup int + +const ( + // keyKept: Coder did not confirm that the key is gone. The record stays, + // and the operator tries again. + keyKept keyCleanup = iota + // keyGone: no key of the record exists in Coder. + keyGone + // keyUnverified: the record has no key IDs, and Coder holds a key with + // its name that nothing shows this provisioner created. The operator + // leaves that key, and the record stays. + keyUnverified +) + // 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 is deleted by name, unless that name holds the applied key. -// revoked reports whether Coder confirmed that the key is gone. +// without key IDs deletes no key. func (r *CoderProvisionerReconciler) revokeKey( ctx context.Context, provisioner *coderv1alpha1.CoderProvisioner, record coderv1alpha1.CoderProvisionerKeyRecord, -) (revoked bool, err error) { +) (keyCleanup, error) { log := ctrl.LoggerFrom(ctx).WithValues("coderURL", record.CoderURL, "controlPlaneRef", record.ControlPlaneRefName, "keyName", record.KeyName, "keyID", record.KeyID) controlPlane, ok, err := r.readControlPlane(ctx, provisioner.Namespace, record.ControlPlaneRefName) if err != nil || !ok { - return false, err + return keyKept, err } 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", "reason", tokenReason, "message", tokenMessage, "error", tokenErr) - return false, nil + return keyKept, nil } if organizationID, keyID, ok := parseKeyIDs(record.OrganizationID, record.KeyID); ok { - err = r.BootstrapClient.DeleteProvisionerKeyByID(ctx, coderbootstrap.DeleteProvisionerKeyByIDRequest{ + if err := r.BootstrapClient.DeleteProvisionerKeyByID(ctx, coderbootstrap.DeleteProvisionerKeyByIDRequest{ CoderURL: record.CoderURL, SessionToken: sessionToken, OrganizationID: organizationID, KeyID: keyID, - }) - } else { - // Keep the applied key: another URL can reach the same Coder. - _, appliedKeyID, _ := parseKeyIDs(provisioner.Status.OrganizationID, provisioner.Status.ProvisionerKeyID) - err = r.BootstrapClient.DeleteProvisionerKeyByName(ctx, coderbootstrap.DeleteProvisionerKeyByNameRequest{ - CoderURL: record.CoderURL, - SessionToken: sessionToken, - OrganizationName: record.OrganizationName, - KeyName: record.KeyName, - KeepKeyID: appliedKeyID, - }) + }); err != nil { + log.Info("failed to delete the recorded provisioner key, keeping its record", "error", err) + return keyKept, nil + } + return keyGone, nil } + + key, err := r.BootstrapClient.ProvisionerKeyByName(ctx, coderbootstrap.ProvisionerKeyByNameRequest{ + CoderURL: record.CoderURL, + SessionToken: sessionToken, + OrganizationName: record.OrganizationName, + KeyName: record.KeyName, + }) if err != nil { - log.Info("failed to delete the recorded provisioner key, keeping its record", "error", err) - return false, nil + log.Info("failed to look up the recorded provisioner key, keeping its record", "error", err) + return keyKept, nil + } + // Coder allows one key per name in an organization, so no other key of + // this record exists when no key or the applied key holds the name. The + // applied key holds it when record.CoderURL reaches the same Coder. + _, appliedKeyID, _ := parseKeyIDs(provisioner.Status.OrganizationID, provisioner.Status.ProvisionerKeyID) + if key == nil || (appliedKeyID != uuid.Nil && key.ID == appliedKeyID) { + return keyGone, nil + } + // Without the key ID that the create returned, nothing ties this key to + // this provisioner: another provisioner or a person can use the name. + log.Info("leaving a provisioner key that this provisioner cannot show it created", "foundKeyID", key.ID) + return keyUnverified, nil +} + +// setUnverifiedKeyCondition sets the UnverifiedKeyKept condition for the +// records in unverified, or removes it when there are none. It reports +// whether the conditions changed. +func setUnverifiedKeyCondition(provisioner *coderv1alpha1.CoderProvisioner, unverified []coderv1alpha1.CoderProvisionerKeyRecord) bool { + if len(unverified) == 0 { + return meta.RemoveStatusCondition(&provisioner.Status.Conditions, coderv1alpha1.CoderProvisionerConditionUnverifiedKeyKept) } - return true, nil + keys := make([]string, 0, len(unverified)) + for _, record := range unverified { + keys = append(keys, fmt.Sprintf("%q in organization %q at %s", record.KeyName, record.OrganizationName, record.CoderURL)) + } + return meta.SetStatusCondition(&provisioner.Status.Conditions, metav1.Condition{ + Type: coderv1alpha1.CoderProvisionerConditionUnverifiedKeyKept, + Status: metav1.ConditionTrue, + ObservedGeneration: provisioner.Generation, + Reason: "KeyIDUnknown", + Message: fmt.Sprintf("Coder holds provisioner key %s, and status.pendingKeys has no key ID for it, so nothing shows that this provisioner created it. "+ + "The operator does not delete it, because another provisioner or a person can use the same name. Delete it in Coder if nothing uses it.", + strings.Join(keys, ", ")), + }) } func (r *CoderProvisionerReconciler) ensureCleanupFinalizer(ctx context.Context, provisioner *coderv1alpha1.CoderProvisioner) (bool, error) { diff --git a/internal/controller/coderprovisioner_pendingkeys_test.go b/internal/controller/coderprovisioner_pendingkeys_test.go index 6e6856a41..8b3f3bf3b 100644 --- a/internal/controller/coderprovisioner_pendingkeys_test.go +++ b/internal/controller/coderprovisioner_pendingkeys_test.go @@ -8,10 +8,12 @@ import ( "slices" "testing" + "github.com/coder/coder/v2/codersdk" "github.com/google/uuid" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" @@ -67,9 +69,6 @@ func keyDeletes(client *fakeBootstrapClient) []keyDelete { for _, req := range client.deleteKeyRequests { deletes = append(deletes, keyDelete{coderURL: req.CoderURL, token: req.SessionToken, keyName: req.KeyName}) } - for _, req := range client.deleteKeyByNameRequests { - 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()}) } @@ -87,9 +86,11 @@ func createControlPlaneWithToken(ctx context.Context, t *testing.T, namespace, n // TestCoderProvisionerKey_IntermediateEndpointRevoked checks a control plane // target that changes twice, A to B to C, while the attempt at B fails after -// Coder created the key at B. Both the next rotation and deletion must delete -// the key at B, with the operator token of control plane B, and must send no -// token to the endpoint of another control plane. +// Coder created the key at B. When status recorded the ID of key B, both the +// next rotation and deletion must delete key B at B, with the operator token +// of control plane B. When it did not, nothing shows that this provisioner +// created the key at B, so key B must stay and the provisioner must report +// it. No delete may send a token to the endpoint of another control plane. func TestCoderProvisionerKey_IntermediateEndpointRevoked(t *testing.T) { t.Parallel() @@ -97,7 +98,8 @@ func TestCoderProvisionerKey_IntermediateEndpointRevoked(t *testing.T) { name string // statusWriteFails makes the status write that records key B fail, // and the revoke of the unpublished key B fail too, as after a crash - // right after the create. Otherwise the Secret write fails. + // right after the create, so status never holds the ID of key B. + // Otherwise the Secret write fails. statusWriteFails bool // deleteAtC deletes the provisioner instead of reconciling at C. deleteAtC bool @@ -131,6 +133,14 @@ func TestCoderProvisionerKey_IntermediateEndpointRevoked(t *testing.T) { keyB := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: uuid.New(), KeyID: uuid.New(), KeyName: "provisioner", Key: "material-b"} keyC := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: uuid.New(), KeyID: uuid.New(), KeyName: "provisioner", Key: "material-c"} 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") { + return nil + } + return &codersdk.ProvisionerKey{ID: keyB.KeyID, OrganizationID: keyB.OrganizationID, Name: keyB.KeyName} + } + } provisioner := f.createProvisioner(ctx, t, nil, nil) name := types.NamespacedName{Name: provisioner.Name, Namespace: provisioner.Namespace} @@ -193,14 +203,17 @@ func TestCoderProvisionerKey_IntermediateEndpointRevoked(t *testing.T) { require.True(t, pendingKeyRecorded(f.get(ctx, t, provisioner), keyB.KeyID), "a failed delete must keep the record of key B") client.failRevoke = uuid.Nil } - f.reconcile(ctx, t, provisioner) + _, err = f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: name}) + require.NoError(t, err) deletes := keyDeletes(f.bootstrapClient) - revokedAtB := slices.ContainsFunc(deletes, func(d keyDelete) bool { - return d.coderURL == apiURL("coder-b") && d.token == "operator-token-b" && - (d.keyID == keyB.KeyID.String() || (d.keyID == "" && d.keyName == keyB.KeyName)) - }) - require.True(t, revokedAtB, "key B must be deleted at B with the operator token of B, got %+v", deletes) + if tt.statusWriteFails { + require.False(t, slices.ContainsFunc(deletes, func(d keyDelete) bool { return d.coderURL == apiURL("coder-b") }), + "a key whose ID status never held must not be deleted, got %+v", deletes) + } else { + require.Contains(t, deletes, keyDelete{coderURL: apiURL("coder-b"), token: "operator-token-b", keyID: keyB.KeyID.String()}, + "key B must be deleted at B with the operator token of B") + } require.Contains(t, deletes, keyDelete{coderURL: apiURL("coder"), token: testOperatorToken, keyID: keyA.KeyID.String()}, "key A must be deleted at A with the operator token of A") for _, d := range deletes { @@ -218,7 +231,17 @@ func TestCoderProvisionerKey_IntermediateEndpointRevoked(t *testing.T) { final := f.get(ctx, t, provisioner) require.Equal(t, keyC.KeyID.String(), final.Status.ProvisionerKeyID) require.Equal(t, "https://coder-c.example.com", final.Status.ControlPlaneURL) + unverified := meta.FindStatusCondition(final.Status.Conditions, coderv1alpha1.CoderProvisionerConditionUnverifiedKeyKept) + if tt.statusWriteFails { + require.Len(t, final.Status.PendingKeys, 1) + require.Equal(t, apiURL("coder-b"), final.Status.PendingKeys[0].CoderURL, "the record of key B must stay") + require.Empty(t, final.Status.PendingKeys[0].KeyID) + require.NotNil(t, unverified, "the provisioner must report the key that it left at B") + require.Equal(t, metav1.ConditionTrue, unverified.Status) + return + } require.Empty(t, final.Status.PendingKeys, "every replaced key must be confirmed deleted") + require.Nil(t, unverified) }) } } @@ -476,12 +499,13 @@ func (c *keyStoreClient) DeleteProvisionerKeyByID(_ context.Context, req coderbo return nil } -func (c *keyStoreClient) DeleteProvisionerKeyByName(_ context.Context, req coderbootstrap.DeleteProvisionerKeyByNameRequest) error { +func (c *keyStoreClient) ProvisionerKeyByName(_ context.Context, req coderbootstrap.ProvisionerKeyByNameRequest) (*codersdk.ProvisionerKey, error) { slot := newKeySlot(req.CoderURL, req.OrganizationName, req.KeyName) - if keyID, ok := c.keys[slot]; ok && keyID != req.KeepKeyID { - delete(c.keys, slot) + keyID, ok := c.keys[slot] + if !ok { + return nil, nil } - return nil + return &codersdk.ProvisionerKey{ID: keyID, OrganizationID: slot.organizationID(), Name: slot.keyName}, nil } // TestCoderProvisionerKey_IDLessRecordKeepsOtherProvisionersKey checks a @@ -538,13 +562,27 @@ func TestCoderProvisionerKey_IDLessRecordKeepsOtherProvisionersKey(t *testing.T) require.NoError(t, k8sClient.Delete(ctx, f.get(ctx, t, p))) f.reconcile(ctx, t, p) requireProvisionerGone(ctx, t, pName) - } else { - moveTo("coder") - _, err = f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: pName}) - require.NoError(t, err) + require.Equal(t, qKeyID, store.keys[slotB].String(), "the key of Q at B must still exist") + return } + moveTo("coder") + result, err := f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: pName}) + require.NoError(t, err) require.Equal(t, qKeyID, store.keys[slotB].String(), "the key of Q at B must still exist") + latest := f.get(ctx, t, p) + require.Len(t, latest.Status.PendingKeys, 1, "the record at B must stay while Coder holds a key with its name") + unverified := meta.FindStatusCondition(latest.Status.Conditions, coderv1alpha1.CoderProvisionerConditionUnverifiedKeyKept) + require.NotNil(t, unverified, "P must report the key at B that it does not delete") + require.Equal(t, metav1.ConditionTrue, unverified.Status) + require.Contains(t, unverified.Message, urlB) + require.Positive(t, result.RequeueAfter, "P must check the record at B again") + + delete(store.keys, slotB) + f.reconcile(ctx, t, p) + latest = f.get(ctx, t, p) + require.Empty(t, latest.Status.PendingKeys, "the record at B must go once Coder has no key with its name") + require.Nil(t, meta.FindStatusCondition(latest.Status.Conditions, coderv1alpha1.CoderProvisionerConditionUnverifiedKeyKept)) }) } } diff --git a/internal/controller/coderworkspaceproxy_controller_test.go b/internal/controller/coderworkspaceproxy_controller_test.go index dfb4da071..93f7674eb 100644 --- a/internal/controller/coderworkspaceproxy_controller_test.go +++ b/internal/controller/coderworkspaceproxy_controller_test.go @@ -34,7 +34,9 @@ type fakeBootstrapClient struct { deleteKeyRequests []deleteKeyRequest deleteKeyByIDErr error deleteKeyByIDRequests []coderbootstrap.DeleteProvisionerKeyByIDRequest - deleteKeyByNameRequests []coderbootstrap.DeleteProvisionerKeyByNameRequest + // provisionerKeyByName answers ProvisionerKeyByName. When it is nil, + // Coder has no key with the name. + provisionerKeyByName func(coderbootstrap.ProvisionerKeyByNameRequest) *codersdk.ProvisionerKey // Entitlements support. entitlementsResponse codersdk.Entitlements @@ -94,9 +96,11 @@ func (f *fakeBootstrapClient) DeleteProvisionerKeyByID(_ context.Context, req co return f.deleteKeyByIDErr } -func (f *fakeBootstrapClient) DeleteProvisionerKeyByName(_ context.Context, req coderbootstrap.DeleteProvisionerKeyByNameRequest) error { - f.deleteKeyByNameRequests = append(f.deleteKeyByNameRequests, req) - return nil +func (f *fakeBootstrapClient) ProvisionerKeyByName(_ context.Context, req coderbootstrap.ProvisionerKeyByNameRequest) (*codersdk.ProvisionerKey, error) { + if f.provisionerKeyByName == nil { + return nil, nil + } + return f.provisionerKeyByName(req), nil } func (f *fakeBootstrapClient) Entitlements(_ context.Context, coderURL, sessionToken string) (codersdk.Entitlements, error) { From 3b29933ccbe5db8e8ddc99055fbb3a9e75fc9404 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 17:00:41 +0000 Subject: [PATCH 09/10] fix: keep UnverifiedKeyKept through a failed key lookup, and name both key IDs in the pendingKeys docs (#259) Change-Id: Ibc2f22637712134a1a7c7b6e352d58a373a14a77 Signed-off-by: Thomas Kosiewski --- api/v1alpha1/coderprovisioner_types.go | 10 +++++----- config/crd/bases/coder.com_coderprovisioners.yaml | 8 ++++---- dist/install.yaml | 8 ++++---- docs/reference/api/coderprovisioner.md | 4 ++-- docs/tutorials/deploy-coderprovisioner.md | 2 +- internal/controller/coderprovisioner_controller.go | 13 +++++++++++-- .../coderprovisioner_pendingkeys_test.go | 14 +++++++++++++- 7 files changed, 40 insertions(+), 19 deletions(-) diff --git a/api/v1alpha1/coderprovisioner_types.go b/api/v1alpha1/coderprovisioner_types.go index 4e7752c36..413978026 100644 --- a/api/v1alpha1/coderprovisioner_types.go +++ b/api/v1alpha1/coderprovisioner_types.go @@ -30,7 +30,7 @@ const ( // When it is False, the operator keeps the last applied Deployment. CoderProvisionerConditionCredentialsReady = "CredentialsReady" // CoderProvisionerConditionUnverifiedKeyKept is True while an entry in - // status.pendingKeys has no key ID and Coder holds a key with its name. + // status.pendingKeys has no key IDs and Coder holds a key with its name. // Nothing shows that this provisioner created that key, so the operator // does not delete it. The operator removes the condition when no such // entry is left. @@ -115,9 +115,9 @@ type CoderProvisionerStatus struct { // 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 keyID at its coderURL with the operator token of its - // control plane, and removes the entry when Coder confirms that the key - // is gone. + // 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. // +listType=atomic PendingKeys []CoderProvisionerKeyRecord `json:"pendingKeys,omitempty"` } @@ -135,7 +135,7 @@ type CoderProvisionerKeyRecord struct { // KeyName is the provisioner key name in Coder. KeyName string `json:"keyName"` // OrganizationID is the organization ID that Coder returned for the key. - // Without organizationID and keyID, the operator deletes no key. It + // When organizationID or keyID is empty, the operator deletes no key. It // removes the entry when Coder has no key with keyName, or when that key // is the key in provisionerKeyID. Otherwise it keeps the entry and sets // the condition UnverifiedKeyKept. diff --git a/config/crd/bases/coder.com_coderprovisioners.yaml b/config/crd/bases/coder.com_coderprovisioners.yaml index d2f33ff8e..09d2b0b5a 100644 --- a/config/crd/bases/coder.com_coderprovisioners.yaml +++ b/config/crd/bases/coder.com_coderprovisioners.yaml @@ -440,9 +440,9 @@ spec: 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 keyID at its coderURL with the operator token of its - control plane, and removes the entry when Coder confirms that the key - is gone. + 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. items: description: |- CoderProvisionerKeyRecord names a provisioner key that the operator created, @@ -466,7 +466,7 @@ spec: organizationID: description: |- OrganizationID is the organization ID that Coder returned for the key. - Without organizationID and keyID, the operator deletes no key. It + When organizationID or keyID is empty, the operator deletes no key. It removes the entry when Coder has no key with keyName, or when that key is the key in provisionerKeyID. Otherwise it keeps the entry and sets the condition UnverifiedKeyKept. diff --git a/dist/install.yaml b/dist/install.yaml index a94d972be..f9f08fdef 100644 --- a/dist/install.yaml +++ b/dist/install.yaml @@ -6196,9 +6196,9 @@ spec: 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 keyID at its coderURL with the operator token of its - control plane, and removes the entry when Coder confirms that the key - is gone. + 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. items: description: |- CoderProvisionerKeyRecord names a provisioner key that the operator created, @@ -6222,7 +6222,7 @@ spec: organizationID: description: |- OrganizationID is the organization ID that Coder returned for the key. - Without organizationID and keyID, the operator deletes no key. It + When organizationID or keyID is empty, the operator deletes no key. It removes the entry when Coder has no key with keyName, or when that key is the key in provisionerKeyID. Otherwise it keeps the entry and sets the condition UnverifiedKeyKept. diff --git a/docs/reference/api/coderprovisioner.md b/docs/reference/api/coderprovisioner.md index fe206989f..9394015f7 100644 --- a/docs/reference/api/coderprovisioner.md +++ b/docs/reference/api/coderprovisioner.md @@ -41,7 +41,7 @@ | `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. | | `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 keyID at its coderURL with the operator token of its control plane, and removes the entry when Coder confirms that the key is gone. | +| `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. | ## Referenced types @@ -56,7 +56,7 @@ or was about to create, at one Coder endpoint. | `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. | -| `organizationID` | string | OrganizationID is the organization ID that Coder returned for the key. Without organizationID and keyID, the operator deletes no key. It removes the entry when Coder has no key with keyName, or when that key is the key in provisionerKeyID. Otherwise it keeps the entry and sets the condition UnverifiedKeyKept. | +| `organizationID` | string | OrganizationID is the organization ID that Coder returned for the key. When organizationID or keyID is empty, the operator deletes no key. It removes the entry when Coder has no key with keyName, or when that key is the key in provisionerKeyID. Otherwise it keeps the entry and sets the condition UnverifiedKeyKept. | | `keyID` | string | KeyID is the provisioner key ID that Coder returned. | | `unpublished` | boolean | Unpublished is true from before the create until the operator records a key as applied. While it is true, the Secret can hold this key, or a key that a rotation deleted, so the operator replaces the key in the Secret. | diff --git a/docs/tutorials/deploy-coderprovisioner.md b/docs/tutorials/deploy-coderprovisioner.md index 7e4854d5f..8921ca672 100644 --- a/docs/tutorials/deploy-coderprovisioner.md +++ b/docs/tutorials/deploy-coderprovisioner.md @@ -78,7 +78,7 @@ kubectl get secret coderprovisioner-sample-provisioner-key -n coder 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 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 `keyID` 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 the entry stays, the condition `UnverifiedKeyKept` is `True` and names the key, and deletion of the provisioner leaves that 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 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 the entry stays, the condition `UnverifiedKeyKept` is `True` and names the key, and deletion of the provisioner leaves that 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"}]'`. diff --git a/internal/controller/coderprovisioner_controller.go b/internal/controller/coderprovisioner_controller.go index f48dfc6fb..81e9da224 100644 --- a/internal/controller/coderprovisioner_controller.go +++ b/internal/controller/coderprovisioner_controller.go @@ -339,6 +339,7 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req pending = append(next, target) } var kept, unverified []coderv1alpha1.CoderProvisionerKeyRecord + lookupFailed := false for _, record := range pending { if sameKeySlot(record, target) { kept = append(kept, record) @@ -354,8 +355,15 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req if cleanup == keyUnverified { unverified = append(unverified, record) } + if cleanup == keyKept && record.KeyID == "" { + lookupFailed = true + } + } + // A failed lookup does not show that an unverified key is gone. + conditionChanged := false + if len(unverified) > 0 || !lookupFailed { + conditionChanged = setUnverifiedKeyCondition(provisioner, unverified) } - conditionChanged := setUnverifiedKeyCondition(provisioner, unverified) if conditionChanged || !slices.Equal(kept, provisioner.Status.PendingKeys) { provisioner.Status.PendingKeys = kept if err := r.Status().Update(ctx, provisioner); err != nil { @@ -983,7 +991,8 @@ const ( // keyKept: Coder did not confirm that the key is gone. The record stays, // and the operator tries again. keyKept keyCleanup = iota - // keyGone: no key of the record exists in Coder. + // keyGone: Coder confirmed that the key is gone, or that no key other + // than the applied key holds the name of a record without key IDs. keyGone // keyUnverified: the record has no key IDs, and Coder holds a key with // its name that nothing shows this provisioner created. The operator diff --git a/internal/controller/coderprovisioner_pendingkeys_test.go b/internal/controller/coderprovisioner_pendingkeys_test.go index 8b3f3bf3b..08af42413 100644 --- a/internal/controller/coderprovisioner_pendingkeys_test.go +++ b/internal/controller/coderprovisioner_pendingkeys_test.go @@ -456,11 +456,13 @@ func TestCoderProvisionerKey_RevertAfterFailedRotation(t *testing.T) { type keySlot struct{ coderURL, organizationName, keyName string } // keyStoreClient is a Coder that keeps provisioner keys, one per slot. -// failEnsureAt fails each key create at that URL before it reaches Coder. +// failEnsureAt fails each key create at that URL before it reaches Coder, and +// failLookup fails each key lookup by name. type keyStoreClient struct { *fakeBootstrapClient keys map[keySlot]uuid.UUID failEnsureAt string + failLookup bool } func newKeySlot(coderURL, organizationName, keyName string) keySlot { @@ -500,6 +502,9 @@ func (c *keyStoreClient) DeleteProvisionerKeyByID(_ context.Context, req coderbo } func (c *keyStoreClient) ProvisionerKeyByName(_ context.Context, req coderbootstrap.ProvisionerKeyByNameRequest) (*codersdk.ProvisionerKey, error) { + if c.failLookup { + return nil, errors.New("injected lookup failure") + } slot := newKeySlot(req.CoderURL, req.OrganizationName, req.KeyName) keyID, ok := c.keys[slot] if !ok { @@ -578,6 +583,13 @@ func TestCoderProvisionerKey_IDLessRecordKeepsOtherProvisionersKey(t *testing.T) require.Contains(t, unverified.Message, urlB) require.Positive(t, result.RequeueAfter, "P must check the record at B again") + store.failLookup = true + _, err = f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: pName}) + require.NoError(t, err) + require.NotNil(t, meta.FindStatusCondition(f.get(ctx, t, p).Status.Conditions, coderv1alpha1.CoderProvisionerConditionUnverifiedKeyKept), + "a failed lookup must not clear the report of the key at B") + store.failLookup = false + delete(store.keys, slotB) f.reconcile(ctx, t, p) latest = f.get(ctx, t, p) From 45e64056a90d3a975aa537e4ac5929b69dcc49e2 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 17:15:12 +0000 Subject: [PATCH 10/10] fix: rebuild the UnverifiedKeyKept report from every pending entry without key IDs on each reconcile (#259) Change-Id: I0c64a3588a9c25c94fea84e3d8f8c9cf89fb1453 Signed-off-by: Thomas Kosiewski --- api/v1alpha1/coderprovisioner_types.go | 11 ++- docs/tutorials/deploy-coderprovisioner.md | 2 +- .../controller/coderprovisioner_controller.go | 41 ++++---- .../coderprovisioner_pendingkeys_test.go | 93 +++++++++++-------- 4 files changed, 79 insertions(+), 68 deletions(-) diff --git a/api/v1alpha1/coderprovisioner_types.go b/api/v1alpha1/coderprovisioner_types.go index 413978026..7d6655ec0 100644 --- a/api/v1alpha1/coderprovisioner_types.go +++ b/api/v1alpha1/coderprovisioner_types.go @@ -29,11 +29,12 @@ const ( // referenced credential Secret and setting passed the operator's checks. // When it is False, the operator keeps the last applied Deployment. CoderProvisionerConditionCredentialsReady = "CredentialsReady" - // CoderProvisionerConditionUnverifiedKeyKept is True while an entry in - // status.pendingKeys has no key IDs and Coder holds a key with its name. - // Nothing shows that this provisioner created that key, so the operator - // does not delete it. The operator removes the condition when no such - // entry is left. + // CoderProvisionerConditionUnverifiedKeyKept is True while + // status.pendingKeys holds an entry without key IDs that the operator + // could not remove: Coder holds a key with its name, which nothing shows + // this provisioner created, or the operator could not check Coder. The + // 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" // DefaultProvisionerKeySecretKey is the default data key for provisioner key secrets. diff --git a/docs/tutorials/deploy-coderprovisioner.md b/docs/tutorials/deploy-coderprovisioner.md index 8921ca672..47090eb52 100644 --- a/docs/tutorials/deploy-coderprovisioner.md +++ b/docs/tutorials/deploy-coderprovisioner.md @@ -78,7 +78,7 @@ kubectl get secret coderprovisioner-sample-provisioner-key -n coder 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 the entry stays, the condition `UnverifiedKeyKept` is `True` and names the key, and deletion of the provisioner leaves that 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 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"}]'`. diff --git a/internal/controller/coderprovisioner_controller.go b/internal/controller/coderprovisioner_controller.go index 81e9da224..d8b92780a 100644 --- a/internal/controller/coderprovisioner_controller.go +++ b/internal/controller/coderprovisioner_controller.go @@ -338,8 +338,8 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req } pending = append(next, target) } - var kept, unverified []coderv1alpha1.CoderProvisionerKeyRecord - lookupFailed := false + var kept []coderv1alpha1.CoderProvisionerKeyRecord + var unverified []string for _, record := range pending { if sameKeySlot(record, target) { kept = append(kept, record) @@ -349,21 +349,19 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req if revokeErr != nil { return ctrl.Result{}, revokeErr } - if cleanup != keyGone { - kept = append(kept, record) - } - if cleanup == keyUnverified { - unverified = append(unverified, record) + if cleanup == keyGone { + continue } - if cleanup == keyKept && record.KeyID == "" { - lookupFailed = true + kept = append(kept, record) + 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)) } } - // A failed lookup does not show that an unverified key is gone. - conditionChanged := false - if len(unverified) > 0 || !lookupFailed { - conditionChanged = setUnverifiedKeyCondition(provisioner, unverified) - } + conditionChanged := setUnverifiedKeyCondition(provisioner, unverified) if conditionChanged || !slices.Equal(kept, provisioner.Status.PendingKeys) { provisioner.Status.PendingKeys = kept if err := r.Status().Update(ctx, provisioner); err != nil { @@ -1058,24 +1056,19 @@ func (r *CoderProvisionerReconciler) revokeKey( } // setUnverifiedKeyCondition sets the UnverifiedKeyKept condition for the -// records in unverified, or removes it when there are none. It reports -// whether the conditions changed. -func setUnverifiedKeyCondition(provisioner *coderv1alpha1.CoderProvisioner, unverified []coderv1alpha1.CoderProvisionerKeyRecord) bool { +// pending entries without key IDs that unverified describes, or removes it +// when there are none. It reports whether the conditions changed. +func setUnverifiedKeyCondition(provisioner *coderv1alpha1.CoderProvisioner, unverified []string) bool { if len(unverified) == 0 { return meta.RemoveStatusCondition(&provisioner.Status.Conditions, coderv1alpha1.CoderProvisionerConditionUnverifiedKeyKept) } - keys := make([]string, 0, len(unverified)) - for _, record := range unverified { - keys = append(keys, fmt.Sprintf("%q in organization %q at %s", record.KeyName, record.OrganizationName, record.CoderURL)) - } return meta.SetStatusCondition(&provisioner.Status.Conditions, metav1.Condition{ Type: coderv1alpha1.CoderProvisionerConditionUnverifiedKeyKept, Status: metav1.ConditionTrue, ObservedGeneration: provisioner.Generation, Reason: "KeyIDUnknown", - Message: fmt.Sprintf("Coder holds provisioner key %s, and status.pendingKeys has no key ID for it, so nothing shows that this provisioner created it. "+ - "The operator does not delete it, because another provisioner or a person can use the same name. Delete it in Coder if nothing uses it.", - strings.Join(keys, ", ")), + Message: "status.pendingKeys holds entries without key IDs, and the operator deletes no key for them, because another provisioner or a person can use the same name: " + + strings.Join(unverified, "; ") + ". Delete such a key in Coder if nothing uses it.", }) } diff --git a/internal/controller/coderprovisioner_pendingkeys_test.go b/internal/controller/coderprovisioner_pendingkeys_test.go index 08af42413..eafd13791 100644 --- a/internal/controller/coderprovisioner_pendingkeys_test.go +++ b/internal/controller/coderprovisioner_pendingkeys_test.go @@ -455,14 +455,14 @@ func TestCoderProvisionerKey_RevertAfterFailedRotation(t *testing.T) { // per name in an organization. type keySlot struct{ coderURL, organizationName, keyName string } -// keyStoreClient is a Coder that keeps provisioner keys, one per slot. -// failEnsureAt fails each key create at that URL before it reaches Coder, and -// failLookup fails each key lookup by name. +// keyStoreClient is a Coder that keeps provisioner keys, one per slot. Each +// key create at a URL in failEnsureAt fails before it reaches Coder, and each +// key lookup by name at failLookupAt fails. type keyStoreClient struct { *fakeBootstrapClient keys map[keySlot]uuid.UUID - failEnsureAt string - failLookup bool + failEnsureAt []string + failLookupAt string } func newKeySlot(coderURL, organizationName, keyName string) keySlot { @@ -474,7 +474,7 @@ func (s keySlot) organizationID() uuid.UUID { } func (c *keyStoreClient) EnsureProvisionerKey(_ context.Context, req coderbootstrap.EnsureProvisionerKeyRequest) (coderbootstrap.EnsureProvisionerKeyResponse, error) { - if req.CoderURL == c.failEnsureAt { + if slices.Contains(c.failEnsureAt, req.CoderURL) { return coderbootstrap.EnsureProvisionerKeyResponse{}, errors.New("injected connection refused") } slot := newKeySlot(req.CoderURL, req.OrganizationName, req.KeyName) @@ -502,7 +502,7 @@ func (c *keyStoreClient) DeleteProvisionerKeyByID(_ context.Context, req coderbo } func (c *keyStoreClient) ProvisionerKeyByName(_ context.Context, req coderbootstrap.ProvisionerKeyByNameRequest) (*codersdk.ProvisionerKey, error) { - if c.failLookup { + if req.CoderURL == c.failLookupAt { return nil, errors.New("injected lookup failure") } slot := newKeySlot(req.CoderURL, req.OrganizationName, req.KeyName) @@ -514,10 +514,13 @@ func (c *keyStoreClient) ProvisionerKeyByName(_ context.Context, req coderbootst } // TestCoderProvisionerKey_IDLessRecordKeepsOtherProvisionersKey checks a -// provisioner P whose target moves to endpoint B, where provisioner Q holds -// its key under the same key name. P records the create at B without key IDs, -// and the create fails before it reaches Coder. Neither the cleanup after P -// moves back nor the deletion of P may delete the key of Q. +// 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. func TestCoderProvisionerKey_IDLessRecordKeepsOtherProvisionersKey(t *testing.T) { t.Parallel() @@ -531,7 +534,11 @@ func TestCoderProvisionerKey_IDLessRecordKeepsOtherProvisionersKey(t *testing.T) ctx := context.Background() f := newProvisionerCredentialFixture(ctx, t, "cprov-idless") createControlPlaneWithToken(ctx, t, f.namespace, "coder-b", "https://coder-b.example.com", "operator-token-b") - urlB := "http://coder-b." + f.namespace + ".svc.cluster.local:80" + createControlPlaneWithToken(ctx, t, f.namespace, "coder-c", "https://coder-c.example.com", "operator-token-c") + apiURL := func(controlPlane string) string { + return "http://" + controlPlane + "." + f.namespace + ".svc.cluster.local:80" + } + urlB, urlC := apiURL("coder-b"), apiURL("coder-c") store := &keyStoreClient{fakeBootstrapClient: f.bootstrapClient, keys: map[keySlot]uuid.UUID{}} f.reconciler.BootstrapClient = store @@ -549,19 +556,29 @@ func TestCoderProvisionerKey_IDLessRecordKeepsOtherProvisionersKey(t *testing.T) pName := types.NamespacedName{Name: p.Name, Namespace: p.Namespace} f.reconcile(ctx, t, p) f.reconcile(ctx, t, p) + reconcileP := func() ctrl.Result { + result, err := f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: pName}) + require.NoError(t, err) + return result + } moveTo := func(controlPlane string) { latest := f.get(ctx, t, p) latest.Spec.ControlPlaneRef.Name = controlPlane require.NoError(t, k8sClient.Update(ctx, latest)) } + unverifiedReport := func() *metav1.Condition { + return meta.FindStatusCondition(f.get(ctx, t, p).Status.Conditions, coderv1alpha1.CoderProvisionerConditionUnverifiedKeyKept) + } - store.failEnsureAt = urlB - moveTo("coder-b") - _, err := f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: pName}) - require.Error(t, err, "the create at B must fail") - require.True(t, slices.ContainsFunc(f.get(ctx, t, p).Status.PendingKeys, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { - return record.CoderURL == urlB && record.KeyName == "provisioner" && record.KeyID == "" - }), "P must record its create at B without key IDs") + store.failEnsureAt = []string{urlB, urlC} + for _, controlPlane := range []string{"coder-b", "coder-c"} { + moveTo(controlPlane) + _, err := f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: pName}) + require.Error(t, err, "the create at %s must fail", controlPlane) + 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 deleteP { require.NoError(t, k8sClient.Delete(ctx, f.get(ctx, t, p))) @@ -571,30 +588,30 @@ func TestCoderProvisionerKey_IDLessRecordKeepsOtherProvisionersKey(t *testing.T) return } + store.failLookupAt = urlC moveTo("coder") - result, err := f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: pName}) - require.NoError(t, err) + result := reconcileP() require.Equal(t, qKeyID, store.keys[slotB].String(), "the key of Q at B must still exist") - latest := f.get(ctx, t, p) - require.Len(t, latest.Status.PendingKeys, 1, "the record at B must stay while Coder holds a key with its name") - unverified := meta.FindStatusCondition(latest.Status.Conditions, coderv1alpha1.CoderProvisionerConditionUnverifiedKeyKept) - require.NotNil(t, unverified, "P must report the key at B that it does not delete") - require.Equal(t, metav1.ConditionTrue, unverified.Status) - require.Contains(t, unverified.Message, urlB) - require.Positive(t, result.RequeueAfter, "P must check the record at B again") - - store.failLookup = true - _, err = f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: pName}) - require.NoError(t, err) - require.NotNil(t, meta.FindStatusCondition(f.get(ctx, t, p).Status.Conditions, coderv1alpha1.CoderProvisionerConditionUnverifiedKeyKept), - "a failed lookup must not clear the report of the key at B") - store.failLookup = false + require.Len(t, f.get(ctx, t, p).Status.PendingKeys, 2, "the entries at B and C must stay") + report := unverifiedReport() + require.NotNil(t, report, "P must report the entries that it keeps") + require.Equal(t, metav1.ConditionTrue, report.Status) + require.Contains(t, report.Message, urlB) + require.Contains(t, report.Message, urlC) + require.Positive(t, result.RequeueAfter, "P must check the entries again") delete(store.keys, slotB) + reconcileP() + require.Len(t, f.get(ctx, t, p).Status.PendingKeys, 1, "the entry at B must go once Coder has no key with its name") + report = unverifiedReport() + require.NotNil(t, report, "a failed lookup must keep the report of the entry at C") + require.NotContains(t, report.Message, urlB, "the report must not name the entry at B any more") + require.Contains(t, report.Message, urlC) + + store.failLookupAt = "" f.reconcile(ctx, t, p) - latest = f.get(ctx, t, p) - require.Empty(t, latest.Status.PendingKeys, "the record at B must go once Coder has no key with its name") - require.Nil(t, meta.FindStatusCondition(latest.Status.Conditions, coderv1alpha1.CoderProvisionerConditionUnverifiedKeyKept)) + require.Empty(t, f.get(ctx, t, p).Status.PendingKeys) + require.Nil(t, unverifiedReport()) }) } }