diff --git a/api/v1alpha1/coderprovisioner_types.go b/api/v1alpha1/coderprovisioner_types.go index 13e56ac53..7d6655ec0 100644 --- a/api/v1alpha1/coderprovisioner_types.go +++ b/api/v1alpha1/coderprovisioner_types.go @@ -29,6 +29,13 @@ const ( // referenced credential Secret and setting passed the operator's checks. // When it is False, the operator keeps the last applied Deployment. CoderProvisionerConditionCredentialsReady = "CredentialsReady" + // CoderProvisionerConditionUnverifiedKeyKept is True while + // status.pendingKeys holds an entry without key IDs that the operator + // could not remove: Coder holds a key with its name, which nothing shows + // this provisioner created, or the operator could not check Coder. The + // operator deletes no key for such an entry. The message names each + // entry, and the operator removes the condition when none is left. + CoderProvisionerConditionUnverifiedKeyKept = "UnverifiedKeyKept" // DefaultProvisionerKeySecretKey is the default data key for provisioner key secrets. DefaultProvisionerKeySecretKey = "key" @@ -105,6 +112,42 @@ type CoderProvisionerStatus struct { ControlPlaneURL string `json:"controlPlaneURL,omitempty"` // SecretRef references the provisioner key secret data currently in use. SecretRef *SecretKeySelector `json:"secretRef,omitempty"` + // PendingKeys lists the provisioner keys that can exist in Coder besides + // the key in provisionerKeyID: a key that the operator is about to create + // or has not yet written to the provisioner key Secret, and replaced keys + // whose deletion Coder has not confirmed. The operator deletes 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. + // +listType=atomic + PendingKeys []CoderProvisionerKeyRecord `json:"pendingKeys,omitempty"` +} + +// CoderProvisionerKeyRecord names a provisioner key that the operator created, +// or was about to create, at one Coder endpoint. +type CoderProvisionerKeyRecord struct { + // ControlPlaneRefName is the CoderControlPlane whose operator token the + // operator used for the key. + ControlPlaneRefName string `json:"controlPlaneRefName"` + // CoderURL is the Coder API URL that the operator sent the key request to. + CoderURL string `json:"coderURL"` + // OrganizationName is the Coder organization of the key. + OrganizationName string `json:"organizationName"` + // KeyName is the provisioner key name in Coder. + KeyName string `json:"keyName"` + // OrganizationID is the organization ID that Coder returned for the key. + // When organizationID or keyID is empty, the operator deletes no key. It + // removes the entry when Coder has no key with keyName, or when that key + // is the key in provisionerKeyID. Otherwise it keeps the entry and sets + // the condition UnverifiedKeyKept. + OrganizationID string `json:"organizationID,omitempty"` + // KeyID is the provisioner key ID that Coder returned. + KeyID string `json:"keyID,omitempty"` + // Unpublished is true from before the create until the operator records + // a key as applied. While it is true, the Secret can hold this key, or a + // key that a rotation deleted, so the operator replaces the key in the + // Secret. + Unpublished bool `json:"unpublished,omitempty"` } // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 4c37b03bc..b4b3529fd 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -948,6 +948,22 @@ func (in *CoderProvisioner) DeepCopyObject() runtime.Object { return nil } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *CoderProvisionerKeyRecord) DeepCopyInto(out *CoderProvisionerKeyRecord) { + *out = *in + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new CoderProvisionerKeyRecord. +func (in *CoderProvisionerKeyRecord) DeepCopy() *CoderProvisionerKeyRecord { + if in == nil { + return nil + } + out := new(CoderProvisionerKeyRecord) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *CoderProvisionerKeySpec) DeepCopyInto(out *CoderProvisionerKeySpec) { *out = *in @@ -1065,6 +1081,11 @@ func (in *CoderProvisionerStatus) DeepCopyInto(out *CoderProvisionerStatus) { *out = new(SecretKeySelector) **out = **in } + if in.PendingKeys != nil { + in, out := &in.PendingKeys, &out.PendingKeys + *out = make([]CoderProvisionerKeyRecord, len(*in)) + copy(*out, *in) + } return } diff --git a/config/crd/bases/coder.com_coderprovisioners.yaml b/config/crd/bases/coder.com_coderprovisioners.yaml index e5e78be9f..09d2b0b5a 100644 --- a/config/crd/bases/coder.com_coderprovisioners.yaml +++ b/config/crd/bases/coder.com_coderprovisioners.yaml @@ -434,6 +434,62 @@ spec: description: OrganizationName is the organization name last applied to the provisioner key. type: string + pendingKeys: + description: |- + PendingKeys lists the provisioner keys that can exist in Coder besides + the key in provisionerKeyID: a key that the operator is about to create + or has not yet written to the provisioner key Secret, and replaced keys + whose deletion Coder has not confirmed. The operator deletes 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. + items: + description: |- + CoderProvisionerKeyRecord names a provisioner key that the operator created, + or was about to create, at one Coder endpoint. + properties: + coderURL: + description: CoderURL is the Coder API URL that the operator + sent the key request to. + type: string + controlPlaneRefName: + description: |- + ControlPlaneRefName is the CoderControlPlane whose operator token the + operator used for the key. + type: string + keyID: + description: KeyID is the provisioner key ID that Coder returned. + type: string + keyName: + description: KeyName is the provisioner key name in Coder. + type: string + organizationID: + description: |- + OrganizationID is the organization ID that Coder returned for the key. + When organizationID or keyID is empty, the operator deletes no key. It + removes the entry when Coder has no key with keyName, or when that key + is the key in provisionerKeyID. Otherwise it keeps the entry and sets + the condition UnverifiedKeyKept. + type: string + organizationName: + description: OrganizationName is the Coder organization of the + key. + type: string + unpublished: + description: |- + Unpublished is true from before the create until the operator records + a key as applied. While it is true, the Secret can hold this key, or a + key that a rotation deleted, so the operator replaces the key in the + Secret. + type: boolean + required: + - coderURL + - controlPlaneRefName + - keyName + - organizationName + type: object + type: array + x-kubernetes-list-type: atomic phase: description: Phase is a high-level readiness indicator. type: string diff --git a/dist/install.yaml b/dist/install.yaml index 7172ee67e..f9f08fdef 100644 --- a/dist/install.yaml +++ b/dist/install.yaml @@ -6190,6 +6190,62 @@ spec: description: OrganizationName is the organization name last applied to the provisioner key. type: string + pendingKeys: + description: |- + PendingKeys lists the provisioner keys that can exist in Coder besides + the key in provisionerKeyID: a key that the operator is about to create + or has not yet written to the provisioner key Secret, and replaced keys + whose deletion Coder has not confirmed. The operator deletes 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. + items: + description: |- + CoderProvisionerKeyRecord names a provisioner key that the operator created, + or was about to create, at one Coder endpoint. + properties: + coderURL: + description: CoderURL is the Coder API URL that the operator + sent the key request to. + type: string + controlPlaneRefName: + description: |- + ControlPlaneRefName is the CoderControlPlane whose operator token the + operator used for the key. + type: string + keyID: + description: KeyID is the provisioner key ID that Coder returned. + type: string + keyName: + description: KeyName is the provisioner key name in Coder. + type: string + organizationID: + description: |- + OrganizationID is the organization ID that Coder returned for the key. + When organizationID or keyID is empty, the operator deletes no key. It + removes the entry when Coder has no key with keyName, or when that key + is the key in provisionerKeyID. Otherwise it keeps the entry and sets + the condition UnverifiedKeyKept. + type: string + organizationName: + description: OrganizationName is the Coder organization of the + key. + type: string + unpublished: + description: |- + Unpublished is true from before the create until the operator records + a key as applied. While it is true, the Secret can hold this key, or a + key that a rotation deleted, so the operator replaces the key in the + Secret. + type: boolean + required: + - coderURL + - controlPlaneRefName + - keyName + - organizationName + type: object + type: array + x-kubernetes-list-type: atomic phase: description: Phase is a high-level readiness indicator. type: string diff --git a/docs/reference/api/coderprovisioner.md b/docs/reference/api/coderprovisioner.md index 438762b4f..9394015f7 100644 --- a/docs/reference/api/coderprovisioner.md +++ b/docs/reference/api/coderprovisioner.md @@ -41,9 +41,25 @@ | `controlPlaneRefName` | string | ControlPlaneRefName is the control plane ref name last applied to the provisioner key. | | `controlPlaneURL` | string | ControlPlaneURL is the control plane URL last applied to the provisioner key. | | `secretRef` | [SecretKeySelector](#secretkeyselector) | SecretRef references the provisioner key secret data currently in use. | +| `pendingKeys` | [CoderProvisionerKeyRecord](#coderprovisionerkeyrecord) array | PendingKeys lists the provisioner keys that can exist in Coder besides the key in provisionerKeyID: a key that the operator is about to create or has not yet written to the provisioner key Secret, and replaced keys whose deletion Coder has not confirmed. The operator deletes the key of each entry with organizationID and keyID at its coderURL with the operator token of its control plane, and removes the entry when Coder confirms that the key is gone. | ## Referenced types +### CoderProvisionerKeyRecord + +CoderProvisionerKeyRecord names a provisioner key that the operator created, +or was about to create, at one Coder endpoint. + +| Field | Type | Description | +| --- | --- | --- | +| `controlPlaneRefName` | string | ControlPlaneRefName is the CoderControlPlane whose operator token the operator used for the key. | +| `coderURL` | string | CoderURL is the Coder API URL that the operator sent the key request to. | +| `organizationName` | string | OrganizationName is the Coder organization of the key. | +| `keyName` | string | KeyName is the provisioner key name in Coder. | +| `organizationID` | string | OrganizationID is the organization ID that Coder returned for the key. When organizationID or keyID is empty, the operator deletes no key. It removes the entry when Coder has no key with keyName, or when that key is the key in provisionerKeyID. Otherwise it keeps the entry and sets the condition UnverifiedKeyKept. | +| `keyID` | string | KeyID is the provisioner key ID that Coder returned. | +| `unpublished` | boolean | Unpublished is true from before the create until the operator records a key as applied. While it is true, the Secret can hold this key, or a key that a rotation deleted, so the operator replaces the key in the Secret. | + ### CoderProvisionerKeySpec CoderProvisionerKeySpec configures provisioner key naming and storage. diff --git a/docs/tutorials/deploy-coderprovisioner.md b/docs/tutorials/deploy-coderprovisioner.md index 3c4efc744..47090eb52 100644 --- a/docs/tutorials/deploy-coderprovisioner.md +++ b/docs/tutorials/deploy-coderprovisioner.md @@ -78,7 +78,9 @@ kubectl get secret coderprovisioner-sample-provisioner-key -n coder kubectl delete coderprovisioner coderprovisioner-sample -n coder ``` -The operator then deletes the provisioner key in Coder that this provisioner created. It finds the key by the organization and key IDs in `status.organizationID` and `status.provisionerKeyID`, not by the names in the spec. When the status has no key, for example because the provisioner never passed its checks, the operator removes its finalizer without a call to Coder. +The operator then deletes the provisioner keys in Coder that this provisioner created. It finds the key in the provisioner key Secret by the organization and key IDs in `status.organizationID` and `status.provisionerKeyID`, not by the names in the spec. It also deletes 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"}]'`. 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 9b99bc0cb..e2385d1ba 100644 --- a/internal/coderbootstrap/client.go +++ b/internal/coderbootstrap/client.go @@ -96,6 +96,7 @@ type Client interface { EnsureProvisionerKey(context.Context, EnsureProvisionerKeyRequest) (EnsureProvisionerKeyResponse, error) DeleteProvisionerKey(ctx context.Context, coderURL, sessionToken, orgName, keyName string) error DeleteProvisionerKeyByID(context.Context, DeleteProvisionerKeyByIDRequest) error + 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 7bfab48eb..785cfb1c1 100644 --- a/internal/coderbootstrap/provisionerkeys.go +++ b/internal/coderbootstrap/provisionerkeys.go @@ -1,6 +1,7 @@ package coderbootstrap import ( + "cmp" "context" "errors" "net/http" @@ -140,6 +141,50 @@ func (c *SDKClient) DeleteProvisionerKey(ctx context.Context, coderURL, sessionT return xerrors.Errorf("delete provisioner key %q: %w", keyName, err) } +// ProvisionerKeyByNameRequest identifies a provisioner key by its name. +type ProvisionerKeyByNameRequest struct { + CoderURL string + SessionToken string + OrganizationName string + KeyName string +} + +// ProvisionerKeyByName returns the provisioner key named req.KeyName in the +// organization req.OrganizationName. It returns nil when Coder answers that +// the organization or a key with that name does not exist. +func (c *SDKClient) ProvisionerKeyByName(ctx context.Context, req ProvisionerKeyByNameRequest) (*codersdk.ProvisionerKey, error) { + if err := validateProvisionerKeyInputs(req.CoderURL, req.SessionToken, req.KeyName); err != nil { + return nil, err + } + + client, err := newAuthenticatedClient(req.CoderURL, req.SessionToken) + if err != nil { + return nil, err + } + + organization, err := resolveOrganizationByName(ctx, client, cmp.Or(req.OrganizationName, codersdk.DefaultOrganization)) + if isNotFound(err) { + return nil, nil + } + if err != nil { + return nil, err + } + key, err := withOptionalRateLimitBypass(ctx, func(requestCtx context.Context) (*codersdk.ProvisionerKey, error) { + return findOrganizationProvisionerKey(requestCtx, client, organization.ID, req.KeyName) + }) + if isNotFound(err) { + return nil, nil + } + if err != nil { + return nil, err + } + if key != nil && key.ID == uuid.Nil { + return nil, xerrors.Errorf("assertion failed: provisioner key %q returned an empty ID", req.KeyName) + } + + return key, nil +} + // DeleteProvisionerKeyByIDRequest identifies one provisioner key by the IDs // that the caller recorded when it applied the key. type DeleteProvisionerKeyByIDRequest struct { @@ -208,9 +253,15 @@ func (c *SDKClient) DeleteProvisionerKeyByID(ctx context.Context, req DeleteProv return nil } +// coderNotFoundMessage is the message of the 404 that Coder answers for a +// missing organization or provisioner key (coderd/httpapi/httpapi.go:149 and +// coderd/httpmw/provisionerkey.go:45-46 in v2.37.3). Another 404, for example +// from a proxy or for a wrong base URL, does not prove that the key is gone. +const coderNotFoundMessage = "Resource not found or you do not have access to this resource" + func isNotFound(err error) bool { var apiErr *codersdk.Error - return errors.As(err, &apiErr) && apiErr.StatusCode() == http.StatusNotFound + return errors.As(err, &apiErr) && apiErr.StatusCode() == http.StatusNotFound && apiErr.Message == coderNotFoundMessage } // Entitlements returns deployment entitlements for the given coderd instance. diff --git a/internal/coderbootstrap/provisionerkeys_deletebyid_test.go b/internal/coderbootstrap/provisionerkeys_deletebyid_test.go index 9339fe242..48f726531 100644 --- a/internal/coderbootstrap/provisionerkeys_deletebyid_test.go +++ b/internal/coderbootstrap/provisionerkeys_deletebyid_test.go @@ -14,6 +14,10 @@ import ( "github.com/coder/coder-k8s/internal/coderbootstrap" ) +// coderNotFound is the body of the 404 that Coder answers for a missing +// organization or provisioner key. +var coderNotFound = map[string]any{"message": "Resource not found or you do not have access to this resource"} + func TestDeleteProvisionerKeyByID(t *testing.T) { t.Parallel() @@ -25,12 +29,15 @@ func TestDeleteProvisionerKeyByID(t *testing.T) { tests := []struct { name string listStatus int + listBody map[string]any listedID uuid.UUID wantDeletes []string + wantErr bool }{ {name: "recorded key exists", listStatus: http.StatusOK, listedID: recordedKeyID, wantDeletes: []string{keysPath + "/shared-name"}}, {name: "same name, other ID", listStatus: http.StatusOK, listedID: uuid.New()}, - {name: "organization gone", listStatus: http.StatusNotFound}, + {name: "organization gone", listStatus: http.StatusNotFound, listBody: coderNotFound}, + {name: "404 not from Coder", listStatus: http.StatusNotFound, listBody: map[string]any{"message": "Route not found."}, wantErr: true}, } for _, tt := range tests { @@ -43,7 +50,7 @@ func TestDeleteProvisionerKeyByID(t *testing.T) { switch { case r.Method == http.MethodGet && r.URL.Path == keysPath: if tt.listStatus != http.StatusOK { - writeJSONResponse(t, w, tt.listStatus, map[string]any{"message": "not found"}) + writeJSONResponse(t, w, tt.listStatus, tt.listBody) return } writeJSONResponse(t, w, http.StatusOK, []map[string]any{{ @@ -71,7 +78,11 @@ func TestDeleteProvisionerKeyByID(t *testing.T) { OrganizationID: orgID, KeyID: recordedKeyID, }) - require.NoError(t, err) + if tt.wantErr { + require.Error(t, err, "only Coder's own 404 proves that the key is gone") + } else { + require.NoError(t, err) + } mu.Lock() defer mu.Unlock() require.Equal(t, tt.wantDeletes, deletes) @@ -108,3 +119,73 @@ func TestDeleteProvisionerKeyByID_ValidationErrors(t *testing.T) { }) } } + +func TestProvisionerKeyByName(t *testing.T) { + t.Parallel() + + orgID := uuid.New() + listedID := uuid.New() + now := time.Now().UTC().Format(time.RFC3339) + keysPath := "/api/v2/organizations/" + orgID.String() + "/provisionerkeys" + + tests := []struct { + name string + orgGone bool + listBody map[string]any + listedName string + wantKeyID uuid.UUID + wantErr bool + }{ + {name: "organization gone", orgGone: true}, + {name: "organization gone before the list", listBody: coderNotFound}, + {name: "key exists", listedName: "provisioner", wantKeyID: listedID}, + {name: "no key with the name", listedName: "other"}, + {name: "404 not from Coder", listBody: map[string]any{"message": "Route not found."}, wantErr: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodGet && r.URL.Path == "/api/v2/organizations/default" && tt.orgGone: + writeJSONResponse(t, w, http.StatusNotFound, coderNotFound) + case r.Method == http.MethodGet && r.URL.Path == "/api/v2/organizations/default": + writeJSONResponse(t, w, http.StatusOK, map[string]any{"id": orgID.String(), "name": "default", "created_at": now, "updated_at": now}) + case r.Method == http.MethodGet && r.URL.Path == keysPath && tt.listBody != nil: + writeJSONResponse(t, w, http.StatusNotFound, tt.listBody) + case r.Method == http.MethodGet && r.URL.Path == keysPath: + writeJSONResponse(t, w, http.StatusOK, []map[string]any{{ + "id": listedID.String(), + "created_at": now, + "organization": orgID.String(), + "name": tt.listedName, + "tags": map[string]string{}, + }}) + default: + t.Errorf("unexpected request %s %s", r.Method, r.URL.Path) + w.WriteHeader(http.StatusNotFound) + } + })) + defer server.Close() + + key, err := coderbootstrap.NewSDKClient().ProvisionerKeyByName(context.Background(), coderbootstrap.ProvisionerKeyByNameRequest{ + CoderURL: server.URL, + SessionToken: "session-token", + KeyName: "provisioner", + }) + 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.wantKeyID == uuid.Nil { + require.Nil(t, key) + return + } + require.NotNil(t, key) + require.Equal(t, tt.wantKeyID, key.ID) + }) + } +} diff --git a/internal/controller/coderprovisioner_controller.go b/internal/controller/coderprovisioner_controller.go index a9ca11001..d8b92780a 100644 --- a/internal/controller/coderprovisioner_controller.go +++ b/internal/controller/coderprovisioner_controller.go @@ -2,6 +2,7 @@ package controller import ( + "cmp" "context" "crypto/rand" "encoding/binary" @@ -55,6 +56,7 @@ const ( externalProvisionerEntitlementRetryInterval = 2 * time.Minute provisionerOperatorAccessRetryInterval = 15 * time.Second + provisionerPendingKeyRetryInterval = 2 * time.Minute // #nosec G101 -- this is a field index key, not a credential. provisionerControlPlaneRefNameFieldIndex = ".spec.controlPlaneRef.name" @@ -286,6 +288,91 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req provisionerKeyName = keyName } + metadataMissing := status.OrganizationName == "" || status.TagsHash == "" || status.ControlPlaneRefName == "" || status.ControlPlaneURL == "" + + // Every key that can exist in Coder has a record in status: the applied + // fields for the key in the Secret, status.pendingKeys for the others. A + // key to create is recorded by name before the create, because Coder + // returns its ID only in the create response. Keys recorded with IDs in + // another slot than the target are deleted here. + target := coderv1alpha1.CoderProvisionerKeyRecord{ + ControlPlaneRefName: desiredControlPlaneRefName, + CoderURL: coderURL, + OrganizationName: organizationName, + KeyName: keyName, + Unpublished: true, + } + // An unpublished record names a key that an earlier reconcile created, or + // started to create, for the Secret and did not record as applied. The + // Secret can hold that key, or a key that the rotation already deleted, + // so a rotation replaces it with a key that status records. + rotate := !secretUsable || slices.ContainsFunc(provisioner.Status.PendingKeys, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { + return record.Unpublished + }) + pending := slices.Clone(provisioner.Status.PendingKeys) + var replaced coderv1alpha1.CoderProvisionerKeyRecord + if driftDetected { + replaced, err = r.appliedKeyRecord(ctx, provisioner, organizationName, keyName) + if err != nil { + return ctrl.Result{}, err + } + if !sameKeySlot(replaced, target) && !slices.Contains(pending, replaced) { + pending = append(pending, replaced) + } + } + if driftDetected || 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. + newest := -1 + for i, record := range pending { + if sameKeySlot(record, target) && record.KeyID != "" { + newest = i + } + } + var next []coderv1alpha1.CoderProvisionerKeyRecord + for i, record := range pending { + if !sameKeySlot(record, target) || i == newest { + next = append(next, record) + } + } + pending = append(next, target) + } + var kept []coderv1alpha1.CoderProvisionerKeyRecord + 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)) + } + } + conditionChanged := setUnverifiedKeyCondition(provisioner, unverified) + if conditionChanged || !slices.Equal(kept, provisioner.Status.PendingKeys) { + provisioner.Status.PendingKeys = kept + if err := r.Status().Update(ctx, provisioner); err != nil { + return ctrl.Result{}, fmt.Errorf("record pending provisioner keys in status: %w", err) + } + statusSnapshot = provisioner.Status.DeepCopy() + if statusSnapshot == nil { + return ctrl.Result{}, fmt.Errorf("assertion failed: status snapshot must not be nil after pending key status update") + } + } + log := ctrl.LoggerFrom(ctx) keyMaterial := "" if driftDetected { @@ -293,25 +380,19 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req "orgDrift", orgDrift, "keyNameDrift", keyNameDrift, "tagsDrift", tagsDrift, "controlPlaneRefDrift", controlPlaneRefDrift, "controlPlaneURLDrift", controlPlaneURLDrift) - oldOrg := provisioner.Status.OrganizationName - if oldOrg == "" { - oldOrg = organizationName - } - oldKeyName := provisioner.Status.ProvisionerKeyName - if oldKeyName == "" { - oldKeyName = keyName - } - oldControlPlaneURL := recordedCoderAPIURL(provisioner.Status.ControlPlaneURL, controlPlane, coderURL) - - if deleteErr := r.BootstrapClient.DeleteProvisionerKey( - ctx, - oldControlPlaneURL, - sessionToken, - oldOrg, - oldKeyName, - ); deleteErr != nil { - log.Info("failed to delete old provisioner key during drift rotation, creating new key anyway", - "oldKeyName", oldKeyName, "error", deleteErr) + // A replaced key in another slot was deleted above. One in the target + // slot is deleted here, so that the create below can use its name. + if sameKeySlot(replaced, target) { + if deleteErr := r.BootstrapClient.DeleteProvisionerKey( + ctx, + coderURL, + sessionToken, + replaced.OrganizationName, + replaced.KeyName, + ); deleteErr != nil { + log.Info("failed to delete old provisioner key during drift rotation, creating new key anyway", + "oldKeyName", replaced.KeyName, "error", deleteErr) + } } response, ensureErr := r.BootstrapClient.EnsureProvisionerKey(ctx, coderbootstrap.EnsureProvisionerKeyRequest{ @@ -394,7 +475,7 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req "ProvisionerKeyReady", "Provisioner key is available in coderd", ) - } else if !secretUsable { + } else if rotate { response, ensureErr := r.BootstrapClient.EnsureProvisionerKey(ctx, coderbootstrap.EnsureProvisionerKeyRequest{ CoderURL: coderURL, SessionToken: sessionToken, @@ -424,7 +505,7 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req // deleted), coderd won't return plaintext again. Rotate the key // by deleting and recreating it to obtain fresh material. if keyMaterial == "" { - log.Info("provisioner key exists in coderd but secret is missing, rotating key to recover", + log.Info("provisioner key exists in coderd but its plaintext is not in the secret, rotating key to recover", "keyName", keyName, "secretName", keySecretName) if deleteErr := r.BootstrapClient.DeleteProvisionerKey( @@ -479,7 +560,7 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req "ProvisionerKeyReady", "Provisioner key is available in coderd", ) - } else if status.OrganizationName == "" || status.TagsHash == "" || status.ControlPlaneRefName == "" || status.ControlPlaneURL == "" { + } else if metadataMissing { // Secret is usable and no drift detected, but status metadata is empty // (e.g. upgrade from older version). Call EnsureProvisionerKey to populate // IDs and key name. If coderd reports an existing key (no plaintext key @@ -567,17 +648,20 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req } if keyMaterial != "" { - // A new key exists in Coder. Record its IDs in status before its - // plaintext reaches the Secret, so that deletion can find the key to - // revoke (at status.controlPlaneURL or, during a control plane URL - // change, at the current control plane URL). Only the IDs: the applied spec values stay as they are - // until the Secret holds the key, so a failed Secret write is detected - // as drift (or missing metadata) and rotated again on the next try. - // A failed status write (for example a conflict because the - // provisioner is being deleted) revokes the new key before anyone can - // read it. - provisioner.Status.OrganizationID = organizationID - provisioner.Status.ProvisionerKeyID = provisionerKeyID + // A new key exists in Coder. Record its IDs in its pending record + // before its plaintext reaches the Secret, so that deletion and a + // later rotation delete it by ID at the target endpoint. The applied + // status fields stay as they are until the Secret holds the key, so a + // failed Secret write is detected as drift (or missing metadata) and + // rotated again on the next try. A failed status write (for example a + // conflict because the provisioner is being deleted) revokes the new + // key before anyone can read it. + i := slices.Index(provisioner.Status.PendingKeys, target) + if i < 0 { + return ctrl.Result{}, fmt.Errorf("assertion failed: status has no pending record of the new provisioner key %q", keyName) + } + provisioner.Status.PendingKeys[i].OrganizationID = organizationID + provisioner.Status.PendingKeys[i].KeyID = provisionerKeyID if err := r.Status().Update(ctx, provisioner); err != nil { r.revokeUnpublishedKey(ctx, coderURL, sessionToken, organizationID, provisionerKeyID) return ctrl.Result{}, fmt.Errorf("record new provisioner key in status: %w", err) @@ -620,6 +704,16 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req if keyStatusSnapshot == nil { return ctrl.Result{}, fmt.Errorf("assertion failed: key status snapshot must not be nil") } + // The Secret holds the key at the target, so the applied fields take over + // its record. 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. + provisioner.Status.PendingKeys = slices.DeleteFunc(provisioner.Status.PendingKeys, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { + return sameKeySlot(record, target) + }) + for i := range provisioner.Status.PendingKeys { + provisioner.Status.PendingKeys[i].Unpublished = false + } recordAppliedKeyStatus(provisioner, organizationID, appliedOrgName, provisionerKeyID, provisionerKeyName, appliedTagsHash, appliedControlPlaneRefName, appliedControlPlaneURL) provisioner.Status.SecretRef = &coderv1alpha1.SecretKeySelector{Name: keySecretName, Key: keySecretKey} @@ -677,6 +771,9 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req return ctrl.Result{}, err } + if len(provisioner.Status.PendingKeys) > 0 { + return ctrl.Result{RequeueAfter: provisionerPendingKeyRetryInterval}, nil + } return ctrl.Result{}, nil } @@ -687,78 +784,48 @@ func (r *CoderProvisionerReconciler) reconcileDeletion(ctx context.Context, prov log := ctrl.LoggerFrom(ctx) - // Delete only the key that status records for this resource. A provisioner - // that never applied a key (refused, not entitled, never ready) must not - // delete a Coder key that happens to have the name from its spec. - organizationID, keyID, recorded := recordedProvisionerKey(provisioner.Status) - if !recorded { + // Delete only the keys that status records for this resource. A + // provisioner that never sent a key create (refused, not entitled, never + // ready) has no record, and must not delete a Coder key that happens to + // have the name from its spec. + records := slices.Clone(provisioner.Status.PendingKeys) + if _, _, ok := parseKeyIDs(provisioner.Status.OrganizationID, provisioner.Status.ProvisionerKeyID); ok { + applied, err := r.appliedKeyRecord(ctx, provisioner, provisioner.Status.OrganizationName, provisioner.Status.ProvisionerKeyName) + if err != nil { + return ctrl.Result{}, err + } + records = append(records, applied) + // Before status.pendingKeys, the operator wrote the IDs of a new key + // before status.controlPlaneURL, so status from an older operator + // can name a key at the current endpoint under the old URL. The + // delete matches the key ID only. + current, err := r.currentEndpointRecord(ctx, provisioner, applied) + if err != nil { + return ctrl.Result{}, err + } + if current.CoderURL != applied.CoderURL { + records = append(records, current) + } + } + if len(records) == 0 { log.Info("status records no Coder provisioner key, removing the finalizer without a Coder call") return r.removeCleanupFinalizer(ctx, provisioner) } - keyName := provisioner.Status.ProvisionerKeyName - // Best-effort remote key cleanup: if the referenced control plane, - // its URL, operator access token, or any other prerequisite is - // unavailable, log a warning and proceed to finalizer removal so the - // CR does not get stuck in Terminating. This is common during - // namespace teardown, when the control plane was never ready, or - // when operator access bootstrap has not completed. - controlPlaneURL := provisioner.Status.ControlPlaneURL - controlPlaneName := strings.TrimSpace(provisioner.Spec.ControlPlaneRef.Name) - var controlPlane *coderv1alpha1.CoderControlPlane - if controlPlaneName != "" { - controlPlane = &coderv1alpha1.CoderControlPlane{} - namespacedName := types.NamespacedName{Name: controlPlaneName, Namespace: provisioner.Namespace} - if err := r.Get(ctx, namespacedName, controlPlane); err != nil { - log.Info("unable to read referenced CoderControlPlane during deletion, skipping remote key cleanup", - "controlPlaneRef", controlPlaneName, "error", err) - controlPlane = nil - } else if controlPlane.Name != controlPlaneName || controlPlane.Namespace != provisioner.Namespace { - return ctrl.Result{}, fmt.Errorf("assertion failed: fetched control plane %s/%s does not match expected %s/%s", - controlPlane.Namespace, controlPlane.Name, provisioner.Namespace, controlPlaneName) + // Best-effort remote key cleanup: when a key's control plane, its + // operator access token, or Coder is unavailable, the key stays and the + // finalizer is still removed, so the CR does not get stuck in + // Terminating. This is common during namespace teardown, when the + // control plane was never ready, or when operator access bootstrap has + // not completed. + for _, record := range records { + cleanup, err := r.revokeKey(ctx, provisioner, record) + if err != nil { + return ctrl.Result{}, err } - } - if controlPlaneURL == "" && controlPlane != nil { - controlPlaneURL = strings.TrimSpace(controlPlane.Status.URL) - } - - if controlPlaneURL != "" && controlPlane != nil { - sessionToken, tokenResult, tokenReason, tokenMessage, tokenErr := r.readOperatorSessionToken(ctx, provisioner, controlPlane) - switch { - case tokenErr != nil: - log.Info("unable to resolve operator access token during deletion, skipping remote key cleanup", - "error", tokenErr) - case tokenResult.RequeueAfter > 0: - log.Info("operator access is not ready during deletion, skipping remote key cleanup", - "reason", tokenReason, "message", tokenMessage) - default: - currentURL, urlErr := coderAPIURL(controlPlane) - if urlErr != nil { - log.Info("unable to derive the Coder API URL during deletion, skipping remote key cleanup", "error", urlErr) - break - } - // During a control plane URL change, status can hold the new key's - // IDs while status.controlPlaneURL still names the old endpoint. - // Try the current endpoint too: the delete matches the key ID only. - coderURLs := []string{recordedCoderAPIURL(provisioner.Status.ControlPlaneURL, controlPlane, currentURL)} - if currentURL != coderURLs[0] { - coderURLs = append(coderURLs, currentURL) - } - for _, coderURL := range coderURLs { - if deleteErr := r.BootstrapClient.DeleteProvisionerKeyByID(ctx, coderbootstrap.DeleteProvisionerKeyByIDRequest{ - CoderURL: coderURL, - SessionToken: sessionToken, - OrganizationID: organizationID, - KeyID: keyID, - }); deleteErr != nil { - // Treat key deletion failures as best-effort so the - // finalizer is still removed. Transient errors, auth - // issues, or org-lookup failures should not block CR - // cleanup. - log.Info("failed to delete remote provisioner key during deletion, proceeding with finalizer removal", - "keyName", keyName, "keyID", keyID, "error", deleteErr) - } - } + if 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) } } @@ -777,18 +844,6 @@ func coderAPIURL(controlPlane *coderv1alpha1.CoderControlPlane) (string, error) return coderURL.String(), nil } -// recordedCoderAPIURL maps a status.url that status.controlPlaneURL recorded -// to the URL for Coder API calls. An empty recorded URL, or one equal to the -// control plane's current status.url, maps to currentAPIURL. Any other URL -// names an earlier endpoint and is used as recorded. -func recordedCoderAPIURL(recordedURL string, controlPlane *coderv1alpha1.CoderControlPlane, currentAPIURL string) string { - recordedURL = strings.TrimSpace(recordedURL) - if recordedURL == "" || recordedURL == strings.TrimSpace(controlPlane.Status.URL) { - return currentAPIURL - } - return recordedURL -} - // recordAppliedKeyStatus sets the status fields that describe the key applied // in Coder. func recordAppliedKeyStatus( @@ -805,7 +860,8 @@ func recordAppliedKeyStatus( } // revokeUnpublishedKey deletes a key whose plaintext was never written to a -// Secret. It is best-effort: the key is unknown outside this reconcile. +// Secret. It is best-effort: when it fails, the pending record without key +// IDs still names the key. func (r *CoderProvisionerReconciler) revokeUnpublishedKey(ctx context.Context, coderURL, sessionToken, organizationID, keyID string) { log := ctrl.LoggerFrom(ctx) orgID, orgErr := uuid.Parse(organizationID) @@ -833,15 +889,14 @@ func (r *CoderProvisionerReconciler) removeCleanupFinalizer(ctx context.Context, return ctrl.Result{}, nil } -// recordedProvisionerKey returns the organization and key IDs that status -// records for the key this provisioner created. ok is false when status does -// not record valid IDs. -func recordedProvisionerKey(status coderv1alpha1.CoderProvisionerStatus) (organizationID, keyID uuid.UUID, ok bool) { - organizationID, err := uuid.Parse(status.OrganizationID) +// parseKeyIDs parses the organization and key IDs of a recorded provisioner +// key. ok is false when the record has no valid IDs. +func parseKeyIDs(rawOrganizationID, rawKeyID string) (organizationID, keyID uuid.UUID, ok bool) { + organizationID, err := uuid.Parse(rawOrganizationID) if err != nil || organizationID == uuid.Nil { return uuid.Nil, uuid.Nil, false } - keyID, err = uuid.Parse(status.ProvisionerKeyID) + keyID, err = uuid.Parse(rawKeyID) if err != nil || keyID == uuid.Nil { return uuid.Nil, uuid.Nil, false } @@ -849,6 +904,174 @@ func recordedProvisionerKey(status coderv1alpha1.CoderProvisionerStatus) (organi return organizationID, keyID, true } +// sameKeySlot reports whether two records name the same key name in the same +// organization at the same Coder URL. Coder allows one key per name in an +// organization, so both records refer to the same key. +func sameKeySlot(a, b coderv1alpha1.CoderProvisionerKeyRecord) bool { + return a.CoderURL == b.CoderURL && a.OrganizationName == b.OrganizationName && a.KeyName == b.KeyName +} + +// appliedKeyRecord returns a record of the key that the applied status fields +// describe, at the Coder API URL that status.controlPlaneURL maps to for its +// control plane (see coderAPIURL). When that control plane cannot be read, +// the record keeps status.controlPlaneURL. +func (r *CoderProvisionerReconciler) appliedKeyRecord( + ctx context.Context, + provisioner *coderv1alpha1.CoderProvisioner, + defaultOrganizationName, defaultKeyName string, +) (coderv1alpha1.CoderProvisionerKeyRecord, error) { + status := provisioner.Status + record := coderv1alpha1.CoderProvisionerKeyRecord{ + ControlPlaneRefName: cmp.Or(status.ControlPlaneRefName, provisioner.Spec.ControlPlaneRef.Name), + CoderURL: strings.TrimSpace(status.ControlPlaneURL), + OrganizationName: cmp.Or(status.OrganizationName, defaultOrganizationName), + KeyName: cmp.Or(status.ProvisionerKeyName, defaultKeyName), + OrganizationID: status.OrganizationID, + KeyID: status.ProvisionerKeyID, + } + controlPlane, ok, err := r.readControlPlane(ctx, provisioner.Namespace, record.ControlPlaneRefName) + if err != nil || !ok { + return record, err + } + applied := controlPlane.DeepCopy() + applied.Status.URL = cmp.Or(record.CoderURL, controlPlane.Status.URL) + apiURL, err := coderAPIURL(applied) + if err != nil { + ctrl.LoggerFrom(ctx).Info("unable to derive the Coder API URL of the applied provisioner key", "error", err) + return record, nil + } + record.CoderURL = apiURL + return record, nil +} + +// currentEndpointRecord returns applied at the current Coder API URL of the +// control plane that spec.controlPlaneRef names, with that control plane's +// operator token. It returns applied when that control plane cannot be read. +func (r *CoderProvisionerReconciler) currentEndpointRecord( + ctx context.Context, + provisioner *coderv1alpha1.CoderProvisioner, + applied coderv1alpha1.CoderProvisionerKeyRecord, +) (coderv1alpha1.CoderProvisionerKeyRecord, error) { + controlPlane, ok, err := r.readControlPlane(ctx, provisioner.Namespace, provisioner.Spec.ControlPlaneRef.Name) + if err != nil || !ok { + return applied, err + } + apiURL, urlErr := coderAPIURL(controlPlane) + if urlErr != nil { + ctrl.LoggerFrom(ctx).Info("unable to derive the current Coder API URL during deletion", "error", urlErr) + return applied, nil + } + current := applied + current.ControlPlaneRefName = controlPlane.Name + current.CoderURL = apiURL + return current, nil +} + +// readControlPlane reads a control plane in namespace. ok is false when it +// cannot be read. +func (r *CoderProvisionerReconciler) readControlPlane(ctx context.Context, namespace, name string) (*coderv1alpha1.CoderControlPlane, bool, error) { + controlPlane := &coderv1alpha1.CoderControlPlane{} + if err := r.Get(ctx, types.NamespacedName{Name: name, Namespace: namespace}, controlPlane); err != nil { + ctrl.LoggerFrom(ctx).Info("unable to read CoderControlPlane", "controlPlaneRef", name, "error", err) + return nil, false, nil + } + if controlPlane.Name != name || controlPlane.Namespace != namespace { + return nil, false, fmt.Errorf("assertion failed: fetched control plane %s/%s does not match expected %s/%s", + controlPlane.Namespace, controlPlane.Name, namespace, name) + } + return controlPlane, true, nil +} + +// keyCleanup is the result of the cleanup of one recorded provisioner key. +type keyCleanup int + +const ( + // keyKept: Coder did not confirm that the key is gone. The record stays, + // and the operator tries again. + keyKept keyCleanup = iota + // keyGone: Coder confirmed that the key is gone, or that no key other + // than the applied key holds the name of a record without key IDs. + keyGone + // keyUnverified: the record has no key IDs, and Coder holds a key with + // its name that nothing shows this provisioner created. The operator + // leaves that key, and the record stays. + keyUnverified +) + +// revokeKey deletes the key that record names, at record.CoderURL with the +// operator token of the control plane record.ControlPlaneRefName. A record +// without key IDs deletes no key. +func (r *CoderProvisionerReconciler) revokeKey( + ctx context.Context, + provisioner *coderv1alpha1.CoderProvisioner, + record coderv1alpha1.CoderProvisionerKeyRecord, +) (keyCleanup, error) { + log := ctrl.LoggerFrom(ctx).WithValues("coderURL", record.CoderURL, "controlPlaneRef", record.ControlPlaneRefName, + "keyName", record.KeyName, "keyID", record.KeyID) + controlPlane, ok, err := r.readControlPlane(ctx, provisioner.Namespace, record.ControlPlaneRefName) + if err != nil || !ok { + return keyKept, err + } + sessionToken, tokenResult, tokenReason, tokenMessage, tokenErr := r.readOperatorSessionToken(ctx, provisioner, controlPlane) + if tokenErr != nil || tokenResult.RequeueAfter > 0 { + log.Info("operator access is not available, keeping the provisioner key record", + "reason", tokenReason, "message", tokenMessage, "error", tokenErr) + return keyKept, nil + } + + if organizationID, keyID, ok := parseKeyIDs(record.OrganizationID, record.KeyID); ok { + if err := r.BootstrapClient.DeleteProvisionerKeyByID(ctx, coderbootstrap.DeleteProvisionerKeyByIDRequest{ + CoderURL: record.CoderURL, + SessionToken: sessionToken, + OrganizationID: organizationID, + KeyID: keyID, + }); err != nil { + log.Info("failed to delete the recorded provisioner key, keeping its record", "error", err) + return keyKept, nil + } + return keyGone, nil + } + + key, err := r.BootstrapClient.ProvisionerKeyByName(ctx, coderbootstrap.ProvisionerKeyByNameRequest{ + CoderURL: record.CoderURL, + SessionToken: sessionToken, + OrganizationName: record.OrganizationName, + KeyName: record.KeyName, + }) + if err != nil { + log.Info("failed to look up the recorded provisioner key, keeping its record", "error", err) + return keyKept, nil + } + // Coder allows one key per name in an organization, so no other key of + // this record exists when no key or the applied key holds the name. The + // applied key holds it when record.CoderURL reaches the same Coder. + _, appliedKeyID, _ := parseKeyIDs(provisioner.Status.OrganizationID, provisioner.Status.ProvisionerKeyID) + if key == nil || (appliedKeyID != uuid.Nil && key.ID == appliedKeyID) { + return keyGone, nil + } + // Without the key ID that the create returned, nothing ties this key to + // this provisioner: another provisioner or a person can use the name. + log.Info("leaving a provisioner key that this provisioner cannot show it created", "foundKeyID", key.ID) + return keyUnverified, nil +} + +// setUnverifiedKeyCondition sets the UnverifiedKeyKept condition for the +// pending entries without key IDs that unverified describes, or removes it +// when there are none. It reports whether the conditions changed. +func setUnverifiedKeyCondition(provisioner *coderv1alpha1.CoderProvisioner, unverified []string) bool { + if len(unverified) == 0 { + return meta.RemoveStatusCondition(&provisioner.Status.Conditions, coderv1alpha1.CoderProvisionerConditionUnverifiedKeyKept) + } + return meta.SetStatusCondition(&provisioner.Status.Conditions, metav1.Condition{ + Type: coderv1alpha1.CoderProvisionerConditionUnverifiedKeyKept, + Status: metav1.ConditionTrue, + ObservedGeneration: provisioner.Generation, + Reason: "KeyIDUnknown", + Message: "status.pendingKeys holds entries without key IDs, and the operator deletes no key for them, because another provisioner or a person can use the same name: " + + strings.Join(unverified, "; ") + ". Delete such a key in Coder if nothing uses it.", + }) +} + 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 9387e6164..cc5e1dc9e 100644 --- a/internal/controller/coderprovisioner_controller_test.go +++ b/internal/controller/coderprovisioner_controller_test.go @@ -876,10 +876,11 @@ func TestCoderProvisionerReconciler_KeyNameDrift(t *testing.T) { _ = k8sClient.Delete(context.Background(), provisioner) }) + keyV1 := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: uuid.New(), KeyID: uuid.New(), KeyName: "key-v1", Key: "key-v1-material"} bootstrapClient := &fakeBootstrapClient{ provisionerKeyResponses: []coderbootstrap.EnsureProvisionerKeyResponse{ - {KeyName: "key-v1", Key: "key-v1-material"}, - {KeyName: "key-v2", Key: "key-v2-material"}, + keyV1, + {OrganizationID: keyV1.OrganizationID, KeyID: uuid.New(), KeyName: "key-v2", Key: "key-v2-material"}, }, } reconciler := &controller.CoderProvisionerReconciler{Client: k8sClient, Scheme: scheme, BootstrapClient: bootstrapClient} @@ -895,10 +896,8 @@ func TestCoderProvisionerReconciler_KeyNameDrift(t *testing.T) { reconcileProvisioner(ctx, t, reconciler, request) - require.GreaterOrEqual(t, bootstrapClient.deleteKeyCalls, 1) - require.NotEmpty(t, bootstrapClient.deleteKeyRequests) - lastDelete := bootstrapClient.deleteKeyRequests[len(bootstrapClient.deleteKeyRequests)-1] - require.Equal(t, "key-v1", lastDelete.KeyName) + require.Len(t, bootstrapClient.deleteKeyByIDRequests, 1) + require.Equal(t, keyV1.KeyID, bootstrapClient.deleteKeyByIDRequests[0].KeyID, "the key named key-v1 must be deleted") require.GreaterOrEqual(t, len(bootstrapClient.provisionerKeyRequests), 2) lastEnsure := bootstrapClient.provisionerKeyRequests[len(bootstrapClient.provisionerKeyRequests)-1] diff --git a/internal/controller/coderprovisioner_deletion_test.go b/internal/controller/coderprovisioner_deletion_test.go index 510ae65c9..473ded997 100644 --- a/internal/controller/coderprovisioner_deletion_test.go +++ b/internal/controller/coderprovisioner_deletion_test.go @@ -3,6 +3,7 @@ package controller_test import ( "context" "errors" + "slices" "testing" "time" @@ -143,10 +144,10 @@ func TestCoderProvisionerKey_RecordedBeforePublished(t *testing.T) { base, err := ctrlclient.NewWithWatch(cfg, ctrlclient.Options{Scheme: scheme}) require.NoError(t, err) - var statusAtSecretWrite string + var recordedAtSecretWrite bool ordered := interceptor.NewClient(base, interceptor.Funcs{ SubResourceUpdate: func(ctx context.Context, c ctrlclient.Client, sub string, obj ctrlclient.Object, opts ...ctrlclient.SubResourceUpdateOption) error { - if p, ok := obj.(*coderv1alpha1.CoderProvisioner); ok && failStatus && p.Status.ProvisionerKeyID == created.KeyID.String() { + if p, ok := obj.(*coderv1alpha1.CoderProvisioner); ok && failStatus && pendingKeyRecorded(p, created.KeyID) { return apierrors.NewConflict(coderv1alpha1.GroupVersion.WithResource("coderprovisioners").GroupResource(), p.Name, errors.New("injected")) } return c.SubResource(sub).Update(ctx, obj, opts...) @@ -155,7 +156,7 @@ func TestCoderProvisionerKey_RecordedBeforePublished(t *testing.T) { if s, ok := obj.(*corev1.Secret); ok && s.Name == keySecretName.Name { latest := &coderv1alpha1.CoderProvisioner{} require.NoError(t, c.Get(ctx, types.NamespacedName{Name: provisioner.Name, Namespace: provisioner.Namespace}, latest)) - statusAtSecretWrite = latest.Status.ProvisionerKeyID + recordedAtSecretWrite = pendingKeyRecorded(latest, created.KeyID) } return c.Create(ctx, obj, opts...) }, @@ -169,7 +170,7 @@ func TestCoderProvisionerKey_RecordedBeforePublished(t *testing.T) { if !failStatus { require.NoError(t, err) - require.Equal(t, created.KeyID.String(), statusAtSecretWrite, "status must record the key before the Secret holds it") + require.True(t, recordedAtSecretWrite, "status must record the key before the Secret holds it") require.Empty(t, f.bootstrapClient.deleteKeyByIDRequests) return } @@ -224,7 +225,7 @@ func TestCoderProvisionerKey_FailedSecretUpdateRotatesAgain(t *testing.T) { _, err = f.reconciler.Reconcile(ctx, request) require.Error(t, err, "the injected Secret update failure must fail the reconcile") afterFailure := f.get(ctx, t, provisioner) - require.Equal(t, f.bootstrapClient.provisionerKeyResponses[1].KeyID.String(), afterFailure.Status.ProvisionerKeyID, + require.True(t, pendingKeyRecorded(afterFailure, f.bootstrapClient.provisionerKeyResponses[1].KeyID), "status must record the new key before the Secret write") _, err = f.reconciler.Reconcile(ctx, request) @@ -234,43 +235,48 @@ func TestCoderProvisionerKey_FailedSecretUpdateRotatesAgain(t *testing.T) { "the retry must rotate again and publish a live key") final := f.get(ctx, t, provisioner) require.Equal(t, f.bootstrapClient.provisionerKeyResponses[2].KeyID.String(), final.Status.ProvisionerKeyID) + require.Empty(t, final.Status.PendingKeys, "the published key moves to the applied status fields") } -// TestCoderProvisionerDeletion_TriesCurrentControlPlaneURL checks the window -// of a control plane URL change: status holds the new key's IDs but still the -// old status.controlPlaneURL. Deletion must also try the current URL. +// pendingKeyRecorded reports whether status.pendingKeys records the key ID. +func pendingKeyRecorded(provisioner *coderv1alpha1.CoderProvisioner, keyID uuid.UUID) bool { + return slices.ContainsFunc(provisioner.Status.PendingKeys, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { + return record.KeyID == keyID.String() + }) +} + +// TestCoderProvisionerDeletion_TriesCurrentControlPlaneURL checks status that +// an older operator wrote during a control plane URL change: the new key's +// IDs under the old status.controlPlaneURL. Deletion must also try the +// current URL. func TestCoderProvisionerDeletion_TriesCurrentControlPlaneURL(t *testing.T) { t.Parallel() ctx := context.Background() f := newProvisionerCredentialFixture(ctx, t, "cprov-delete-urldrift") + const oldURL, newURL = "http://coder-old.example.com", "http://coder-new.example.com" + setControlPlaneURL(ctx, t, f, oldURL) provisioner := f.createProvisioner(ctx, t, nil, nil) f.reconcile(ctx, t, provisioner) f.reconcile(ctx, t, provisioner) newKey := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: uuid.New(), KeyID: uuid.New()} latest := f.get(ctx, t, provisioner) - require.Equal(t, testCoderOrigin, latest.Status.ControlPlaneURL) + require.Equal(t, oldURL, latest.Status.ControlPlaneURL) latest.Status.OrganizationID = newKey.OrganizationID.String() latest.Status.ProvisionerKeyID = newKey.KeyID.String() require.NoError(t, k8sClient.Status().Update(ctx, latest)) - - const newURL = "https://coder-new.example.com" - controlPlane := &coderv1alpha1.CoderControlPlane{} - require.NoError(t, k8sClient.Get(ctx, types.NamespacedName{Name: f.controlPlane.Name, Namespace: f.namespace}, controlPlane)) - controlPlane.Status.URL = newURL - require.NoError(t, k8sClient.Status().Update(ctx, controlPlane)) + setControlPlaneURL(ctx, t, f, newURL) require.NoError(t, k8sClient.Delete(ctx, f.get(ctx, t, provisioner))) f.reconcile(ctx, t, provisioner) require.Len(t, f.bootstrapClient.deleteKeyByIDRequests, 2) - // The recorded URL names an earlier endpoint and is used as recorded. The - // current endpoint is HTTPS, so the call uses the internal HTTP URL. - require.Equal(t, testCoderOrigin, f.bootstrapClient.deleteKeyByIDRequests[0].CoderURL) - require.Equal(t, internalCoderURL(f), f.bootstrapClient.deleteKeyByIDRequests[1].CoderURL) + require.Equal(t, oldURL, f.bootstrapClient.deleteKeyByIDRequests[0].CoderURL) + require.Equal(t, newURL, f.bootstrapClient.deleteKeyByIDRequests[1].CoderURL) for _, req := range f.bootstrapClient.deleteKeyByIDRequests { require.Equal(t, newKey.OrganizationID, req.OrganizationID) require.Equal(t, newKey.KeyID, req.KeyID) + require.Equal(t, testOperatorToken, req.SessionToken) } require.Zero(t, f.bootstrapClient.deleteKeyCalls, "no name-based key deletion") requireProvisionerGone(ctx, t, types.NamespacedName{Name: provisioner.Name, Namespace: provisioner.Namespace}) diff --git a/internal/controller/coderprovisioner_pendingkeys_test.go b/internal/controller/coderprovisioner_pendingkeys_test.go new file mode 100644 index 000000000..eafd13791 --- /dev/null +++ b/internal/controller/coderprovisioner_pendingkeys_test.go @@ -0,0 +1,617 @@ +package controller_test + +import ( + "cmp" + "context" + "errors" + "maps" + "slices" + "testing" + + "github.com/coder/coder/v2/codersdk" + "github.com/google/uuid" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" + ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" + "github.com/coder/coder-k8s/internal/coderbootstrap" +) + +// keyRecordingClient checks that status records every key create before the +// create reaches Coder, and can fail the revoke of one key. +type keyRecordingClient struct { + *fakeBootstrapClient + t *testing.T + provisioner types.NamespacedName + // onCreated runs after Coder returned a new key. + onCreated func(coderbootstrap.EnsureProvisionerKeyResponse) + // failRevoke makes the delete of this key ID fail. + failRevoke uuid.UUID +} + +func (c *keyRecordingClient) EnsureProvisionerKey(ctx context.Context, req coderbootstrap.EnsureProvisionerKeyRequest) (coderbootstrap.EnsureProvisionerKeyResponse, error) { + latest := &coderv1alpha1.CoderProvisioner{} + require.NoError(c.t, k8sClient.Get(ctx, c.provisioner, latest)) + recorded := slices.ContainsFunc(latest.Status.PendingKeys, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { + return record.CoderURL == req.CoderURL && record.OrganizationName == req.OrganizationName && record.KeyName == req.KeyName + }) + if !recorded { + c.t.Errorf("key %q at %s can be created without a record in status.pendingKeys %+v", req.KeyName, req.CoderURL, latest.Status.PendingKeys) + } + response, err := c.fakeBootstrapClient.EnsureProvisionerKey(ctx, req) + if err == nil && response.Key != "" && c.onCreated != nil { + c.onCreated(response) + } + return response, err +} + +func (c *keyRecordingClient) DeleteProvisionerKeyByID(ctx context.Context, req coderbootstrap.DeleteProvisionerKeyByIDRequest) error { + if req.KeyID == c.failRevoke { + return errors.New("injected revoke failure") + } + return c.fakeBootstrapClient.DeleteProvisionerKeyByID(ctx, req) +} + +// keyDelete is one key delete request that reached the fake Coder. +type keyDelete struct { + coderURL, token, keyName, keyID string +} + +func keyDeletes(client *fakeBootstrapClient) []keyDelete { + var deletes []keyDelete + for _, req := range client.deleteKeyRequests { + deletes = append(deletes, keyDelete{coderURL: req.CoderURL, token: req.SessionToken, keyName: req.KeyName}) + } + for _, req := range client.deleteKeyByIDRequests { + deletes = append(deletes, keyDelete{coderURL: req.CoderURL, token: req.SessionToken, keyID: req.KeyID.String()}) + } + return deletes +} + +func createControlPlaneWithToken(ctx context.Context, t *testing.T, namespace, name, url, token string) { + t.Helper() + createTestControlPlane(ctx, t, namespace, name, url) + secret := &corev1.Secret{} + require.NoError(t, k8sClient.Get(ctx, types.NamespacedName{Name: name + "-operator-token", Namespace: namespace}, secret)) + secret.Data[coderv1alpha1.DefaultTokenSecretKey] = []byte(token) + require.NoError(t, k8sClient.Update(ctx, secret)) +} + +// TestCoderProvisionerKey_IntermediateEndpointRevoked checks a control plane +// target that changes twice, A to B to C, while the attempt at B fails after +// Coder created the key at B. When status recorded the ID of key B, both the +// next rotation and deletion must delete key B at B, with the operator token +// of control plane B. When it did not, nothing shows that this provisioner +// created the key at B, so key B must stay and the provisioner must report +// it. No delete may send a token to the endpoint of another control plane. +func TestCoderProvisionerKey_IntermediateEndpointRevoked(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + // statusWriteFails makes the status write that records key B fail, + // and the revoke of the unpublished key B fail too, as after a crash + // right after the create, so status never holds the ID of key B. + // Otherwise the Secret write fails. + statusWriteFails bool + // deleteAtC deletes the provisioner instead of reconciling at C. + deleteAtC bool + // revokeFailsOnce fails the first delete of key B at C. The record + // must stay until a later reconcile deletes the key. + revokeFailsOnce bool + }{ + {name: "secret write fails at B, rotation at C"}, + {name: "secret write fails at B, deletion at C", deleteAtC: true}, + {name: "key B unrecorded by ID, rotation at C", statusWriteFails: true}, + {name: "key B unrecorded by ID, deletion at C", statusWriteFails: true, deleteAtC: true}, + {name: "revoke of key B fails once, rotation at C", revokeFailsOnce: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + f := newProvisionerCredentialFixture(ctx, t, "cprov-intermediate") + createControlPlaneWithToken(ctx, t, f.namespace, "coder-b", "https://coder-b.example.com", "operator-token-b") + createControlPlaneWithToken(ctx, t, f.namespace, "coder-c", "https://coder-c.example.com", "operator-token-c") + apiURL := func(controlPlane string) string { + return "http://" + controlPlane + "." + f.namespace + ".svc.cluster.local:80" + } + tokenAt := map[string]string{ + apiURL("coder"): testOperatorToken, + apiURL("coder-b"): "operator-token-b", + apiURL("coder-c"): "operator-token-c", + } + keyA := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: uuid.New(), KeyID: uuid.New(), KeyName: "provisioner", Key: "material-a"} + keyB := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: uuid.New(), KeyID: uuid.New(), KeyName: "provisioner", Key: "material-b"} + keyC := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: uuid.New(), KeyID: uuid.New(), KeyName: "provisioner", Key: "material-c"} + f.bootstrapClient.provisionerKeyResponses = []coderbootstrap.EnsureProvisionerKeyResponse{keyA, keyB, keyC} + if tt.statusWriteFails { + f.bootstrapClient.provisionerKeyByName = func(req coderbootstrap.ProvisionerKeyByNameRequest) *codersdk.ProvisionerKey { + if req.CoderURL != apiURL("coder-b") { + return nil + } + return &codersdk.ProvisionerKey{ID: keyB.KeyID, OrganizationID: keyB.OrganizationID, Name: keyB.KeyName} + } + } + + provisioner := f.createProvisioner(ctx, t, nil, nil) + name := types.NamespacedName{Name: provisioner.Name, Namespace: provisioner.Namespace} + keySecretName := provisioner.Name + "-provisioner-key" + client := &keyRecordingClient{fakeBootstrapClient: f.bootstrapClient, t: t, provisioner: name} + f.reconciler.BootstrapClient = client + var failSecretWrite, failStatusWrite bool + base, err := ctrlclient.NewWithWatch(cfg, ctrlclient.Options{Scheme: scheme}) + require.NoError(t, err) + f.reconciler.Client = interceptor.NewClient(base, interceptor.Funcs{ + SubResourceUpdate: func(ctx context.Context, c ctrlclient.Client, sub string, obj ctrlclient.Object, opts ...ctrlclient.SubResourceUpdateOption) error { + if _, ok := obj.(*coderv1alpha1.CoderProvisioner); ok && failStatusWrite { + failStatusWrite = false + return apierrors.NewServiceUnavailable("injected status write failure") + } + return c.SubResource(sub).Update(ctx, obj, opts...) + }, + Update: func(ctx context.Context, c ctrlclient.WithWatch, obj ctrlclient.Object, opts ...ctrlclient.UpdateOption) error { + if s, ok := obj.(*corev1.Secret); ok && s.Name == keySecretName && failSecretWrite { + failSecretWrite = false + return apierrors.NewServiceUnavailable("injected Secret write failure") + } + return c.Update(ctx, obj, opts...) + }, + }) + moveTo := func(controlPlane string) { + latest := f.get(ctx, t, provisioner) + latest.Spec.ControlPlaneRef.Name = controlPlane + require.NoError(t, k8sClient.Update(ctx, latest)) + } + + f.reconcile(ctx, t, provisioner) + f.reconcile(ctx, t, provisioner) + require.Equal(t, keyA.KeyID.String(), f.get(ctx, t, provisioner).Status.ProvisionerKeyID) + + moveTo("coder-b") + if tt.statusWriteFails { + client.failRevoke = keyB.KeyID + client.onCreated = func(created coderbootstrap.EnsureProvisionerKeyResponse) { + failStatusWrite = created.KeyID == keyB.KeyID + } + } else { + failSecretWrite = true + } + _, err = f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: name}) + require.Error(t, err, "the injected failure must fail the attempt at B") + require.Equal(t, 2, f.bootstrapClient.provisionerKeyCalls, "Coder must have created key B") + client.failRevoke = uuid.Nil + client.onCreated = nil + + moveTo("coder-c") + if tt.deleteAtC { + require.NoError(t, k8sClient.Delete(ctx, f.get(ctx, t, provisioner))) + } + if tt.revokeFailsOnce { + client.failRevoke = keyB.KeyID + result, err := f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: name}) + require.NoError(t, err) + require.Positive(t, result.RequeueAfter, "a failed delete must schedule another attempt") + require.True(t, pendingKeyRecorded(f.get(ctx, t, provisioner), keyB.KeyID), "a failed delete must keep the record of key B") + client.failRevoke = uuid.Nil + } + _, err = f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: name}) + require.NoError(t, err) + + deletes := keyDeletes(f.bootstrapClient) + if tt.statusWriteFails { + require.False(t, slices.ContainsFunc(deletes, func(d keyDelete) bool { return d.coderURL == apiURL("coder-b") }), + "a key whose ID status never held must not be deleted, got %+v", deletes) + } else { + require.Contains(t, deletes, keyDelete{coderURL: apiURL("coder-b"), token: "operator-token-b", keyID: keyB.KeyID.String()}, + "key B must be deleted at B with the operator token of B") + } + require.Contains(t, deletes, keyDelete{coderURL: apiURL("coder"), token: testOperatorToken, keyID: keyA.KeyID.String()}, + "key A must be deleted at A with the operator token of A") + for _, d := range deletes { + want, known := tokenAt[d.coderURL] + require.True(t, known, "delete sent to %s, which no record names", d.coderURL) + require.Equal(t, want, d.token, "delete at %s must use the operator token of that control plane", d.coderURL) + } + + if tt.deleteAtC { + requireProvisionerGone(ctx, t, name) + return + } + require.False(t, slices.ContainsFunc(deletes, func(d keyDelete) bool { return d.coderURL == apiURL("coder-c") }), + "nothing may delete key C, which the Secret holds") + final := f.get(ctx, t, provisioner) + require.Equal(t, keyC.KeyID.String(), final.Status.ProvisionerKeyID) + require.Equal(t, "https://coder-c.example.com", final.Status.ControlPlaneURL) + unverified := meta.FindStatusCondition(final.Status.Conditions, coderv1alpha1.CoderProvisionerConditionUnverifiedKeyKept) + if tt.statusWriteFails { + require.Len(t, final.Status.PendingKeys, 1) + require.Equal(t, apiURL("coder-b"), final.Status.PendingKeys[0].CoderURL, "the record of key B must stay") + require.Empty(t, final.Status.PendingKeys[0].KeyID) + require.NotNil(t, unverified, "the provisioner must report the key that it left at B") + require.Equal(t, metav1.ConditionTrue, unverified.Status) + return + } + require.Empty(t, final.Status.PendingKeys, "every replaced key must be confirmed deleted") + require.Nil(t, unverified) + }) + } +} + +// TestCoderProvisionerKey_UnpublishedKeyIsRotated checks keys that an +// earlier reconcile created at the target and did not publish. The next +// reconcile must replace them with a key that status records as applied, also +// when nothing else asks for a rotation, and deletion must delete that key. +func TestCoderProvisionerKey_UnpublishedKeyIsRotated(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + // fail makes one reconcile leave an unpublished key at the target. + fail func(ctx context.Context, t *testing.T, f provisionerCredentialFixture, provisioner *coderv1alpha1.CoderProvisioner, unpublished coderbootstrap.EnsureProvisionerKeyResponse) + }{ + { + // The create reached Coder, but the operator got no answer. The + // spec change that caused the rotation is then reverted. + name: "create answer lost, spec reverted", + fail: func(ctx context.Context, t *testing.T, f provisionerCredentialFixture, provisioner *coderv1alpha1.CoderProvisioner, _ coderbootstrap.EnsureProvisionerKeyResponse) { + latest := f.get(ctx, t, provisioner) + latest.Spec.Tags = map[string]string{"region": "rotated"} + require.NoError(t, k8sClient.Update(ctx, latest)) + f.bootstrapClient.provisionerKeyErr = errors.New("injected lost create answer") + _, err := f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{Name: provisioner.Name, Namespace: provisioner.Namespace}}) + require.Error(t, err) + f.bootstrapClient.provisionerKeyErr = nil + latest = f.get(ctx, t, provisioner) + latest.Spec.Tags = nil + require.NoError(t, k8sClient.Update(ctx, latest)) + }, + }, + { + // The Secret holds the new key, but the status write after it fails. + name: "status write after the Secret write fails", + fail: func(ctx context.Context, t *testing.T, f provisionerCredentialFixture, provisioner *coderv1alpha1.CoderProvisioner, unpublished coderbootstrap.EnsureProvisionerKeyResponse) { + require.NoError(t, k8sClient.Delete(ctx, &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: provisioner.Name + "-provisioner-key", Namespace: provisioner.Namespace}})) + base, err := ctrlclient.NewWithWatch(cfg, ctrlclient.Options{Scheme: scheme}) + require.NoError(t, err) + failStatusWrite := true + f.reconciler.Client = interceptor.NewClient(base, interceptor.Funcs{ + SubResourceUpdate: func(ctx context.Context, c ctrlclient.Client, sub string, obj ctrlclient.Object, opts ...ctrlclient.SubResourceUpdateOption) error { + if p, ok := obj.(*coderv1alpha1.CoderProvisioner); ok && failStatusWrite && p.Status.ProvisionerKeyID == unpublished.KeyID.String() { + failStatusWrite = false + return apierrors.NewServiceUnavailable("injected status write failure") + } + return c.SubResource(sub).Update(ctx, obj, opts...) + }, + }) + _, err = f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{Name: provisioner.Name, Namespace: provisioner.Namespace}}) + require.Error(t, err) + f.reconciler.Client = k8sClient + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + f := newProvisionerCredentialFixture(ctx, t, "cprov-unpublished") + first := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: uuid.New(), KeyID: uuid.New(), KeyName: "provisioner", Key: "material-1"} + unpublished := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: first.OrganizationID, KeyID: uuid.New(), KeyName: "provisioner", Key: "material-2"} + // Coder lists the unpublished key, without plaintext, until the + // rotation deletes it by name. + listed := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: unpublished.OrganizationID, KeyID: unpublished.KeyID, KeyName: "provisioner"} + final := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: first.OrganizationID, KeyID: uuid.New(), KeyName: "provisioner", Key: "material-3"} + f.bootstrapClient.provisionerKeyResponses = []coderbootstrap.EnsureProvisionerKeyResponse{first, unpublished, listed, final} + provisioner := f.createProvisioner(ctx, t, nil, nil) + f.reconcile(ctx, t, provisioner) + f.reconcile(ctx, t, provisioner) + + tt.fail(ctx, t, f, provisioner, unpublished) + deletesBefore := len(f.bootstrapClient.deleteKeyRequests) + f.reconcile(ctx, t, provisioner) + + latest := f.get(ctx, t, provisioner) + require.Equal(t, final.KeyID.String(), latest.Status.ProvisionerKeyID, "status must name the key in the Secret") + require.Empty(t, latest.Status.PendingKeys) + secret := &corev1.Secret{} + require.NoError(t, k8sClient.Get(ctx, types.NamespacedName{Name: provisioner.Name + "-provisioner-key", Namespace: provisioner.Namespace}, secret)) + require.Equal(t, "material-3", string(secret.Data[coderv1alpha1.DefaultProvisionerKeySecretKey])) + require.Contains(t, f.bootstrapClient.deleteKeyRequests[deletesBefore:], + deleteKeyRequest{CoderURL: internalCoderURL(f), SessionToken: testOperatorToken, OrganizationName: "default", KeyName: "provisioner"}, + "the rotation must delete the unpublished key") + + require.NoError(t, k8sClient.Delete(ctx, latest)) + f.reconcile(ctx, t, provisioner) + require.True(t, slices.ContainsFunc(f.bootstrapClient.deleteKeyByIDRequests, func(req coderbootstrap.DeleteProvisionerKeyByIDRequest) bool { + return req.KeyID == final.KeyID + }), "deletion must delete the key in the Secret, got %+v", f.bootstrapClient.deleteKeyByIDRequests) + requireProvisionerGone(ctx, t, types.NamespacedName{Name: provisioner.Name, Namespace: provisioner.Namespace}) + }) + } +} + +// TestCoderProvisionerKey_FailedRotationKeepsKeyIDs checks that a rotation +// that fails before its create keeps the IDs of the unpublished key at the +// target, so that a later delete still finds that key by ID. +func TestCoderProvisionerKey_FailedRotationKeepsKeyIDs(t *testing.T) { + t.Parallel() + ctx := context.Background() + f := newProvisionerCredentialFixture(ctx, t, "cprov-keep-ids") + first := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: uuid.New(), KeyID: uuid.New(), KeyName: "provisioner", Key: "material-1"} + unpublished := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: first.OrganizationID, KeyID: uuid.New(), KeyName: "provisioner", Key: "material-2"} + f.bootstrapClient.provisionerKeyResponses = []coderbootstrap.EnsureProvisionerKeyResponse{first, unpublished} + provisioner := f.createProvisioner(ctx, t, nil, nil) + name := types.NamespacedName{Name: provisioner.Name, Namespace: provisioner.Namespace} + f.reconcile(ctx, t, provisioner) + f.reconcile(ctx, t, provisioner) + + keySecretName := provisioner.Name + "-provisioner-key" + require.NoError(t, k8sClient.Delete(ctx, &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: keySecretName, Namespace: provisioner.Namespace}})) + base, err := ctrlclient.NewWithWatch(cfg, ctrlclient.Options{Scheme: scheme}) + require.NoError(t, err) + f.reconciler.Client = interceptor.NewClient(base, interceptor.Funcs{ + Create: func(ctx context.Context, c ctrlclient.WithWatch, obj ctrlclient.Object, opts ...ctrlclient.CreateOption) error { + if s, ok := obj.(*corev1.Secret); ok && s.Name == keySecretName { + return apierrors.NewServiceUnavailable("injected Secret write failure") + } + return c.Create(ctx, obj, opts...) + }, + }) + _, err = f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: name}) + require.Error(t, err, "the injected Secret write failure must fail the rotation") + require.True(t, pendingKeyRecorded(f.get(ctx, t, provisioner), unpublished.KeyID)) + + f.bootstrapClient.provisionerKeyErr = errors.New("injected Coder failure") + _, err = f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: name}) + require.Error(t, err) + require.True(t, pendingKeyRecorded(f.get(ctx, t, provisioner), unpublished.KeyID), + "a failed rotation must keep the IDs of the unpublished key") +} + +// setControlPlaneURL sets status.url of the fixture's control plane. +func setControlPlaneURL(ctx context.Context, t *testing.T, f provisionerCredentialFixture, url string) { + t.Helper() + controlPlane := &coderv1alpha1.CoderControlPlane{} + require.NoError(t, k8sClient.Get(ctx, types.NamespacedName{Name: f.controlPlane.Name, Namespace: f.namespace}, controlPlane)) + controlPlane.Status.URL = url + require.NoError(t, k8sClient.Status().Update(ctx, controlPlane)) +} + +// TestCoderProvisionerKey_RevertAfterFailedRotation checks a rotation from +// endpoint A to B that fails after its create, followed by a revert to A. +// The Secret then holds key B, which is valid only at B, or key A, which the +// rotation deleted. The next reconcile must delete key B and write a new key +// for A. +func TestCoderProvisionerKey_RevertAfterFailedRotation(t *testing.T) { + t.Parallel() + + for _, secretWritten := range []bool{true, false} { + name := "status write after the Secret write fails" + if !secretWritten { + name = "Secret write fails" + } + t.Run(name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + f := newProvisionerCredentialFixture(ctx, t, "cprov-revert") + first := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: uuid.New(), KeyID: uuid.New(), KeyName: "provisioner", Key: "material-1"} + atB := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: uuid.New(), KeyID: uuid.New(), KeyName: "provisioner", Key: "material-b"} + final := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: first.OrganizationID, KeyID: uuid.New(), KeyName: "provisioner", Key: "material-3"} + f.bootstrapClient.provisionerKeyResponses = []coderbootstrap.EnsureProvisionerKeyResponse{first, atB, final} + provisioner := f.createProvisioner(ctx, t, nil, nil) + name := types.NamespacedName{Name: provisioner.Name, Namespace: provisioner.Namespace} + keySecret := types.NamespacedName{Name: provisioner.Name + "-provisioner-key", Namespace: provisioner.Namespace} + f.reconcile(ctx, t, provisioner) + f.reconcile(ctx, t, provisioner) + + const urlB = "http://coder-b.example.com" + setControlPlaneURL(ctx, t, f, urlB) + base, err := ctrlclient.NewWithWatch(cfg, ctrlclient.Options{Scheme: scheme}) + require.NoError(t, err) + f.reconciler.Client = interceptor.NewClient(base, interceptor.Funcs{ + SubResourceUpdate: func(ctx context.Context, c ctrlclient.Client, sub string, obj ctrlclient.Object, opts ...ctrlclient.SubResourceUpdateOption) error { + if p, ok := obj.(*coderv1alpha1.CoderProvisioner); ok && secretWritten && p.Status.ProvisionerKeyID == atB.KeyID.String() { + return apierrors.NewServiceUnavailable("injected status write failure") + } + return c.SubResource(sub).Update(ctx, obj, opts...) + }, + Update: func(ctx context.Context, c ctrlclient.WithWatch, obj ctrlclient.Object, opts ...ctrlclient.UpdateOption) error { + if s, ok := obj.(*corev1.Secret); ok && !secretWritten && s.Name == keySecret.Name { + return apierrors.NewServiceUnavailable("injected Secret write failure") + } + return c.Update(ctx, obj, opts...) + }, + }) + _, err = f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: name}) + require.Error(t, err, "the injected failure must fail the rotation to B") + + f.reconciler.Client = k8sClient + setControlPlaneURL(ctx, t, f, testCoderOrigin) + f.reconcile(ctx, t, provisioner) + + require.Contains(t, keyDeletes(f.bootstrapClient), keyDelete{coderURL: urlB, token: testOperatorToken, keyID: atB.KeyID.String()}, + "key B must be deleted at B") + secret := &corev1.Secret{} + require.NoError(t, k8sClient.Get(ctx, keySecret, secret)) + require.Equal(t, "material-3", string(secret.Data[coderv1alpha1.DefaultProvisionerKeySecretKey]), "the Secret must hold a new key for A") + latest := f.get(ctx, t, provisioner) + require.Equal(t, final.KeyID.String(), latest.Status.ProvisionerKeyID) + require.Equal(t, testCoderOrigin, latest.Status.ControlPlaneURL) + require.Empty(t, latest.Status.PendingKeys) + }) + } +} + +// keySlot is the place of a provisioner key in Coder: Coder allows one key +// per name in an organization. +type keySlot struct{ coderURL, organizationName, keyName string } + +// keyStoreClient is a Coder that keeps provisioner keys, one per slot. Each +// key create at a URL in failEnsureAt fails before it reaches Coder, and each +// key lookup by name at failLookupAt fails. +type keyStoreClient struct { + *fakeBootstrapClient + keys map[keySlot]uuid.UUID + failEnsureAt []string + failLookupAt string +} + +func newKeySlot(coderURL, organizationName, keyName string) keySlot { + return keySlot{coderURL: coderURL, organizationName: cmp.Or(organizationName, "default"), keyName: keyName} +} + +func (s keySlot) organizationID() uuid.UUID { + return uuid.NewSHA1(uuid.NameSpaceURL, []byte(s.coderURL+"/"+s.organizationName)) +} + +func (c *keyStoreClient) EnsureProvisionerKey(_ context.Context, req coderbootstrap.EnsureProvisionerKeyRequest) (coderbootstrap.EnsureProvisionerKeyResponse, error) { + if slices.Contains(c.failEnsureAt, req.CoderURL) { + return coderbootstrap.EnsureProvisionerKeyResponse{}, errors.New("injected connection refused") + } + slot := newKeySlot(req.CoderURL, req.OrganizationName, req.KeyName) + response := coderbootstrap.EnsureProvisionerKeyResponse{OrganizationID: slot.organizationID(), KeyName: req.KeyName} + if keyID, ok := c.keys[slot]; ok { + response.KeyID = keyID + return response, nil + } + response.KeyID = uuid.New() + response.Key = "material-" + response.KeyID.String() + c.keys[slot] = response.KeyID + return response, nil +} + +func (c *keyStoreClient) DeleteProvisionerKey(_ context.Context, coderURL, _, organizationName, keyName string) error { + delete(c.keys, newKeySlot(coderURL, organizationName, keyName)) + return nil +} + +func (c *keyStoreClient) DeleteProvisionerKeyByID(_ context.Context, req coderbootstrap.DeleteProvisionerKeyByIDRequest) error { + maps.DeleteFunc(c.keys, func(slot keySlot, keyID uuid.UUID) bool { + return slot.coderURL == req.CoderURL && slot.organizationID() == req.OrganizationID && keyID == req.KeyID + }) + return nil +} + +func (c *keyStoreClient) ProvisionerKeyByName(_ context.Context, req coderbootstrap.ProvisionerKeyByNameRequest) (*codersdk.ProvisionerKey, error) { + if req.CoderURL == c.failLookupAt { + return nil, errors.New("injected lookup failure") + } + slot := newKeySlot(req.CoderURL, req.OrganizationName, req.KeyName) + keyID, ok := c.keys[slot] + if !ok { + return nil, nil + } + return &codersdk.ProvisionerKey{ID: keyID, OrganizationID: slot.organizationID(), Name: slot.keyName}, nil +} + +// 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. +func TestCoderProvisionerKey_IDLessRecordKeepsOtherProvisionersKey(t *testing.T) { + t.Parallel() + + for _, deleteP := range []bool{false, true} { + name := "P moves back to A" + if deleteP { + name = "P is deleted" + } + t.Run(name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + f := newProvisionerCredentialFixture(ctx, t, "cprov-idless") + createControlPlaneWithToken(ctx, t, f.namespace, "coder-b", "https://coder-b.example.com", "operator-token-b") + createControlPlaneWithToken(ctx, t, f.namespace, "coder-c", "https://coder-c.example.com", "operator-token-c") + apiURL := func(controlPlane string) string { + return "http://" + controlPlane + "." + f.namespace + ".svc.cluster.local:80" + } + urlB, urlC := apiURL("coder-b"), apiURL("coder-c") + store := &keyStoreClient{fakeBootstrapClient: f.bootstrapClient, keys: map[keySlot]uuid.UUID{}} + f.reconciler.BootstrapClient = store + + q := f.createKeyProvisioner(ctx, t, "q", "") + q.Spec.ControlPlaneRef.Name = "coder-b" + q.Spec.Key.Name = "provisioner" + require.NoError(t, k8sClient.Update(ctx, q)) + f.reconcile(ctx, t, q) + f.reconcile(ctx, t, q) + slotB := newKeySlot(urlB, "", "provisioner") + qKeyID := f.get(ctx, t, q).Status.ProvisionerKeyID + require.Equal(t, store.keys[slotB].String(), qKeyID, "Q must hold the key named provisioner at B") + + p := f.createProvisioner(ctx, t, nil, nil) + pName := types.NamespacedName{Name: p.Name, Namespace: p.Namespace} + f.reconcile(ctx, t, p) + f.reconcile(ctx, t, p) + reconcileP := func() ctrl.Result { + result, err := f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: pName}) + require.NoError(t, err) + return result + } + moveTo := func(controlPlane string) { + latest := f.get(ctx, t, p) + latest.Spec.ControlPlaneRef.Name = controlPlane + require.NoError(t, k8sClient.Update(ctx, latest)) + } + unverifiedReport := func() *metav1.Condition { + return meta.FindStatusCondition(f.get(ctx, t, p).Status.Conditions, coderv1alpha1.CoderProvisionerConditionUnverifiedKeyKept) + } + + store.failEnsureAt = []string{urlB, urlC} + for _, controlPlane := range []string{"coder-b", "coder-c"} { + moveTo(controlPlane) + _, err := f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: pName}) + require.Error(t, err, "the create at %s must fail", controlPlane) + require.True(t, slices.ContainsFunc(f.get(ctx, t, p).Status.PendingKeys, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { + return record.CoderURL == apiURL(controlPlane) && record.KeyName == "provisioner" && record.KeyID == "" + }), "P must record its create at %s without key IDs", controlPlane) + } + + if deleteP { + require.NoError(t, k8sClient.Delete(ctx, f.get(ctx, t, p))) + f.reconcile(ctx, t, p) + requireProvisionerGone(ctx, t, pName) + require.Equal(t, qKeyID, store.keys[slotB].String(), "the key of Q at B must still exist") + return + } + + store.failLookupAt = urlC + moveTo("coder") + result := reconcileP() + require.Equal(t, qKeyID, store.keys[slotB].String(), "the key of Q at B must still exist") + require.Len(t, f.get(ctx, t, p).Status.PendingKeys, 2, "the entries at B and C must stay") + report := unverifiedReport() + require.NotNil(t, report, "P must report the entries that it keeps") + require.Equal(t, metav1.ConditionTrue, report.Status) + require.Contains(t, report.Message, urlB) + require.Contains(t, report.Message, urlC) + require.Positive(t, result.RequeueAfter, "P must check the entries again") + + delete(store.keys, slotB) + reconcileP() + require.Len(t, f.get(ctx, t, p).Status.PendingKeys, 1, "the entry at B must go once Coder has no key with its name") + report = unverifiedReport() + require.NotNil(t, report, "a failed lookup must keep the report of the entry at C") + require.NotContains(t, report.Message, urlB, "the report must not name the entry at B any more") + require.Contains(t, report.Message, urlC) + + store.failLookupAt = "" + f.reconcile(ctx, t, p) + require.Empty(t, f.get(ctx, t, p).Status.PendingKeys) + require.Nil(t, unverifiedReport()) + }) + } +} diff --git a/internal/controller/coderworkspaceproxy_controller_test.go b/internal/controller/coderworkspaceproxy_controller_test.go index 66b03f112..93f7674eb 100644 --- a/internal/controller/coderworkspaceproxy_controller_test.go +++ b/internal/controller/coderworkspaceproxy_controller_test.go @@ -34,6 +34,9 @@ type fakeBootstrapClient struct { 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 // Entitlements support. entitlementsResponse codersdk.Entitlements @@ -93,6 +96,13 @@ func (f *fakeBootstrapClient) DeleteProvisionerKeyByID(_ context.Context, req co return f.deleteKeyByIDErr } +func (f *fakeBootstrapClient) ProvisionerKeyByName(_ context.Context, req coderbootstrap.ProvisionerKeyByNameRequest) (*codersdk.ProvisionerKey, error) { + if f.provisionerKeyByName == nil { + return nil, nil + } + return f.provisionerKeyByName(req), nil +} + func (f *fakeBootstrapClient) Entitlements(_ context.Context, coderURL, sessionToken string) (codersdk.Entitlements, error) { f.entitlementsCalls++ f.entitlementsRequests = append(f.entitlementsRequests, entitlementsRequest{