diff --git a/internal/controller/coderaigatewaykey_delete.go b/internal/controller/coderaigatewaykey_delete.go index 03ab799d2..9cb82b895 100644 --- a/internal/controller/coderaigatewaykey_delete.go +++ b/internal/controller/coderaigatewaykey_delete.go @@ -2,7 +2,7 @@ package controller import ( "context" - "net/http" + "slices" "time" "github.com/coder/coder/v2/codersdk" @@ -58,10 +58,10 @@ func (r *CoderAIGatewayKeyReconciler) deleteStep(ctx context.Context, key *coder var cancel context.CancelFunc ctx, cancel = context.WithTimeout(ctx, aiGatewayKeyBoundedDeleteTimeout) defer cancel() - if _, failure, err := r.deleteKeys(ctx, sdk, key); err != nil { + if _, left, failure, err := r.deleteKeys(ctx, sdk, key); err != nil { return false, nil, err } else if failure != nil { - return r.releaseWithoutDelete(ctx, key, "the one pass while CoderControlPlane is being deleted failed: "+failure.message), nil, nil + return r.releaseWithoutDelete(ctx, key, "the one pass while CoderControlPlane is being deleted failed: "+failure.message, left...), nil, nil } return true, nil, nil } @@ -81,7 +81,7 @@ func (r *CoderAIGatewayKeyReconciler) deleteStep(ctx context.Context, key *coder // The pass repeats until a LIST shows no key of this object, so the // release follows a LIST that proves it. for range 2 { - deleted, failure, err := r.deleteKeys(ctx, sdk, key) + deleted, _, failure, err := r.deleteKeys(ctx, sdk, key) if err != nil || failure != nil { return false, failure, err } else if deleted == 0 { @@ -93,27 +93,32 @@ func (r *CoderAIGatewayKeyReconciler) deleteStep(ctx context.Context, key *coder } // deleteKeys lists the keys of this object, deletes each one, and returns -// how many it deleted, or the failure that stopped it. -func (r *CoderAIGatewayKeyReconciler) deleteKeys(ctx context.Context, sdk *codersdk.Client, key *coderv1alpha1.CoderAIGatewayKey) (int, *aiGatewayKeyStep, error) { +// how many it deleted. On a failed DELETE it also returns the failure and +// the IDs of the listed keys that it did not delete. +func (r *CoderAIGatewayKeyReconciler) deleteKeys(ctx context.Context, sdk *codersdk.Client, key *coderv1alpha1.CoderAIGatewayKey) (int, []string, *aiGatewayKeyStep, error) { prefix, err := aiGatewayKeyNamePrefix(key.UID) if err != nil { - return 0, nil, err + return 0, nil, nil, err } listed, err := sdk.ListAIGatewayKeys(ctx) if err != nil { - return 0, aiGatewayKeyDeleteFailure("list AI Gateway keys", err), nil + return 0, nil, aiGatewayKeyDeleteFailure("list AI Gateway keys", err), nil } - deleted := 0 - for _, k := range ownAIGatewayKeys(prefix, listed) { - if serial, _ := aiGatewayKeySerial(prefix, k.Name); serial >= key.Status.NextSerial { - continue // No create of this object used this serial (plan I3). + own := slices.DeleteFunc(ownAIGatewayKeys(prefix, listed), func(k codersdk.AIGatewayKey) bool { + serial, _ := aiGatewayKeySerial(prefix, k.Name) + return serial >= key.Status.NextSerial // No create of this object used this serial (plan I3). + }) + for i, k := range own { + // Only Coder's own 404 proves that the key is gone (as for providers, #325). + if err := sdk.DeleteAIGatewayKey(ctx, k.ID); err != nil && !isCoderResourceNotFound(err) { + var left []string + for _, rest := range own[i:] { + left = append(left, rest.ID.String()) + } + return i, left, aiGatewayKeyDeleteFailure("delete key "+k.Name, err), nil } - if err := sdk.DeleteAIGatewayKey(ctx, k.ID); (err != nil && coderStatus(err) != http.StatusNotFound) || coderRouteMissing(err) { - return deleted, aiGatewayKeyDeleteFailure("delete key "+k.Name, err), nil - } - deleted++ } - return deleted, nil, nil + return len(own), nil, nil, nil } // aiGatewayKeyDeleteFailure classifies a failed call of the deletion like a @@ -131,8 +136,9 @@ func aiGatewayKeyDeleteFailure(action string, err error) *aiGatewayKeyStep { } // releaseWithoutDelete logs the key IDs that can stay in Coder, because -// nothing can delete them any more, and returns true (ControlPlaneGone). -func (r *CoderAIGatewayKeyReconciler) releaseWithoutDelete(ctx context.Context, key *coderv1alpha1.CoderAIGatewayKey, why string) bool { +// nothing can delete them any more, and returns true (ControlPlaneGone). left +// holds the IDs that a failed pass listed but did not delete. +func (r *CoderAIGatewayKeyReconciler) releaseWithoutDelete(ctx context.Context, key *coderv1alpha1.CoderAIGatewayKey, why string, left ...string) bool { var ids []string if current := key.Status.CurrentKey; current != nil { ids = append(ids, current.ID) @@ -141,6 +147,6 @@ func (r *CoderAIGatewayKeyReconciler) releaseWithoutDelete(ctx context.Context, ids = append(ids, retiring.ID) } log.FromContext(ctx).Info("Releasing the finalizer without deleting the keys in Coder: the keys can stay in Coder", - "reason", "ControlPlaneGone", "why", why, "keyIDs", ids, "nextSerial", key.Status.NextSerial) + "reason", "ControlPlaneGone", "why", why, "keyIDs", ids, "listedKeyIDs", left, "nextSerial", key.Status.NextSerial) return true } diff --git a/internal/controller/coderaigatewaykey_delete_test.go b/internal/controller/coderaigatewaykey_delete_test.go index 738e80c8f..ab12d0d6c 100644 --- a/internal/controller/coderaigatewaykey_delete_test.go +++ b/internal/controller/coderaigatewaykey_delete_test.go @@ -9,9 +9,13 @@ import ( "github.com/coder/coder/v2/codersdk" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" + "github.com/coder/coder-k8s/internal/controller" ) // rotatedKey issues the first key and rotates once, so Coder holds the @@ -89,6 +93,15 @@ func TestCoderAIGatewayKeyDeleteRetries(t *testing.T) { heal: func(*fakeCoder) {}, reason: "CoderUnavailable", requeue: time.Minute, }, + // A 404 that is not Coder's own (an HTML page of a proxy) proves + // nothing, so the key stays and the deletion retries. + "proxy 404": { + fail: func(f *fakeCoder) { + f.failNext(routeDeleteAIGatewayKey, fakeFault{Status: http.StatusNotFound, HTML: "404"}) + }, + heal: func(*fakeCoder) {}, + reason: "CoderUnavailable", requeue: time.Minute, + }, "premium feature": { fail: func(f *fakeCoder) { f.setAIGatewayFeature(false) }, heal: func(f *fakeCoder) { f.setAIGatewayFeature(true) }, stalled: true, reason: "LicenseRequired", requeue: 10 * time.Minute, @@ -147,15 +160,27 @@ func TestCoderAIGatewayKeyDeleteWithControlPlaneGoing(t *testing.T) { require.NoError(t, k8sClient.Delete(ctx, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: env.namespace}})) holdControlPlane(ctx, t, env.namespace) require.NoError(t, k8sClient.Delete(ctx, got)) + var orphan codersdk.CreateAIGatewayKeyResponse if fail { + // An orphan that status does not name: only the pass's LIST knows it. + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(got), got)) + editStatus(ctx, t, got, func(s *coderv1alpha1.CoderAIGatewayKeyStatus) { s.NextSerial = 3 }) + var err error + orphan, err = env.fake.client(t, 5*time.Second).CreateAIGatewayKey(ctx, codersdk.CreateAIGatewayKeyRequest{Name: aiGatewayKeyPrefix(got) + "2"}) + require.NoError(t, err) env.fake.failNext(routeDeleteAIGatewayKey, fakeFault{Status: http.StatusBadGateway}) } - gone, _ := env.reconcile(ctx, t, got) - require.Nil(t, gone, "the finalizer went inside the settle window") + r := &controller.CoderAIGatewayKeyReconciler{Client: env.client, APIReader: env.client, Scheme: scheme, Clock: env.clock} + logCtx, logs := capturingContext(ctx) + _, err := r.Reconcile(logCtx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(got)}) + require.NoError(t, err) + require.True(t, apierrors.IsNotFound(k8sClient.Get(ctx, client.ObjectKeyFromObject(got), got)), "the finalizer went inside the settle window") require.Equal(t, 1, env.fake.requestCount(routeAIGatewayKeys)-2, "one LIST") if fail { require.Equal(t, 1, env.fake.requestCount(routeDeleteAIGatewayKey), "the pass stops at the failed DELETE") - require.Len(t, env.fake.aiGatewayKeys(), 2) + require.Len(t, env.fake.aiGatewayKeys(), 3) + // The release log names the keys that the pass listed and left. + require.Contains(t, logs.String(), orphan.ID.String()) } else { require.Empty(t, env.fake.aiGatewayKeys()) } diff --git a/internal/controller/coderaigatewaykey_mismatch_test.go b/internal/controller/coderaigatewaykey_mismatch_test.go index 2e37977d2..77053803a 100644 --- a/internal/controller/coderaigatewaykey_mismatch_test.go +++ b/internal/controller/coderaigatewaykey_mismatch_test.go @@ -7,6 +7,7 @@ import ( "time" "github.com/coder/coder/v2/codersdk" + "github.com/google/uuid" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/meta" @@ -178,3 +179,37 @@ func TestCoderAIGatewayKeyAnswerMismatchWithFlappingList(t *testing.T) { }) } } + +// A kill-switch re-arm issues a key behind the same gate: a mismatched +// answer to the re-armed create blocks every later create in that +// generation, also across a failed LIST and the settle window. +func TestCoderAIGatewayKeyRearmKeepsAnswerMismatchGate(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newAIGatewayKeyEnv(ctx, t, ptrTo(true)) + got, _ := env.reconcile(ctx, t, env.createKey(ctx, t, "coder", testAIGatewaySecret)) + requireAIGatewayKeyReady(t, got) + require.NoError(t, env.fake.client(t, 5*time.Second).DeleteAIGatewayKey(ctx, uuid.MustParse(got.Status.CurrentKey.ID))) + got, _ = env.reconcile(ctx, t, got) + requireAIGatewayKeyStep(t, got, true, "KeyDeletedExternally") + + got.Spec.Rotation.Trigger = "re-arm" + require.NoError(t, k8sClient.Update(ctx, got)) + got, _ = env.reconcile(ctx, t, got) // The re-arm clears the current key. + env.fake.failNext(routeCreateAIGatewayKey, answerMismatch()) + got, _ = env.reconcile(ctx, t, got) + requireMismatchStalled(t, got) + require.Equal(t, got.Generation, got.Status.AnswerMismatchGeneration) + creates := env.fake.requestCount(routeCreateAIGatewayKey) + + env.fake.failNext(routeAIGatewayKeys, fakeFault{Status: http.StatusBadGateway}) + got, _ = env.reconcile(ctx, t, got) + requireAIGatewayKeyStep(t, got, false, "CoderUnavailable") + for _, step := range []time.Duration{0, 5 * time.Minute, 0} { + env.clock.Step(step) + got, _ = env.reconcile(ctx, t, got) + requireAIGatewayKeyStep(t, got, true, "CoderAnswerMismatch") + } + require.Equal(t, creates, env.fake.requestCount(routeCreateAIGatewayKey), "no create after the mismatch") + require.Empty(t, env.fake.aiGatewayKeys(), "the sweep deleted the mismatched key") +} diff --git a/internal/controller/coderaigatewaykey_recover.go b/internal/controller/coderaigatewaykey_recover.go index bcd9880af..423cb7ba7 100644 --- a/internal/controller/coderaigatewaykey_recover.go +++ b/internal/controller/coderaigatewaykey_recover.go @@ -2,9 +2,7 @@ package controller import ( "context" - "errors" "fmt" - "net/http" "strings" "time" @@ -293,7 +291,9 @@ func (r *CoderAIGatewayKeyReconciler) deleteOwnKey( if held == k.ID { return aiGatewayKeyWait(time.Second, reason, "Secret %q now holds key %s, so the operator does not delete it.", key.Spec.SecretName, k.Name) } - if err := sdk.DeleteAIGatewayKey(ctx, k.ID); (err != nil && coderStatus(err) != http.StatusNotFound) || coderRouteMissing(err) { + // Only Coder's own 404 proves that the key is gone. An HTML 404 of a proxy + // or "Route not found." proves nothing. + if err := sdk.DeleteAIGatewayKey(ctx, k.ID); err != nil && !isCoderResourceNotFound(err) { failure := aiGatewayKeyCoderFailure("delete "+noun+" "+k.Name, err) if failure.stalled { failure.message = fmt.Sprintf("The operator cannot delete %s %s. %s", noun, k.Name, failure.message) @@ -318,14 +318,6 @@ func aiGatewayKeyKeepReady(key *coderv1alpha1.CoderAIGatewayKey, step *aiGateway return step } -// coderRouteMissing reports Coder's 404 for a route that it does not have -// (plan F11). Unlike the 404 for a missing key, it proves nothing was -// deleted. -func coderRouteMissing(err error) bool { - var sdkErr *codersdk.Error - return errors.As(err, &sdkErr) && sdkErr.StatusCode() == http.StatusNotFound && sdkErr.Message == aiGatewayRouteNotFound -} - // liveSecretHolds live-reads the output Secret and returns the key in own // that it holds (plan I1), whoever owns the Secret: in delete-only mode a // replaced Secret can still hold a key of this object (plan 4.8 rule 2). The diff --git a/internal/controller/coderaigatewaykey_stall_test.go b/internal/controller/coderaigatewaykey_stall_test.go index 78b7c9971..890a0ded3 100644 --- a/internal/controller/coderaigatewaykey_stall_test.go +++ b/internal/controller/coderaigatewaykey_stall_test.go @@ -345,3 +345,41 @@ func TestCoderAIGatewayKeyUndeletableOrphanWhileReady(t *testing.T) { }) } } + +// Only Coder's own 404 proves that a key is gone. An HTML 404 of a proxy on +// a sweep or retire DELETE is CoderUnavailable: the orphan stays an orphan, +// the replaced key stays retiring, and the next DELETE runs. +func TestCoderAIGatewayKeyProxyNotFoundDeletesNothing(t *testing.T) { + t.Parallel() + ctx := context.Background() + proxy404 := fakeFault{Status: http.StatusNotFound, HTML: "404"} + t.Run("sweep", func(t *testing.T) { + t.Parallel() + env := newAIGatewayKeyEnv(ctx, t, ptrTo(true)) + got, orphan := readyWithOrphan(ctx, t, &env) + env.fake.failNext(routeDeleteAIGatewayKey, proxy404) + got, _ = env.reconcile(ctx, t, got) + requireReadyWhile(t, got, "CoderUnavailable") + require.Len(t, env.fake.aiGatewayKeys(), 2) + got, _ = env.reconcile(ctx, t, got) + requireReadyWhile(t, got, "DeletingOrphans") + require.Equal(t, 2, env.fake.requestCount(routeDeleteAIGatewayKey), "the sweep tries again") + for _, k := range env.fake.aiGatewayKeys() { + require.NotEqual(t, orphan.id, k.id) + } + }) + t.Run("retire", func(t *testing.T) { + t.Parallel() + env := newAIGatewayKeyEnv(ctx, t, ptrTo(true)) + got, old := retiringAfterDrift(ctx, t, &env) + env.clock.Step(time.Hour) + env.fake.failNext(routeDeleteAIGatewayKey, proxy404) + got, _ = env.reconcile(ctx, t, got) + requireReadyWhile(t, got, "CoderUnavailable") + require.Equal(t, []string{old.id.String()}, retiringIDs(got), "the key stays retiring") + got, _ = env.reconcile(ctx, t, got) + requireReadyWhile(t, got, "RetiringKey") + require.Empty(t, got.Status.RetiringKeys) + require.Len(t, env.fake.aiGatewayKeys(), 1) + }) +}