From ccc726e0ba03c70c88bbdcd3ab7a6fa5fc50f034 Mon Sep 17 00:00:00 2001 From: Josh Siviour Date: Sun, 2 Aug 2026 08:03:00 +1000 Subject: [PATCH] fix: close the LB name-claim orphan-reclaim race window (#631) ClaimLBName's crash-orphan reclaim treated any owner with no persisted LB record as abandoned, but a legitimate in-flight create has no record yet either during the gap between claiming the name and PutLoadBalancer. Two concurrent creates for the same name could both win. Track a claim timestamp and only allow reclaim once it exceeds a TTL, so a fresh in-flight claim blocks a second claimer like a live LB would, while a claim abandoned by a genuine crash is still reclaimed. --- spinifex/handlers/elbv2/name_claim_test.go | 52 ++++++++++++++++-- spinifex/handlers/elbv2/store.go | 62 ++++++++++++++++++---- 2 files changed, 100 insertions(+), 14 deletions(-) diff --git a/spinifex/handlers/elbv2/name_claim_test.go b/spinifex/handlers/elbv2/name_claim_test.go index 1da346e01..ac1cb7b81 100644 --- a/spinifex/handlers/elbv2/name_claim_test.go +++ b/spinifex/handlers/elbv2/name_claim_test.go @@ -4,6 +4,7 @@ import ( "context" "sync" "testing" + "time" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/elbv2" @@ -50,18 +51,40 @@ func TestCreateLoadBalancer_ConcurrentSameNameSingleOwner(t *testing.T) { require.NotNil(t, rec) } -// A name claim whose owner lbID resolves to no live record is a crashed prior -// create. A fresh CreateLoadBalancer with that name must reclaim the orphan and -// succeed, not wedge on DuplicateLoadBalancerName forever. +// TestClaimLBName_PendingClaimBlocksSecondClaimer isolates the orphan-reclaim +// window: a claim with no persisted LB record yet — the gap a real create +// leaves open — must not be stealable by a second claimer within the TTL. +func TestClaimLBName_PendingClaimBlocksSecondClaimer(t *testing.T) { + svc := setupTestService(t) + + ok1, dup1, err := svc.store.ClaimLBName(t.Context(), "pending-lb", testAccountID, "lb-first") + require.NoError(t, err) + require.True(t, ok1) + require.False(t, dup1) + + // lb-first has not persisted a record yet — the second claimer must not + // mistake that for a crash orphan. + ok2, dup2, err := svc.store.ClaimLBName(t.Context(), "pending-lb", testAccountID, "lb-second") + require.NoError(t, err) + assert.False(t, ok2, "a pending claim within its TTL must not be stealable") + assert.True(t, dup2, "the second claimer must see the name as unavailable") +} + +// A name claim whose owner resolves to no live record, past its TTL, is a +// crashed prior create. A fresh CreateLoadBalancer must reclaim it and succeed. +// The injectable clock ages the claim so the test never sleeps. func TestCreateLoadBalancer_ReclaimsCrashOrphanNameClaim(t *testing.T) { svc := setupTestService(t) // Seed a name claim pointing at a non-existent LB (crashed create left no - // record). ClaimLBName writes the key with this bogus owner. + // record), stamped as claimed well before the TTL via a fake clock. + realNow := svc.store.now + svc.store.now = func() time.Time { return realNow().Add(-2 * lbNameClaimTTL) } ok, dup, err := svc.store.ClaimLBName(t.Context(), "orphan-lb", testAccountID, "lb-doesnotexist") require.NoError(t, err) require.True(t, ok) require.False(t, dup) + svc.store.now = realNow // A fresh create for the same name reclaims the orphan and succeeds. out, err := svc.CreateLoadBalancer(context.Background(), &elbv2.CreateLoadBalancerInput{ @@ -93,3 +116,24 @@ func TestDeleteLoadBalancer_ReleasesNameForReuse(t *testing.T) { }, testAccountID) require.NoError(t, err) } + +// A create that claims the name but then fails before PutLoadBalancer (here, ENI +// provisioning on a nonexistent subnet) must release the claim on its way out, so +// a retry with the same name is not permanently locked out by the failed attempt. +func TestCreateLoadBalancer_FailureAfterClaimReleasesNameForRetry(t *testing.T) { + svc, _ := setupTestServiceWithVPC(t) + + _, err := svc.CreateLoadBalancer(context.Background(), &elbv2.CreateLoadBalancerInput{ + Name: aws.String("retry-lb"), + Subnets: []*string{aws.String("subnet-doesnotexist")}, + }, testAccountID) + require.Error(t, err, "ENI creation on a nonexistent subnet must fail") + + // The failed attempt released the claim: a retry with no subnets (nothing + // left to fail) succeeds under the same name. + out, err := svc.CreateLoadBalancer(context.Background(), &elbv2.CreateLoadBalancerInput{ + Name: aws.String("retry-lb"), + }, testAccountID) + require.NoError(t, err) + require.Len(t, out.LoadBalancers, 1) +} diff --git a/spinifex/handlers/elbv2/store.go b/spinifex/handlers/elbv2/store.go index f96550c95..b8453ecb4 100644 --- a/spinifex/handlers/elbv2/store.go +++ b/spinifex/handlers/elbv2/store.go @@ -8,6 +8,7 @@ import ( "log/slog" "sort" "strings" + "time" "github.com/mulgadc/spinifex/spinifex/kvutil" "github.com/mulgadc/spinifex/spinifex/migrate" @@ -32,6 +33,11 @@ const ( // maxLBNameClaimRetries bounds the crash-orphan CAS-reclaim loop in ClaimLBName. maxLBNameClaimRetries = 5 + + // lbNameClaimTTL bounds how long an unresolved claim blocks other creators + // before ClaimLBName treats it as a crash orphan. A legitimate create finishes + // well under this window; it only exists to reclaim an abandoned name. + lbNameClaimTTL = 5 * time.Minute ) // Store provides CRUD operations for ELBv2 resources backed by JetStream KV. @@ -40,6 +46,31 @@ const ( // internal wait the legacy KV API applied. type Store struct { kv jetstream.KeyValue + + // now and claimTTL back ClaimLBName's pending-vs-orphan check. Both are + // overridden in tests (same package) so the crash-orphan reclaim path never + // needs a real sleep past the TTL. + now func() time.Time + claimTTL time.Duration +} + +// lbNameClaim is the value stored at a name-claim key. ClaimedAt lets a second +// claimer distinguish a legitimate in-flight creator (no LB record yet, but the +// claim is fresh) from a crash orphan (no record, and the claim is stale). +type lbNameClaim struct { + OwnerID string `json:"ownerId"` + ClaimedAt time.Time `json:"claimedAt"` +} + +// decodeLBNameClaim parses a name-claim value. A value that isn't valid JSON +// predates ClaimedAt tracking (the bare owner lbID); treat it as maximally +// stale so it stays immediately reclaimable, matching the old behaviour. +func decodeLBNameClaim(raw []byte) lbNameClaim { + var claim lbNameClaim + if err := json.Unmarshal(raw, &claim); err != nil || claim.OwnerID == "" { + return lbNameClaim{OwnerID: string(raw)} + } + return claim } // NewStore creates a new ELBv2 store using the provided NATS connection. The @@ -61,7 +92,7 @@ func NewStore(ctx context.Context, nc *nats.Conn) (*Store, error) { } slog.Info("ELBv2 store initialized", "bucket", KVBucketELBv2) - return &Store{kv: kv}, nil + return &Store{kv: kv, now: time.Now, claimTTL: lbNameClaimTTL}, nil } // --- Load Balancer CRUD --- @@ -72,12 +103,17 @@ func LBNameKey(name, accountID string) string { } // ClaimLBName atomically claims the LB name; ok=true means this caller owns it, -// dup=true means a live LB already holds it. An orphaned claim (owner resolves to -// no record) is reclaimed via CAS. Idempotency barrier for CreateLoadBalancer. +// dup=true means it's unavailable (a live LB, or another claim within its TTL). +// A claim past its TTL with no LB record is a crash orphan, reclaimed via CAS. func (s *Store) ClaimLBName(ctx context.Context, name, accountID, lbID string) (ok bool, dup bool, err error) { key := LBNameKey(name, accountID) + now := s.now() + claimBytes, merr := json.Marshal(lbNameClaim{OwnerID: lbID, ClaimedAt: now}) + if merr != nil { + return false, false, fmt.Errorf("marshal LB name claim %s: %w", key, merr) + } for range maxLBNameClaimRetries { - if _, cerr := s.kv.Create(ctx, key, []byte(lbID)); cerr == nil { + if _, cerr := s.kv.Create(ctx, key, claimBytes); cerr == nil { return true, false, nil } else if !errors.Is(cerr, jetstream.ErrKeyExists) { return false, false, fmt.Errorf("kv create %s: %w", key, cerr) @@ -89,18 +125,24 @@ func (s *Store) ClaimLBName(ctx context.Context, name, accountID, lbID string) ( } return false, false, fmt.Errorf("kv get %s: %w", key, gerr) } - ownerID := string(entry.Value()) - if ownerID != "" && ownerID != lbID { - rec, rerr := s.GetLoadBalancer(ctx, ownerID) + claim := decodeLBNameClaim(entry.Value()) + if claim.OwnerID != "" && claim.OwnerID != lbID { + rec, rerr := s.GetLoadBalancer(ctx, claim.OwnerID) if rerr != nil { - return false, false, fmt.Errorf("resolve LB name owner %s: %w", ownerID, rerr) + return false, false, fmt.Errorf("resolve LB name owner %s: %w", claim.OwnerID, rerr) } if rec != nil { return false, true, nil // live LB holds the name } + // No record yet: either a legitimate create still mid-flight, or a + // crashed one. Only a claim older than the TTL is an abandoned + // orphan; a fresh one blocks a second claimer like a live LB would. + if now.Sub(claim.ClaimedAt) < s.claimTTL { + return false, true, nil + } } - // Orphaned (crashed prior create) or already ours: CAS-take the claim. - if _, uerr := s.kv.Update(ctx, key, []byte(lbID), entry.Revision()); uerr != nil { + // Orphaned (crashed prior create, past its TTL) or already ours: CAS-take. + if _, uerr := s.kv.Update(ctx, key, claimBytes, entry.Revision()); uerr != nil { if errors.Is(uerr, jetstream.ErrKeyExists) { continue // lost the CAS race; re-read }