diff --git a/docs/tutorials/deploy-coderprovisioner.md b/docs/tutorials/deploy-coderprovisioner.md index 5de36a7d..6a9a0ceb 100644 --- a/docs/tutorials/deploy-coderprovisioner.md +++ b/docs/tutorials/deploy-coderprovisioner.md @@ -82,7 +82,7 @@ kubectl delete coderprovisioner coderprovisioner-sample -n coder The operator then deletes the provisioner keys in Coder that this provisioner created. It finds the key in the provisioner key Secret by the organization and key IDs in `status.organizationID` and `status.provisionerKeyID`, not by the names in the spec. It also deletes the key of each entry in `status.pendingKeys` that has an `organizationID` and a `keyID`. The entries name keys that the operator created but did not write to the Secret, for example because a reconcile failed, and keys that a rotation replaced but that Coder did not confirm as deleted. Each entry names the Coder URL of the key and the control plane whose operator token the operator uses to delete it. An entry without these IDs names a key that the operator started to create before Coder returned its ID. For such an entry, the operator deletes no key, because another provisioner or a person can use the same key name in that organization. It removes the entry when Coder has no key with that name, or when that key is the key in `status.provisionerKeyID`. Otherwise, and while the operator cannot reach Coder for that check, the entry stays, the condition `UnverifiedKeyKept` is `True` and names the entry, and deletion of the provisioner leaves the key in Coder. If no other provisioner or person uses the key, delete it with `coder provisioner keys delete --org `. The operator checks again every 2 minutes, and removes the entry and the condition once the key is gone. When the status has no key, for example because the provisioner never passed its checks, the operator removes its finalizer without a call to Coder. -The same records apply while the provisioner runs. When `spec.controlPlaneRef`, the control plane URL, the organization, the key name, or the tags change, the operator creates a new key and deletes the replaced keys 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"}]'`. +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. The operator also deletes the keys of entries for earlier targets while the current target is not ready, for example while its operator token is missing or its Coder deployment is not entitled to external provisioners. Each delete goes to the Coder URL of the entry, with the operator token of its control plane. While `CredentialsReady` is `False` or the control plane is missing, the operator sends nothing to Coder, so these keys stay. An entry with `unpublished: true`, and an entry for the key in the Secret, waits for the current target, because the running Deployment can still use its key. Each entry and `status.controlPlaneUID` also record the UID of the control plane. The operator sends the operator token of a control plane for a record only while the control plane has the recorded UID. When a control plane is deleted and created again under the same name, the new one can run another Coder backend, also at the same URL. The operator then creates a new key for the provisioner, and sends the new token for no key of the old control plane. Such an entry stays, the condition `ControlPlaneChangedKeyKept` is `True` and names it, and deletion of the provisioner leaves that key in Coder. A record without a UID, from an operator version that did not record it, counts as bound to its control plane only at the current URL of that control plane. For status without `status.controlPlaneUID`, the next reconcile looks the applied key up at the current control plane by `status.organizationID` and `status.provisionerKeyID`, so a renamed organization does not hide it. When Coder holds the key there, the operator records the UID of that control plane. When Coder answers that it has no such key, the operator creates a new key. When a change of the spec or the control plane replaces the key of such status, its entry in `status.pendingKeys` records the UID of its control plane while that control plane has the recorded URL. An entry whose control plane was deleted, or that `ControlPlaneChangedKeyKept` names, stays for good: delete its key by hand in Coder, then remove the entry, for example the first one with `kubectl patch coderprovisioner coderprovisioner-sample -n coder --subresource=status --type=json -p '[{"op":"remove","path":"/status/pendingKeys/0"}]'`. This cleanup is best-effort. When the operator cannot reach Coder, for example because the control plane or its operator token is gone, or when the delete call fails, the operator still removes its finalizer, and the key stays valid in Coder. Delete the provisioner while the control plane runs. If cleanup could not reach Coder, list the keys with `coder provisioner keys list --org `, and delete the key by hand with `coder provisioner keys delete --org `. diff --git a/internal/controller/coderprovisioner_controller.go b/internal/controller/coderprovisioner_controller.go index 95134ab7..7ab91d43 100644 --- a/internal/controller/coderprovisioner_controller.go +++ b/internal/controller/coderprovisioner_controller.go @@ -80,6 +80,10 @@ type CoderProvisionerReconciler struct { client.Client Scheme *runtime.Scheme BootstrapClient coderbootstrap.Client + + // earlierKeyRetryAt maps a provisioner UID to the earlierKeyRetry of its + // last try in revokeEarlierTargetKeys. + earlierKeyRetryAt sync.Map } // +kubebuilder:rbac:groups=coder.com,resources=coderprovisioners,verbs=get;list;watch;create;update;patch;delete @@ -205,6 +209,51 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req organizationName := provisionerOrganizationName(provisioner.Spec.OrganizationName) keyName, keySecretName, keySecretKey := provisionerKeyConfig(provisioner) + desiredControlPlaneRefName := provisioner.Spec.ControlPlaneRef.Name + + coderURL, err := coderAPIURL(controlPlane) + if err != nil { + return ctrl.Result{}, err + } + + // 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, + ControlPlaneUID: string(controlPlane.UID), + CoderURL: coderURL, + OrganizationName: organizationName, + KeyName: keyName, + Unpublished: true, + } + // Each earlier target record names its own URL and control plane, so its + // cleanup does not wait for the checks of the current target below. The + // key in the Secret and unpublished keys, which the Secret can hold, wait + // for a replacement below, because the running Deployment can still use + // them. + appliedKeyID, _ := uuid.Parse(provisioner.Status.ProvisionerKeyID) + inUse := func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { + return record.Unpublished || isKeyID(record.KeyID, appliedKeyID) + } + earlierReports, err := r.revokeEarlierTargetKeys(ctx, provisioner, target, inUse) + if err != nil { + return ctrl.Result{}, err + } + // A dropped record is written at once. A change of the reports alone + // waits for the next status write, so that a replacement below can + // update them first. + if !slices.Equal(provisioner.Status.PendingKeys, statusSnapshot.PendingKeys) { + if err := r.Status().Update(ctx, provisioner); err != nil { + return ctrl.Result{}, fmt.Errorf("record deleted provisioner keys of earlier targets 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 earlier target key status update") + } + } sessionToken, operatorAccessResult, operatorAccessReason, operatorAccessMessage, err := r.readOperatorSessionToken(ctx, provisioner, controlPlane) if err != nil { @@ -229,11 +278,6 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req operatorAccessMessage, ) - coderURL, err := coderAPIURL(controlPlane) - if err != nil { - return ctrl.Result{}, err - } - entitlementResult, entitlementErr := r.reconcileExternalProvisionerEntitlement(ctx, provisioner, controlPlane, sessionToken) if entitlementErr != nil { if statusSnapshot == nil { @@ -255,7 +299,6 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req } desiredTagsHash := hashProvisionerTags(provisioner.Spec.Tags) - desiredControlPlaneRefName := provisioner.Spec.ControlPlaneRef.Name status := provisioner.Status orgDrift := status.OrganizationName != "" && status.OrganizationName != organizationName keyNameDrift := status.ProvisionerKeyName != "" && status.ProvisionerKeyName != keyName @@ -295,19 +338,6 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req 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, - ControlPlaneUID: string(controlPlane.UID), - 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, @@ -369,15 +399,6 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req 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) } } @@ -405,22 +426,31 @@ func (r *CoderProvisionerReconciler) Reconcile(ctx context.Context, req ctrl.Req return sameKeySlot(record, target) && record.KeyID == "" }) pending = append(pending, target) - } - // The replacement below deletes the key that the target holds. - kept, conditionChanged, err := r.revokePendingKeys(ctx, provisioner, pending, target, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { - return existingKey != nil && isKeyID(record.KeyID, existingKey.ID) - }) - if err != nil { - return ctrl.Result{}, err - } - 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) + // revokeEarlierTargetKeys above handles the records that are not in + // use. The replacement below deletes the key that the target holds. + replacedByName := func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { + return existingKey != nil && isKeyID(record.KeyID, existingKey.ID) } - 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") + kept, reports, err := r.revokePendingKeys(ctx, provisioner, pending, target, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { + return !inUse(record) || replacedByName(record) + }) + if err != nil { + return ctrl.Result{}, err + } + // The publish step below drops the records of the key found by name. + reports = slices.DeleteFunc(mergeKeptKeyReports(kept, inUse, earlierReports, reports), func(report keptKeyReport) bool { + return replacedByName(report.record) + }) + conditionChanged := setKeptKeyConditions(provisioner, reports) + if conditionChanged || !slices.Equal(kept, statusSnapshot.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") + } } } @@ -652,6 +682,7 @@ func (r *CoderProvisionerReconciler) reconcileDeletion(ctx context.Context, prov } log := ctrl.LoggerFrom(ctx) + r.earlierKeyRetryAt.Delete(provisioner.UID) // Delete only the keys that status records for this resource. A // provisioner that never sent a key create (refused, not entitled, never @@ -1015,49 +1046,158 @@ func (r *CoderProvisionerReconciler) revokeKey( return keyUnverified, nil } +// keptKeyReport is the line that names a kept pending record outside the +// target slot in the ControlPlaneChangedKeyKept condition, or, with a note, in +// the UnverifiedKeyKept condition. +type keptKeyReport struct { + record coderv1alpha1.CoderProvisionerKeyRecord + controlPlaneChanged bool + note string +} + // 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. +// also returns the reports of the kept records outside the target slot. For a +// record that keep reports, it sends no request to Coder. 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) { +) ([]coderv1alpha1.CoderProvisionerKeyRecord, []keptKeyReport, error) { var kept []coderv1alpha1.CoderProvisionerKeyRecord - var unverified, controlPlaneChanged []string + var reports []keptKeyReport for _, record := range pending { - if sameKeySlot(record, target) || keep(record) { + if sameKeySlot(record, target) { kept = append(kept, record) continue } - cleanup, err := r.revokeKey(ctx, provisioner, record) - if err != nil { - return nil, false, err - } - if cleanup == keyGone { - continue + cleanup := keyKept + heldBack := keep(record) + if heldBack { + controlPlane, ok, err := r.readControlPlane(ctx, provisioner.Namespace, record.ControlPlaneRefName) + if err != nil { + return nil, nil, err + } + if ok && !tokenBoundTo(record, controlPlane) { + cleanup = keyControlPlaneChanged + } + } else { + var err error + cleanup, err = r.revokeKey(ctx, provisioner, record) + if err != nil { + return nil, nil, 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)) + reports = append(reports, keptKeyReport{record: record, controlPlaneChanged: true}) continue } if _, _, ok := parseKeyIDs(record.OrganizationID, record.KeyID); !ok { note := "the operator could not check Coder" + if heldBack { + note = "the operator checks Coder for it at the next key replacement" + } 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)) + reports = append(reports, keptKeyReport{record: record, note: note}) } } + return kept, reports, nil +} + +// mergeKeptKeyReports returns, in the order of kept, the reports of the +// records in use from replacement, and of the other records from earlier. +func mergeKeptKeyReports( + kept []coderv1alpha1.CoderProvisionerKeyRecord, + inUse func(coderv1alpha1.CoderProvisionerKeyRecord) bool, + earlier, replacement []keptKeyReport, +) []keptKeyReport { + var merged []keptKeyReport + for _, record := range kept { + from := replacement + if !inUse(record) { + from = earlier + } + if i := slices.IndexFunc(from, func(report keptKeyReport) bool { return report.record == record }); i >= 0 { + merged = append(merged, from[i]) + } + } + return merged +} + +// setKeptKeyConditions sets the UnverifiedKeyKept and +// ControlPlaneChangedKeyKept conditions from reports, and reports whether the +// conditions changed. +func setKeptKeyConditions(provisioner *coderv1alpha1.CoderProvisioner, reports []keptKeyReport) bool { + var unverified, controlPlaneChanged []string + for _, report := range reports { + record := report.record + if report.controlPlaneChanged { + controlPlaneChanged = append(controlPlaneChanged, fmt.Sprintf("key %q in organization %q at %s (control plane %q)", + record.KeyName, record.OrganizationName, record.CoderURL, record.ControlPlaneRefName)) + continue + } + unverified = append(unverified, fmt.Sprintf("key %q in organization %q at %s: %s", record.KeyName, record.OrganizationName, record.CoderURL, report.note)) + } conditionChanged := setUnverifiedKeyCondition(provisioner, unverified) - conditionChanged = setControlPlaneChangedKeyCondition(provisioner, controlPlaneChanged) || conditionChanged - return kept, conditionChanged, nil + return setControlPlaneChangedKeyCondition(provisioner, controlPlaneChanged) || conditionChanged +} + +// earlierKeyRetry is the last try of revokeEarlierTargetKeys that left a key +// that Coder did not confirm gone. +type earlierKeyRetry struct { + // at is the time before which revokeEarlierTargetKeys does not try again. + at time.Time + // reports are the reports of that try. + reports []keptKeyReport +} + +// revokeEarlierTargetKeys deletes the keys of the status.pendingKeys records +// outside target that are not in use, each with the operator token of its own +// control plane (see revokeKey). It updates status.pendingKeys and the +// conditions of the kept records in provisioner only, and returns the reports +// of the kept records outside target. After a try that leaves a key that Coder +// did not confirm gone, it does not try again for +// provisionerPendingKeyRetryInterval, so an unreachable earlier endpoint does +// not get a request on every reconcile. Until then it changes nothing and +// returns the reports of that try. +func (r *CoderProvisionerReconciler) revokeEarlierTargetKeys( + ctx context.Context, + provisioner *coderv1alpha1.CoderProvisioner, + target coderv1alpha1.CoderProvisionerKeyRecord, + inUse func(coderv1alpha1.CoderProvisionerKeyRecord) bool, +) ([]keptKeyReport, error) { + if value, ok := r.earlierKeyRetryAt.Load(provisioner.UID); ok { + retry, ok := value.(earlierKeyRetry) + if !ok { + return nil, fmt.Errorf("assertion failed: earlier key retry of provisioner %s is %T, not earlierKeyRetry", provisioner.UID, value) + } + if time.Now().Before(retry.at) { + return retry.reports, nil + } + } + kept, reports, err := r.revokePendingKeys(ctx, provisioner, provisioner.Status.PendingKeys, target, inUse) + if err != nil { + return nil, err + } + if slices.ContainsFunc(kept, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { + return !sameKeySlot(record, target) && !inUse(record) + }) { + r.earlierKeyRetryAt.Store(provisioner.UID, earlierKeyRetry{at: time.Now().Add(provisionerPendingKeyRetryInterval), reports: reports}) + } else { + r.earlierKeyRetryAt.Delete(provisioner.UID) + } + provisioner.Status.PendingKeys = kept + setKeptKeyConditions(provisioner, reports) + return reports, nil } // setUnverifiedKeyCondition sets the UnverifiedKeyKept condition for the diff --git a/internal/controller/coderprovisioner_earliertargets_test.go b/internal/controller/coderprovisioner_earliertargets_test.go new file mode 100644 index 00000000..9f77c262 --- /dev/null +++ b/internal/controller/coderprovisioner_earliertargets_test.go @@ -0,0 +1,334 @@ +package controller_test + +import ( + "context" + "errors" + "slices" + "testing" + "time" + + "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" + "github.com/coder/coder-k8s/internal/controller" +) + +// coderCallLog records the Coder URL of every call, can fail the deletes of +// one key, and can fail every key lookup by name. +type coderCallLog struct { + coderbootstrap.Client + urls []string + failDelete uuid.UUID + failedDeletes int + failLookup bool +} + +func (c *coderCallLog) EnsureProvisionerKey(ctx context.Context, req coderbootstrap.EnsureProvisionerKeyRequest) (coderbootstrap.EnsureProvisionerKeyResponse, error) { + c.urls = append(c.urls, req.CoderURL) + return c.Client.EnsureProvisionerKey(ctx, req) +} + +func (c *coderCallLog) DeleteProvisionerKeyByID(ctx context.Context, req coderbootstrap.DeleteProvisionerKeyByIDRequest) error { + c.urls = append(c.urls, req.CoderURL) + if req.KeyID == c.failDelete { + c.failedDeletes++ + return errors.New("injected delete failure") + } + return c.Client.DeleteProvisionerKeyByID(ctx, req) +} + +func (c *coderCallLog) ProvisionerKeyByID(ctx context.Context, req coderbootstrap.ProvisionerKeyByIDRequest) (*codersdk.ProvisionerKey, error) { + c.urls = append(c.urls, req.CoderURL) + return c.Client.ProvisionerKeyByID(ctx, req) +} + +func (c *coderCallLog) ProvisionerKeyByName(ctx context.Context, req coderbootstrap.ProvisionerKeyByNameRequest) (*codersdk.ProvisionerKey, error) { + c.urls = append(c.urls, req.CoderURL) + if c.failLookup { + return nil, errors.New("injected lookup failure") + } + return c.Client.ProvisionerKeyByName(ctx, req) +} + +func (c *coderCallLog) Entitlements(ctx context.Context, coderURL, sessionToken string) (codersdk.Entitlements, error) { + c.urls = append(c.urls, coderURL) + return c.Client.Entitlements(ctx, coderURL, sessionToken) +} + +// TestCoderProvisionerKey_EarlierTargetRevokedWhileTargetNotReady checks a +// key that a rotation from control plane A to B replaced and could not delete. +// When the provisioner moves on to a control plane C that is not ready, the +// key at A must still be deleted at A with the operator token of A, and C must +// get no request. Key B, which the Secret holds, must stay. A failed delete is +// retried at the pending key cadence, not on every reconcile, also by the +// rotation to a ready C. When the Secret +// write at B failed instead, the unpublished key B, which the Secret can hold, +// must stay until C is ready. +func TestCoderProvisionerKey_EarlierTargetRevokedWhileTargetNotReady(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + // notReady makes control plane C fail one of its checks. When it is + // nil, C is ready. + notReady func(ctx context.Context, t *testing.T, f provisionerCredentialFixture, provisioner *coderv1alpha1.CoderProvisioner) + // deleteFails fails every delete of key A at C. + deleteFails bool + // unpublishedAtB fails the Secret write at B instead of the delete + // of key A. + unpublishedAtB bool + }{ + {name: "target not entitled", notReady: markNotEntitled}, + {name: "credentials refused", notReady: refuseCredentials}, + {name: "target operator token missing", notReady: deleteOperatorToken}, + {name: "delete fails at the earlier target", notReady: markNotEntitled, deleteFails: true}, + {name: "delete fails at the earlier target, target ready", deleteFails: true}, + {name: "unpublished key at the earlier target", notReady: markNotEntitled, unpublishedAtB: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + f := newProvisionerCredentialFixture(ctx, t, "cprov-earlier") + 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" + } + 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} + calls := &coderCallLog{Client: f.bootstrapClient} + f.reconciler.BootstrapClient = calls + + provisioner := f.createProvisioner(ctx, t, nil, nil) + name := types.NamespacedName{Name: provisioner.Name, Namespace: provisioner.Namespace} + keySecretName := provisioner.Name + "-provisioner-key" + failSecretWrite := false + base, err := ctrlclient.NewWithWatch(cfg, ctrlclient.Options{Scheme: scheme}) + require.NoError(t, err) + f.reconciler.Client = interceptor.NewClient(base, interceptor.Funcs{ + 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)) + } + reconcile := func() { + _, err := f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: name}) + require.NoError(t, err) + } + + 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.unpublishedAtB { + failSecretWrite = true + _, err := f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: name}) + require.Error(t, err, "the injected Secret write failure must fail the attempt at B") + require.True(t, pendingKeyRecorded(f.get(ctx, t, provisioner), keyB.KeyID), "status must record the unpublished key B") + } else { + calls.failDelete = keyA.KeyID + reconcile() + atB := f.get(ctx, t, provisioner) + require.Equal(t, keyB.KeyID.String(), atB.Status.ProvisionerKeyID) + require.True(t, pendingKeyRecorded(atB, keyA.KeyID), "the failed delete must keep the record of key A") + } + + if !tt.deleteFails { + calls.failDelete = uuid.Nil + } + calls.failedDeletes = 0 + calls.urls = nil + f.bootstrapClient.deleteKeyByIDRequests = nil + if tt.notReady != nil { + tt.notReady(ctx, t, f, provisioner) + } + moveTo("coder-c") + reconcile() + if tt.deleteFails { + reconcile() + } + + if tt.notReady != nil { + require.NotContains(t, calls.urls, apiURL("coder-c"), "control plane C must get no request while it is not ready") + require.NotContains(t, keyDeletes(f.bootstrapClient), keyDelete{coderURL: apiURL("coder-b"), token: "operator-token-b", keyID: keyB.KeyID.String()}, + "nothing may delete key B, which the Secret holds or can hold, while C is not ready") + } + final := f.get(ctx, t, provisioner) + if tt.name == "credentials refused" { + require.Empty(t, calls.urls, "a failed credential check must send nothing to Coder") + require.True(t, pendingKeyRecorded(final, keyA.KeyID), "the record of key A must stay") + return + } + if tt.unpublishedAtB { + require.Empty(t, keyDeletes(f.bootstrapClient)) + require.True(t, pendingKeyRecorded(final, keyB.KeyID), "the record of the unpublished key B must stay") + return + } + applied := keyB + if tt.notReady == nil { + applied = keyC + } + require.Equal(t, applied.KeyID.String(), final.Status.ProvisionerKeyID) + if tt.deleteFails { + require.Equal(t, 1, calls.failedDeletes, "a failed delete must wait for the pending key cadence") + require.True(t, pendingKeyRecorded(final, keyA.KeyID), "a failed delete must keep the record of key A") + controller.ElapseEarlierKeyRetryInterval(f.reconciler, final.UID) + reconcile() + require.Equal(t, 2, calls.failedDeletes, "the delete must be tried again after the pending key cadence") + return + } + require.Equal(t, []keyDelete{{coderURL: apiURL("coder"), token: testOperatorToken, keyID: keyA.KeyID.String()}}, keyDeletes(f.bootstrapClient), + "key A must be deleted at A with the operator token of A") + require.False(t, pendingKeyRecorded(final, keyA.KeyID), "the record of key A must go once Coder confirmed the delete") + }) + } +} + +// TestCoderProvisionerKey_EarlierTargetReportsBesideUnpublishedKey checks the +// reports of earlier target records while an unpublished record waits for a +// control plane C that is not ready. A record whose control plane was created +// again must be reported, also an unpublished one, and the report of a record +// without key IDs must go once Coder has no key with its name. When C gets +// ready while the retry interval runs, and its key replacement deletes the key +// of a reported record, the report of that record must go too. +func TestCoderProvisionerKey_EarlierTargetReportsBesideUnpublishedKey(t *testing.T) { + t.Parallel() + ctx := context.Background() + f := newProvisionerCredentialFixture(ctx, t, "cprov-earlier-reports") + 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") + calls := &coderCallLog{Client: f.bootstrapClient} + f.reconciler.BootstrapClient = calls + 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) + calls.failLookup = true + + controlPlaneB := &coderv1alpha1.CoderControlPlane{} + require.NoError(t, k8sClient.Get(ctx, types.NamespacedName{Name: "coder-b", Namespace: f.namespace}, controlPlaneB)) + record := func(keyName string, withIDs bool) coderv1alpha1.CoderProvisionerKeyRecord { + record := coderv1alpha1.CoderProvisionerKeyRecord{ + ControlPlaneRefName: "coder-b", + ControlPlaneUID: string(controlPlaneB.UID), + CoderURL: "http://coder-b." + f.namespace + ".svc.cluster.local:80", + OrganizationName: "default", + KeyName: keyName, + } + if withIDs { + record.OrganizationID = uuid.NewString() + record.KeyID = uuid.NewString() + } + return record + } + recreated := record("recreated", true) + recreated.ControlPlaneUID = uuid.NewString() + unpublished := record("unpublished", true) + unpublished.Unpublished = true + unpublishedRecreated := record("unpublished-recreated", true) + unpublishedRecreated.Unpublished = true + unpublishedRecreated.ControlPlaneUID = recreated.ControlPlaneUID + idLess := record("id-less", false) + latest := f.get(ctx, t, provisioner) + latest.Status.PendingKeys = []coderv1alpha1.CoderProvisionerKeyRecord{recreated, unpublished, unpublishedRecreated, idLess} + require.NoError(t, k8sClient.Status().Update(ctx, latest)) + markNotEntitled(ctx, t, f, provisioner) + latest = f.get(ctx, t, provisioner) + latest.Spec.ControlPlaneRef.Name = "coder-c" + require.NoError(t, k8sClient.Update(ctx, latest)) + reconcile := func() *coderv1alpha1.CoderProvisioner { + _, err := f.reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: name}) + require.NoError(t, err) + return f.get(ctx, t, provisioner) + } + + got := reconcile() + require.Equal(t, []coderv1alpha1.CoderProvisionerKeyRecord{recreated, unpublished, unpublishedRecreated, idLess}, got.Status.PendingKeys) + changed := meta.FindStatusCondition(got.Status.Conditions, coderv1alpha1.CoderProvisionerConditionControlPlaneChangedKeyKept) + require.NotNil(t, changed, "the record whose control plane was created again must be reported") + require.Contains(t, changed.Message, `"recreated"`) + require.Contains(t, changed.Message, `"unpublished-recreated"`) + require.NotContains(t, changed.Message, `"unpublished"`) + unverified := meta.FindStatusCondition(got.Status.Conditions, coderv1alpha1.CoderProvisionerConditionUnverifiedKeyKept) + require.NotNil(t, unverified, "the record without key IDs must be reported") + require.Contains(t, unverified.Message, `"id-less"`) + + calls.failLookup = false + controller.ElapseEarlierKeyRetryInterval(f.reconciler, got.UID) + got = reconcile() + require.Equal(t, []coderv1alpha1.CoderProvisionerKeyRecord{recreated, unpublished, unpublishedRecreated}, got.Status.PendingKeys, + "the record without key IDs must go once Coder has no key with its name") + require.Nil(t, meta.FindStatusCondition(got.Status.Conditions, coderv1alpha1.CoderProvisionerConditionUnverifiedKeyKept)) + require.NotNil(t, meta.FindStatusCondition(got.Status.Conditions, coderv1alpha1.CoderProvisionerConditionControlPlaneChangedKeyKept)) + require.Empty(t, keyDeletes(f.bootstrapClient), "no recorded key may be deleted: one has another control plane, one is unpublished") + + urlC := "http://coder-c." + f.namespace + ".svc.cluster.local:80" + f.bootstrapClient.provisionerKeyByName = func(req coderbootstrap.ProvisionerKeyByNameRequest) *codersdk.ProvisionerKey { + if req.CoderURL != urlC { + return nil + } + return &codersdk.ProvisionerKey{ID: uuid.MustParse(recreated.KeyID), OrganizationID: uuid.MustParse(recreated.OrganizationID), Name: req.KeyName} + } + controlPlaneC := &coderv1alpha1.CoderControlPlane{} + require.NoError(t, k8sClient.Get(ctx, types.NamespacedName{Name: "coder-c", Namespace: f.namespace}, controlPlaneC)) + controlPlaneC.Status.ExternalProvisionerDaemonsEntitlement = string(codersdk.EntitlementEntitled) + require.NoError(t, k8sClient.Status().Update(ctx, controlPlaneC)) + got = reconcile() + require.Contains(t, keyDeletes(f.bootstrapClient), keyDelete{coderURL: urlC, token: "operator-token-c", keyID: recreated.KeyID}, + "the key replacement at C must delete the key that holds the target name") + require.False(t, slices.ContainsFunc(got.Status.PendingKeys, func(record coderv1alpha1.CoderProvisionerKeyRecord) bool { + return record.KeyName == "recreated" + }), "the replacement must drop the record of the deleted key") + changed = meta.FindStatusCondition(got.Status.Conditions, coderv1alpha1.CoderProvisionerConditionControlPlaneChangedKeyKept) + require.NotNil(t, changed) + require.NotContains(t, changed.Message, `"recreated"`, "the report of the deleted key must go") + require.Contains(t, changed.Message, `"unpublished-recreated"`) +} + +func markNotEntitled(ctx context.Context, t *testing.T, f provisionerCredentialFixture, _ *coderv1alpha1.CoderProvisioner) { + t.Helper() + controlPlane := &coderv1alpha1.CoderControlPlane{} + require.NoError(t, k8sClient.Get(ctx, types.NamespacedName{Name: "coder-c", Namespace: f.namespace}, controlPlane)) + now := metav1.NewTime(time.Now()) + controlPlane.Status.EntitlementsLastChecked = &now + controlPlane.Status.ExternalProvisionerDaemonsEntitlement = string(codersdk.EntitlementNotEntitled) + require.NoError(t, k8sClient.Status().Update(ctx, controlPlane)) +} + +func deleteOperatorToken(ctx context.Context, t *testing.T, f provisionerCredentialFixture, _ *coderv1alpha1.CoderProvisioner) { + t.Helper() + require.NoError(t, k8sClient.Delete(ctx, &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "coder-c-operator-token", Namespace: f.namespace}})) +} + +// refuseCredentials adds an extraEnv Secret without the credential opt-in. +func refuseCredentials(ctx context.Context, t *testing.T, f provisionerCredentialFixture, provisioner *coderv1alpha1.CoderProvisioner) { + t.Helper() + createSecret(ctx, t, tokenSecret(metav1.ObjectMeta{Name: "extra", Namespace: f.namespace}, "value", testExtraEnvValue)) + latest := f.get(ctx, t, provisioner) + latest.Spec.ExtraEnv = []corev1.EnvVar{secretEnv("EXTRA", "extra", "value")} + require.NoError(t, k8sClient.Update(ctx, latest)) +} diff --git a/internal/controller/coderprovisioner_pendingkeys_test.go b/internal/controller/coderprovisioner_pendingkeys_test.go index 8be61ffa..cd906d05 100644 --- a/internal/controller/coderprovisioner_pendingkeys_test.go +++ b/internal/controller/coderprovisioner_pendingkeys_test.go @@ -22,6 +22,7 @@ import ( coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" "github.com/coder/coder-k8s/internal/coderbootstrap" + "github.com/coder/coder-k8s/internal/controller" ) // keyRecordingClient checks that status records every key create before the @@ -623,6 +624,7 @@ func TestCoderProvisionerKey_IDLessRecordKeepsOtherProvisionersKey(t *testing.T) require.Contains(t, report.Message, urlC) store.failLookupAt = "" + controller.ElapseEarlierKeyRetryInterval(f.reconciler, f.get(ctx, t, p).UID) f.reconcile(ctx, t, p) require.Empty(t, f.get(ctx, t, p).Status.PendingKeys) require.Nil(t, unverifiedReport()) diff --git a/internal/controller/export_test.go b/internal/controller/export_test.go index 1d29d470..260b2deb 100644 --- a/internal/controller/export_test.go +++ b/internal/controller/export_test.go @@ -44,3 +44,9 @@ func HasSentCredentials(r *CoderMCPServerReconciler, uid types.UID) bool { }) return found } + +// ElapseEarlierKeyRetryInterval lets the next reconcile of the provisioner uid +// retry the keys of earlier targets, as after provisionerPendingKeyRetryInterval. +func ElapseEarlierKeyRetryInterval(r *CoderProvisionerReconciler, uid types.UID) { + r.earlierKeyRetryAt.Delete(uid) +}