gsadmin/spinifex
1
0
Fork 0
mirror of https://github.com/mulgadc/spinifex.git synced 2026-10-15 17:11:30 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: close the LB name-claim orphan-reclaim race window (#631)

Browse Source 
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.
This commit is contained in:
Josh Siviour
2026-08-02 08:03:00 +10:00
committed by GitHub
parent ee0bf68d71
commit ccc726e0ba
2 changed files with 100 additions and 14 deletions
Download Patch File Download Diff File Expand all files Collapse all files
spinifex/handlers/elbv2/name_claim_test.go
+48 -4
View File
@@ -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)
}
spinifex/handlers/elbv2/store.go
+52 -10
View File
@@ -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
}