diff --git a/api/v1alpha1/coderprovisioner_types.go b/api/v1alpha1/coderprovisioner_types.go index 7d6655ec..952878d4 100644 --- a/api/v1alpha1/coderprovisioner_types.go +++ b/api/v1alpha1/coderprovisioner_types.go @@ -36,6 +36,13 @@ const ( // operator deletes no key for such an entry. The message names each // entry, and the operator removes the condition when none is left. CoderProvisionerConditionUnverifiedKeyKept = "UnverifiedKeyKept" + // CoderProvisionerConditionControlPlaneChangedKeyKept is True while + // status.pendingKeys holds an entry whose control plane has another UID + // than the entry records, or an entry without a UID whose coderURL is not + // the current Coder API URL of its control plane. The operator sends no + // operator token for such an entry. The message names each entry, and the + // operator removes the condition when none is left. + CoderProvisionerConditionControlPlaneChangedKeyKept = "ControlPlaneChangedKeyKept" // DefaultProvisionerKeySecretKey is the default data key for provisioner key secrets. DefaultProvisionerKeySecretKey = "key" @@ -110,6 +117,15 @@ type CoderProvisionerStatus struct { ControlPlaneRefName string `json:"controlPlaneRefName,omitempty"` // ControlPlaneURL is the control plane URL last applied to the provisioner key. ControlPlaneURL string `json:"controlPlaneURL,omitempty"` + // ControlPlaneUID is the UID of the control plane last applied to the + // provisioner key. The operator sends the operator token of the control + // plane with controlPlaneRefName for this key only while that control + // plane has this UID, and rotates the key when it has another UID. When it + // is empty, it sends that token only to the current Coder API URL of that + // control plane, and the next reconcile looks the key up there by + // organizationID and provisionerKeyID: it sets this field when Coder holds + // the key, and creates a new key when Coder answers that it has none. + ControlPlaneUID string `json:"controlPlaneUID,omitempty"` // SecretRef references the provisioner key secret data currently in use. SecretRef *SecretKeySelector `json:"secretRef,omitempty"` // PendingKeys lists the provisioner keys that can exist in Coder besides @@ -129,6 +145,15 @@ type CoderProvisionerKeyRecord struct { // ControlPlaneRefName is the CoderControlPlane whose operator token the // operator used for the key. ControlPlaneRefName string `json:"controlPlaneRefName"` + // ControlPlaneUID is the UID of that CoderControlPlane. The operator + // sends its operator token for this entry only while the control plane + // with controlPlaneRefName has this UID. When it is empty, it sends that + // token only when coderURL is the current Coder API URL of that control + // plane. Otherwise it keeps the entry and sets the condition + // ControlPlaneChangedKeyKept. When a rotation replaces the key of status + // without status.controlPlaneUID, the entry of that key records the UID + // while coderURL is the current Coder API URL of that control plane. + ControlPlaneUID string `json:"controlPlaneUID,omitempty"` // CoderURL is the Coder API URL that the operator sent the key request to. CoderURL string `json:"coderURL"` // OrganizationName is the Coder organization of the key. diff --git a/config/crd/bases/coder.com_coderprovisioners.yaml b/config/crd/bases/coder.com_coderprovisioners.yaml index 09d2b0b5..77bf93d8 100644 --- a/config/crd/bases/coder.com_coderprovisioners.yaml +++ b/config/crd/bases/coder.com_coderprovisioners.yaml @@ -417,6 +417,17 @@ spec: description: ControlPlaneRefName is the control plane ref name last applied to the provisioner key. type: string + controlPlaneUID: + description: |- + ControlPlaneUID is the UID of the control plane last applied to the + provisioner key. The operator sends the operator token of the control + plane with controlPlaneRefName for this key only while that control + plane has this UID, and rotates the key when it has another UID. When it + is empty, it sends that token only to the current Coder API URL of that + control plane, and the next reconcile looks the key up there by + organizationID and provisionerKeyID: it sets this field when Coder holds + the key, and creates a new key when Coder answers that it has none. + type: string controlPlaneURL: description: ControlPlaneURL is the control plane URL last applied to the provisioner key. @@ -457,6 +468,17 @@ spec: ControlPlaneRefName is the CoderControlPlane whose operator token the operator used for the key. type: string + controlPlaneUID: + description: |- + ControlPlaneUID is the UID of that CoderControlPlane. The operator + sends its operator token for this entry only while the control plane + with controlPlaneRefName has this UID. When it is empty, it sends that + token only when coderURL is the current Coder API URL of that control + plane. Otherwise it keeps the entry and sets the condition + ControlPlaneChangedKeyKept. When a rotation replaces the key of status + without status.controlPlaneUID, the entry of that key records the UID + while coderURL is the current Coder API URL of that control plane. + type: string keyID: description: KeyID is the provisioner key ID that Coder returned. type: string diff --git a/dist/install.yaml b/dist/install.yaml index 4b9492e8..9a0c3df5 100644 --- a/dist/install.yaml +++ b/dist/install.yaml @@ -6185,6 +6185,17 @@ spec: description: ControlPlaneRefName is the control plane ref name last applied to the provisioner key. type: string + controlPlaneUID: + description: |- + ControlPlaneUID is the UID of the control plane last applied to the + provisioner key. The operator sends the operator token of the control + plane with controlPlaneRefName for this key only while that control + plane has this UID, and rotates the key when it has another UID. When it + is empty, it sends that token only to the current Coder API URL of that + control plane, and the next reconcile looks the key up there by + organizationID and provisionerKeyID: it sets this field when Coder holds + the key, and creates a new key when Coder answers that it has none. + type: string controlPlaneURL: description: ControlPlaneURL is the control plane URL last applied to the provisioner key. @@ -6225,6 +6236,17 @@ spec: ControlPlaneRefName is the CoderControlPlane whose operator token the operator used for the key. type: string + controlPlaneUID: + description: |- + ControlPlaneUID is the UID of that CoderControlPlane. The operator + sends its operator token for this entry only while the control plane + with controlPlaneRefName has this UID. When it is empty, it sends that + token only when coderURL is the current Coder API URL of that control + plane. Otherwise it keeps the entry and sets the condition + ControlPlaneChangedKeyKept. When a rotation replaces the key of status + without status.controlPlaneUID, the entry of that key records the UID + while coderURL is the current Coder API URL of that control plane. + type: string keyID: description: KeyID is the provisioner key ID that Coder returned. type: string diff --git a/docs/reference/api/coderprovisioner.md b/docs/reference/api/coderprovisioner.md index 9394015f..655ae2b9 100644 --- a/docs/reference/api/coderprovisioner.md +++ b/docs/reference/api/coderprovisioner.md @@ -40,6 +40,7 @@ | `tagsHash` | string | TagsHash is a deterministic hash of spec.tags last applied to the provisioner key. | | `controlPlaneRefName` | string | ControlPlaneRefName is the control plane ref name last applied to the provisioner key. | | `controlPlaneURL` | string | ControlPlaneURL is the control plane URL last applied to the provisioner key. | +| `controlPlaneUID` | string | ControlPlaneUID is the UID of the control plane last applied to the provisioner key. The operator sends the operator token of the control plane with controlPlaneRefName for this key only while that control plane has this UID, and rotates the key when it has another UID. When it is empty, it sends that token only to the current Coder API URL of that control plane, and the next reconcile looks the key up there by organizationID and provisionerKeyID: it sets this field when Coder holds the key, and creates a new key when Coder answers that it has none. | | `secretRef` | [SecretKeySelector](#secretkeyselector) | SecretRef references the provisioner key secret data currently in use. | | `pendingKeys` | [CoderProvisionerKeyRecord](#coderprovisionerkeyrecord) array | PendingKeys lists the provisioner keys that can exist in Coder besides the key in provisionerKeyID: a key that the operator is about to create or has not yet written to the provisioner key Secret, and replaced keys whose deletion Coder has not confirmed. The operator deletes the key of each entry with organizationID and keyID at its coderURL with the operator token of its control plane, and removes the entry when Coder confirms that the key is gone. | @@ -53,6 +54,7 @@ or was about to create, at one Coder endpoint. | Field | Type | Description | | --- | --- | --- | | `controlPlaneRefName` | string | ControlPlaneRefName is the CoderControlPlane whose operator token the operator used for the key. | +| `controlPlaneUID` | string | ControlPlaneUID is the UID of that CoderControlPlane. The operator sends its operator token for this entry only while the control plane with controlPlaneRefName has this UID. When it is empty, it sends that token only when coderURL is the current Coder API URL of that control plane. Otherwise it keeps the entry and sets the condition ControlPlaneChangedKeyKept. When a rotation replaces the key of status without status.controlPlaneUID, the entry of that key records the UID while coderURL is the current Coder API URL of that control plane. | | `coderURL` | string | CoderURL is the Coder API URL that the operator sent the key request to. | | `organizationName` | string | OrganizationName is the Coder organization of the key. | | `keyName` | string | KeyName is the provisioner key name in Coder. | diff --git a/docs/tutorials/deploy-coderprovisioner.md b/docs/tutorials/deploy-coderprovisioner.md index 47090eb5..9dcc043f 100644 --- a/docs/tutorials/deploy-coderprovisioner.md +++ b/docs/tutorials/deploy-coderprovisioner.md @@ -54,6 +54,8 @@ Expected result: the phase is `Ready`, and the conditions include `CredentialsRe The operator creates the provisioner key Secret and owns it. If you set `spec.key.secretName`, name a Secret that does not exist yet: the operator refuses a Secret that it does not own (reason `KeySecretNotOwned`). +Each provisioner needs its own key name in its Coder organization. The key name is `spec.key.name`, or the provisioner name when `spec.key.name` is empty. Before the operator creates or replaces a key, it looks up the key name in Coder. When Coder holds a key with that name, and the status of this provisioner does not record its ID, the operator does not delete or replace that key, because another `CoderProvisioner` (for example one with the same name in another namespace) or a person can use it. It keeps the current key, Secret and Deployment of this provisioner, and only deletes the replaced keys that `status.pendingKeys` records for earlier targets: `ProvisionerKeyReady` is `False` with reason `KeyNameInUse`, and the message names the key and its ID. Set `spec.key.name` to a name that nothing else uses, or delete that key in Coder if nothing uses it. The operator checks again every 2 minutes. The same happens when the operator lost the answer to a key create, because status then has no ID of the new key, and for status without key records, for example after a restore without status. + If you add `spec.extraEnv` entries with a `secretKeyRef`, each Secret needs the label `coder.com/credential=true` and the annotation `coder.com/credential-destinations` with the origin of the control plane `status.url`. Otherwise `CredentialsReady` is `False` and the operator does not change the Deployment. See [Configure credential Secrets for proxies and provisioners](../how-to/workspace-proxy-credentials.md). The operator creates these resources in the `coder` namespace: @@ -80,7 +82,7 @@ kubectl delete coderprovisioner coderprovisioner-sample -n coder The operator then deletes the provisioner keys in Coder that this provisioner created. It finds the key in the provisioner key Secret by the organization and key IDs in `status.organizationID` and `status.provisionerKeyID`, not by the names in the spec. It also deletes the key of each entry in `status.pendingKeys` that has an `organizationID` and a `keyID`. The entries name keys that the operator created but did not write to the Secret, for example because a reconcile failed, and keys that a rotation replaced but that Coder did not confirm as deleted. Each entry names the Coder URL of the key and the control plane whose operator token the operator uses to delete it. An entry without these IDs names a key that the operator started to create before Coder returned its ID. For such an entry, the operator deletes no key, because another provisioner or a person can use the same key name in that organization. It removes the entry when Coder has no key with that name, or when that key is the key in `status.provisionerKeyID`. Otherwise, and while the operator cannot reach Coder for that check, the entry stays, the condition `UnverifiedKeyKept` is `True` and names the entry, and deletion of the provisioner leaves the key in Coder. If no other provisioner or person uses the key, delete it with `coder provisioner keys delete --org `. The operator checks again every 2 minutes, and removes the entry and the condition once the key is gone. When the status has no key, for example because the provisioner never passed its checks, the operator removes its finalizer without a call to Coder. -The same records apply while the provisioner runs. When `spec.controlPlaneRef`, the control plane URL, the organization, the key name, or the tags change, the operator creates a new key and deletes the replaced keys, each at its own Coder URL. When a rotation fails after Coder created the new key, the next reconcile writes another new key to the Secret, also when you reverted the change. An entry stays in `status.pendingKeys` until Coder confirms that its key is gone, and the operator tries again every 2 minutes. An entry whose control plane was deleted stays for good: delete its key by hand in Coder, then remove the entry, for example the first one with `kubectl patch coderprovisioner coderprovisioner-sample -n coder --subresource=status --type=json -p '[{"op":"remove","path":"/status/pendingKeys/0"}]'`. +The same records apply while the provisioner runs. When `spec.controlPlaneRef`, the control plane URL, the organization, the key name, or the tags change, the operator creates a new key and deletes the replaced keys by their recorded organization and key IDs, each at its own Coder URL, so a renamed Coder organization does not hide a replaced key. When a rotation fails after Coder created the new key, the next reconcile writes another new key to the Secret, also when you reverted the change. An entry stays in `status.pendingKeys` until Coder confirms that its key is gone, and the operator tries again every 2 minutes. Each entry and `status.controlPlaneUID` also record the UID of the control plane. The operator sends the operator token of a control plane for a record only while the control plane has the recorded UID. When a control plane is deleted and created again under the same name, the new one can run another Coder backend, also at the same URL. The operator then creates a new key for the provisioner, and sends the new token for no key of the old control plane. Such an entry stays, the condition `ControlPlaneChangedKeyKept` is `True` and names it, and deletion of the provisioner leaves that key in Coder. A record without a UID, from an operator version that did not record it, counts as bound to its control plane only at the current URL of that control plane. For status without `status.controlPlaneUID`, the next reconcile looks the applied key up at the current control plane by `status.organizationID` and `status.provisionerKeyID`, so a renamed organization does not hide it. When Coder holds the key there, the operator records the UID of that control plane. When Coder answers that it has no such key, the operator creates a new key. When a change of the spec or the control plane replaces the key of such status, its entry in `status.pendingKeys` records the UID of its control plane while that control plane has the recorded URL. An entry whose control plane was deleted, or that `ControlPlaneChangedKeyKept` names, stays for good: delete its key by hand in Coder, then remove the entry, for example the first one with `kubectl patch coderprovisioner coderprovisioner-sample -n coder --subresource=status --type=json -p '[{"op":"remove","path":"/status/pendingKeys/0"}]'`. This cleanup is best-effort. When the operator cannot reach Coder, for example because the control plane or its operator token is gone, or when the delete call fails, the operator still removes its finalizer, and the key stays valid in Coder. Delete the provisioner while the control plane runs. If cleanup could not reach Coder, list the keys with `coder provisioner keys list --org `, and delete the key by hand with `coder provisioner keys delete --org `. diff --git a/internal/coderbootstrap/client.go b/internal/coderbootstrap/client.go index e2385d1b..70b5445f 100644 --- a/internal/coderbootstrap/client.go +++ b/internal/coderbootstrap/client.go @@ -94,8 +94,8 @@ type RegisterWorkspaceProxyResponse struct { type Client interface { EnsureWorkspaceProxy(context.Context, RegisterWorkspaceProxyRequest) (RegisterWorkspaceProxyResponse, error) EnsureProvisionerKey(context.Context, EnsureProvisionerKeyRequest) (EnsureProvisionerKeyResponse, error) - DeleteProvisionerKey(ctx context.Context, coderURL, sessionToken, orgName, keyName string) error DeleteProvisionerKeyByID(context.Context, DeleteProvisionerKeyByIDRequest) error + ProvisionerKeyByID(context.Context, ProvisionerKeyByIDRequest) (*codersdk.ProvisionerKey, error) ProvisionerKeyByName(context.Context, ProvisionerKeyByNameRequest) (*codersdk.ProvisionerKey, error) Entitlements(ctx context.Context, coderURL, sessionToken string) (codersdk.Entitlements, error) } diff --git a/internal/coderbootstrap/provisionerkeys.go b/internal/coderbootstrap/provisionerkeys.go index 785cfb1c..2b9f3e39 100644 --- a/internal/coderbootstrap/provisionerkeys.go +++ b/internal/coderbootstrap/provisionerkeys.go @@ -104,43 +104,6 @@ func (c *SDKClient) EnsureProvisionerKey(ctx context.Context, req EnsureProvisio }, nil } -// DeleteProvisionerKey deletes a provisioner key by name. -// A missing key is treated as success for idempotency. -func (c *SDKClient) DeleteProvisionerKey(ctx context.Context, coderURL, sessionToken, orgName, keyName string) error { - if err := validateProvisionerKeyInputs(coderURL, sessionToken, keyName); err != nil { - return err - } - - client, err := newAuthenticatedClient(coderURL, sessionToken) - if err != nil { - return err - } - - organizationName := orgName - if organizationName == "" { - organizationName = codersdk.DefaultOrganization - } - - organization, err := resolveOrganizationByName(ctx, client, organizationName) - if err != nil { - return err - } - - _, err = withOptionalRateLimitBypass(ctx, func(requestCtx context.Context) (struct{}, error) { - return struct{}{}, client.DeleteProvisionerKey(requestCtx, organization.ID, keyName) - }) - if err == nil { - return nil - } - - var apiErr *codersdk.Error - if errors.As(err, &apiErr) && apiErr.StatusCode() == http.StatusNotFound { - return nil - } - - return xerrors.Errorf("delete provisioner key %q: %w", keyName, err) -} - // ProvisionerKeyByNameRequest identifies a provisioner key by its name. type ProvisionerKeyByNameRequest struct { CoderURL string @@ -185,6 +148,27 @@ func (c *SDKClient) ProvisionerKeyByName(ctx context.Context, req ProvisionerKey return key, nil } +// ProvisionerKeyByIDRequest identifies one provisioner key by the IDs that +// the caller recorded when it applied the key. +type ProvisionerKeyByIDRequest struct { + CoderURL string + SessionToken string + OrganizationID uuid.UUID + KeyID uuid.UUID +} + +// ProvisionerKeyByID returns the provisioner key with req.KeyID in the +// organization req.OrganizationID. It returns nil when Coder answers that the +// organization does not exist, or lists no key with that ID in it. +func (c *SDKClient) ProvisionerKeyByID(ctx context.Context, req ProvisionerKeyByIDRequest) (*codersdk.ProvisionerKey, error) { + client, err := newProvisionerKeyByIDClient(req) + if err != nil { + return nil, err + } + + return findProvisionerKeyByID(ctx, client, req.OrganizationID, req.KeyID) +} + // DeleteProvisionerKeyByIDRequest identifies one provisioner key by the IDs // that the caller recorded when it applied the key. type DeleteProvisionerKeyByIDRequest struct { @@ -201,56 +185,71 @@ type DeleteProvisionerKeyByIDRequest struct { // and the delete are two requests: a key deleted and created again with the // same name between them is deleted too. func (c *SDKClient) DeleteProvisionerKeyByID(ctx context.Context, req DeleteProvisionerKeyByIDRequest) error { + client, err := newProvisionerKeyByIDClient(ProvisionerKeyByIDRequest(req)) + if err != nil { + return err + } + + match, err := findProvisionerKeyByID(ctx, client, req.OrganizationID, req.KeyID) + if err != nil { + return err + } + if match == nil { + return nil + } + + _, err = withOptionalRateLimitBypass(ctx, func(requestCtx context.Context) (struct{}, error) { + return struct{}{}, client.DeleteProvisionerKey(requestCtx, req.OrganizationID, match.Name) + }) + if err != nil && !isNotFound(err) { + return xerrors.Errorf("delete provisioner key %q: %w", match.Name, err) + } + + return nil +} + +func newProvisionerKeyByIDClient(req ProvisionerKeyByIDRequest) (*codersdk.Client, error) { if req.CoderURL == "" { - return xerrors.New("coder URL is required") + return nil, xerrors.New("coder URL is required") } if req.SessionToken == "" { - return xerrors.New("session token is required") + return nil, xerrors.New("session token is required") } if req.OrganizationID == uuid.Nil { - return xerrors.New("organization ID is required") + return nil, xerrors.New("organization ID is required") } if req.KeyID == uuid.Nil { - return xerrors.New("provisioner key ID is required") + return nil, xerrors.New("provisioner key ID is required") } - client, err := newAuthenticatedClient(req.CoderURL, req.SessionToken) - if err != nil { - return err - } + return newAuthenticatedClient(req.CoderURL, req.SessionToken) +} +// findProvisionerKeyByID returns the key with keyID in the organization +// organizationID, or nil when Coder answers that the organization does not +// exist or lists no key with that ID. +func findProvisionerKeyByID(ctx context.Context, client *codersdk.Client, organizationID, keyID uuid.UUID) (*codersdk.ProvisionerKey, error) { keys, err := withOptionalRateLimitBypass(ctx, func(requestCtx context.Context) ([]codersdk.ProvisionerKey, error) { - return client.ListProvisionerKeys(requestCtx, req.OrganizationID) + return client.ListProvisionerKeys(requestCtx, organizationID) }) if isNotFound(err) { - return nil + return nil, nil } if err != nil { - return xerrors.Errorf("list provisioner keys for organization %q: %w", req.OrganizationID, err) + return nil, xerrors.Errorf("list provisioner keys for organization %q: %w", organizationID, err) } - var match *codersdk.ProvisionerKey for i := range keys { - if keys[i].ID == req.KeyID { - match = &keys[i] - break + if keys[i].ID != keyID { + continue } - } - if match == nil { - return nil - } - if match.Name == "" { - return xerrors.Errorf("assertion failed: provisioner key %q has an empty name", req.KeyID) - } - - _, err = withOptionalRateLimitBypass(ctx, func(requestCtx context.Context) (struct{}, error) { - return struct{}{}, client.DeleteProvisionerKey(requestCtx, req.OrganizationID, match.Name) - }) - if err != nil && !isNotFound(err) { - return xerrors.Errorf("delete provisioner key %q: %w", match.Name, err) + if keys[i].Name == "" { + return nil, xerrors.Errorf("assertion failed: provisioner key %q has an empty name", keyID) + } + return &keys[i], nil } - return nil + return nil, nil } // coderNotFoundMessage is the message of the 404 that Coder answers for a diff --git a/internal/coderbootstrap/provisionerkeys_deletebyid_test.go b/internal/coderbootstrap/provisionerkeys_deletebyid_test.go index 48f72653..87e97a5c 100644 --- a/internal/coderbootstrap/provisionerkeys_deletebyid_test.go +++ b/internal/coderbootstrap/provisionerkeys_deletebyid_test.go @@ -90,6 +90,73 @@ func TestDeleteProvisionerKeyByID(t *testing.T) { } } +func TestProvisionerKeyByID(t *testing.T) { + t.Parallel() + + orgID := uuid.New() + recordedKeyID := uuid.New() + now := time.Now().UTC().Format(time.RFC3339) + keysPath := "/api/v2/organizations/" + orgID.String() + "/provisionerkeys" + + tests := []struct { + name string + listStatus int + listBody map[string]any + listedID uuid.UUID + wantKey bool + wantErr bool + }{ + {name: "recorded key exists", listStatus: http.StatusOK, listedID: recordedKeyID, wantKey: true}, + {name: "same name, other ID", listStatus: http.StatusOK, listedID: uuid.New()}, + {name: "organization gone", listStatus: http.StatusNotFound, listBody: coderNotFound}, + {name: "404 not from Coder", listStatus: http.StatusNotFound, listBody: map[string]any{"message": "Route not found."}, wantErr: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet || r.URL.Path != keysPath { + t.Errorf("unexpected request %s %s", r.Method, r.URL.Path) + w.WriteHeader(http.StatusNotFound) + return + } + if tt.listStatus != http.StatusOK { + writeJSONResponse(t, w, tt.listStatus, tt.listBody) + return + } + writeJSONResponse(t, w, http.StatusOK, []map[string]any{{ + "id": tt.listedID.String(), + "created_at": now, + "organization": orgID.String(), + "name": "shared-name", + "tags": map[string]string{}, + }}) + })) + defer server.Close() + + key, err := coderbootstrap.NewSDKClient().ProvisionerKeyByID(context.Background(), coderbootstrap.ProvisionerKeyByIDRequest{ + CoderURL: server.URL, + SessionToken: "session-token", + OrganizationID: orgID, + KeyID: recordedKeyID, + }) + if tt.wantErr { + require.Error(t, err, "only Coder's own 404 proves that the key is gone") + return + } + require.NoError(t, err) + if !tt.wantKey { + require.Nil(t, key) + return + } + require.NotNil(t, key) + require.Equal(t, recordedKeyID, key.ID) + }) + } +} + func TestDeleteProvisionerKeyByID_ValidationErrors(t *testing.T) { t.Parallel() diff --git a/internal/coderbootstrap/provisionerkeys_test.go b/internal/coderbootstrap/provisionerkeys_test.go index 57728940..d5abdec1 100644 --- a/internal/coderbootstrap/provisionerkeys_test.go +++ b/internal/coderbootstrap/provisionerkeys_test.go @@ -226,49 +226,6 @@ func TestEnsureProvisionerKey_FallsBackWhenRateLimitBypassRejected(t *testing.T) require.Equal(t, "plaintext-provisioner-key", resp.Key) } -func TestDeleteProvisionerKey_FallsBackWhenRateLimitBypassRejected(t *testing.T) { - t.Parallel() - - const keyName = "delete-me" - orgID := uuid.New() - now := time.Now().UTC().Format(time.RFC3339) - deleteCalls := 0 - bypassRejectedCalls := 0 - - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.Header.Get(codersdk.BypassRatelimitHeader) == "true" { - bypassRejectedCalls++ - writeJSONResponse(t, w, http.StatusPreconditionRequired, map[string]any{"message": "bypass is not allowed"}) - return - } - - switch { - case r.Method == http.MethodGet && r.URL.Path == "/api/v2/organizations/default": - writeJSONResponse(t, w, http.StatusOK, map[string]any{ - "id": orgID.String(), - "name": "default", - "created_at": now, - "updated_at": now, - }) - return - case r.Method == http.MethodDelete && r.URL.Path == "/api/v2/organizations/"+orgID.String()+"/provisionerkeys/"+keyName: - deleteCalls++ - w.WriteHeader(http.StatusNoContent) - return - default: - writeJSONResponse(t, w, http.StatusNotFound, map[string]any{"message": "unexpected route"}) - return - } - })) - defer server.Close() - - client := coderbootstrap.NewSDKClient() - err := client.DeleteProvisionerKey(context.Background(), server.URL, "session-token", "", keyName) - require.NoError(t, err) - require.Equal(t, 1, deleteCalls) - require.Equal(t, 1, bypassRejectedCalls) -} - func TestEnsureProvisionerKey_ValidationErrors(t *testing.T) { t.Parallel() @@ -410,78 +367,6 @@ func TestEntitlements_ValidationErrors(t *testing.T) { require.Contains(t, err.Error(), "session token is required") } -func TestDeleteProvisionerKey_Success(t *testing.T) { - t.Parallel() - - const keyName = "delete-me" - orgID := uuid.New() - now := time.Now().UTC().Format(time.RFC3339) - deleteCalls := 0 - - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - switch { - case r.Method == http.MethodGet && r.URL.Path == "/api/v2/organizations/default": - writeJSONResponse(t, w, http.StatusOK, map[string]any{ - "id": orgID.String(), - "name": "default", - "created_at": now, - "updated_at": now, - }) - return - case r.Method == http.MethodDelete && r.URL.Path == "/api/v2/organizations/"+orgID.String()+"/provisionerkeys/"+keyName: - deleteCalls++ - w.WriteHeader(http.StatusNoContent) - return - default: - writeJSONResponse(t, w, http.StatusNotFound, map[string]any{"message": "unexpected route"}) - return - } - })) - defer server.Close() - - client := coderbootstrap.NewSDKClient() - err := client.DeleteProvisionerKey(context.Background(), server.URL, "session-token", "", keyName) - require.NoError(t, err) - require.Equal(t, 1, deleteCalls) -} - -func TestDeleteProvisionerKey_NotFound(t *testing.T) { - t.Parallel() - - const keyName = "already-deleted" - orgID := uuid.New() - now := time.Now().UTC().Format(time.RFC3339) - deleteCalls := 0 - - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - switch { - case r.Method == http.MethodGet && r.URL.Path == "/api/v2/organizations/default": - writeJSONResponse(t, w, http.StatusOK, map[string]any{ - "id": orgID.String(), - "name": "default", - "created_at": now, - "updated_at": now, - }) - return - case r.Method == http.MethodDelete && r.URL.Path == "/api/v2/organizations/"+orgID.String()+"/provisionerkeys/"+keyName: - deleteCalls++ - writeJSONResponse(t, w, http.StatusNotFound, map[string]any{ - "message": "provisioner key not found", - }) - return - default: - writeJSONResponse(t, w, http.StatusNotFound, map[string]any{"message": "unexpected route"}) - return - } - })) - defer server.Close() - - client := coderbootstrap.NewSDKClient() - err := client.DeleteProvisionerKey(context.Background(), server.URL, "session-token", "", keyName) - require.NoError(t, err) - require.Equal(t, 1, deleteCalls) -} - func writeJSONResponse(t *testing.T, w http.ResponseWriter, statusCode int, payload any) { t.Helper() diff --git a/internal/controller/coderprovisioner_controller.go b/internal/controller/coderprovisioner_controller.go index d8b92780..cbaf2dd9 100644 --- a/internal/controller/coderprovisioner_controller.go +++ b/internal/controller/coderprovisioner_controller.go @@ -57,6 +57,7 @@ const ( externalProvisionerEntitlementRetryInterval = 2 * time.Minute provisionerOperatorAccessRetryInterval = 15 * time.Second provisionerPendingKeyRetryInterval = 2 * time.Minute + provisionerKeyNameInUseRetryInterval = 2 * time.Minute // #nosec G101 -- this is a field index key, not a credential. provisionerControlPlaneRefNameFieldIndex = ".spec.controlPlaneRef.name" @@ -261,11 +262,15 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req tagsDrift := status.TagsHash != "" && status.TagsHash != desiredTagsHash controlPlaneRefDrift := status.ControlPlaneRefName != "" && status.ControlPlaneRefName != desiredControlPlaneRefName controlPlaneURLDrift := status.ControlPlaneURL != "" && status.ControlPlaneURL != controlPlane.Status.URL - driftDetected := orgDrift || keyNameDrift || tagsDrift || controlPlaneRefDrift || controlPlaneURLDrift + // A control plane deleted and created again under the same name can run + // another Coder backend at the same URL, where the applied key is unknown. + controlPlaneUIDDrift := status.ControlPlaneUID != "" && status.ControlPlaneUID != string(controlPlane.UID) + driftDetected := orgDrift || keyNameDrift || tagsDrift || controlPlaneRefDrift || controlPlaneURLDrift || controlPlaneUIDDrift appliedOrgName := provisioner.Status.OrganizationName appliedTagsHash := provisioner.Status.TagsHash appliedControlPlaneRefName := provisioner.Status.ControlPlaneRefName appliedControlPlaneURL := provisioner.Status.ControlPlaneURL + appliedControlPlaneUID := provisioner.Status.ControlPlaneUID // Check whether a usable provisioner key secret already exists. // The secret is considered "usable" only if this provisioner owns it @@ -297,6 +302,7 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req // another slot than the target are deleted here. target := coderv1alpha1.CoderProvisionerKeyRecord{ ControlPlaneRefName: desiredControlPlaneRefName, + ControlPlaneUID: string(controlPlane.UID), CoderURL: coderURL, OrganizationName: organizationName, KeyName: keyName, @@ -309,21 +315,89 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req rotate := !secretUsable || slices.ContainsFunc(provisioner.Status.PendingKeys, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { return record.Unpublished }) + replacing := driftDetected || rotate || metadataMissing + // Status from an operator that did not record the control plane UID does + // not show that the applied key belongs to the Coder backend of the + // current control plane, which has the recorded name and URL here. The + // applied key is kept, and bound to that control plane, only when Coder + // there lists it under its recorded IDs. An organization can be renamed + // and its name reused, so the lookup does not use the name. + appliedGone := false + if !replacing && status.ControlPlaneUID == "" { + var applied *codersdk.ProvisionerKey + if appliedOrgID, appliedKeyID, ok := parseKeyIDs(status.OrganizationID, status.ProvisionerKeyID); ok { + applied, err = r.BootstrapClient.ProvisionerKeyByID(ctx, coderbootstrap.ProvisionerKeyByIDRequest{ + CoderURL: coderURL, + SessionToken: sessionToken, + OrganizationID: appliedOrgID, + KeyID: appliedKeyID, + }) + if err != nil { + setCondition(provisioner, coderv1alpha1.CoderProvisionerConditionProvisionerKeyReady, + metav1.ConditionFalse, "ProvisionerKeyFailed", + fmt.Sprintf("Failed to look up provisioner key %q", provisionerKeyName)) + _ = r.Status().Update(ctx, provisioner) + return ctrl.Result{}, fmt.Errorf("look up the applied provisioner key %q: %w", provisionerKeyName, err) + } + } + if applied != nil { + appliedControlPlaneUID = string(controlPlane.UID) + } else { + replacing = true + appliedGone = true + } + } + // Coder allows one key per name in an organization. A key with the target + // name that status does not record can belong to another provisioner or + // a person, so the operator neither deletes nor replaces it, and changes + // nothing else until the name is free. + var existingKey *codersdk.ProvisionerKey + if replacing { + existingKey, err = r.BootstrapClient.ProvisionerKeyByName(ctx, coderbootstrap.ProvisionerKeyByNameRequest{ + CoderURL: coderURL, + SessionToken: sessionToken, + OrganizationName: organizationName, + KeyName: keyName, + }) + if err != nil { + setCondition(provisioner, coderv1alpha1.CoderProvisionerConditionProvisionerKeyReady, + metav1.ConditionFalse, "ProvisionerKeyFailed", + fmt.Sprintf("Failed to look up provisioner key %q", keyName)) + _ = r.Status().Update(ctx, provisioner) + return ctrl.Result{}, fmt.Errorf("look up provisioner key %q: %w", keyName, err) + } + if existingKey != nil && !recordsKeyID(provisioner.Status, existingKey.ID) { + // Keys recorded at earlier targets do not wait for this name. + // Unpublished records stay, because the Secret can hold their key. + kept, _, revokeErr := r.revokePendingKeys(ctx, provisioner, provisioner.Status.PendingKeys, target, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { + return record.Unpublished + }) + if revokeErr != nil { + return ctrl.Result{}, revokeErr + } + provisioner.Status.PendingKeys = kept + return r.refuseKeyNameInUse(ctx, provisioner, coderURL, organizationName, keyName, existingKey.ID) + } + } pending := slices.Clone(provisioner.Status.PendingKeys) var replaced coderv1alpha1.CoderProvisionerKeyRecord - if driftDetected { + if replacing { replaced, err = r.appliedKeyRecord(ctx, provisioner, organizationName, keyName) if err != nil { return ctrl.Result{}, err } - if !sameKeySlot(replaced, target) && !slices.Contains(pending, replaced) { - pending = append(pending, replaced) + replaced, err = r.bindToControlPlane(ctx, provisioner.Namespace, replaced) + if err != nil { + return ctrl.Result{}, err } } - if driftDetected || rotate || metadataMissing { - // 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. + if driftDetected && !sameKeySlot(replaced, target) && !slices.Contains(pending, replaced) { + pending = append(pending, replaced) + } + if replacing { + // The newest target record with IDs stays until the replacement below + // deletes its key by ID. Each earlier replacement at the target + // already deleted the older ones by ID before its create. newest := -1 for i, record := range pending { if sameKeySlot(record, target) && record.KeyID != "" { @@ -338,30 +412,13 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req } pending = append(next, target) } - var kept []coderv1alpha1.CoderProvisionerKeyRecord - var unverified []string - for _, record := range pending { - if sameKeySlot(record, target) { - kept = append(kept, record) - continue - } - cleanup, revokeErr := r.revokeKey(ctx, provisioner, record) - if revokeErr != nil { - return ctrl.Result{}, revokeErr - } - if cleanup == keyGone { - continue - } - kept = append(kept, record) - if _, _, ok := parseKeyIDs(record.OrganizationID, record.KeyID); !ok { - note := "the operator could not check Coder" - if cleanup == keyUnverified { - note = "Coder holds a key with this name that nothing shows this provisioner created" - } - unverified = append(unverified, fmt.Sprintf("key %q in organization %q at %s: %s", record.KeyName, record.OrganizationName, record.CoderURL, note)) - } + // The replacement below deletes the key that the target holds. + kept, conditionChanged, err := r.revokePendingKeys(ctx, provisioner, pending, target, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { + return existingKey != nil && isKeyID(record.KeyID, existingKey.ID) + }) + if err != nil { + return ctrl.Result{}, err } - conditionChanged := setUnverifiedKeyCondition(provisioner, unverified) if conditionChanged || !slices.Equal(kept, provisioner.Status.PendingKeys) { provisioner.Status.PendingKeys = kept if err := r.Status().Update(ctx, provisioner); err != nil { @@ -373,109 +430,56 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req } } - log := ctrl.LoggerFrom(ctx) keyMaterial := "" - if driftDetected { - log.Info("spec drift detected, rotating provisioner key", - "orgDrift", orgDrift, "keyNameDrift", keyNameDrift, "tagsDrift", tagsDrift, - "controlPlaneRefDrift", controlPlaneRefDrift, "controlPlaneURLDrift", controlPlaneURLDrift) - - // A replaced key in another slot was deleted above. One in the target - // slot is deleted here, so that the create below can use its name. - if sameKeySlot(replaced, target) { - if deleteErr := r.BootstrapClient.DeleteProvisionerKey( - ctx, - coderURL, - sessionToken, - replaced.OrganizationName, - replaced.KeyName, - ); deleteErr != nil { - log.Info("failed to delete old provisioner key during drift rotation, creating new key anyway", - "oldKeyName", replaced.KeyName, "error", deleteErr) - } - } - - response, ensureErr := r.BootstrapClient.EnsureProvisionerKey(ctx, coderbootstrap.EnsureProvisionerKeyRequest{ - CoderURL: coderURL, - SessionToken: sessionToken, - OrganizationName: organizationName, - KeyName: keyName, - Tags: provisioner.Spec.Tags, - }) - if ensureErr != nil { - setCondition(provisioner, coderv1alpha1.CoderProvisionerConditionProvisionerKeyReady, - metav1.ConditionFalse, "ProvisionerKeyFailed", - fmt.Sprintf("Failed to ensure provisioner key %q after drift rotation", keyName)) - _ = r.Status().Update(ctx, provisioner) - return ctrl.Result{}, fmt.Errorf("ensure provisioner key %q: %w", keyName, ensureErr) - } - if response.OrganizationID != uuid.Nil { - organizationID = response.OrganizationID.String() - } - if response.KeyID != uuid.Nil { - provisionerKeyID = response.KeyID.String() + if replacing { + if driftDetected { + ctrl.LoggerFrom(ctx).Info("spec drift detected, rotating provisioner key", + "orgDrift", orgDrift, "keyNameDrift", keyNameDrift, "tagsDrift", tagsDrift, + "controlPlaneRefDrift", controlPlaneRefDrift, "controlPlaneURLDrift", controlPlaneURLDrift, + "controlPlaneUIDDrift", controlPlaneUIDDrift) + } + // Coder returns the plaintext of a key only in the create response, + // so a key recorded in status is replaced: deleted by its ID, then + // created again with the desired tags. An organization can be renamed + // and its name reused, so the lookup by name above can miss a key that + // status records at the target. Such keys are deleted by their + // recorded IDs too, before the create overwrites or drops the record. + var replacedKeys []coderbootstrap.DeleteProvisionerKeyByIDRequest + if existingKey != nil { + replacedKeys = append(replacedKeys, coderbootstrap.DeleteProvisionerKeyByIDRequest{ + CoderURL: coderURL, + SessionToken: sessionToken, + OrganizationID: existingKey.OrganizationID, + KeyID: existingKey.ID, + }) } - if response.KeyName != "" { - provisionerKeyName = response.KeyName + recorded := slices.Clone(provisioner.Status.PendingKeys) + if !appliedGone { + recorded = append(recorded, replaced) } - keyMaterial = response.Key - if keyMaterial == "" { - log.Info("drift-rotated key exists in coderd but returned no plaintext, rotating to recover", - "keyName", keyName) - - if deleteErr := r.BootstrapClient.DeleteProvisionerKey( - ctx, coderURL, sessionToken, organizationName, keyName, - ); deleteErr != nil { - setCondition(provisioner, coderv1alpha1.CoderProvisionerConditionProvisionerKeyReady, - metav1.ConditionFalse, "ProvisionerKeyFailed", - fmt.Sprintf("Failed to delete provisioner key %q for drift recovery", keyName)) - _ = r.Status().Update(ctx, provisioner) - return ctrl.Result{}, fmt.Errorf("delete provisioner key %q for drift recovery: %w", keyName, deleteErr) + for _, record := range recorded { + recordOrgID, recordKeyID, ok := parseKeyIDs(record.OrganizationID, record.KeyID) + if !ok || !sameKeySlot(record, target) || slices.ContainsFunc(replacedKeys, func(req coderbootstrap.DeleteProvisionerKeyByIDRequest) bool { + return req.KeyID == recordKeyID + }) { + continue } - rotated, rotateErr := r.BootstrapClient.EnsureProvisionerKey(ctx, coderbootstrap.EnsureProvisionerKeyRequest{ - CoderURL: coderURL, - SessionToken: sessionToken, - OrganizationName: organizationName, - KeyName: keyName, - Tags: provisioner.Spec.Tags, + replacedKeys = append(replacedKeys, coderbootstrap.DeleteProvisionerKeyByIDRequest{ + CoderURL: coderURL, + SessionToken: sessionToken, + OrganizationID: recordOrgID, + KeyID: recordKeyID, }) - if rotateErr != nil { - setCondition(provisioner, coderv1alpha1.CoderProvisionerConditionProvisionerKeyReady, - metav1.ConditionFalse, "ProvisionerKeyFailed", - fmt.Sprintf("Failed to recreate provisioner key %q after drift recovery", keyName)) - _ = r.Status().Update(ctx, provisioner) - return ctrl.Result{}, fmt.Errorf("recreate provisioner key %q after drift recovery: %w", keyName, rotateErr) - } - if rotated.OrganizationID != uuid.Nil { - organizationID = rotated.OrganizationID.String() - } - if rotated.KeyID != uuid.Nil { - provisionerKeyID = rotated.KeyID.String() - } - if rotated.KeyName != "" { - provisionerKeyName = rotated.KeyName - } - keyMaterial = rotated.Key - if keyMaterial == "" { + } + for _, replacedKey := range replacedKeys { + if deleteErr := r.BootstrapClient.DeleteProvisionerKeyByID(ctx, replacedKey); deleteErr != nil { setCondition(provisioner, coderv1alpha1.CoderProvisionerConditionProvisionerKeyReady, metav1.ConditionFalse, "ProvisionerKeyFailed", - fmt.Sprintf("Provisioner key %q returned empty material after drift recovery rotation", keyName)) + fmt.Sprintf("Failed to delete provisioner key %q to replace it", keyName)) _ = r.Status().Update(ctx, provisioner) - return ctrl.Result{}, fmt.Errorf("assertion failed: provisioner key %q returned empty material after drift recovery rotation", keyName) + return ctrl.Result{}, fmt.Errorf("delete provisioner key %q to replace it: %w", keyName, deleteErr) } } - appliedOrgName = organizationName - appliedTagsHash = desiredTagsHash - appliedControlPlaneRefName = desiredControlPlaneRefName - appliedControlPlaneURL = controlPlane.Status.URL - setCondition( - provisioner, - coderv1alpha1.CoderProvisionerConditionProvisionerKeyReady, - metav1.ConditionTrue, - "ProvisionerKeyReady", - "Provisioner key is available in coderd", - ) - } else if rotate { response, ensureErr := r.BootstrapClient.EnsureProvisionerKey(ctx, coderbootstrap.EnsureProvisionerKeyRequest{ CoderURL: coderURL, SessionToken: sessionToken, @@ -490,6 +494,10 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req _ = r.Status().Update(ctx, provisioner) return ctrl.Result{}, fmt.Errorf("ensure provisioner key %q: %w", keyName, ensureErr) } + if response.Key == "" { + // Another key took the name after the lookup above. + return r.refuseKeyNameInUse(ctx, provisioner, coderURL, organizationName, keyName, response.KeyID) + } if response.OrganizationID != uuid.Nil { organizationID = response.OrganizationID.String() } @@ -500,152 +508,19 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req provisionerKeyName = response.KeyName } keyMaterial = response.Key - - // If the key already exists in coderd (e.g. the K8s secret was - // deleted), coderd won't return plaintext again. Rotate the key - // by deleting and recreating it to obtain fresh material. - if keyMaterial == "" { - log.Info("provisioner key exists in coderd but its plaintext is not in the secret, rotating key to recover", - "keyName", keyName, "secretName", keySecretName) - - if deleteErr := r.BootstrapClient.DeleteProvisionerKey( - ctx, coderURL, sessionToken, organizationName, keyName, - ); deleteErr != nil { - setCondition(provisioner, coderv1alpha1.CoderProvisionerConditionProvisionerKeyReady, - metav1.ConditionFalse, "ProvisionerKeyFailed", - fmt.Sprintf("Failed to delete stale provisioner key %q for rotation", keyName)) - _ = r.Status().Update(ctx, provisioner) - return ctrl.Result{}, fmt.Errorf("delete stale provisioner key %q for rotation: %w", keyName, deleteErr) - } - rotated, rotateErr := r.BootstrapClient.EnsureProvisionerKey(ctx, coderbootstrap.EnsureProvisionerKeyRequest{ - CoderURL: coderURL, - SessionToken: sessionToken, - OrganizationName: organizationName, - KeyName: keyName, - Tags: provisioner.Spec.Tags, - }) - if rotateErr != nil { - setCondition(provisioner, coderv1alpha1.CoderProvisionerConditionProvisionerKeyReady, - metav1.ConditionFalse, "ProvisionerKeyFailed", - fmt.Sprintf("Failed to recreate provisioner key %q after rotation", keyName)) - _ = r.Status().Update(ctx, provisioner) - return ctrl.Result{}, fmt.Errorf("recreate provisioner key %q after rotation: %w", keyName, rotateErr) - } - if rotated.OrganizationID != uuid.Nil { - organizationID = rotated.OrganizationID.String() - } - if rotated.KeyID != uuid.Nil { - provisionerKeyID = rotated.KeyID.String() - } - if rotated.KeyName != "" { - provisionerKeyName = rotated.KeyName - } - keyMaterial = rotated.Key - if keyMaterial == "" { - setCondition(provisioner, coderv1alpha1.CoderProvisionerConditionProvisionerKeyReady, - metav1.ConditionFalse, "ProvisionerKeyFailed", - fmt.Sprintf("Provisioner key %q returned empty key material after rotation", keyName)) - _ = r.Status().Update(ctx, provisioner) - return ctrl.Result{}, fmt.Errorf("assertion failed: provisioner key %q returned empty key material after rotation", keyName) - } - } appliedOrgName = organizationName appliedTagsHash = desiredTagsHash appliedControlPlaneRefName = desiredControlPlaneRefName appliedControlPlaneURL = controlPlane.Status.URL - setCondition( - provisioner, - coderv1alpha1.CoderProvisionerConditionProvisionerKeyReady, - metav1.ConditionTrue, - "ProvisionerKeyReady", - "Provisioner key is available in coderd", - ) - } else if metadataMissing { - // Secret is usable and no drift detected, but status metadata is empty - // (e.g. upgrade from older version). Call EnsureProvisionerKey to populate - // IDs and key name. If coderd reports an existing key (no plaintext key - // returned), rotate it best-effort so desired tags are applied before - // stamping the metadata baseline. - response, ensureErr := r.BootstrapClient.EnsureProvisionerKey(ctx, coderbootstrap.EnsureProvisionerKeyRequest{ - CoderURL: coderURL, - SessionToken: sessionToken, - OrganizationName: organizationName, - KeyName: keyName, - Tags: provisioner.Spec.Tags, - }) - if ensureErr != nil { - // Return a requeue so metadata population retries. - return ctrl.Result{}, fmt.Errorf("verify provisioner key metadata: %w", ensureErr) - } - if response.OrganizationID != uuid.Nil { - organizationID = response.OrganizationID.String() - } - if response.KeyID != uuid.Nil { - provisionerKeyID = response.KeyID.String() - } - if response.KeyName != "" { - provisionerKeyName = response.KeyName - } - if response.Key != "" { - // Key was freshly created with desired tags; capture material and stamp baseline. - keyMaterial = response.Key - appliedOrgName = organizationName - appliedTagsHash = desiredTagsHash - appliedControlPlaneRefName = desiredControlPlaneRefName - appliedControlPlaneURL = controlPlane.Status.URL - } else { - // Key already exists; tags may be stale. Rotate to ensure desired tags are applied. - log.Info("existing key found during metadata backfill, rotating to ensure desired tags", - "keyName", keyName) - if deleteErr := r.BootstrapClient.DeleteProvisionerKey( - ctx, coderURL, sessionToken, organizationName, keyName, - ); deleteErr != nil { - return ctrl.Result{}, fmt.Errorf("delete provisioner key %q for metadata backfill rotation: %w", keyName, deleteErr) - } - rotated, rotateErr := r.BootstrapClient.EnsureProvisionerKey(ctx, coderbootstrap.EnsureProvisionerKeyRequest{ - CoderURL: coderURL, - SessionToken: sessionToken, - OrganizationName: organizationName, - KeyName: keyName, - Tags: provisioner.Spec.Tags, - }) - if rotateErr != nil { - setCondition(provisioner, coderv1alpha1.CoderProvisionerConditionProvisionerKeyReady, - metav1.ConditionFalse, "ProvisionerKeyFailed", - fmt.Sprintf("Failed to recreate provisioner key %q after metadata backfill rotation", keyName)) - _ = r.Status().Update(ctx, provisioner) - return ctrl.Result{}, fmt.Errorf("recreate provisioner key %q after metadata backfill rotation: %w", keyName, rotateErr) - } - if rotated.OrganizationID != uuid.Nil { - organizationID = rotated.OrganizationID.String() - } - if rotated.KeyID != uuid.Nil { - provisionerKeyID = rotated.KeyID.String() - } - if rotated.KeyName != "" { - provisionerKeyName = rotated.KeyName - } - keyMaterial = rotated.Key - if keyMaterial == "" { - setCondition(provisioner, coderv1alpha1.CoderProvisionerConditionProvisionerKeyReady, - metav1.ConditionFalse, "ProvisionerKeyFailed", - fmt.Sprintf("Provisioner key %q returned empty material after metadata backfill rotation", keyName)) - _ = r.Status().Update(ctx, provisioner) - return ctrl.Result{}, fmt.Errorf("assertion failed: provisioner key %q returned empty material after metadata backfill rotation", keyName) - } - appliedOrgName = organizationName - appliedTagsHash = desiredTagsHash - appliedControlPlaneRefName = desiredControlPlaneRefName - appliedControlPlaneURL = controlPlane.Status.URL - } - setCondition( - provisioner, - coderv1alpha1.CoderProvisionerConditionProvisionerKeyReady, - metav1.ConditionTrue, - "ProvisionerKeyReady", - "Provisioner key is available in coderd", - ) + appliedControlPlaneUID = string(controlPlane.UID) } + setCondition( + provisioner, + coderv1alpha1.CoderProvisionerConditionProvisionerKeyReady, + metav1.ConditionTrue, + "ProvisionerKeyReady", + "Provisioner key is available in coderd", + ) if keyMaterial != "" { // A new key exists in Coder. Record its IDs in its pending record @@ -705,17 +580,17 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req return ctrl.Result{}, fmt.Errorf("assertion failed: key status snapshot must not be nil") } // The Secret holds the key at the target, so the applied fields take over - // its record. Coder allows one key per name, so the create also proved - // that the keys of the other target records are gone. No other key is in - // the Secret now. + // its record. The replacement deleted the keys of the other target + // records by ID, and the key that the name lookup found, wherever it was + // recorded. No other key is in the Secret now. provisioner.Status.PendingKeys = slices.DeleteFunc(provisioner.Status.PendingKeys, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { - return sameKeySlot(record, target) + return sameKeySlot(record, target) || (existingKey != nil && isKeyID(record.KeyID, existingKey.ID)) }) for i := range provisioner.Status.PendingKeys { provisioner.Status.PendingKeys[i].Unpublished = false } recordAppliedKeyStatus(provisioner, organizationID, appliedOrgName, provisionerKeyID, provisionerKeyName, - appliedTagsHash, appliedControlPlaneRefName, appliedControlPlaneURL) + appliedTagsHash, appliedControlPlaneRefName, appliedControlPlaneURL, appliedControlPlaneUID) provisioner.Status.SecretRef = &coderv1alpha1.SecretKeySelector{Name: keySecretName, Key: keySecretKey} if !equality.Semantic.DeepEqual(*keyStatusSnapshot, provisioner.Status) { if err := r.Status().Update(ctx, provisioner); err != nil { @@ -825,7 +700,8 @@ func (r *CoderProvisionerReconciler) reconcileDeletion(ctx context.Context, prov } if cleanup != keyGone { log.Info("did not delete a recorded provisioner key during deletion, proceeding with finalizer removal", - "coderURL", record.CoderURL, "keyName", record.KeyName, "keyID", record.KeyID, "unverified", cleanup == keyUnverified) + "coderURL", record.CoderURL, "keyName", record.KeyName, "keyID", record.KeyID, "unverified", cleanup == keyUnverified, + "controlPlaneChanged", cleanup == keyControlPlaneChanged) } } @@ -848,7 +724,7 @@ func coderAPIURL(controlPlane *coderv1alpha1.CoderControlPlane) (string, error) // in Coder. func recordAppliedKeyStatus( provisioner *coderv1alpha1.CoderProvisioner, - organizationID, organizationName, keyID, keyName, tagsHash, controlPlaneRefName, controlPlaneURL string, + organizationID, organizationName, keyID, keyName, tagsHash, controlPlaneRefName, controlPlaneURL, controlPlaneUID string, ) { provisioner.Status.OrganizationID = organizationID provisioner.Status.OrganizationName = organizationName @@ -857,6 +733,44 @@ func recordAppliedKeyStatus( provisioner.Status.TagsHash = tagsHash provisioner.Status.ControlPlaneRefName = controlPlaneRefName provisioner.Status.ControlPlaneURL = controlPlaneURL + provisioner.Status.ControlPlaneUID = controlPlaneUID +} + +// recordsKeyID reports whether status records keyID as a key of this +// provisioner: the applied key or a pending entry. +func recordsKeyID(status coderv1alpha1.CoderProvisionerStatus, keyID uuid.UUID) bool { + return isKeyID(status.ProvisionerKeyID, keyID) || slices.ContainsFunc(status.PendingKeys, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { + return isKeyID(record.KeyID, keyID) + }) +} + +// isKeyID reports whether rawKeyID is the key ID keyID. +func isKeyID(rawKeyID string, keyID uuid.UUID) bool { + parsed, err := uuid.Parse(rawKeyID) + return err == nil && keyID != uuid.Nil && parsed == keyID +} + +// refuseKeyNameInUse reports that Coder holds a key with the target name that +// status does not record, and stops the reconcile before any key change. It +// checks again later, because nothing signals when that key goes. +func (r *CoderProvisionerReconciler) refuseKeyNameInUse( + ctx context.Context, + provisioner *coderv1alpha1.CoderProvisioner, + coderURL, organizationName, keyName string, + keyID uuid.UUID, +) (ctrl.Result, error) { + ctrl.LoggerFrom(ctx).Info("leaving a provisioner key with the target name that this provisioner did not record", + "coderURL", coderURL, "organization", organizationName, "keyName", keyName, "foundKeyID", keyID) + setCondition(provisioner, coderv1alpha1.CoderProvisionerConditionProvisionerKeyReady, + metav1.ConditionFalse, "KeyNameInUse", + fmt.Sprintf("Coder at %s holds provisioner key %q (ID %s) in organization %q, and status does not record it as a key of this provisioner. "+ + "Another provisioner or a person can use this name, so the operator does not delete or replace that key. "+ + "Set spec.key.name to a name that nothing else uses, or delete that key in Coder if nothing uses it.", + coderURL, keyName, keyID, organizationName)) + if err := r.Status().Update(ctx, provisioner); err != nil { + return ctrl.Result{}, fmt.Errorf("report provisioner key name %q in use: %w", keyName, err) + } + return ctrl.Result{RequeueAfter: provisionerKeyNameInUseRetryInterval}, nil } // revokeUnpublishedKey deletes a key whose plaintext was never written to a @@ -905,10 +819,15 @@ func parseKeyIDs(rawOrganizationID, rawKeyID string) (organizationID, keyID uuid } // sameKeySlot reports whether two records name the same key name in the same -// organization at the same Coder URL. Coder allows one key per name in an -// organization, so both records refer to the same key. +// organization at the same Coder URL of the same control plane. Coder allows +// one key per name in an organization, so both records refer to the same key. +// A control plane created again under the same name can serve another Coder +// backend at the same URL, so records with different control plane UIDs name +// different slots. A record without a UID is from an operator that did not +// record it, and matches any UID. func sameKeySlot(a, b coderv1alpha1.CoderProvisionerKeyRecord) bool { - return a.CoderURL == b.CoderURL && a.OrganizationName == b.OrganizationName && a.KeyName == b.KeyName + sameControlPlane := a.ControlPlaneUID == "" || b.ControlPlaneUID == "" || a.ControlPlaneUID == b.ControlPlaneUID + return sameControlPlane && a.CoderURL == b.CoderURL && a.OrganizationName == b.OrganizationName && a.KeyName == b.KeyName } // appliedKeyRecord returns a record of the key that the applied status fields @@ -923,6 +842,7 @@ func (r *CoderProvisionerReconciler) appliedKeyRecord( status := provisioner.Status record := coderv1alpha1.CoderProvisionerKeyRecord{ ControlPlaneRefName: cmp.Or(status.ControlPlaneRefName, provisioner.Spec.ControlPlaneRef.Name), + ControlPlaneUID: status.ControlPlaneUID, CoderURL: strings.TrimSpace(status.ControlPlaneURL), OrganizationName: cmp.Or(status.OrganizationName, defaultOrganizationName), KeyName: cmp.Or(status.ProvisionerKeyName, defaultKeyName), @@ -982,6 +902,28 @@ func (r *CoderProvisionerReconciler) readControlPlane(ctx context.Context, names return controlPlane, true, nil } +// bindToControlPlane records in record, when it has no control plane UID, the +// UID of the control plane that it names, while that control plane has the +// recorded Coder API URL (see tokenBoundTo). A control plane created again +// under that name later then gets no operator token for record. +func (r *CoderProvisionerReconciler) bindToControlPlane( + ctx context.Context, + namespace string, + record coderv1alpha1.CoderProvisionerKeyRecord, +) (coderv1alpha1.CoderProvisionerKeyRecord, error) { + if record.ControlPlaneUID != "" { + return record, nil + } + controlPlane, ok, err := r.readControlPlane(ctx, namespace, record.ControlPlaneRefName) + if err != nil || !ok { + return record, err + } + if tokenBoundTo(record, controlPlane) { + record.ControlPlaneUID = string(controlPlane.UID) + } + return record, nil +} + // keyCleanup is the result of the cleanup of one recorded provisioner key. type keyCleanup int @@ -996,8 +938,27 @@ const ( // its name that nothing shows this provisioner created. The operator // leaves that key, and the record stays. keyUnverified + // keyControlPlaneChanged: the control plane of the record may not be the + // one that the record names (see tokenBoundTo), so the operator sends it + // no operator token. The record stays. + keyControlPlaneChanged ) +// tokenBoundTo reports whether the operator may send the operator token of +// controlPlane for record: only while controlPlane has the UID that record +// names. A control plane deleted and created again under the same name must +// not get its token sent to the URL of the old one, and its answer does not +// show whether the key of the old one is gone, also at the same URL. A record +// without a UID, from an operator that did not record it, is bound only at +// the current Coder API URL of controlPlane. +func tokenBoundTo(record coderv1alpha1.CoderProvisionerKeyRecord, controlPlane *coderv1alpha1.CoderControlPlane) bool { + if record.ControlPlaneUID != "" { + return record.ControlPlaneUID == string(controlPlane.UID) + } + current, err := coderAPIURL(controlPlane) + return err == nil && current == record.CoderURL +} + // revokeKey deletes the key that record names, at record.CoderURL with the // operator token of the control plane record.ControlPlaneRefName. A record // without key IDs deletes no key. @@ -1012,6 +973,11 @@ func (r *CoderProvisionerReconciler) revokeKey( if err != nil || !ok { return keyKept, err } + if !tokenBoundTo(record, controlPlane) { + log.Info("the control plane of the provisioner key record changed, sending it no operator token", + "recordedUID", record.ControlPlaneUID, "currentUID", controlPlane.UID) + return keyControlPlaneChanged, nil + } sessionToken, tokenResult, tokenReason, tokenMessage, tokenErr := r.readOperatorSessionToken(ctx, provisioner, controlPlane) if tokenErr != nil || tokenResult.RequeueAfter > 0 { log.Info("operator access is not available, keeping the provisioner key record", @@ -1055,6 +1021,51 @@ func (r *CoderProvisionerReconciler) revokeKey( return keyUnverified, nil } +// revokePendingKeys deletes the keys of the records in pending outside the +// target slot, and returns the records to keep: the target slot, the records +// that keep reports, and the records whose key Coder did not confirm gone. It +// sets the UnverifiedKeyKept and ControlPlaneChangedKeyKept conditions for the +// records it tried, and reports whether the conditions changed. +func (r *CoderProvisionerReconciler) revokePendingKeys( + ctx context.Context, + provisioner *coderv1alpha1.CoderProvisioner, + pending []coderv1alpha1.CoderProvisionerKeyRecord, + target coderv1alpha1.CoderProvisionerKeyRecord, + keep func(coderv1alpha1.CoderProvisionerKeyRecord) bool, +) ([]coderv1alpha1.CoderProvisionerKeyRecord, bool, error) { + var kept []coderv1alpha1.CoderProvisionerKeyRecord + var unverified, controlPlaneChanged []string + for _, record := range pending { + if sameKeySlot(record, target) || keep(record) { + kept = append(kept, record) + continue + } + cleanup, err := r.revokeKey(ctx, provisioner, record) + if err != nil { + return nil, false, err + } + if cleanup == keyGone { + continue + } + kept = append(kept, record) + if cleanup == keyControlPlaneChanged { + controlPlaneChanged = append(controlPlaneChanged, fmt.Sprintf("key %q in organization %q at %s (control plane %q)", + record.KeyName, record.OrganizationName, record.CoderURL, record.ControlPlaneRefName)) + continue + } + if _, _, ok := parseKeyIDs(record.OrganizationID, record.KeyID); !ok { + note := "the operator could not check Coder" + if cleanup == keyUnverified { + note = "Coder holds a key with this name that nothing shows this provisioner created" + } + unverified = append(unverified, fmt.Sprintf("key %q in organization %q at %s: %s", record.KeyName, record.OrganizationName, record.CoderURL, note)) + } + } + conditionChanged := setUnverifiedKeyCondition(provisioner, unverified) + conditionChanged = setControlPlaneChangedKeyCondition(provisioner, controlPlaneChanged) || conditionChanged + return kept, conditionChanged, nil +} + // setUnverifiedKeyCondition sets the UnverifiedKeyKept condition for the // pending entries without key IDs that unverified describes, or removes it // when there are none. It reports whether the conditions changed. @@ -1072,6 +1083,24 @@ func setUnverifiedKeyCondition(provisioner *coderv1alpha1.CoderProvisioner, unve }) } +// setControlPlaneChangedKeyCondition sets the ControlPlaneChangedKeyKept +// condition for the pending entries that changed describes, or removes it when +// there are none. It reports whether the conditions changed. +func setControlPlaneChangedKeyCondition(provisioner *coderv1alpha1.CoderProvisioner, changed []string) bool { + if len(changed) == 0 { + return meta.RemoveStatusCondition(&provisioner.Status.Conditions, coderv1alpha1.CoderProvisionerConditionControlPlaneChangedKeyKept) + } + return meta.SetStatusCondition(&provisioner.Status.Conditions, metav1.Condition{ + Type: coderv1alpha1.CoderProvisionerConditionControlPlaneChangedKeyKept, + Status: metav1.ConditionTrue, + ObservedGeneration: provisioner.Generation, + Reason: "ControlPlaneUIDMismatch", + Message: "status.pendingKeys holds entries whose control plane now has another UID, or has no recorded UID and another Coder API URL, " + + "so the operator sends no operator token for them: " + strings.Join(changed, "; ") + + ". Delete such a key in Coder by hand, then remove its entry from status.pendingKeys.", + }) +} + func (r *CoderProvisionerReconciler) ensureCleanupFinalizer(ctx context.Context, provisioner *coderv1alpha1.CoderProvisioner) (bool, error) { if controllerutil.ContainsFinalizer(provisioner, coderv1alpha1.ProvisionerKeyCleanupFinalizer) { return false, nil diff --git a/internal/controller/coderprovisioner_controller_test.go b/internal/controller/coderprovisioner_controller_test.go index cc5e1dc9..05c8790f 100644 --- a/internal/controller/coderprovisioner_controller_test.go +++ b/internal/controller/coderprovisioner_controller_test.go @@ -452,7 +452,6 @@ func TestCoderProvisionerReconciler_BasicCreate(t *testing.T) { require.Equal(t, 1, bootstrapClient.provisionerKeyCalls) require.Len(t, bootstrapClient.provisionerKeyRequests, 1) require.Equal(t, "operator-session-token", bootstrapClient.provisionerKeyRequests[0].SessionToken) - require.Equal(t, 0, bootstrapClient.deleteKeyCalls) keySecret := &corev1.Secret{} keySecretName := types.NamespacedName{Name: provisioner.Spec.Key.SecretName, Namespace: provisioner.Namespace} @@ -581,7 +580,6 @@ func TestCoderProvisionerReconciler_Deletion(t *testing.T) { require.False(t, markedForDeletion.DeletionTimestamp.IsZero()) reconcileProvisioner(ctx, t, reconciler, namespacedName) - require.Zero(t, bootstrapClient.deleteKeyCalls) require.Len(t, bootstrapClient.deleteKeyByIDRequests, 1) require.Equal(t, bootstrapClient.provisionerKeyResponses[0].KeyID, bootstrapClient.deleteKeyByIDRequests[0].KeyID) @@ -636,9 +634,8 @@ func TestCoderProvisionerReconciler_DeletionControlPlaneGone(t *testing.T) { reconcileProvisioner(ctx, t, reconciler, namespacedName) - // DeleteProvisionerKey cannot run once operator token source is gone with the + // No key delete can run once the operator token source is gone with the // deleted control plane, but finalizer cleanup must still complete. - require.Equal(t, 0, bootstrapClient.deleteKeyCalls) require.Empty(t, bootstrapClient.deleteKeyByIDRequests) // The finalizer should still be removed. @@ -771,11 +768,11 @@ func TestCoderProvisionerReconciler_RotationOnSecretLoss(t *testing.T) { _ = k8sClient.Delete(context.Background(), provisioner) }) + initial := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: uuid.New(), KeyID: uuid.New(), KeyName: "rotation-key", Key: "initial-key-material"} bootstrapClient := &fakeBootstrapClient{ provisionerKeyResponses: []coderbootstrap.EnsureProvisionerKeyResponse{ - {KeyName: "rotation-key", Key: ""}, - {KeyName: "rotation-key", Key: "rotated-key-material"}, - {KeyName: "rotation-key", Key: "rotated-key-material"}, + initial, + {OrganizationID: initial.OrganizationID, KeyID: uuid.New(), KeyName: "rotation-key", Key: "rotated-key-material"}, }, } reconciler := &controller.CoderProvisionerReconciler{Client: k8sClient, Scheme: scheme, BootstrapClient: bootstrapClient} @@ -783,15 +780,22 @@ func TestCoderProvisionerReconciler_RotationOnSecretLoss(t *testing.T) { reconcileProvisioner(ctx, t, reconciler, request) reconcileProvisioner(ctx, t, reconciler, request) + + // The Secret is lost, and Coder still holds the key, which it returns + // without plaintext. + bootstrapClient.provisionerKeyByName = func(coderbootstrap.ProvisionerKeyByNameRequest) *codersdk.ProvisionerKey { + return &codersdk.ProvisionerKey{ID: initial.KeyID, OrganizationID: initial.OrganizationID, Name: initial.KeyName} + } + keySecretName := types.NamespacedName{Name: provisioner.Spec.Key.SecretName, Namespace: provisioner.Namespace} + require.NoError(t, k8sClient.Delete(ctx, &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: keySecretName.Name, Namespace: keySecretName.Namespace}})) reconcileProvisioner(ctx, t, reconciler, request) require.Equal(t, 2, bootstrapClient.provisionerKeyCalls) - require.Equal(t, 1, bootstrapClient.deleteKeyCalls) - require.Len(t, bootstrapClient.deleteKeyRequests, 1) - require.Equal(t, "rotation-key", bootstrapClient.deleteKeyRequests[0].KeyName) + require.Len(t, bootstrapClient.deleteKeyByIDRequests, 1) + require.Equal(t, initial.KeyID, bootstrapClient.deleteKeyByIDRequests[0].KeyID, "the rotation must delete the recorded key by its ID") keySecret := &corev1.Secret{} - require.NoError(t, k8sClient.Get(ctx, types.NamespacedName{Name: provisioner.Spec.Key.SecretName, Namespace: provisioner.Namespace}, keySecret)) + require.NoError(t, k8sClient.Get(ctx, keySecretName, keySecret)) require.Equal(t, "rotated-key-material", string(keySecret.Data[provisioner.Spec.Key.SecretKey])) } @@ -815,11 +819,11 @@ func TestCoderProvisionerReconciler_TagsDrift(t *testing.T) { _ = k8sClient.Delete(context.Background(), provisioner) }) + initial := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: uuid.New(), KeyID: uuid.New(), KeyName: "tags-drift-key", Key: "initial-key-material"} bootstrapClient := &fakeBootstrapClient{ provisionerKeyResponses: []coderbootstrap.EnsureProvisionerKeyResponse{ - {KeyName: "tags-drift-key", Key: "initial-key-material"}, - {KeyName: "tags-drift-key", Key: ""}, - {KeyName: "tags-drift-key", Key: "rotated-key-material"}, + initial, + {OrganizationID: initial.OrganizationID, KeyID: uuid.New(), KeyName: "tags-drift-key", Key: "rotated-key-material"}, }, } reconciler := &controller.CoderProvisionerReconciler{Client: k8sClient, Scheme: scheme, BootstrapClient: bootstrapClient} @@ -835,14 +839,15 @@ func TestCoderProvisionerReconciler_TagsDrift(t *testing.T) { before.Spec.Tags = map[string]string{"region": "eu-west"} require.NoError(t, k8sClient.Update(ctx, before)) + bootstrapClient.provisionerKeyByName = func(coderbootstrap.ProvisionerKeyByNameRequest) *codersdk.ProvisionerKey { + return &codersdk.ProvisionerKey{ID: initial.KeyID, OrganizationID: initial.OrganizationID, Name: initial.KeyName} + } reconcileProvisioner(ctx, t, reconciler, request) - require.Equal(t, 3, bootstrapClient.provisionerKeyCalls) - require.Equal(t, 2, bootstrapClient.deleteKeyCalls) - require.Len(t, bootstrapClient.deleteKeyRequests, 2) - require.Equal(t, "tags-drift-key", bootstrapClient.deleteKeyRequests[0].KeyName) - require.Equal(t, "tags-drift-key", bootstrapClient.deleteKeyRequests[1].KeyName) + require.Equal(t, 2, bootstrapClient.provisionerKeyCalls) + require.Len(t, bootstrapClient.deleteKeyByIDRequests, 1) + require.Equal(t, initial.KeyID, bootstrapClient.deleteKeyByIDRequests[0].KeyID, "the rotation must delete the old key by its ID") keySecret := &corev1.Secret{} require.NoError(t, k8sClient.Get(ctx, types.NamespacedName{Name: fmt.Sprintf("%s-provisioner-key", provisioner.Name), Namespace: provisioner.Namespace}, keySecret)) diff --git a/internal/controller/coderprovisioner_deletion_test.go b/internal/controller/coderprovisioner_deletion_test.go index 473ded99..881a206d 100644 --- a/internal/controller/coderprovisioner_deletion_test.go +++ b/internal/controller/coderprovisioner_deletion_test.go @@ -72,7 +72,6 @@ func TestCoderProvisionerDeletion_NoRecordedKeySkipsCoder(t *testing.T) { _, err := f.reconciler.Reconcile(ctx, request) require.NoError(t, err) - require.Zero(t, f.bootstrapClient.deleteKeyCalls, "no name-based key deletion") require.Empty(t, f.bootstrapClient.deleteKeyByIDRequests, "no key deletion without a recorded key") requireProvisionerGone(ctx, t, request.NamespacedName) }) @@ -101,7 +100,6 @@ func TestCoderProvisionerDeletion_DeletesRecordedKey(t *testing.T) { require.NoError(t, k8sClient.Delete(ctx, latest)) f.reconcile(ctx, t, provisioner) - require.Zero(t, f.bootstrapClient.deleteKeyCalls, "no name-based key deletion") require.Len(t, f.bootstrapClient.deleteKeyByIDRequests, 1) deleted := f.bootstrapClient.deleteKeyByIDRequests[0] require.Equal(t, recorded.OrganizationID, deleted.OrganizationID) @@ -278,6 +276,5 @@ func TestCoderProvisionerDeletion_TriesCurrentControlPlaneURL(t *testing.T) { require.Equal(t, newKey.KeyID, req.KeyID) require.Equal(t, testOperatorToken, req.SessionToken) } - require.Zero(t, f.bootstrapClient.deleteKeyCalls, "no name-based key deletion") requireProvisionerGone(ctx, t, types.NamespacedName{Name: provisioner.Name, Namespace: provisioner.Namespace}) } diff --git a/internal/controller/coderprovisioner_keyownership_test.go b/internal/controller/coderprovisioner_keyownership_test.go new file mode 100644 index 00000000..f4bac049 --- /dev/null +++ b/internal/controller/coderprovisioner_keyownership_test.go @@ -0,0 +1,576 @@ +package controller_test + +import ( + "cmp" + "context" + "encoding/json" + "errors" + "maps" + "net/http" + "net/http/httptest" + "slices" + "strings" + "sync" + "testing" + + "github.com/coder/coder/v2/codersdk" + "github.com/google/uuid" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" + ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" + "github.com/coder/coder-k8s/internal/coderbootstrap" +) + +// TestCoderProvisionerKey_OtherProvisionersKeyKept checks two provisioners P +// and Q with the same key name at one Coder endpoint and organization. Q holds +// the key. Whatever way P reaches that key name, P must not delete or replace +// the key of Q, and must not change its own key, Secret or Deployment. It must +// report the conflict, and get its own key once it has a distinct key name or +// the key of Q is gone, or keep its key after a revert to its key name. +func TestCoderProvisionerKey_OtherProvisionersKeyKept(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + // holdsKey makes P hold the key p-key before it reaches the key name + // of Q. + holdsKey bool + // dropKeyStatus removes the key fields from the status of P, as + // status that lost its key records. + dropKeyStatus bool + // staleRecord records a replaced key of P at another key name, whose + // delete Coder has not confirmed yet. + staleRecord bool + // resolveName resolves the conflict with this key name for P. + // Otherwise the key of Q goes. + resolveName string + }{ + {name: "first key of P"}, + {name: "key name change of P", holdsKey: true, resolveName: "p-own"}, + {name: "key name change of P, reverted", holdsKey: true, resolveName: "p-key"}, + {name: "key name change of P with a replaced key left", holdsKey: true, staleRecord: true, resolveName: "p-own"}, + {name: "status without key records", holdsKey: true, dropKeyStatus: true, resolveName: "p-own"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + f := newProvisionerCredentialFixture(ctx, t, "cprov-samename") + store := &keyStoreClient{fakeBootstrapClient: f.bootstrapClient, keys: map[keySlot]uuid.UUID{}} + f.reconciler.BootstrapClient = store + slotOf := func(keyName string) keySlot { return newKeySlot(internalCoderURL(f), "", keyName) } + + q := f.createKeyProvisioner(ctx, t, "q", "") + q.Spec.Key.Name = "provisioner" + require.NoError(t, k8sClient.Update(ctx, q)) + f.reconcile(ctx, t, q) + f.reconcile(ctx, t, q) + qKeyID := store.keys[slotOf("provisioner")] + require.Equal(t, qKeyID.String(), f.get(ctx, t, q).Status.ProvisionerKeyID, "Q must hold the key named provisioner") + + p := f.createProvisioner(ctx, t, nil, nil) + pName := types.NamespacedName{Name: p.Name, Namespace: p.Namespace} + keySecret := types.NamespacedName{Name: p.Name + "-provisioner-key", Namespace: p.Namespace} + var pKeyID uuid.UUID + var pSecret corev1.Secret + if tt.holdsKey { + latest := f.get(ctx, t, p) + latest.Spec.Key.Name = "p-key" + require.NoError(t, k8sClient.Update(ctx, latest)) + f.reconcile(ctx, t, p) + f.reconcile(ctx, t, p) + pKeyID = store.keys[slotOf("p-key")] + require.Equal(t, pKeyID.String(), f.get(ctx, t, p).Status.ProvisionerKeyID) + require.NoError(t, k8sClient.Get(ctx, keySecret, &pSecret)) + + latest = f.get(ctx, t, p) + latest.Spec.Key.Name = "" + require.NoError(t, k8sClient.Update(ctx, latest)) + if tt.dropKeyStatus { + latest = f.get(ctx, t, p) + latest.Status = coderv1alpha1.CoderProvisionerStatus{SecretRef: latest.Status.SecretRef} + require.NoError(t, k8sClient.Status().Update(ctx, latest)) + } + } else { + f.reconcile(ctx, t, p) + } + + if tt.staleRecord { + stale := slotOf("p-stale") + store.keys[stale] = uuid.New() + latest := f.get(ctx, t, p) + latest.Status.PendingKeys = append(latest.Status.PendingKeys, coderv1alpha1.CoderProvisionerKeyRecord{ + ControlPlaneRefName: f.controlPlane.Name, + ControlPlaneUID: string(f.controlPlane.UID), + CoderURL: internalCoderURL(f), + OrganizationName: "default", + KeyName: "p-stale", + OrganizationID: stale.organizationID().String(), + KeyID: store.keys[stale].String(), + }) + require.NoError(t, k8sClient.Status().Update(ctx, latest)) + } + + for range 2 { + result, err := f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: pName}) + require.NoError(t, err) + require.Equal(t, qKeyID, store.keys[slotOf("provisioner")], "the key of Q must still exist") + require.Positive(t, result.RequeueAfter, "P must check the key name again") + } + latest := f.get(ctx, t, p) + keyReady := meta.FindStatusCondition(latest.Status.Conditions, coderv1alpha1.CoderProvisionerConditionProvisionerKeyReady) + require.NotNil(t, keyReady, "P must report the conflict") + require.Equal(t, metav1.ConditionFalse, keyReady.Status) + require.Equal(t, "KeyNameInUse", keyReady.Reason) + require.Contains(t, keyReady.Message, `"provisioner"`) + require.Contains(t, keyReady.Message, qKeyID.String()) + require.Nil(t, meta.FindStatusCondition(latest.Status.Conditions, coderv1alpha1.CoderProvisionerConditionUnverifiedKeyKept)) + require.Empty(t, latest.Status.PendingKeys, "P must record no create") + require.NotContains(t, store.keys, slotOf("p-stale"), "a key recorded at an earlier target must not wait for the conflict") + if tt.holdsKey { + require.Equal(t, pKeyID, store.keys[slotOf("p-key")], "the key of P must stay while its new key name is taken") + secret := &corev1.Secret{} + require.NoError(t, k8sClient.Get(ctx, keySecret, secret)) + require.Equal(t, pSecret.Data, secret.Data, "the Secret of P must keep its key") + } else { + _, exists := f.deployment(ctx, t, p) + require.False(t, exists, "P must not start without a key") + } + resourceVersion := latest.ResourceVersion + _, err := f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: pName}) + require.NoError(t, err) + require.Equal(t, resourceVersion, f.get(ctx, t, p).ResourceVersion, "a repeated conflict must not write status") + + keyName := cmp.Or(tt.resolveName, "provisioner") + if tt.resolveName != "" { + latest.Spec.Key.Name = keyName + require.NoError(t, k8sClient.Update(ctx, latest)) + } else { + delete(store.keys, slotOf("provisioner")) + } + f.reconcile(ctx, t, p) + latest = f.get(ctx, t, p) + require.Equal(t, store.keys[slotOf(keyName)].String(), latest.Status.ProvisionerKeyID, "P must hold its own key") + keyReady = meta.FindStatusCondition(latest.Status.Conditions, coderv1alpha1.CoderProvisionerConditionProvisionerKeyReady) + require.NotNil(t, keyReady) + require.Equal(t, metav1.ConditionTrue, keyReady.Status) + require.Nil(t, meta.FindStatusCondition(latest.Status.Conditions, coderv1alpha1.CoderProvisionerConditionUnverifiedKeyKept)) + if tt.resolveName != "" { + require.Equal(t, qKeyID, store.keys[slotOf("provisioner")], "the key of Q must still exist") + } + if tt.resolveName == "p-key" { + require.Equal(t, pKeyID.String(), latest.Status.ProvisionerKeyID, "a revert must keep the key of P") + } + }) + } +} + +// urlToken is the Coder URL and the operator token of one Coder request. +type urlToken struct{ coderURL, token string } + +// sentTokens lists the Coder URL and token of every Coder request that the +// fake received, and records lookups by name into lookups. +func sentTokens(client *fakeBootstrapClient, lookups []urlToken) []urlToken { + sent := slices.Clone(lookups) + for _, req := range client.provisionerKeyRequests { + sent = append(sent, urlToken{req.CoderURL, req.SessionToken}) + } + for _, req := range client.entitlementsRequests { + sent = append(sent, urlToken{req.CoderURL, req.SessionToken}) + } + for _, req := range keyDeletes(client) { + sent = append(sent, urlToken{req.coderURL, req.token}) + } + return sent +} + +// TestCoderProvisionerKey_ControlPlaneRecreated checks a control plane that is +// deleted and created again under the same name with another operator token, +// at another URL or at the same URL with another Coder backend. The new token +// must never reach the URL of the old control plane, and must never be used +// for the key of the old one: the record of the old key stays, and the +// provisioner reports it. The provisioner must create a new key. A record +// from an operator that did not record control plane UIDs may use the token +// of its control plane only at that control plane's current URL, and its +// applied key stays only when the current backend holds it. +func TestCoderProvisionerKey_ControlPlaneRecreated(t *testing.T) { + t.Parallel() + + const ( + oldURL = "http://coder-old.example.com" + newURL = "http://coder-new.example.com" + newToken = "operator-token-new" + ) + // What a rotation after the change leaves of the old key. + const ( + oldKeyKept = "kept" // its record stays, and the provisioner reports it + oldKeyDeleted = "deleted" // deleted where Coder holds it, no record left + oldKeyDropped = "dropped" // the current backend has no such key, no record left + ) + tests := []struct { + name string + // legacy removes the control plane UID from status, as an operator + // before UID records wrote it. + legacy bool + // verifyFirst reconciles once before the recreate, while the backend + // holds the old key. + verifyFirst bool + // keep keeps the control plane and moves P to another one instead. + keep bool + // sameURL creates the control plane again at its old URL. + sameURL bool + // sameBackend: the new control plane serves the Coder backend of the + // old one, which still holds the old key. + sameBackend bool + deleteP bool + // oldKey is what a rotation leaves of the old key. + oldKey string + }{ + {name: "rotation after the recreate", oldKey: oldKeyKept}, + {name: "deletion after the recreate", deleteP: true}, + {name: "rotation after the recreate at the same URL", sameURL: true, oldKey: oldKeyKept}, + {name: "deletion after the recreate at the same URL", sameURL: true, deleteP: true}, + {name: "rotation after the recreate at the same URL and backend", sameURL: true, sameBackend: true, oldKey: oldKeyDeleted}, + {name: "status without control plane UID, rotation after the recreate", legacy: true, oldKey: oldKeyKept}, + {name: "status without control plane UID, deletion after the recreate", legacy: true, deleteP: true}, + {name: "status without control plane UID, recreate at the same URL", legacy: true, sameURL: true, oldKey: oldKeyDropped}, + {name: "status without control plane UID, verified before the recreate at the same URL", legacy: true, verifyFirst: true, sameURL: true, oldKey: oldKeyKept}, + {name: "status without control plane UID, control plane kept", legacy: true, keep: true, oldKey: oldKeyDeleted}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + f := newProvisionerCredentialFixture(ctx, t, "cprov-recreated") + setControlPlaneURL(ctx, t, f, oldURL) + keyOld := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: uuid.New(), KeyID: uuid.New(), KeyName: "provisioner", Key: "material-old"} + keyNew := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: uuid.New(), KeyID: uuid.New(), KeyName: "provisioner", Key: "material-new"} + f.bootstrapClient.provisionerKeyResponses = []coderbootstrap.EnsureProvisionerKeyResponse{keyOld, keyNew} + var lookups []urlToken + // oldKeyInCoder: the backend at oldURL holds the old key. + oldKeyInCoder := false + oldKeyAt := func(coderURL, token string) *codersdk.ProvisionerKey { + lookups = append(lookups, urlToken{coderURL, token}) + if !oldKeyInCoder || coderURL != oldURL || len(f.bootstrapClient.deleteKeyByIDRequests) > 0 { + return nil + } + return &codersdk.ProvisionerKey{ID: keyOld.KeyID, OrganizationID: keyOld.OrganizationID, Name: keyOld.KeyName} + } + f.bootstrapClient.provisionerKeyByName = func(req coderbootstrap.ProvisionerKeyByNameRequest) *codersdk.ProvisionerKey { + return oldKeyAt(req.CoderURL, req.SessionToken) + } + f.bootstrapClient.provisionerKeyByID = func(req coderbootstrap.ProvisionerKeyByIDRequest) *codersdk.ProvisionerKey { + if req.OrganizationID != keyOld.OrganizationID || req.KeyID != keyOld.KeyID { + return nil + } + return oldKeyAt(req.CoderURL, req.SessionToken) + } + + p := f.createProvisioner(ctx, t, nil, nil) + pName := types.NamespacedName{Name: p.Name, Namespace: p.Namespace} + f.reconcile(ctx, t, p) + f.reconcile(ctx, t, p) + require.Equal(t, keyOld.KeyID.String(), f.get(ctx, t, p).Status.ProvisionerKeyID) + if tt.legacy { + require.NoError(t, k8sClient.Status().Patch(ctx, f.get(ctx, t, p), + ctrlclient.RawPatch(types.MergePatchType, []byte(`{"status":{"controlPlaneUID":null}}`)))) + } + if tt.verifyFirst { + oldKeyInCoder = true + f.reconcile(ctx, t, p) + latest := f.get(ctx, t, p) + require.Equal(t, keyOld.KeyID.String(), latest.Status.ProvisionerKeyID, "a key that the backend holds must stay") + require.NotEmpty(t, latest.Status.ControlPlaneUID, "the operator must bind the verified key to its control plane") + } + oldKeyInCoder = tt.sameBackend + + currentURL := newURL + if tt.sameURL { + currentURL = oldURL + } + if tt.keep { + createControlPlaneWithToken(ctx, t, f.namespace, "coder-b", newURL, newToken) + latest := f.get(ctx, t, p) + latest.Spec.ControlPlaneRef.Name = "coder-b" + require.NoError(t, k8sClient.Update(ctx, latest)) + } else { + require.NoError(t, k8sClient.Delete(ctx, &coderv1alpha1.CoderControlPlane{ObjectMeta: metav1.ObjectMeta{Name: "coder", Namespace: f.namespace}})) + require.NoError(t, k8sClient.Delete(ctx, &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "coder-operator-token", Namespace: f.namespace}})) + createControlPlaneWithToken(ctx, t, f.namespace, "coder", currentURL, newToken) + } + + requireNoLeak := func() { + t.Helper() + // A record without a UID is bound at the current URL, so only + // a recorded UID keeps the new token from the old key. + bound := !tt.legacy || tt.verifyFirst + for _, d := range keyDeletes(f.bootstrapClient) { + require.False(t, bound && !tt.sameBackend && d.keyID == keyOld.KeyID.String() && d.token == newToken, + "the new control plane's token must not be used for the old key, sent %+v", keyDeletes(f.bootstrapClient)) + } + if tt.sameURL { + return + } + sent := sentTokens(f.bootstrapClient, lookups) + for _, s := range sent { + require.False(t, s.coderURL == oldURL && s.token == newToken, "the new control plane's token must not reach %s, sent %+v", oldURL, sent) + } + } + if tt.deleteP { + require.NoError(t, k8sClient.Delete(ctx, f.get(ctx, t, p))) + f.reconcile(ctx, t, p) + requireNoLeak() + requireProvisionerGone(ctx, t, pName) + return + } + + result, err := f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: pName}) + require.NoError(t, err) + requireNoLeak() + latest := f.get(ctx, t, p) + require.Equal(t, keyNew.KeyID.String(), latest.Status.ProvisionerKeyID, "P must create a new key") + require.Equal(t, currentURL, latest.Status.ControlPlaneURL) + kept := slices.ContainsFunc(latest.Status.PendingKeys, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { + return record.KeyID == keyOld.KeyID.String() && record.CoderURL == oldURL + }) + report := meta.FindStatusCondition(latest.Status.Conditions, coderv1alpha1.CoderProvisionerConditionControlPlaneChangedKeyKept) + oldKeyDeletes := slices.DeleteFunc(keyDeletes(f.bootstrapClient), func(d keyDelete) bool { return d.keyID != keyOld.KeyID.String() }) + switch tt.oldKey { + case oldKeyKept: + require.True(t, kept, "the record of the old key must stay") + require.Positive(t, result.RequeueAfter, "P must check the kept record again") + require.NotNil(t, report, "P must report the record that it keeps") + require.Equal(t, metav1.ConditionTrue, report.Status) + require.Contains(t, report.Message, oldURL) + require.Empty(t, oldKeyDeletes) + case oldKeyDeleted: + token := newToken + if tt.keep { + token = testOperatorToken + } + require.Equal(t, []keyDelete{{coderURL: oldURL, token: token, keyID: keyOld.KeyID.String()}}, oldKeyDeletes, + "the old key must be deleted once, where Coder holds it, with the token of the control plane that serves it") + require.False(t, kept, "a deleted key must not stay recorded") + require.Nil(t, report) + case oldKeyDropped: + require.Empty(t, oldKeyDeletes) + require.False(t, kept) + require.Nil(t, report) + default: + t.Fatalf("assertion failed: unknown oldKey %q", tt.oldKey) + } + }) + } +} + +// renamedOrgCoder is a Coder at an httptest server, reached through the real +// SDK client, with one organization named team. Key creates answer from the +// fake and add the key to the server. +type renamedOrgCoder struct { + *coderbootstrap.SDKClient + fake *fakeBootstrapClient + mu sync.Mutex + keys map[uuid.UUID]codersdk.ProvisionerKey + // team is the ID of the organization named team. + team uuid.UUID +} + +func (c *renamedOrgCoder) EnsureProvisionerKey(ctx context.Context, req coderbootstrap.EnsureProvisionerKeyRequest) (coderbootstrap.EnsureProvisionerKeyResponse, error) { + response, err := c.fake.EnsureProvisionerKey(ctx, req) + if err == nil { + c.mu.Lock() + c.keys[response.KeyID] = codersdk.ProvisionerKey{ID: response.KeyID, OrganizationID: response.OrganizationID, Name: response.KeyName} + c.mu.Unlock() + } + return response, err +} + +func (c *renamedOrgCoder) Entitlements(ctx context.Context, coderURL, sessionToken string) (codersdk.Entitlements, error) { + return c.fake.Entitlements(ctx, coderURL, sessionToken) +} + +func (c *renamedOrgCoder) serveHTTP(t *testing.T, w http.ResponseWriter, r *http.Request) { + c.mu.Lock() + defer c.mu.Unlock() + w.Header().Set("Content-Type", "application/json") + if r.Method == http.MethodGet && r.URL.Path == "/api/v2/organizations/team" { + _ = json.NewEncoder(w).Encode(codersdk.Organization{MinimalOrganization: codersdk.MinimalOrganization{ID: c.team, Name: "team"}}) + return + } + parts := strings.Split(strings.Trim(r.URL.Path, "/"), "/") + if len(parts) >= 5 && parts[0] == "api" && parts[1] == "v2" && parts[2] == "organizations" && parts[4] == "provisionerkeys" { + if orgID, err := uuid.Parse(parts[3]); err == nil { + if r.Method == http.MethodGet && len(parts) == 5 { + keys := []codersdk.ProvisionerKey{} + for _, key := range c.keys { + if key.OrganizationID == orgID { + keys = append(keys, key) + } + } + _ = json.NewEncoder(w).Encode(keys) + return + } + if r.Method == http.MethodDelete && len(parts) == 6 { + maps.DeleteFunc(c.keys, func(_ uuid.UUID, key codersdk.ProvisionerKey) bool { + return key.OrganizationID == orgID && key.Name == parts[5] + }) + w.WriteHeader(http.StatusNoContent) + return + } + } + } + t.Errorf("unexpected request %s %s", r.Method, r.URL.Path) + w.WriteHeader(http.StatusNotFound) +} + +// TestCoderProvisionerKey_OrganizationRenamed checks a provisioner whose +// applied key is in organization A, named team. A is renamed, and another +// organization takes the name team, at the same control plane. A lookup of +// the name team then does not find the key. Coder still holds the key in A, +// so a rotation must delete it there by its recorded IDs, and so must the +// deletion of the provisioner. +func TestCoderProvisionerKey_OrganizationRenamed(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + // legacy removes the control plane UID from status, as an operator + // before UID records wrote it. + legacy bool + // loseSecret deletes the provisioner key Secret after the rename. + loseSecret bool + // changeTags changes spec.tags after the rename. + changeTags bool + }{ + {name: "status without control plane UID", legacy: true}, + {name: "Secret loss", loseSecret: true}, + {name: "status without control plane UID, Secret loss", legacy: true, loseSecret: true}, + {name: "tags change", changeTags: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + f := newProvisionerCredentialFixture(ctx, t, "cprov-orgrename") + orgA, orgB := uuid.New(), uuid.New() + keyA, keyB := uuid.New(), uuid.New() + coder := &renamedOrgCoder{ + SDKClient: coderbootstrap.NewSDKClient(), + fake: f.bootstrapClient, + keys: map[uuid.UUID]codersdk.ProvisionerKey{}, + team: orgA, + } + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { coder.serveHTTP(t, w, r) })) + t.Cleanup(server.Close) + setControlPlaneURL(ctx, t, f, server.URL) + f.bootstrapClient.provisionerKeyResponses = []coderbootstrap.EnsureProvisionerKeyResponse{ + {OrganizationID: orgA, KeyID: keyA, KeyName: "provisioner", Key: "material-a"}, + {OrganizationID: orgB, KeyID: keyB, KeyName: "provisioner", Key: "material-b"}, + } + f.reconciler.BootstrapClient = coder + + p := f.createProvisioner(ctx, t, nil, nil) + p.Spec.OrganizationName = "team" + require.NoError(t, k8sClient.Update(ctx, p)) + f.reconcile(ctx, t, p) + f.reconcile(ctx, t, p) + require.Equal(t, keyA.String(), f.get(ctx, t, p).Status.ProvisionerKeyID) + if tt.legacy { + require.NoError(t, k8sClient.Status().Patch(ctx, f.get(ctx, t, p), + ctrlclient.RawPatch(types.MergePatchType, []byte(`{"status":{"controlPlaneUID":null}}`)))) + } + + coder.mu.Lock() + coder.team = orgB + coder.mu.Unlock() + if tt.loseSecret { + require.NoError(t, k8sClient.Delete(ctx, &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: p.Name + "-provisioner-key", Namespace: p.Namespace}})) + } + if tt.changeTags { + latest := f.get(ctx, t, p) + latest.Spec.Tags = map[string]string{"scope": "organization"} + require.NoError(t, k8sClient.Update(ctx, latest)) + } + f.reconcile(ctx, t, p) + if tt.loseSecret || tt.changeTags { + require.Equal(t, keyB.String(), f.get(ctx, t, p).Status.ProvisionerKeyID, "the rotation must create a new key") + coder.mu.Lock() + require.NotContains(t, coder.keys, keyA, "the rotation must delete the replaced key in the renamed organization by ID") + coder.mu.Unlock() + } + require.NoError(t, k8sClient.Delete(ctx, f.get(ctx, t, p))) + f.reconcile(ctx, t, p) + requireProvisionerGone(ctx, t, types.NamespacedName{Name: p.Name, Namespace: p.Namespace}) + + coder.mu.Lock() + defer coder.mu.Unlock() + require.NotContains(t, coder.keys, keyA, "deleting the provisioner must revoke its recorded key in the renamed organization by ID") + require.Empty(t, coder.keys, "deleting the provisioner must leave none of its keys in Coder") + }) + } +} + +// TestCoderProvisionerKey_StatusWithoutUIDReplacedKeyBound checks status +// without a control plane UID when P moves from the control plane coder to +// coder-b, and Coder fails the delete of the replaced key. The control plane +// coder is then deleted and created again at the same URL with another +// operator token. The new token must not be used for the replaced key: its +// entry stays, and P reports it. +func TestCoderProvisionerKey_StatusWithoutUIDReplacedKeyBound(t *testing.T) { + t.Parallel() + + const ( + oldURL = "http://coder-old.example.com" + newToken = "operator-token-new" + ) + ctx := context.Background() + f := newProvisionerCredentialFixture(ctx, t, "cprov-bound") + setControlPlaneURL(ctx, t, f, oldURL) + keyOld := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: uuid.New(), KeyID: uuid.New(), KeyName: "provisioner", Key: "material-old"} + keyNew := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: uuid.New(), KeyID: uuid.New(), KeyName: "provisioner", Key: "material-new"} + f.bootstrapClient.provisionerKeyResponses = []coderbootstrap.EnsureProvisionerKeyResponse{keyOld, keyNew} + p := f.createProvisioner(ctx, t, nil, nil) + pName := types.NamespacedName{Name: p.Name, Namespace: p.Namespace} + f.reconcile(ctx, t, p) + f.reconcile(ctx, t, p) + require.Equal(t, keyOld.KeyID.String(), f.get(ctx, t, p).Status.ProvisionerKeyID) + require.NoError(t, k8sClient.Status().Patch(ctx, f.get(ctx, t, p), + ctrlclient.RawPatch(types.MergePatchType, []byte(`{"status":{"controlPlaneUID":null}}`)))) + + createControlPlaneWithToken(ctx, t, f.namespace, "coder-b", "http://coder-b.example.com", "operator-token-b") + latest := f.get(ctx, t, p) + latest.Spec.ControlPlaneRef.Name = "coder-b" + require.NoError(t, k8sClient.Update(ctx, latest)) + f.bootstrapClient.deleteKeyByIDErr = errors.New("injected delete failure") + _, err := f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: pName}) + require.NoError(t, err) + require.Equal(t, keyNew.KeyID.String(), f.get(ctx, t, p).Status.ProvisionerKeyID, "P must move its key to coder-b") + + require.NoError(t, k8sClient.Delete(ctx, &coderv1alpha1.CoderControlPlane{ObjectMeta: metav1.ObjectMeta{Name: "coder", Namespace: f.namespace}})) + require.NoError(t, k8sClient.Delete(ctx, &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "coder-operator-token", Namespace: f.namespace}})) + createControlPlaneWithToken(ctx, t, f.namespace, "coder", oldURL, newToken) + f.bootstrapClient.deleteKeyByIDErr = nil + _, err = f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: pName}) + require.NoError(t, err) + + for _, d := range keyDeletes(f.bootstrapClient) { + require.False(t, d.keyID == keyOld.KeyID.String() && d.token == newToken, + "the new control plane's token must not be used for the old key, sent %+v", keyDeletes(f.bootstrapClient)) + } + latest = f.get(ctx, t, p) + require.True(t, slices.ContainsFunc(latest.Status.PendingKeys, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { + return record.KeyID == keyOld.KeyID.String() + }), "the record of the replaced key must stay") + report := meta.FindStatusCondition(latest.Status.Conditions, coderv1alpha1.CoderProvisionerConditionControlPlaneChangedKeyKept) + require.NotNil(t, report, "P must report the record that it keeps") + require.Equal(t, metav1.ConditionTrue, report.Status) +} diff --git a/internal/controller/coderprovisioner_keysecret_test.go b/internal/controller/coderprovisioner_keysecret_test.go index 70125579..103adb74 100644 --- a/internal/controller/coderprovisioner_keysecret_test.go +++ b/internal/controller/coderprovisioner_keysecret_test.go @@ -101,10 +101,9 @@ func TestCoderProvisionerKeySecret_OwnedIsReused(t *testing.T) { t.Parallel() ctx := context.Background() f := newProvisionerCredentialFixture(ctx, t, "cprov-key-owned") - // The key exists in coderd, so coderd returns no plaintext and the - // controller rotates it into the Secret that this provisioner owns. + // Status records no key, so the controller creates one and writes it + // into the Secret that this provisioner owns. f.bootstrapClient.provisionerKeyResponses = []coderbootstrap.EnsureProvisionerKeyResponse{ - {OrganizationID: uuid.New(), KeyID: uuid.New(), KeyName: "provisioner", Key: ""}, {OrganizationID: uuid.New(), KeyID: uuid.New(), KeyName: "provisioner", Key: testRotatedKeyValue}, } provisioner := f.createKeyProvisioner(ctx, t, "provisioner", "") @@ -119,8 +118,7 @@ func TestCoderProvisionerKeySecret_OwnedIsReused(t *testing.T) { latest := f.get(ctx, t, provisioner) requireCredentialsReady(t, latest.Status.Conditions, metav1.ConditionTrue, "CredentialsAccepted") - require.Equal(t, 2, f.bootstrapClient.provisionerKeyCalls) - require.Equal(t, 1, f.bootstrapClient.deleteKeyCalls) + require.Equal(t, 1, f.bootstrapClient.provisionerKeyCalls) after := &corev1.Secret{} require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(secret), after)) diff --git a/internal/controller/coderprovisioner_pendingkeys_test.go b/internal/controller/coderprovisioner_pendingkeys_test.go index eafd1379..8be61ffa 100644 --- a/internal/controller/coderprovisioner_pendingkeys_test.go +++ b/internal/controller/coderprovisioner_pendingkeys_test.go @@ -61,14 +61,11 @@ func (c *keyRecordingClient) DeleteProvisionerKeyByID(ctx context.Context, req c // keyDelete is one key delete request that reached the fake Coder. type keyDelete struct { - coderURL, token, keyName, keyID string + coderURL, token, keyID string } func keyDeletes(client *fakeBootstrapClient) []keyDelete { var deletes []keyDelete - for _, req := range client.deleteKeyRequests { - deletes = append(deletes, keyDelete{coderURL: req.CoderURL, token: req.SessionToken, keyName: req.KeyName}) - } for _, req := range client.deleteKeyByIDRequests { deletes = append(deletes, keyDelete{coderURL: req.CoderURL, token: req.SessionToken, keyID: req.KeyID.String()}) } @@ -135,7 +132,7 @@ func TestCoderProvisionerKey_IntermediateEndpointRevoked(t *testing.T) { f.bootstrapClient.provisionerKeyResponses = []coderbootstrap.EnsureProvisionerKeyResponse{keyA, keyB, keyC} if tt.statusWriteFails { f.bootstrapClient.provisionerKeyByName = func(req coderbootstrap.ProvisionerKeyByNameRequest) *codersdk.ProvisionerKey { - if req.CoderURL != apiURL("coder-b") { + if req.CoderURL != apiURL("coder-b") || f.bootstrapClient.provisionerKeyCalls < 2 { return nil } return &codersdk.ProvisionerKey{ID: keyB.KeyID, OrganizationID: keyB.OrganizationID, Name: keyB.KeyName} @@ -250,6 +247,8 @@ func TestCoderProvisionerKey_IntermediateEndpointRevoked(t *testing.T) { // earlier reconcile created at the target and did not publish. The next // reconcile must replace them with a key that status records as applied, also // when nothing else asks for a rotation, and deletion must delete that key. +// A key whose create answer was lost is not recorded, so the provisioner +// must leave it and report it until it is deleted by hand. func TestCoderProvisionerKey_UnpublishedKeyIsRotated(t *testing.T) { t.Parallel() @@ -257,11 +256,14 @@ func TestCoderProvisionerKey_UnpublishedKeyIsRotated(t *testing.T) { name string // fail makes one reconcile leave an unpublished key at the target. fail func(ctx context.Context, t *testing.T, f provisionerCredentialFixture, provisioner *coderv1alpha1.CoderProvisioner, unpublished coderbootstrap.EnsureProvisionerKeyResponse) + // answerLost: status records no ID of the unpublished key. + answerLost bool }{ { // The create reached Coder, but the operator got no answer. The // spec change that caused the rotation is then reverted. - name: "create answer lost, spec reverted", + name: "create answer lost, spec reverted", + answerLost: true, fail: func(ctx context.Context, t *testing.T, f provisionerCredentialFixture, provisioner *coderv1alpha1.CoderProvisioner, _ coderbootstrap.EnsureProvisionerKeyResponse) { latest := f.get(ctx, t, provisioner) latest.Spec.Tags = map[string]string{"region": "rotated"} @@ -306,17 +308,29 @@ func TestCoderProvisionerKey_UnpublishedKeyIsRotated(t *testing.T) { f := newProvisionerCredentialFixture(ctx, t, "cprov-unpublished") first := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: uuid.New(), KeyID: uuid.New(), KeyName: "provisioner", Key: "material-1"} unpublished := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: first.OrganizationID, KeyID: uuid.New(), KeyName: "provisioner", Key: "material-2"} - // Coder lists the unpublished key, without plaintext, until the - // rotation deletes it by name. - listed := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: unpublished.OrganizationID, KeyID: unpublished.KeyID, KeyName: "provisioner"} final := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: first.OrganizationID, KeyID: uuid.New(), KeyName: "provisioner", Key: "material-3"} - f.bootstrapClient.provisionerKeyResponses = []coderbootstrap.EnsureProvisionerKeyResponse{first, unpublished, listed, final} + f.bootstrapClient.provisionerKeyResponses = []coderbootstrap.EnsureProvisionerKeyResponse{first, unpublished, final} + var inCoder *codersdk.ProvisionerKey + f.bootstrapClient.provisionerKeyByName = func(coderbootstrap.ProvisionerKeyByNameRequest) *codersdk.ProvisionerKey { return inCoder } provisioner := f.createProvisioner(ctx, t, nil, nil) + name := types.NamespacedName{Name: provisioner.Name, Namespace: provisioner.Namespace} f.reconcile(ctx, t, provisioner) f.reconcile(ctx, t, provisioner) + inCoder = &codersdk.ProvisionerKey{ID: first.KeyID, OrganizationID: first.OrganizationID, Name: first.KeyName} tt.fail(ctx, t, f, provisioner, unpublished) - deletesBefore := len(f.bootstrapClient.deleteKeyRequests) + inCoder = &codersdk.ProvisionerKey{ID: unpublished.KeyID, OrganizationID: unpublished.OrganizationID, Name: unpublished.KeyName} + if tt.answerLost { + result, err := f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: name}) + require.NoError(t, err) + require.Positive(t, result.RequeueAfter) + keyReady := meta.FindStatusCondition(f.get(ctx, t, provisioner).Status.Conditions, coderv1alpha1.CoderProvisionerConditionProvisionerKeyReady) + require.NotNil(t, keyReady) + require.Equal(t, "KeyNameInUse", keyReady.Reason, "the provisioner must report the key that it cannot show it created") + require.Contains(t, keyReady.Message, unpublished.KeyID.String()) + require.NotContains(t, keyDeletes(f.bootstrapClient), keyDelete{coderURL: internalCoderURL(f), token: testOperatorToken, keyID: unpublished.KeyID.String()}) + inCoder = nil + } f.reconcile(ctx, t, provisioner) latest := f.get(ctx, t, provisioner) @@ -325,9 +339,10 @@ func TestCoderProvisionerKey_UnpublishedKeyIsRotated(t *testing.T) { secret := &corev1.Secret{} require.NoError(t, k8sClient.Get(ctx, types.NamespacedName{Name: provisioner.Name + "-provisioner-key", Namespace: provisioner.Namespace}, secret)) require.Equal(t, "material-3", string(secret.Data[coderv1alpha1.DefaultProvisionerKeySecretKey])) - require.Contains(t, f.bootstrapClient.deleteKeyRequests[deletesBefore:], - deleteKeyRequest{CoderURL: internalCoderURL(f), SessionToken: testOperatorToken, OrganizationName: "default", KeyName: "provisioner"}, - "the rotation must delete the unpublished key") + if !tt.answerLost { + require.Contains(t, keyDeletes(f.bootstrapClient), keyDelete{coderURL: internalCoderURL(f), token: testOperatorToken, keyID: unpublished.KeyID.String()}, + "the rotation must delete the unpublished key") + } require.NoError(t, k8sClient.Delete(ctx, latest)) f.reconcile(ctx, t, provisioner) @@ -489,11 +504,6 @@ func (c *keyStoreClient) EnsureProvisionerKey(_ context.Context, req coderbootst return response, nil } -func (c *keyStoreClient) DeleteProvisionerKey(_ context.Context, coderURL, _, organizationName, keyName string) error { - delete(c.keys, newKeySlot(coderURL, organizationName, keyName)) - return nil -} - func (c *keyStoreClient) DeleteProvisionerKeyByID(_ context.Context, req coderbootstrap.DeleteProvisionerKeyByIDRequest) error { maps.DeleteFunc(c.keys, func(slot keySlot, keyID uuid.UUID) bool { return slot.coderURL == req.CoderURL && slot.organizationID() == req.OrganizationID && keyID == req.KeyID @@ -514,13 +524,13 @@ func (c *keyStoreClient) ProvisionerKeyByName(_ context.Context, req coderbootst } // TestCoderProvisionerKey_IDLessRecordKeepsOtherProvisionersKey checks a -// provisioner P whose target moves from A to endpoint B, where provisioner Q -// holds its key under the same key name, and then to endpoint C. P records -// each create without key IDs, and each create fails before it reaches -// Coder. Neither the cleanup after P moves back to A nor the deletion of P -// may delete the key of Q. After the move back, P must report each entry it -// keeps, and drop an entry and its report once Coder has no key with its -// name. +// provisioner P whose target moves from A to endpoint B and then to endpoint +// C. P records each create without key IDs, and each create fails before it +// reaches Coder. Between the two moves, provisioner Q creates its key at B +// under the same key name. Neither the cleanup after P moves back to A nor +// the deletion of P may delete the key of Q. After the move back, P must +// report each entry it keeps, and drop an entry and its report once Coder has +// no key with its name. func TestCoderProvisionerKey_IDLessRecordKeepsOtherProvisionersKey(t *testing.T) { t.Parallel() @@ -542,16 +552,6 @@ func TestCoderProvisionerKey_IDLessRecordKeepsOtherProvisionersKey(t *testing.T) store := &keyStoreClient{fakeBootstrapClient: f.bootstrapClient, keys: map[keySlot]uuid.UUID{}} f.reconciler.BootstrapClient = store - q := f.createKeyProvisioner(ctx, t, "q", "") - q.Spec.ControlPlaneRef.Name = "coder-b" - q.Spec.Key.Name = "provisioner" - require.NoError(t, k8sClient.Update(ctx, q)) - f.reconcile(ctx, t, q) - f.reconcile(ctx, t, q) - slotB := newKeySlot(urlB, "", "provisioner") - qKeyID := f.get(ctx, t, q).Status.ProvisionerKeyID - require.Equal(t, store.keys[slotB].String(), qKeyID, "Q must hold the key named provisioner at B") - p := f.createProvisioner(ctx, t, nil, nil) pName := types.NamespacedName{Name: p.Name, Namespace: p.Namespace} f.reconcile(ctx, t, p) @@ -570,6 +570,8 @@ func TestCoderProvisionerKey_IDLessRecordKeepsOtherProvisionersKey(t *testing.T) return meta.FindStatusCondition(f.get(ctx, t, p).Status.Conditions, coderv1alpha1.CoderProvisionerConditionUnverifiedKeyKept) } + slotB := newKeySlot(urlB, "", "provisioner") + var qKeyID string store.failEnsureAt = []string{urlB, urlC} for _, controlPlane := range []string{"coder-b", "coder-c"} { moveTo(controlPlane) @@ -578,6 +580,18 @@ func TestCoderProvisionerKey_IDLessRecordKeepsOtherProvisionersKey(t *testing.T) require.True(t, slices.ContainsFunc(f.get(ctx, t, p).Status.PendingKeys, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { return record.CoderURL == apiURL(controlPlane) && record.KeyName == "provisioner" && record.KeyID == "" }), "P must record its create at %s without key IDs", controlPlane) + if controlPlane != "coder-b" { + continue + } + store.failEnsureAt = []string{urlC} + q := f.createKeyProvisioner(ctx, t, "q", "") + q.Spec.ControlPlaneRef.Name = "coder-b" + q.Spec.Key.Name = "provisioner" + require.NoError(t, k8sClient.Update(ctx, q)) + f.reconcile(ctx, t, q) + f.reconcile(ctx, t, q) + qKeyID = f.get(ctx, t, q).Status.ProvisionerKeyID + require.Equal(t, store.keys[slotB].String(), qKeyID, "Q must hold the key named provisioner at B") } if deleteP { diff --git a/internal/controller/coderworkspaceproxy_controller_test.go b/internal/controller/coderworkspaceproxy_controller_test.go index 93f7674e..f7fb80d0 100644 --- a/internal/controller/coderworkspaceproxy_controller_test.go +++ b/internal/controller/coderworkspaceproxy_controller_test.go @@ -29,14 +29,14 @@ type fakeBootstrapClient struct { provisionerKeyErr error provisionerKeyCalls int provisionerKeyRequests []coderbootstrap.EnsureProvisionerKeyRequest - deleteKeyErr error - deleteKeyCalls int - deleteKeyRequests []deleteKeyRequest deleteKeyByIDErr error deleteKeyByIDRequests []coderbootstrap.DeleteProvisionerKeyByIDRequest // provisionerKeyByName answers ProvisionerKeyByName. When it is nil, // Coder has no key with the name. provisionerKeyByName func(coderbootstrap.ProvisionerKeyByNameRequest) *codersdk.ProvisionerKey + // provisionerKeyByID answers ProvisionerKeyByID. When it is nil, Coder + // has no key with the ID. + provisionerKeyByID func(coderbootstrap.ProvisionerKeyByIDRequest) *codersdk.ProvisionerKey // Entitlements support. entitlementsResponse codersdk.Entitlements @@ -45,13 +45,6 @@ type fakeBootstrapClient struct { entitlementsRequests []entitlementsRequest } -type deleteKeyRequest struct { - CoderURL string - SessionToken string - OrganizationName string - KeyName string -} - type entitlementsRequest struct { CoderURL string SessionToken string @@ -80,17 +73,6 @@ func (f *fakeBootstrapClient) EnsureProvisionerKey(_ context.Context, req coderb return f.provisionerKeyResponses[idx], nil } -func (f *fakeBootstrapClient) DeleteProvisionerKey(_ context.Context, coderURL, sessionToken, orgName, keyName string) error { - f.deleteKeyCalls++ - f.deleteKeyRequests = append(f.deleteKeyRequests, deleteKeyRequest{ - CoderURL: coderURL, - SessionToken: sessionToken, - OrganizationName: orgName, - KeyName: keyName, - }) - return f.deleteKeyErr -} - func (f *fakeBootstrapClient) DeleteProvisionerKeyByID(_ context.Context, req coderbootstrap.DeleteProvisionerKeyByIDRequest) error { f.deleteKeyByIDRequests = append(f.deleteKeyByIDRequests, req) return f.deleteKeyByIDErr @@ -103,6 +85,13 @@ func (f *fakeBootstrapClient) ProvisionerKeyByName(_ context.Context, req coderb return f.provisionerKeyByName(req), nil } +func (f *fakeBootstrapClient) ProvisionerKeyByID(_ context.Context, req coderbootstrap.ProvisionerKeyByIDRequest) (*codersdk.ProvisionerKey, error) { + if f.provisionerKeyByID == nil { + return nil, nil + } + return f.provisionerKeyByID(req), nil +} + func (f *fakeBootstrapClient) Entitlements(_ context.Context, coderURL, sessionToken string) (codersdk.Entitlements, error) { f.entitlementsCalls++ f.entitlementsRequests = append(f.entitlementsRequests, entitlementsRequest{