🤖 fix: trust only Coder's own 404 when deleting AI Gateway keys

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
 }

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: "<html>404</html>"})
+			},
+			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())
 			}

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")
+}

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

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: "<html>404</html>"}
+	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)
+	})
+}