From dca7a3835686cec9d23cffe6bcac23508434e6ff Mon Sep 17 00:00:00 2001 From: Ben Duncan <5178243+benduncan@users.noreply.github.com> Date: Thu, 20 Aug 2026 01:52:07 +1000 Subject: [PATCH 1/8] feat: resolve service quotas per account from a KV override record --- ...ttings.local.json.tmp.2466042.87acb86176ea | 25 +++ cmd/spinifex/cmd/templates/awsgw.toml | 14 +- spinifex/gateway/elbv2.go | 3 + spinifex/gateway/rds.go | 8 +- spinifex/gateway/rds/handler.go | 7 +- spinifex/gateway/rds/instance.go | 7 +- spinifex/handlers/quota/live.go | 40 ++++- spinifex/handlers/quota/live_test.go | 12 ++ spinifex/handlers/quota/override.go | 131 ++++++++++++++++ spinifex/handlers/quota/override_kv_test.go | 124 +++++++++++++++ spinifex/handlers/quota/override_test.go | 145 ++++++++++++++++++ spinifex/handlers/quota/quota.go | 28 ++++ spinifex/handlers/quota/storage.go | 13 +- spinifex/handlers/quota/vcpu.go | 5 +- spinifex/services/awsgw/awsgw.go | 33 +++- 15 files changed, 577 insertions(+), 18 deletions(-) create mode 100644 .claude/settings.local.json.tmp.2466042.87acb86176ea create mode 100644 spinifex/handlers/quota/override.go create mode 100644 spinifex/handlers/quota/override_kv_test.go create mode 100644 spinifex/handlers/quota/override_test.go diff --git a/.claude/settings.local.json.tmp.2466042.87acb86176ea b/.claude/settings.local.json.tmp.2466042.87acb86176ea new file mode 100644 index 000000000..8d1eb5d6c --- /dev/null +++ b/.claude/settings.local.json.tmp.2466042.87acb86176ea @@ -0,0 +1,25 @@ +{ + "permissions": { + "allow": [ + "Bash(timeout 30 bd ready)", + "Bash(python3 -c ' *)", + "Bash(timeout 30 bd label list)", + "Bash(timeout 30 bd labels)", + "Bash(git fetch *)", + "Bash(git checkout *)", + "Bash(grep -n \"^#\\\\{2,3\\\\} \" docs/development/improvements/new-multi-node-networking.md)", + "Bash(go version *)", + "Read(//home/tf-user/go/pkg/mod/golang.org/**)", + "Read(//usr/local/go/**)", + "Bash(go build *)", + "Bash(go test *)", + "Bash(go vet *)", + "Bash(python3 -)", + "Bash(bash -n scripts/iso-builder/build/spinifex-init.sh)", + "Bash(timeout 900 make preflight)", + "Bash(timeout 300 golangci-lint run ./cmd/installer/...)", + "Bash(timeout 300 golangci-lint run ./...)", + "Bash(git stash *)" + ] + } +} diff --git a/cmd/spinifex/cmd/templates/awsgw.toml b/cmd/spinifex/cmd/templates/awsgw.toml index f23c04369..022c4bb33 100644 --- a/cmd/spinifex/cmd/templates/awsgw.toml +++ b/cmd/spinifex/cmd/templates/awsgw.toml @@ -40,11 +40,17 @@ burst = 5 [signup] max_accounts = 128 -# Per-account service quotas (default off) +# Per-account service quotas (default off). These values are the baseline every +# account inherits; raise an individual account with +# `spx admin account quota set`, which stores an override in NATS KV. +# A dimension set to -1 is uncapped for accounts that inherit it. [quota] enabled = false -vcpus = 8 -vpcs = 8 +vcpus = 16 +vpcs = 4 subnets = 16 eips = 4 -volumes_gib = 100 +volumes = 16 +volumes_gib = 200 +rds_instances = 2 +load_balancers = 2 diff --git a/spinifex/gateway/elbv2.go b/spinifex/gateway/elbv2.go index d46eca715..03db2050f 100644 --- a/spinifex/gateway/elbv2.go +++ b/spinifex/gateway/elbv2.go @@ -44,6 +44,9 @@ func elbv2Handler[In any](handler func(context.Context, *In, *GatewayConfig, str var elbv2Actions = map[string]ELBv2Handler{ "CreateLoadBalancer": elbv2Handler(func(ctx context.Context, input *elbv2.CreateLoadBalancerInput, gw *GatewayConfig, accountID string) (any, error) { + if err := gw.Quota.EnforceLoadBalancers(ctx, gw.NATSConn, accountID, 1); err != nil { + return nil, err + } return gateway_elbv2.CreateLoadBalancer(ctx, input, gw.NATSConn, accountID) }), "DeleteLoadBalancer": elbv2Handler(func(ctx context.Context, input *elbv2.DeleteLoadBalancerInput, gw *GatewayConfig, accountID string) (any, error) { diff --git a/spinifex/gateway/rds.go b/spinifex/gateway/rds.go index f1d69d5d5..b03b9e139 100644 --- a/spinifex/gateway/rds.go +++ b/spinifex/gateway/rds.go @@ -1,6 +1,7 @@ package gateway import ( + "context" "errors" "log/slog" "net/http" @@ -55,7 +56,12 @@ func (gw *GatewayConfig) RDS_Request(w http.ResponseWriter, r *http.Request) err } xmlOutput, err := gateway_rds.Dispatch(r.Context(), action, queryArgs, gw.NATSConn, caller, - gateway_rds.Env{ExpectedNodes: gw.ExpectedNodes}) + gateway_rds.Env{ + ExpectedNodes: gw.ExpectedNodes, + QuotaCheck: func(ctx context.Context, accountID string, want int) error { + return gw.Quota.EnforceRDSInstances(ctx, gw.NATSConn, accountID, want) + }, + }) if err != nil { return err } diff --git a/spinifex/gateway/rds/handler.go b/spinifex/gateway/rds/handler.go index e50f9914e..fb9d7c4f8 100644 --- a/spinifex/gateway/rds/handler.go +++ b/spinifex/gateway/rds/handler.go @@ -36,6 +36,11 @@ type Env struct { // How many nodes a fan-out waits for before it stops waiting. Without it a // gather burns its full timeout on every call instead of early-exiting. ExpectedNodes int + + // QuotaCheck gates a create on the account's DB-instance cap. It is a + // function so this package stays free of the quota dependency; nil skips + // the check, which is what a gateway with quotas disabled supplies. + QuotaCheck func(ctx context.Context, accountID string, want int) error } type Handler func(ctx context.Context, action string, q map[string]string, nc *nats.Conn, caller Caller, env Env) ([]byte, error) @@ -94,7 +99,7 @@ type actionDef struct { // distinct from an unknown one. var actions = map[string]actionDef{ // Instance lifecycle. - "CreateDBInstance": {handler: typed(CreateDBInstance)}, + "CreateDBInstance": {handler: typedEnv(CreateDBInstance)}, "DescribeDBInstances": {handler: typed(DescribeDBInstances)}, "ModifyDBInstance": {handler: typed(ModifyDBInstance), scope: dbInstanceScope}, "DeleteDBInstance": {handler: typed(DeleteDBInstance), scope: dbInstanceScope}, diff --git a/spinifex/gateway/rds/instance.go b/spinifex/gateway/rds/instance.go index f832cba58..d176ee860 100644 --- a/spinifex/gateway/rds/instance.go +++ b/spinifex/gateway/rds/instance.go @@ -10,7 +10,12 @@ import ( // The whole orchestration runs on the daemon side, so this only carries the // caller's account through — the request body never names the owner. -func CreateDBInstance(ctx context.Context, input *rds.CreateDBInstanceInput, nc *nats.Conn, caller Caller) (any, error) { +func CreateDBInstance(ctx context.Context, input *rds.CreateDBInstanceInput, nc *nats.Conn, caller Caller, env Env) (any, error) { + if env.QuotaCheck != nil { + if err := env.QuotaCheck(ctx, caller.AccountID, 1); err != nil { + return nil, err + } + } return handlers_rds.NewNATSService(nc).CreateDBInstance(ctx, input, caller.AccountID) } diff --git a/spinifex/handlers/quota/live.go b/spinifex/handlers/quota/live.go index 0ed68b20f..62ac56f0e 100644 --- a/spinifex/handlers/quota/live.go +++ b/spinifex/handlers/quota/live.go @@ -5,9 +5,13 @@ import ( "errors" "github.com/aws/aws-sdk-go/service/ec2" + "github.com/aws/aws-sdk-go/service/elbv2" + "github.com/aws/aws-sdk-go/service/rds" "github.com/mulgadc/spinifex/spinifex/awserrors" gateway_ec2_eip "github.com/mulgadc/spinifex/spinifex/gateway/ec2/eip" gateway_ec2_vpc "github.com/mulgadc/spinifex/spinifex/gateway/ec2/vpc" + gateway_elbv2 "github.com/mulgadc/spinifex/spinifex/gateway/elbv2" + handlers_rds "github.com/mulgadc/spinifex/spinifex/handlers/rds" "github.com/nats-io/nats.go" ) @@ -16,6 +20,9 @@ import ( // the shared comparison for every live dimension; the per-dimension methods // supply count from the relevant Describe* call. func exceeds(count, want, limit int) error { + if limit == Unlimited { + return nil + } if count+want > limit { return errors.New(awserrors.ErrorResourceLimitExceeded) } @@ -31,7 +38,7 @@ func (s *Service) EnforceVPCs(ctx context.Context, natsConn *nats.Conn, accountI if err != nil { return err } - return exceeds(len(out.Vpcs), want, s.limits.VPCs) + return exceeds(len(out.Vpcs), want, s.limitsFor(ctx, accountID).VPCs) } // EnforceSubnets gates CreateSubnet on the account's live DescribeSubnets count. @@ -44,7 +51,7 @@ func (s *Service) EnforceSubnets(ctx context.Context, natsConn *nats.Conn, accou if err != nil { return err } - return exceeds(len(out.Subnets), want, s.limits.Subnets) + return exceeds(len(out.Subnets), want, s.limitsFor(ctx, accountID).Subnets) } // EnforceEIPs gates AllocateAddress on the account's live DescribeAddresses count. @@ -56,5 +63,32 @@ func (s *Service) EnforceEIPs(ctx context.Context, natsConn *nats.Conn, accountI if err != nil { return err } - return exceeds(len(out.Addresses), want, s.limits.EIPs) + return exceeds(len(out.Addresses), want, s.limitsFor(ctx, accountID).EIPs) +} + +// EnforceLoadBalancers gates CreateLoadBalancer on the account's live count. +// ALBs and NLBs share one cap, as both consume a load-balancer appliance VM. +func (s *Service) EnforceLoadBalancers(ctx context.Context, natsConn *nats.Conn, accountID string, want int) error { + if s.Exempt(accountID) { + return nil + } + out, err := gateway_elbv2.DescribeLoadBalancers(ctx, &elbv2.DescribeLoadBalancersInput{}, natsConn, accountID) + if err != nil { + return err + } + return exceeds(len(out.LoadBalancers), want, s.limitsFor(ctx, accountID).LoadBalancers) +} + +// EnforceRDSInstances gates CreateDBInstance on the account's live count. A DB +// instance is capped by count rather than by vCPU because its VM runs in the +// system account and never reaches the tenant's vCPU counter. +func (s *Service) EnforceRDSInstances(ctx context.Context, natsConn *nats.Conn, accountID string, want int) error { + if s.Exempt(accountID) { + return nil + } + out, err := handlers_rds.NewNATSService(natsConn).DescribeDBInstances(ctx, &rds.DescribeDBInstancesInput{}, accountID) + if err != nil { + return err + } + return exceeds(len(out.DBInstances), want, s.limitsFor(ctx, accountID).RDSInstances) } diff --git a/spinifex/handlers/quota/live_test.go b/spinifex/handlers/quota/live_test.go index b7c2c00da..020ee23e9 100644 --- a/spinifex/handlers/quota/live_test.go +++ b/spinifex/handlers/quota/live_test.go @@ -57,6 +57,18 @@ func TestEnforceLiveExemptShortCircuits(t *testing.T) { {"vpc system account", func() error { return enabled.EnforceVPCs(context.Background(), nil, utils.GlobalAccountID, 1) }}, {"subnet system account", func() error { return enabled.EnforceSubnets(context.Background(), nil, utils.GlobalAccountID, 1) }}, {"eip system account", func() error { return enabled.EnforceEIPs(context.Background(), nil, utils.GlobalAccountID, 1) }}, + {"lb disabled", func() error { + return disabled.EnforceLoadBalancers(context.Background(), nil, normalAccount, 1) + }}, + {"lb system account", func() error { + return enabled.EnforceLoadBalancers(context.Background(), nil, utils.GlobalAccountID, 1) + }}, + {"rds disabled", func() error { + return disabled.EnforceRDSInstances(context.Background(), nil, normalAccount, 1) + }}, + {"rds system account", func() error { + return enabled.EnforceRDSInstances(context.Background(), nil, utils.GlobalAccountID, 1) + }}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { diff --git a/spinifex/handlers/quota/override.go b/spinifex/handlers/quota/override.go new file mode 100644 index 000000000..a9ac5bd3e --- /dev/null +++ b/spinifex/handlers/quota/override.go @@ -0,0 +1,131 @@ +package handlers_quota + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "log/slog" + "time" + + "github.com/nats-io/nats.go/jetstream" +) + +// Overrides raises or lowers individual dimensions for one account. Every field +// is a pointer so an explicit 0 (a limit that denies everything) is distinct +// from an absent field, which inherits the configured value. +type Overrides struct { + VCPUs *int `json:"vcpus,omitempty"` + VPCs *int `json:"vpcs,omitempty"` + Subnets *int `json:"subnets,omitempty"` + EIPs *int `json:"eips,omitempty"` + VolumesGiB *int `json:"volumes_gib,omitempty"` + Volumes *int `json:"volumes,omitempty"` + RDSInstances *int `json:"rds_instances,omitempty"` + LoadBalancers *int `json:"load_balancers,omitempty"` + + // Raising a customer's quota is a commercial act, so it is attributable. + UpdatedAt string `json:"updated_at,omitempty"` + UpdatedBy string `json:"updated_by,omitempty"` +} + +// Empty reports whether no dimension is overridden, in which case the record +// carries nothing but provenance and the account resolves to the config. +func (o Overrides) Empty() bool { + return o.VCPUs == nil && o.VPCs == nil && o.Subnets == nil && o.EIPs == nil && + o.VolumesGiB == nil && o.Volumes == nil && o.RDSInstances == nil && o.LoadBalancers == nil +} + +// apply overlays the set fields onto base, leaving Enabled and the Bedrock +// dimensions alone: enablement is a cluster-wide switch, never per-account. +func (o Overrides) apply(base Limits) Limits { + out := base + for _, f := range []struct { + src *int + dst *int + }{ + {o.VCPUs, &out.VCPUs}, + {o.VPCs, &out.VPCs}, + {o.Subnets, &out.Subnets}, + {o.EIPs, &out.EIPs}, + {o.VolumesGiB, &out.VolumesGiB}, + {o.Volumes, &out.Volumes}, + {o.RDSInstances, &out.RDSInstances}, + {o.LoadBalancers, &out.LoadBalancers}, + } { + if f.src != nil { + *f.dst = *f.src + } + } + return out +} + +// limitsFor resolves the effective limits for accountID: its overrides where +// set, the configured limits everywhere else. A read failure falls back to the +// configured limits rather than to no limit, so a KV outage cannot lift a cap. +func (s *Service) limitsFor(ctx context.Context, accountID string) Limits { + over, err := s.overridesFor(ctx, accountID) + if err != nil { + slog.WarnContext(ctx, "quota: override read failed, using configured limits", + "account", accountID, "err", err) + return s.limits + } + return over.apply(s.limits) +} + +// overridesFor reads accountID's override record. An absent key is the normal +// case for nearly every account and yields the zero value, not an error; only a +// genuine read or decode failure is returned. +func (s *Service) overridesFor(ctx context.Context, accountID string) (Overrides, error) { + if s == nil || s.overrides == nil { + return Overrides{}, nil + } + entry, err := s.overrides.Get(ctx, accountID) + if errors.Is(err, jetstream.ErrKeyNotFound) { + return Overrides{}, nil + } + if err != nil { + return Overrides{}, fmt.Errorf("read quota override for %s: %w", accountID, err) + } + var over Overrides + if err := json.Unmarshal(entry.Value(), &over); err != nil { + return Overrides{}, fmt.Errorf("decode quota override for %s: %w", accountID, err) + } + return over, nil +} + +// GetAccountQuota returns accountID's stored overrides and the limits they +// resolve to. A read failure is surfaced here rather than masked, because an +// operator inspecting a quota must not be shown the fallback as if it were real. +func (s *Service) GetAccountQuota(ctx context.Context, accountID string) (Overrides, Limits, error) { + over, err := s.overridesFor(ctx, accountID) + if err != nil { + return Overrides{}, Limits{}, err + } + return over, over.apply(s.limits), nil +} + +// PutAccountQuota stores accountID's overrides, stamping provenance. An empty +// set deletes the record so the account resolves to the config, which keeps +// "has this account been touched?" a single existence check. +func (s *Service) PutAccountQuota(ctx context.Context, accountID string, over Overrides, updatedBy string) error { + if s == nil || s.overrides == nil { + return errors.New("quota: override bucket not configured") + } + if over.Empty() { + if err := s.overrides.Delete(ctx, accountID); err != nil && !errors.Is(err, jetstream.ErrKeyNotFound) { + return fmt.Errorf("clear quota override for %s: %w", accountID, err) + } + return nil + } + over.UpdatedAt = time.Now().UTC().Format(time.RFC3339) + over.UpdatedBy = updatedBy + data, err := json.Marshal(over) + if err != nil { + return fmt.Errorf("encode quota override for %s: %w", accountID, err) + } + if _, err := s.overrides.Put(ctx, accountID, data); err != nil { + return fmt.Errorf("store quota override for %s: %w", accountID, err) + } + return nil +} diff --git a/spinifex/handlers/quota/override_kv_test.go b/spinifex/handlers/quota/override_kv_test.go new file mode 100644 index 000000000..62fb664d9 --- /dev/null +++ b/spinifex/handlers/quota/override_kv_test.go @@ -0,0 +1,124 @@ +package handlers_quota + +import ( + "testing" + + "github.com/mulgadc/spinifex/spinifex/testutil" + "github.com/nats-io/nats.go/jetstream" + "github.com/stretchr/testify/require" +) + +// newOverrideService builds a Service backed by a real JetStream KV bucket, so +// the resolver is exercised against the store it uses in production rather than +// a stand-in that cannot reproduce a read failure. +func newOverrideService(t *testing.T) (*Service, jetstream.KeyValue) { + t.Helper() + _, _, js := testutil.StartTestJetStream(t) + kv, err := js.CreateKeyValue(t.Context(), jetstream.KeyValueConfig{ + Bucket: KVBucketAccountQuota, + History: 1, + }) + require.NoError(t, err) + + svc := New(baseLimits(), nil) + svc.SetOverrides(kv) + return svc, kv +} + +// An account with no record is the normal state, and it must inherit the +// configured baseline without erroring. +func TestLimitsForAbsentKeyInherits(t *testing.T) { + svc, _ := newOverrideService(t) + + got := svc.limitsFor(t.Context(), "123456789012") + require.Equal(t, baseLimits(), got) +} + +// The property the whole tier rests on: two accounts on one gateway resolve to +// different limits. +func TestLimitsForIsPerAccount(t *testing.T) { + svc, _ := newOverrideService(t) + const upgraded, sandbox = "000000000002", "000000000003" + + require.NoError(t, svc.PutAccountQuota(t.Context(), upgraded, + Overrides{VCPUs: ptr(32), RDSInstances: ptr(8)}, "operator")) + + require.Equal(t, 32, svc.limitsFor(t.Context(), upgraded).VCPUs) + require.Equal(t, 8, svc.limitsFor(t.Context(), upgraded).RDSInstances) + require.Equal(t, baseLimits().VCPUs, svc.limitsFor(t.Context(), sandbox).VCPUs) + require.Equal(t, baseLimits().RDSInstances, svc.limitsFor(t.Context(), sandbox).RDSInstances) +} + +// A stored override must round-trip through KV with its provenance, and an +// unset dimension must still inherit. +func TestPutAndGetAccountQuota(t *testing.T) { + svc, _ := newOverrideService(t) + const account = "000000000002" + + require.NoError(t, svc.PutAccountQuota(t.Context(), account, Overrides{EIPs: ptr(12)}, "operator")) + + over, effective, err := svc.GetAccountQuota(t.Context(), account) + require.NoError(t, err) + require.NotNil(t, over.EIPs) + require.Equal(t, 12, *over.EIPs) + require.Equal(t, "operator", over.UpdatedBy) + require.NotEmpty(t, over.UpdatedAt) + require.Equal(t, 12, effective.EIPs) + require.Equal(t, baseLimits().VCPUs, effective.VCPUs, "unset dimension must inherit") +} + +// Clearing must delete the record rather than leave an empty one, so "has this +// account been touched?" stays a single existence check. +func TestPutAccountQuotaEmptyClears(t *testing.T) { + svc, kv := newOverrideService(t) + const account = "000000000002" + + require.NoError(t, svc.PutAccountQuota(t.Context(), account, Overrides{VCPUs: ptr(32)}, "operator")) + require.NoError(t, svc.PutAccountQuota(t.Context(), account, Overrides{}, "operator")) + + _, err := kv.Get(t.Context(), account) + require.ErrorIs(t, err, jetstream.ErrKeyNotFound) + require.Equal(t, baseLimits(), svc.limitsFor(t.Context(), account)) +} + +// Clearing an account that never had a record must succeed rather than fail on +// the missing key. +func TestPutAccountQuotaEmptyOnAbsentKey(t *testing.T) { + svc, _ := newOverrideService(t) + require.NoError(t, svc.PutAccountQuota(t.Context(), "000000000009", Overrides{}, "operator")) +} + +// An explicit zero must survive the store and deny every request, which is the +// difference between a limit of zero and an absent override. +func TestPutAccountQuotaExplicitZero(t *testing.T) { + svc, _ := newOverrideService(t) + const account = "000000000002" + + require.NoError(t, svc.PutAccountQuota(t.Context(), account, Overrides{VPCs: ptr(0)}, "operator")) + + require.Equal(t, 0, svc.limitsFor(t.Context(), account).VPCs) + require.Error(t, exceeds(0, 1, svc.limitsFor(t.Context(), account).VPCs)) +} + +// A corrupt record is a read failure, not an absent one: it must fall back to +// the configured limits rather than to no limit at all. +func TestLimitsForCorruptRecordFailsClosed(t *testing.T) { + svc, kv := newOverrideService(t) + const account = "000000000002" + + _, err := kv.Put(t.Context(), account, []byte("{not json")) + require.NoError(t, err) + + require.Equal(t, baseLimits(), svc.limitsFor(t.Context(), account), + "a corrupt override must fall back to the configured limits") + + _, _, err = svc.GetAccountQuota(t.Context(), account) + require.Error(t, err, "an operator inspecting a quota must see the failure, not the fallback") +} + +// A gateway with no override bucket must refuse a write rather than silently +// accept one it cannot store. +func TestPutAccountQuotaWithoutBucket(t *testing.T) { + svc := New(baseLimits(), nil) + require.Error(t, svc.PutAccountQuota(t.Context(), "000000000002", Overrides{VCPUs: ptr(32)}, "operator")) +} diff --git a/spinifex/handlers/quota/override_test.go b/spinifex/handlers/quota/override_test.go new file mode 100644 index 000000000..77ce8ae0f --- /dev/null +++ b/spinifex/handlers/quota/override_test.go @@ -0,0 +1,145 @@ +package handlers_quota + +import ( + "encoding/json" + "testing" +) + +func ptr(v int) *int { return &v } + +// baseLimits is the sandbox baseline every account inherits when it has no +// override, mirroring the shipped [quota] block. +func baseLimits() Limits { + return Limits{ + Enabled: true, + VCPUs: 16, + VPCs: 4, + Subnets: 16, + EIPs: 4, + Volumes: 16, + VolumesGiB: 200, + RDSInstances: 2, + LoadBalancers: 2, + } +} + +// An override replaces only the dimensions it sets; every other dimension must +// come through from the configured baseline untouched. +func TestOverridesApplyIsSparse(t *testing.T) { + got := Overrides{VCPUs: ptr(32), RDSInstances: ptr(8)}.apply(baseLimits()) + + if got.VCPUs != 32 { + t.Errorf("VCPUs = %d, want 32", got.VCPUs) + } + if got.RDSInstances != 8 { + t.Errorf("RDSInstances = %d, want 8", got.RDSInstances) + } + want := baseLimits() + if got.VPCs != want.VPCs || got.EIPs != want.EIPs || got.Volumes != want.Volumes || + got.VolumesGiB != want.VolumesGiB || got.Subnets != want.Subnets || + got.LoadBalancers != want.LoadBalancers { + t.Errorf("unset dimensions changed: got %+v, want %+v", got, want) + } +} + +// An empty override must leave the baseline exactly as it is: this is the state +// of nearly every account and the reason no migration is needed. +func TestOverridesApplyEmptyInherits(t *testing.T) { + if got := (Overrides{}).apply(baseLimits()); got != baseLimits() { + t.Fatalf("empty override = %+v, want %+v", got, baseLimits()) + } +} + +// Enablement is a cluster-wide switch, so no override may turn quotas off for +// one account. +func TestOverridesCannotDisableEnforcement(t *testing.T) { + if got := (Overrides{VCPUs: ptr(0)}).apply(baseLimits()); !got.Enabled { + t.Fatal("override cleared Enabled, want it preserved") + } +} + +// Zero is a real limit that denies every request, so it must survive a JSON +// round trip rather than marshalling away and silently meaning "inherit". +func TestOverridesZeroSurvivesRoundTrip(t *testing.T) { + data, err := json.Marshal(Overrides{VCPUs: ptr(0)}) + if err != nil { + t.Fatalf("marshal: %v", err) + } + + var back Overrides + if err := json.Unmarshal(data, &back); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if back.VCPUs == nil { + t.Fatalf("explicit 0 was dropped by the round trip: %s", data) + } + if *back.VCPUs != 0 { + t.Fatalf("VCPUs = %d, want 0", *back.VCPUs) + } + if got := back.apply(baseLimits()); got.VCPUs != 0 { + t.Fatalf("applied VCPUs = %d, want 0", got.VCPUs) + } +} + +// An absent field must stay absent through a round trip, so inheritance is not +// silently converted into an explicit zero. +func TestOverridesAbsentStaysAbsent(t *testing.T) { + data, err := json.Marshal(Overrides{VPCs: ptr(8)}) + if err != nil { + t.Fatalf("marshal: %v", err) + } + + var back Overrides + if err := json.Unmarshal(data, &back); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if back.VCPUs != nil { + t.Fatalf("unset VCPUs decoded as %d, want nil", *back.VCPUs) + } + if back.apply(baseLimits()).VCPUs != baseLimits().VCPUs { + t.Fatal("unset VCPUs did not inherit the baseline") + } +} + +func TestOverridesEmpty(t *testing.T) { + tests := []struct { + name string + over Overrides + want bool + }{ + {"zero value", Overrides{}, true}, + {"provenance only", Overrides{UpdatedBy: "operator"}, true}, + {"one dimension", Overrides{EIPs: ptr(1)}, false}, + {"explicit zero", Overrides{VCPUs: ptr(0)}, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := tt.over.Empty(); got != tt.want { + t.Fatalf("Empty() = %v, want %v", got, tt.want) + } + }) + } +} + +// A nil override bucket is what a quota-disabled gateway holds; resolution must +// fall through to the configured limits rather than panicking. +func TestLimitsForWithoutBucket(t *testing.T) { + svc := New(baseLimits(), nil) + if got := svc.limitsFor(t.Context(), "123456789012"); got != baseLimits() { + t.Fatalf("limitsFor = %+v, want the configured limits", got) + } +} + +// The Unlimited sentinel disables one dimension for one account, which zero +// cannot express because zero is a real limit that denies everything. +func TestExceedsUnlimited(t *testing.T) { + if err := exceeds(9000, 9000, Unlimited); err != nil { + t.Fatalf("exceeds with Unlimited = %v, want nil", err) + } + if err := exceeds(0, 1, 0); err == nil { + t.Fatal("a zero cap allowed a request, want it denied") + } + if err := exceeds(0, 0, 0); err != nil { + t.Fatalf("a zero cap denied a zero request: %v", err) + } +} diff --git a/spinifex/handlers/quota/quota.go b/spinifex/handlers/quota/quota.go index 43664eabe..ef0267980 100644 --- a/spinifex/handlers/quota/quota.go +++ b/spinifex/handlers/quota/quota.go @@ -25,6 +25,15 @@ const KVBucketAccountUsage = "spinifex-account-usage" // the two independent loops never share one mutex and block each other. const KVBucketQuotaReconcile = "spinifex-quota-reconcile" +// KVBucketAccountQuota holds the per-account limit overrides, keyed by +// accountID. It is separate from the accounts bucket so a quota write never +// contends with an identity write on the same revision. +const KVBucketAccountQuota = "spinifex-account-quota" + +// Unlimited disables one dimension for one account. Zero is a real limit that +// denies every request, so "no cap" needs a value of its own. +const Unlimited = -1 + // Limits mirrors the [quota] block in awsgw.toml. The zero value (Enabled // false) is a valid no-op, so gateways without a [quota] block are unaffected. // @@ -43,6 +52,12 @@ type Limits struct { EIPs int `toml:"eips"` VolumesGiB int `toml:"volumes_gib"` + // Volumes caps the number of EBS volumes, independently of VolumesGiB: a + // tenant can exhaust per-volume overhead long before it exhausts capacity. + Volumes int `toml:"volumes"` + RDSInstances int `toml:"rds_instances"` + LoadBalancers int `toml:"load_balancers"` + TokensPerMonthEnabled bool `toml:"tokens_per_month_enabled"` TokensPerMonth int64 `toml:"tokens_per_month"` RequestsPerMinuteEnabled bool `toml:"requests_per_minute_enabled"` @@ -69,6 +84,10 @@ type Service struct { // the requests-per-minute dimension. Always non-nil so CheckBedrockRPM // never needs a nil guard beyond Service itself. rpm *rpmLimiter + + // overrides holds the per-account limit overrides. Nil leaves every account + // on the configured limits, which is also what an absent key means. + overrides jetstream.KeyValue } // New constructs a quota Service from the configured limits and the gateway-owned @@ -78,6 +97,15 @@ func New(limits Limits, usage jetstream.KeyValue) *Service { return &Service{limits: limits, usage: usage, rpm: newRPMLimiter()} } +// SetOverrides attaches the per-account override bucket. Called once at wiring +// time; a nil Service or bucket leaves every account on the configured limits. +func (s *Service) SetOverrides(kv jetstream.KeyValue) { + if s == nil { + return + } + s.overrides = kv +} + // Exempt returns true for the global/system account and whenever quotas are // disabled, so those callers bypass every quota check. A nil Service (quotas // never built) is treated as disabled so callers need no nil guard. diff --git a/spinifex/handlers/quota/storage.go b/spinifex/handlers/quota/storage.go index ac2e4c3d4..698c7019e 100644 --- a/spinifex/handlers/quota/storage.go +++ b/spinifex/handlers/quota/storage.go @@ -13,15 +13,22 @@ import ( // VolumesGiB cap. Storage is live-counted, so a delete frees quota on the next // describe. Root/AMI volumes are created out-of-band by the launch path and // never appear in DescribeVolumes, so they are neither summed nor charged. +// The volume count is checked alongside the capacity sum: a tenant can exhaust +// per-volume overhead long before it exhausts GiB. func (s *Service) EnforceVolumeCreate(ctx context.Context, natsConn *nats.Conn, accountID string, requestedGiB int) error { if s.Exempt(accountID) { return nil } - total, _, err := s.volumeUsage(ctx, natsConn, accountID, "") + out, err := gateway_ec2_volume.DescribeVolumes(ctx, &ec2.DescribeVolumesInput{}, natsConn, accountID) if err != nil { return err } - return exceeds(total, requestedGiB, s.limits.VolumesGiB) + limits := s.limitsFor(ctx, accountID) + if err := exceeds(len(out.Volumes), 1, limits.Volumes); err != nil { + return err + } + total, _ := sumVolumeGiB(out.Volumes, "") + return exceeds(total, requestedGiB, limits.VolumesGiB) } // EnforceVolumeModify gates ModifyVolume on the account's live storage usage. @@ -36,7 +43,7 @@ func (s *Service) EnforceVolumeModify(ctx context.Context, natsConn *nats.Conn, if err != nil { return err } - return exceeds(total-oldGiB, newGiB, s.limits.VolumesGiB) + return exceeds(total-oldGiB, newGiB, s.limitsFor(ctx, accountID).VolumesGiB) } // volumeUsage sums the account's volume sizes in GiB via a single DescribeVolumes diff --git a/spinifex/handlers/quota/vcpu.go b/spinifex/handlers/quota/vcpu.go index 62b6a4d5d..a93cf9ed0 100644 --- a/spinifex/handlers/quota/vcpu.go +++ b/spinifex/handlers/quota/vcpu.go @@ -35,10 +35,7 @@ func (s *Service) CheckVCPU(ctx context.Context, accountID string, want int) err if err != nil { return err } - if current+want > s.limits.VCPUs { - return errors.New(awserrors.ErrorResourceLimitExceeded) - } - return nil + return exceeds(current, want, s.limitsFor(ctx, accountID).VCPUs) } // casVCPU applies next to accountID's counter under bounded JetStream CAS so diff --git a/spinifex/services/awsgw/awsgw.go b/spinifex/services/awsgw/awsgw.go index ba6c7e3ac..c72c7c16e 100644 --- a/spinifex/services/awsgw/awsgw.go +++ b/spinifex/services/awsgw/awsgw.go @@ -166,6 +166,32 @@ func openAccountUsageBucket(ctx context.Context, js jetstream.KeyValueManager, r return kv, nil } +// openAccountQuotaBucket opens (or idempotently creates) the per-account quota +// override bucket. History is 1: each key holds the current override set, and +// no earlier revision of a limit is worth keeping. +func openAccountQuotaBucket(ctx context.Context, js jetstream.KeyValueManager, replicas int) (jetstream.KeyValue, error) { + kv, err := js.KeyValue(ctx, handlers_quota.KVBucketAccountQuota) + if err == nil { + return kv, nil + } + if !errors.Is(err, jetstream.ErrBucketNotFound) { + return nil, fmt.Errorf("open account quota bucket: %w", err) + } + + kv, err = js.CreateKeyValue(ctx, jetstream.KeyValueConfig{ + Bucket: handlers_quota.KVBucketAccountQuota, + History: 1, + Replicas: max(replicas, 1), + }) + if errors.Is(err, jetstream.ErrBucketExists) { + return js.KeyValue(ctx, handlers_quota.KVBucketAccountQuota) + } + if err != nil { + return nil, fmt.Errorf("create account quota bucket: %w", err) + } + return kv, nil +} + func launchService(config *config.ClusterConfig) error { nodeConfig := config.Nodes[config.Node] @@ -480,14 +506,19 @@ func launchService(config *config.ClusterConfig) error { // Per-account service quotas. Only the enabled path opens the gateway-owned // usage KV bucket, leaving existing default-off gateways untouched; a disabled // config builds a no-op Service whose Exempt short-circuits every check. - var usageBucket jetstream.KeyValue + var usageBucket, quotaOverrides jetstream.KeyValue if quotaCfg.Enabled { usageBucket, err = openAccountUsageBucket(janitorCtx, js, len(config.Nodes)) if err != nil { return fmt.Errorf("init account usage bucket: %w", err) } + quotaOverrides, err = openAccountQuotaBucket(janitorCtx, js, len(config.Nodes)) + if err != nil { + return fmt.Errorf("init account quota bucket: %w", err) + } } gw.Quota = handlers_quota.New(quotaCfg, usageBucket) + gw.Quota.SetOverrides(quotaOverrides) // Bedrock token quota reads the stream-fed usage counters built above, // independent of whether the standing-infra dimensions are enabled. From 0f8471062f4cda63a7fb19961e1b9fb254c6d38f Mon Sep 17 00:00:00 2001 From: Ben Duncan <5178243+benduncan@users.noreply.github.com> Date: Thu, 20 Aug 2026 02:05:17 +1000 Subject: [PATCH 2/8] feat: add an admin surface for per-account quota overrides --- cmd/spinifex/cmd/admin_account_quota.go | 185 +++++++++++++++++ cmd/spinifex/cmd/admin_account_quota_test.go | 139 +++++++++++++ docs/COMMANDS.md | 2 + spinifex/gateway/admin.go | 6 + spinifex/gateway/admin_accountquota.go | 131 ++++++++++++ spinifex/gateway/admin_accountquota_test.go | 206 +++++++++++++++++++ spinifex/handlers/quota/enforce_test.go | 162 +++++++++++++++ spinifex/handlers/quota/override_kv_test.go | 1 + spinifex/handlers/quota/override_test.go | 1 + 9 files changed, 833 insertions(+) create mode 100644 cmd/spinifex/cmd/admin_account_quota.go create mode 100644 cmd/spinifex/cmd/admin_account_quota_test.go create mode 100644 spinifex/gateway/admin_accountquota.go create mode 100644 spinifex/gateway/admin_accountquota_test.go create mode 100644 spinifex/handlers/quota/enforce_test.go diff --git a/cmd/spinifex/cmd/admin_account_quota.go b/cmd/spinifex/cmd/admin_account_quota.go new file mode 100644 index 000000000..21b816caf --- /dev/null +++ b/cmd/spinifex/cmd/admin_account_quota.go @@ -0,0 +1,185 @@ +package cmd + +import ( + "context" + "fmt" + "sort" + "strconv" + + "github.com/mulgadc/spinifex/spinifex/gateway" + handlers_quota "github.com/mulgadc/spinifex/spinifex/handlers/quota" + "github.com/spf13/cobra" +) + +// quotaDimensions maps each CLI flag to the override field it sets. One table +// drives the flag registration and the parse, so a new dimension cannot be +// added to one and forgotten in the other. +var quotaDimensions = []struct { + flag string + usage string + field func(*handlers_quota.Overrides) **int +}{ + {"vcpus", "Total vCPUs across running and stopped instances", func(o *handlers_quota.Overrides) **int { return &o.VCPUs }}, + {"vpcs", "VPCs", func(o *handlers_quota.Overrides) **int { return &o.VPCs }}, + {"subnets", "Subnets, counted per account rather than per VPC", func(o *handlers_quota.Overrides) **int { return &o.Subnets }}, + {"eips", "Elastic IP addresses", func(o *handlers_quota.Overrides) **int { return &o.EIPs }}, + {"volumes", "EBS volumes", func(o *handlers_quota.Overrides) **int { return &o.Volumes }}, + {"volumes-gib", "Total EBS capacity in GiB", func(o *handlers_quota.Overrides) **int { return &o.VolumesGiB }}, + {"rds-instances", "RDS database instances", func(o *handlers_quota.Overrides) **int { return &o.RDSInstances }}, + {"load-balancers", "Load balancers, ALB and NLB together", func(o *handlers_quota.Overrides) **int { return &o.LoadBalancers }}, +} + +var accountQuotaCmd = &cobra.Command{ + Use: "quota", + Short: "Inspect and adjust an account's service quotas", + Long: `Inspect and adjust the service quotas applied to one account. + +Every account inherits the [quota] limits from awsgw.toml. An override raises or +lowers individual dimensions for one account without affecting any other, so a +customer can be given more vCPUs while every other tenant stays on the baseline. + +Only the dimensions passed as flags are changed; the rest keep inheriting. Use +-1 for no limit on a dimension, and --clear to drop every override.`, +} + +var accountQuotaGetCmd = &cobra.Command{ + Use: "get ", + Short: "Show the limits in force for an account", + Long: `Show the limits in force for an account and where each came from: +"config" for an inherited baseline, "override" for one set on this account.`, + Args: cobra.ExactArgs(1), + RunE: runAccountQuotaGet, +} + +var accountQuotaSetCmd = &cobra.Command{ + Use: "set ", + Short: "Override one or more limits for an account", + Long: `Override one or more limits for an account. Dimensions not named keep +inheriting the configured baseline. + + spx admin account quota set 000000000005 --vcpus 32 + spx admin account quota set 000000000005 --clear`, + Args: cobra.ExactArgs(1), + RunE: runAccountQuotaSet, +} + +// Both subcommands are served only by the /admin surface, so unlike the account +// commands there is no NATS path and no --remote to opt into. +func init() { + accountCmd.AddCommand(accountQuotaCmd) + accountQuotaCmd.AddCommand(accountQuotaGetCmd) + accountQuotaCmd.AddCommand(accountQuotaSetCmd) + + for _, c := range []*cobra.Command{accountQuotaGetCmd, accountQuotaSetCmd} { + c.Flags().String("endpoint", "", "Gateway endpoint (default: this node's AWS gateway)") + c.Flags().String("region", "", "SigV4 region (default: this node's region)") + c.Flags().String("ca-bundle", "", "CA certificate (default: this node's CA)") + } + + for _, d := range quotaDimensions { + accountQuotaSetCmd.Flags().Int(d.flag, 0, d.usage+" (-1 for no limit)") + } + accountQuotaSetCmd.Flags().Bool("clear", false, "Remove every override, returning the account to the configured limits") +} + +func runAccountQuotaGet(cmd *cobra.Command, args []string) error { + target, err := resolveAdminTarget(cmd) + if err != nil { + return err + } + + ctx, cancel := context.WithTimeout(cmd.Context(), adminRequestTimeout) + defer cancel() + + var out gateway.AccountQuotaResponse + req := gateway.AccountQuotaRequest{AccountID: args[0]} + if err := callAdmin(ctx, target, "GetAccountQuota", req, &out); err != nil { + return err + } + printAccountQuota(&out) + return nil +} + +func runAccountQuotaSet(cmd *cobra.Command, args []string) error { + target, err := resolveAdminTarget(cmd) + if err != nil { + return err + } + + over, changed, err := quotaOverridesFromFlags(cmd) + if err != nil { + return err + } + clearAll, err := cmd.Flags().GetBool("clear") + if err != nil { + return err + } + // Refusing both is not pedantry: --clear silently winning would discard + // limits the caller believed it was setting in the same breath. + if clearAll && changed { + return fmt.Errorf("--clear cannot be combined with a dimension flag") + } + if !clearAll && !changed { + return fmt.Errorf("no dimension given; pass at least one limit flag, or --clear to remove every override") + } + if clearAll { + over = handlers_quota.Overrides{} + } + + ctx, cancel := context.WithTimeout(cmd.Context(), adminRequestTimeout) + defer cancel() + + var out gateway.AccountQuotaResponse + req := gateway.AccountQuotaRequest{AccountID: args[0], Overrides: over} + if err := callAdmin(ctx, target, "PutAccountQuota", req, &out); err != nil { + return err + } + printAccountQuota(&out) + return nil +} + +// quotaOverridesFromFlags builds the override set from the flags the caller +// actually passed. An unset flag is left nil so the dimension keeps inheriting, +// which is why Changed is consulted rather than the flag's value. +func quotaOverridesFromFlags(cmd *cobra.Command) (handlers_quota.Overrides, bool, error) { + var over handlers_quota.Overrides + changed := false + for _, d := range quotaDimensions { + if !cmd.Flags().Changed(d.flag) { + continue + } + value, err := cmd.Flags().GetInt(d.flag) + if err != nil { + return over, false, err + } + if value < handlers_quota.Unlimited { + return over, false, fmt.Errorf("--%s must be 0 or greater, or %d for no limit", d.flag, handlers_quota.Unlimited) + } + *d.field(&over) = &value + changed = true + } + return over, changed, nil +} + +func printAccountQuota(resp *gateway.AccountQuotaResponse) { + fmt.Printf("Account %s\n\n", resp.AccountID) + fmt.Printf("%-16s %10s %s\n", "DIMENSION", "LIMIT", "SOURCE") + fmt.Printf("%-16s %10s %s\n", "---------", "-----", "------") + + names := make([]string, 0, len(resp.Limits)) + for name := range resp.Limits { + names = append(names, name) + } + sort.Strings(names) + for _, name := range names { + limit := strconv.Itoa(resp.Limits[name]) + if resp.Limits[name] == handlers_quota.Unlimited { + limit = "unlimited" + } + fmt.Printf("%-16s %10s %s\n", name, limit, resp.Source[name]) + } + + if resp.Overrides.UpdatedAt != "" { + fmt.Printf("\nOverride set by %s at %s\n", resp.Overrides.UpdatedBy, resp.Overrides.UpdatedAt) + } +} diff --git a/cmd/spinifex/cmd/admin_account_quota_test.go b/cmd/spinifex/cmd/admin_account_quota_test.go new file mode 100644 index 000000000..86713d63d --- /dev/null +++ b/cmd/spinifex/cmd/admin_account_quota_test.go @@ -0,0 +1,139 @@ +package cmd + +import ( + "testing" + + "github.com/mulgadc/spinifex/spinifex/gateway" + handlers_quota "github.com/mulgadc/spinifex/spinifex/handlers/quota" + "github.com/spf13/cobra" + "github.com/stretchr/testify/require" +) + +// newQuotaSetCmd builds a command carrying the same flags as the real one, so +// the parse is tested without mutating the package-level command between tests. +func newQuotaSetCmd() *cobra.Command { + cmd := &cobra.Command{Use: "set"} + for _, d := range quotaDimensions { + cmd.Flags().Int(d.flag, 0, d.usage) + } + cmd.Flags().Bool("clear", false, "") + return cmd +} + +// Only the flags actually passed become overrides. Every other dimension must +// stay nil so it keeps inheriting, which is the whole point of a sparse record. +func TestQuotaOverridesFromFlagsIsSparse(t *testing.T) { + cmd := newQuotaSetCmd() + require.NoError(t, cmd.ParseFlags([]string{"--vcpus", "32", "--rds-instances", "8"})) + + over, changed, err := quotaOverridesFromFlags(cmd) + require.NoError(t, err) + require.True(t, changed) + + require.NotNil(t, over.VCPUs) + require.Equal(t, 32, *over.VCPUs) + require.NotNil(t, over.RDSInstances) + require.Equal(t, 8, *over.RDSInstances) + + require.Nil(t, over.VPCs) + require.Nil(t, over.EIPs) + require.Nil(t, over.Volumes) + require.Nil(t, over.VolumesGiB) + require.Nil(t, over.Subnets) + require.Nil(t, over.LoadBalancers) +} + +// A flag left unset must not be read as an explicit 0, which would deny every +// request on that dimension instead of inheriting it. +func TestQuotaOverridesFromFlagsUnsetIsNotZero(t *testing.T) { + cmd := newQuotaSetCmd() + require.NoError(t, cmd.ParseFlags([]string{"--vpcs", "8"})) + + over, changed, err := quotaOverridesFromFlags(cmd) + require.NoError(t, err) + require.True(t, changed) + require.Nil(t, over.VCPUs, "an unset flag must not become an explicit zero") +} + +// Passing 0 deliberately is a real limit and must be preserved. +func TestQuotaOverridesFromFlagsExplicitZero(t *testing.T) { + cmd := newQuotaSetCmd() + require.NoError(t, cmd.ParseFlags([]string{"--eips", "0"})) + + over, changed, err := quotaOverridesFromFlags(cmd) + require.NoError(t, err) + require.True(t, changed) + require.NotNil(t, over.EIPs) + require.Equal(t, 0, *over.EIPs) +} + +func TestQuotaOverridesFromFlagsUnlimited(t *testing.T) { + cmd := newQuotaSetCmd() + require.NoError(t, cmd.ParseFlags([]string{"--vcpus", "-1"})) + + over, _, err := quotaOverridesFromFlags(cmd) + require.NoError(t, err) + require.NotNil(t, over.VCPUs) + require.Equal(t, handlers_quota.Unlimited, *over.VCPUs) +} + +// Anything below the Unlimited sentinel is a typo, not a smaller limit. +func TestQuotaOverridesFromFlagsRejectsBelowUnlimited(t *testing.T) { + cmd := newQuotaSetCmd() + require.NoError(t, cmd.ParseFlags([]string{"--vcpus", "-2"})) + + _, _, err := quotaOverridesFromFlags(cmd) + require.Error(t, err) +} + +// No flags at all means nothing changed, which the caller turns into an error +// rather than a write that silently does nothing. +func TestQuotaOverridesFromFlagsNoneChanged(t *testing.T) { + cmd := newQuotaSetCmd() + require.NoError(t, cmd.ParseFlags(nil)) + + over, changed, err := quotaOverridesFromFlags(cmd) + require.NoError(t, err) + require.False(t, changed) + require.True(t, over.Empty()) +} + +// The table must name every dimension, label its source, and render the +// Unlimited sentinel as a word rather than as -1, which reads like a bug. +func TestPrintAccountQuota(t *testing.T) { + resp := &gateway.AccountQuotaResponse{ + AccountID: "000000000042", + Limits: map[string]int{"vcpus": 32, "vpcs": handlers_quota.Unlimited, "eips": 4}, + Source: map[string]string{"vcpus": "override", "vpcs": "override", "eips": "config"}, + Overrides: handlers_quota.Overrides{UpdatedBy: "operator", UpdatedAt: "2026-08-20T01:00:00Z"}, + } + + out := captureStdout(t, func() { printAccountQuota(resp) }) + + require.Contains(t, out, "000000000042") + require.Contains(t, out, "vcpus") + require.Contains(t, out, "32") + require.Contains(t, out, "override") + require.Contains(t, out, "config") + require.Contains(t, out, "unlimited") + require.NotContains(t, out, "-1") + require.Contains(t, out, "operator") +} + +// Every dimension the override record carries must be reachable from the CLI, +// or a limit exists that an operator cannot set. +func TestQuotaDimensionsCoverEveryOverrideField(t *testing.T) { + var over handlers_quota.Overrides + for _, d := range quotaDimensions { + v := 1 + *d.field(&over) = &v + } + require.False(t, over.Empty()) + + seen := map[string]bool{} + for _, d := range quotaDimensions { + require.False(t, seen[d.flag], "duplicate flag %q", d.flag) + seen[d.flag] = true + } + require.Len(t, quotaDimensions, 8) +} diff --git a/docs/COMMANDS.md b/docs/COMMANDS.md index 8e6fe6287..c712aabef 100644 --- a/docs/COMMANDS.md +++ b/docs/COMMANDS.md @@ -95,6 +95,8 @@ Operational commands for inspecting cluster state. These fan out NATS requests t | `spx admin account create --remote` | `--name` (required), `--endpoint` (default: this node's AWS gateway), `--region` (default: this node's region), `--ca-bundle` (default: this node's CA), `--client-token` (default: generated), `--source` (default: `spx-cli`) | Same account, created through `POST /admin/CreateAccount` over SigV4 instead of NATS — the path the public signup form uses, so a shell can exercise it. Credentials come from the standard AWS chain (env vars or `AWS_PROFILE`), never from the cluster master key; the caller must be an IAM user in the super-admin account holding `spinifex:CreateAccount`. Reuse the printed `--client-token` to retry: a fresh token would create a second account. | | `spx admin account list` | — | Connects to NATS → IAMService.ListAccounts() → prints table with Account ID, Name, Status, Created | | `spx admin account list --remote` | `--endpoint` (default: this node's AWS gateway), `--region` (default: this node's region), `--ca-bundle` (default: this node's CA) | Same listing read through `POST /admin/ListAccounts` over SigV4 instead of NATS, so an off-cluster operator or the load-test harness sees its tenants without SSH access to a node. Credentials come from the standard AWS chain; the caller must be an IAM user in the super-admin account holding `spinifex:ListAccounts`. | +| `spx admin account quota get ` | `--endpoint` (default: this node's AWS gateway), `--region` (default: this node's region), `--ca-bundle` (default: this node's CA) | Reads the limits in force for one account through `POST /admin/GetAccountQuota` over SigV4, and reports for each dimension whether it came from the `[quota]` block in `awsgw.toml` (`config`) or from an override stored against the account (`override`). Credentials come from the standard AWS chain; the caller must be an IAM user in the super-admin account holding `spinifex:GetAccountQuota`. | +| `spx admin account quota set ` | `--vcpus`, `--vpcs`, `--subnets`, `--eips`, `--volumes`, `--volumes-gib`, `--rds-instances`, `--load-balancers`, `--clear`, plus `--endpoint`, `--region`, `--ca-bundle` | Overrides individual limits for one account through `POST /admin/PutAccountQuota`, leaving every dimension not named inheriting the configured baseline. `-1` uncaps a dimension; `0` is a real limit that denies every request on it. `--clear` removes the whole override record and cannot be combined with a limit flag. The account must already exist. Prints the resulting limits, so a caller never has to guess what it applied. The caller must hold `spinifex:PutAccountQuota`. | | `spx admin account delete ` | `--dry-run`, `--force`, `--yes` (requires `--name`), `--name` (account name, must match the stored record) | Prints the inventory it is about to remove → confirms the account name (prompted unless `--name` is given) → marks the account `TERMINATING`, which blocks its credentials cluster-wide → deletes its resources in dependency order (compute, attachments, storage, network, platform, identity, account), waiting for each stage to actually drain before starting the next → releases the name reservation and quota counter → deletes the account record. Anything that will not delete is reported as **stuck** with the reason, and the account stays `TERMINATING` so the residue keeps an owner. `000000000000` and `000000000001` are refused. `--dry-run` lists and changes nothing. The compute stage reaps ECS clusters, EKS nodegroups and clusters, and RDS instances before it reaps plain instances, because their backing VMs belong to the system account and a tenant-scoped instance listing cannot see them. The storage stage reaps the account's S3 buckets: uploads in flight are aborted, then the objects are removed a page at a time, then the bucket. Only buckets the account owns are ever listed, so the shared platform buckets are out of reach rather than filtered out. Bedrock model endpoints are reaped ahead of instances, because only their own delete releases the ENI and weights volume alongside the VM, and spot instance requests are cancelled first so a live request cannot launch a replacement for something already terminated. The network stage reaps load balancers, then target groups, then NAT gateways and egress-only internet gateways before the subnets, security groups and addresses they hold. The platform stage reaps ACM certificates. | | `spx admin account delete --force` | as above | Escalates only for resources the ordinary API refuses: it clears a volume attachment in the control plane without the guest's cooperation (`ec2.ForceDetachVolume`), hard-destroys after a graceful attempt times out, and treats an already-missing resource as deleted. It never reorders the stages and never widens scope beyond the account. Use it once the guest is being destroyed — this is the escape from a volume stuck attached to an instance that will not terminate, where neither can otherwise be deleted. | | `spx admin account delete --remote` | as above, plus `--endpoint`, `--region`, `--ca-bundle`, `--client-token` | Same teardown driven through `POST /admin/DeleteAccount` over SigV4 instead of NATS, then follows `DescribeAccountDeletion` to completion, printing each stage as it lands. Credentials come from the standard AWS chain; the caller must be an IAM user in the super-admin account holding `spinifex:DeleteAccount` and `spinifex:DescribeAccountDeletion`. | diff --git a/spinifex/gateway/admin.go b/spinifex/gateway/admin.go index 06fcd4010..21ee0a662 100644 --- a/spinifex/gateway/admin.go +++ b/spinifex/gateway/admin.go @@ -23,7 +23,9 @@ var adminMethods = map[string]bool{ "CreateAccount": true, "DeleteAccount": true, "DescribeAccountDeletion": true, + "GetAccountQuota": true, "ListAccounts": true, + "PutAccountQuota": true, } // AdminMethodNames returns the callable /admin/ names in sorted order. @@ -129,6 +131,10 @@ func (gw *GatewayConfig) Admin_Request(w http.ResponseWriter, r *http.Request) { output, err = gw.adminDescribeAccountDeletion(ctx, body) case "ListAccounts": output, err = gw.adminListAccounts(ctx, body) + case "GetAccountQuota": + output, err = gw.adminGetAccountQuota(ctx, body) + case "PutAccountQuota": + output, err = gw.adminPutAccountQuota(ctx, body) default: // Unreachable: adminMethods gates the switch. Fail closed anyway so // adding a name to the map without a case cannot silently return 200. diff --git a/spinifex/gateway/admin_accountquota.go b/spinifex/gateway/admin_accountquota.go new file mode 100644 index 000000000..ce2e17acd --- /dev/null +++ b/spinifex/gateway/admin_accountquota.go @@ -0,0 +1,131 @@ +package gateway + +import ( + "context" + "encoding/json" + "errors" + "log/slog" + + "github.com/mulgadc/spinifex/spinifex/awserrors" + handlers_quota "github.com/mulgadc/spinifex/spinifex/handlers/quota" +) + +// AccountQuotaRequest addresses one account. PutAccountQuota also carries the +// overrides; GetAccountQuota ignores them. +type AccountQuotaRequest struct { + AccountID string `json:"accountId"` + Overrides handlers_quota.Overrides `json:"overrides,omitzero"` +} + +// AccountQuotaResponse reports the limits in force for an account and where +// each came from. The source map is what makes a surprising number debuggable: +// a bare limit cannot say whether it was inherited or set. +type AccountQuotaResponse struct { + AccountID string `json:"accountId"` + Limits map[string]int `json:"limits"` + Source map[string]string `json:"source"` + Overrides handlers_quota.Overrides `json:"overrides"` +} + +// sourceConfig and sourceOverride name where a resolved limit came from. +const ( + sourceConfig = "config" + sourceOverride = "override" +) + +// adminGetAccountQuota reports one account's effective limits, its stored +// overrides, and which layer each dimension resolved from. +func (gw *GatewayConfig) adminGetAccountQuota(ctx context.Context, body []byte) (any, error) { + accountID, _, err := gw.parseAccountQuotaRequest(body) + if err != nil { + return nil, err + } + + over, limits, err := gw.Quota.GetAccountQuota(ctx, accountID) + if err != nil { + slog.ErrorContext(ctx, "GetAccountQuota: failed to read override", "accountID", accountID, "err", err) + return nil, errors.New(awserrors.ErrorInternalError) + } + return accountQuotaResponse(accountID, over, limits), nil +} + +// adminPutAccountQuota stores one account's overrides and returns the limits +// they resolve to, so a caller never has to guess what it just applied. An +// empty override set clears the record. +func (gw *GatewayConfig) adminPutAccountQuota(ctx context.Context, body []byte) (any, error) { + accountID, over, err := gw.parseAccountQuotaRequest(body) + if err != nil { + return nil, err + } + + // The caller's own identity, so a quota change names who made it. + updatedBy, _ := ctx.Value(ctxIdentity).(string) + if err := gw.Quota.PutAccountQuota(ctx, accountID, over, updatedBy); err != nil { + slog.ErrorContext(ctx, "PutAccountQuota: failed to store override", "accountID", accountID, "err", err) + return nil, errors.New(awserrors.ErrorInternalError) + } + slog.InfoContext(ctx, "Account quota override updated", + "accountID", accountID, "updatedBy", updatedBy, "cleared", over.Empty()) + + stored, limits, err := gw.Quota.GetAccountQuota(ctx, accountID) + if err != nil { + slog.ErrorContext(ctx, "PutAccountQuota: failed to read back override", "accountID", accountID, "err", err) + return nil, errors.New(awserrors.ErrorInternalError) + } + return accountQuotaResponse(accountID, stored, limits), nil +} + +// parseAccountQuotaRequest decodes and validates a quota request. The account +// must exist: silently accepting an override for a typo'd ID would store a +// record that never applies to anything. +func (gw *GatewayConfig) parseAccountQuotaRequest(body []byte) (string, handlers_quota.Overrides, error) { + if gw.Quota == nil || gw.IAMService == nil { + slog.Error("AccountQuota: quota or IAM service not available") + return "", handlers_quota.Overrides{}, errors.New(awserrors.ErrorInternalError) + } + + var req AccountQuotaRequest + if err := json.Unmarshal(body, &req); err != nil { + return "", handlers_quota.Overrides{}, errors.New(awserrors.ErrorInvalidRequest) + } + if !accountIDRE.MatchString(req.AccountID) { + return "", handlers_quota.Overrides{}, errors.New(awserrors.ErrorInvalidRequest) + } + if _, err := gw.IAMService.GetAccount(req.AccountID); err != nil { + return "", handlers_quota.Overrides{}, errors.New(awserrors.ErrorInvalidRequest) + } + return req.AccountID, req.Overrides, nil +} + +// accountQuotaResponse pairs each resolved limit with the layer it came from. +func accountQuotaResponse(accountID string, over handlers_quota.Overrides, limits handlers_quota.Limits) *AccountQuotaResponse { + dimensions := []struct { + name string + value int + override *int + }{ + {"vcpus", limits.VCPUs, over.VCPUs}, + {"vpcs", limits.VPCs, over.VPCs}, + {"subnets", limits.Subnets, over.Subnets}, + {"eips", limits.EIPs, over.EIPs}, + {"volumes", limits.Volumes, over.Volumes}, + {"volumes_gib", limits.VolumesGiB, over.VolumesGiB}, + {"rds_instances", limits.RDSInstances, over.RDSInstances}, + {"load_balancers", limits.LoadBalancers, over.LoadBalancers}, + } + + resp := &AccountQuotaResponse{ + AccountID: accountID, + Limits: make(map[string]int, len(dimensions)), + Source: make(map[string]string, len(dimensions)), + Overrides: over, + } + for _, d := range dimensions { + resp.Limits[d.name] = d.value + resp.Source[d.name] = sourceConfig + if d.override != nil { + resp.Source[d.name] = sourceOverride + } + } + return resp +} diff --git a/spinifex/gateway/admin_accountquota_test.go b/spinifex/gateway/admin_accountquota_test.go new file mode 100644 index 000000000..00302384c --- /dev/null +++ b/spinifex/gateway/admin_accountquota_test.go @@ -0,0 +1,206 @@ +package gateway + +import ( + "context" + "encoding/json" + "errors" + "testing" + + "github.com/mulgadc/spinifex/spinifex/awserrors" + handlers_iam "github.com/mulgadc/spinifex/spinifex/handlers/iam" + handlers_quota "github.com/mulgadc/spinifex/spinifex/handlers/quota" + "github.com/mulgadc/spinifex/spinifex/testutil" + "github.com/nats-io/nats.go/jetstream" + "github.com/stretchr/testify/require" +) + +func quotaPtr(v int) *int { return &v } + +// quotaIAMService resolves exactly one account, so a request naming any other +// exercises the "account must exist" gate. +type quotaIAMService struct { + policyMockIAMService + + known string +} + +func (m *quotaIAMService) GetAccount(accountID string) (*handlers_iam.Account, error) { + if accountID != m.known { + return nil, errors.New(awserrors.ErrorIAMNoSuchEntity) + } + return &handlers_iam.Account{AccountID: accountID, Status: handlers_iam.AccountStatusActive}, nil +} + +// quotaTestGateway wires a real quota Service over a real JetStream KV bucket, +// so the admin handlers are exercised against the store they use in production. +func quotaTestGateway(t *testing.T) (*GatewayConfig, string) { + t.Helper() + const account = "000000000042" + + _, _, js := testutil.StartTestJetStream(t) + kv, err := js.CreateKeyValue(t.Context(), jetstream.KeyValueConfig{ + Bucket: handlers_quota.KVBucketAccountQuota, + History: 1, + }) + require.NoError(t, err) + + quota := handlers_quota.New(handlers_quota.Limits{ + Enabled: true, VCPUs: 16, VPCs: 4, Subnets: 16, EIPs: 4, + Volumes: 16, VolumesGiB: 200, RDSInstances: 2, LoadBalancers: 2, + }, nil) + quota.SetOverrides(kv) + + return &GatewayConfig{ + DisableLogging: true, + IAMService: "aIAMService{known: account}, + Quota: quota, + }, account +} + +func quotaRequestBody(t *testing.T, req AccountQuotaRequest) []byte { + t.Helper() + body, err := json.Marshal(req) + require.NoError(t, err) + return body +} + +// An account with no override must report the configured baseline, with every +// dimension attributed to the config. +func TestAdminGetAccountQuotaInherits(t *testing.T) { + gw, account := quotaTestGateway(t) + + output, err := gw.adminGetAccountQuota(t.Context(), quotaRequestBody(t, AccountQuotaRequest{AccountID: account})) + require.NoError(t, err) + + resp, ok := output.(*AccountQuotaResponse) + require.True(t, ok) + require.Equal(t, 16, resp.Limits["vcpus"]) + require.Equal(t, sourceConfig, resp.Source["vcpus"]) +} + +// A stored override must come back on the next read, with only the dimension it +// named changed. +func TestAdminPutThenGetAccountQuota(t *testing.T) { + gw, account := quotaTestGateway(t) + ctx := context.WithValue(t.Context(), ctxIdentity, "operator") + + output, err := gw.adminPutAccountQuota(ctx, quotaRequestBody(t, AccountQuotaRequest{ + AccountID: account, + Overrides: handlers_quota.Overrides{VCPUs: quotaPtr(32)}, + })) + require.NoError(t, err) + + put, ok := output.(*AccountQuotaResponse) + require.True(t, ok) + require.Equal(t, 32, put.Limits["vcpus"]) + require.Equal(t, sourceOverride, put.Source["vcpus"]) + require.Equal(t, "operator", put.Overrides.UpdatedBy) + require.Equal(t, 4, put.Limits["vpcs"], "an unnamed dimension must keep inheriting") + + output, err = gw.adminGetAccountQuota(t.Context(), quotaRequestBody(t, AccountQuotaRequest{AccountID: account})) + require.NoError(t, err) + got, ok := output.(*AccountQuotaResponse) + require.True(t, ok) + require.Equal(t, 32, got.Limits["vcpus"]) + require.Equal(t, sourceOverride, got.Source["vcpus"]) +} + +// Clearing must return the account to the configured limits. +func TestAdminPutAccountQuotaClears(t *testing.T) { + gw, account := quotaTestGateway(t) + ctx := context.WithValue(t.Context(), ctxIdentity, "operator") + + _, err := gw.adminPutAccountQuota(ctx, quotaRequestBody(t, AccountQuotaRequest{ + AccountID: account, + Overrides: handlers_quota.Overrides{VCPUs: quotaPtr(32)}, + })) + require.NoError(t, err) + + output, err := gw.adminPutAccountQuota(ctx, quotaRequestBody(t, AccountQuotaRequest{AccountID: account})) + require.NoError(t, err) + + resp, ok := output.(*AccountQuotaResponse) + require.True(t, ok) + require.Equal(t, 16, resp.Limits["vcpus"]) + require.Equal(t, sourceConfig, resp.Source["vcpus"]) +} + +// Storing an override against an account that does not exist would leave a +// record that never applies to anything, so a bad ID is refused. +func TestAdminAccountQuotaRejectsBadRequests(t *testing.T) { + gw, _ := quotaTestGateway(t) + + tests := []struct { + name string + body []byte + }{ + {"malformed json", []byte("{")}, + {"missing account", quotaRequestBody(t, AccountQuotaRequest{})}, + {"not twelve digits", quotaRequestBody(t, AccountQuotaRequest{AccountID: "42"})}, + {"unknown account", quotaRequestBody(t, AccountQuotaRequest{AccountID: "000000000099"})}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := gw.adminGetAccountQuota(t.Context(), tt.body) + require.Error(t, err) + require.Equal(t, awserrors.ErrorInvalidRequest, err.Error()) + + _, err = gw.adminPutAccountQuota(t.Context(), tt.body) + require.Error(t, err) + require.Equal(t, awserrors.ErrorInvalidRequest, err.Error()) + }) + } +} + +// A gateway with quotas disabled has no override bucket; the admin surface must +// report that rather than pretend the write landed. +func TestAdminAccountQuotaWithoutQuotaService(t *testing.T) { + gw := &GatewayConfig{DisableLogging: true, IAMService: "aIAMService{known: "000000000042"}} + + _, err := gw.adminGetAccountQuota(t.Context(), quotaRequestBody(t, AccountQuotaRequest{AccountID: "000000000042"})) + require.Error(t, err) + require.Equal(t, awserrors.ErrorInternalError, err.Error()) +} + +// Every dimension must appear in the response, and each must be labelled with +// the layer it came from: a limit with no provenance cannot be debugged. +func TestAccountQuotaResponseReportsSource(t *testing.T) { + limits := handlers_quota.Limits{ + Enabled: true, VCPUs: 32, VPCs: 4, Subnets: 16, EIPs: 4, + Volumes: 16, VolumesGiB: 200, RDSInstances: 2, LoadBalancers: 2, + } + over := handlers_quota.Overrides{VCPUs: quotaPtr(32)} + + resp := accountQuotaResponse("000000000002", over, limits) + + require.Equal(t, "000000000002", resp.AccountID) + require.Len(t, resp.Limits, 8, "every dimension must be reported") + require.Len(t, resp.Source, 8) + + require.Equal(t, 32, resp.Limits["vcpus"]) + require.Equal(t, sourceOverride, resp.Source["vcpus"]) + for _, inherited := range []string{"vpcs", "subnets", "eips", "volumes", "volumes_gib", "rds_instances", "load_balancers"} { + require.Equal(t, sourceConfig, resp.Source[inherited], "%s must report as inherited", inherited) + } +} + +// An explicit zero is an override, not an absent field, so it must be reported +// as one rather than looking like an inherited limit that happens to be zero. +func TestAccountQuotaResponseExplicitZeroIsAnOverride(t *testing.T) { + resp := accountQuotaResponse("000000000002", + handlers_quota.Overrides{VPCs: quotaPtr(0)}, + handlers_quota.Limits{VPCs: 0, VCPUs: 16}) + + require.Equal(t, 0, resp.Limits["vpcs"]) + require.Equal(t, sourceOverride, resp.Source["vpcs"]) + require.Equal(t, sourceConfig, resp.Source["vcpus"]) +} + +// The two quota methods must be routable, or they return InvalidAction on a +// cluster that otherwise looks correctly configured. +func TestAccountQuotaMethodsAreRegistered(t *testing.T) { + require.True(t, adminMethods["GetAccountQuota"]) + require.True(t, adminMethods["PutAccountQuota"]) + require.Contains(t, AdminMethodNames(), "GetAccountQuota") + require.Contains(t, AdminMethodNames(), "PutAccountQuota") +} diff --git a/spinifex/handlers/quota/enforce_test.go b/spinifex/handlers/quota/enforce_test.go new file mode 100644 index 000000000..60f25a7c7 --- /dev/null +++ b/spinifex/handlers/quota/enforce_test.go @@ -0,0 +1,162 @@ +package handlers_quota_test + +import ( + "encoding/json" + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/aws/aws-sdk-go/service/elbv2" + "github.com/aws/aws-sdk-go/service/rds" + "github.com/mulgadc/spinifex/spinifex/awserrors" + handlers_quota "github.com/mulgadc/spinifex/spinifex/handlers/quota" + "github.com/mulgadc/spinifex/spinifex/testutil" + "github.com/nats-io/nats.go" + "github.com/stretchr/testify/require" +) + +// stubDescribe answers one describe subject with a fixed payload, standing in +// for the daemon so the enforcement path can be driven end to end without one. +func stubDescribe(t *testing.T, nc *nats.Conn, subject string, reply any) { + t.Helper() + data, err := json.Marshal(reply) + require.NoError(t, err) + + sub, err := nc.Subscribe(subject, func(msg *nats.Msg) { + _ = msg.Respond(data) + }) + require.NoError(t, err) + require.NoError(t, nc.Flush()) + t.Cleanup(func() { _ = sub.Unsubscribe() }) +} + +// enforceService builds an enabled Service with per-dimension caps low enough +// that a handful of resources crosses them. +func enforceService(limits handlers_quota.Limits) *handlers_quota.Service { + return handlers_quota.New(limits, nil) +} + +func vpcList(n int) *ec2.DescribeVpcsOutput { + out := &ec2.DescribeVpcsOutput{} + for i := range n { + out.Vpcs = append(out.Vpcs, &ec2.Vpc{VpcId: aws.String("vpc-" + string(rune('a'+i)))}) + } + return out +} + +// The account is one below its cap, so one more is allowed and two are not. +func TestEnforceVPCsAtBoundary(t *testing.T) { + _, nc := testutil.StartTestNATS(t) + stubDescribe(t, nc, "ec2.DescribeVpcs", vpcList(3)) + svc := enforceService(handlers_quota.Limits{Enabled: true, VPCs: 4}) + + require.NoError(t, svc.EnforceVPCs(t.Context(), nc, "000000000002", 1)) + + err := svc.EnforceVPCs(t.Context(), nc, "000000000002", 2) + require.Error(t, err) + require.Equal(t, awserrors.ErrorResourceLimitExceeded, err.Error()) +} + +// An account already at its cap must be refused outright. +func TestEnforceVPCsAtCap(t *testing.T) { + _, nc := testutil.StartTestNATS(t) + stubDescribe(t, nc, "ec2.DescribeVpcs", vpcList(4)) + svc := enforceService(handlers_quota.Limits{Enabled: true, VPCs: 4}) + + err := svc.EnforceVPCs(t.Context(), nc, "000000000002", 1) + require.Error(t, err) + require.Equal(t, awserrors.ErrorResourceLimitExceeded, err.Error()) +} + +func TestEnforceSubnets(t *testing.T) { + _, nc := testutil.StartTestNATS(t) + stubDescribe(t, nc, "ec2.DescribeSubnets", &ec2.DescribeSubnetsOutput{ + Subnets: []*ec2.Subnet{{SubnetId: aws.String("subnet-a")}, {SubnetId: aws.String("subnet-b")}}, + }) + svc := enforceService(handlers_quota.Limits{Enabled: true, Subnets: 2}) + + require.Error(t, svc.EnforceSubnets(t.Context(), nc, "000000000002", 1)) +} + +func TestEnforceEIPs(t *testing.T) { + _, nc := testutil.StartTestNATS(t) + stubDescribe(t, nc, "ec2.DescribeAddresses", &ec2.DescribeAddressesOutput{ + Addresses: []*ec2.Address{{PublicIp: aws.String("203.0.113.1")}}, + }) + svc := enforceService(handlers_quota.Limits{Enabled: true, EIPs: 4}) + + require.NoError(t, svc.EnforceEIPs(t.Context(), nc, "000000000002", 3)) + require.Error(t, svc.EnforceEIPs(t.Context(), nc, "000000000002", 4)) +} + +func TestEnforceLoadBalancers(t *testing.T) { + _, nc := testutil.StartTestNATS(t) + stubDescribe(t, nc, "elbv2.DescribeLoadBalancers", &elbv2.DescribeLoadBalancersOutput{ + LoadBalancers: []*elbv2.LoadBalancer{{LoadBalancerArn: aws.String("arn:a")}, {LoadBalancerArn: aws.String("arn:b")}}, + }) + svc := enforceService(handlers_quota.Limits{Enabled: true, LoadBalancers: 2}) + + err := svc.EnforceLoadBalancers(t.Context(), nc, "000000000002", 1) + require.Error(t, err) + require.Equal(t, awserrors.ErrorResourceLimitExceeded, err.Error()) +} + +func TestEnforceRDSInstances(t *testing.T) { + _, nc := testutil.StartTestNATS(t) + stubDescribe(t, nc, "rds.DescribeDBInstances", &rds.DescribeDBInstancesOutput{ + DBInstances: []*rds.DBInstance{{DBInstanceIdentifier: aws.String("db-a")}}, + }) + svc := enforceService(handlers_quota.Limits{Enabled: true, RDSInstances: 2}) + + require.NoError(t, svc.EnforceRDSInstances(t.Context(), nc, "000000000002", 1)) + require.Error(t, svc.EnforceRDSInstances(t.Context(), nc, "000000000002", 2)) +} + +// The volume count and the capacity sum are separate caps, and either alone +// must be able to refuse a create. +func TestEnforceVolumeCreateCountAndCapacity(t *testing.T) { + _, nc := testutil.StartTestNATS(t) + stubDescribe(t, nc, "ec2.DescribeVolumes", &ec2.DescribeVolumesOutput{ + Volumes: []*ec2.Volume{ + {VolumeId: aws.String("vol-a"), Size: aws.Int64(10)}, + {VolumeId: aws.String("vol-b"), Size: aws.Int64(10)}, + }, + }) + + countBound := enforceService(handlers_quota.Limits{Enabled: true, Volumes: 2, VolumesGiB: 1000}) + err := countBound.EnforceVolumeCreate(t.Context(), nc, "000000000002", 1) + require.Error(t, err, "the count cap must refuse even a tiny volume") + require.Equal(t, awserrors.ErrorResourceLimitExceeded, err.Error()) + + capacityBound := enforceService(handlers_quota.Limits{Enabled: true, Volumes: 16, VolumesGiB: 25}) + require.NoError(t, capacityBound.EnforceVolumeCreate(t.Context(), nc, "000000000002", 5)) + require.Error(t, capacityBound.EnforceVolumeCreate(t.Context(), nc, "000000000002", 6)) +} + +func TestEnforceVolumeModify(t *testing.T) { + _, nc := testutil.StartTestNATS(t) + stubDescribe(t, nc, "ec2.DescribeVolumes", &ec2.DescribeVolumesOutput{ + Volumes: []*ec2.Volume{ + {VolumeId: aws.String("vol-a"), Size: aws.Int64(10)}, + {VolumeId: aws.String("vol-b"), Size: aws.Int64(10)}, + }, + }) + svc := enforceService(handlers_quota.Limits{Enabled: true, Volumes: 16, VolumesGiB: 40}) + + require.NoError(t, svc.EnforceVolumeModify(t.Context(), nc, "000000000002", "vol-a", 30)) + require.Error(t, svc.EnforceVolumeModify(t.Context(), nc, "000000000002", "vol-a", 31)) +} + +// A describe that cannot be answered must fail the check rather than be read as +// "the account holds nothing", which would wave every create past the cap. +func TestEnforceFailsWhenDescribeFails(t *testing.T) { + _, nc := testutil.StartTestNATS(t) + svc := enforceService(handlers_quota.Limits{Enabled: true, VPCs: 4, EIPs: 4, Subnets: 4, LoadBalancers: 2, RDSInstances: 2, Volumes: 4}) + + require.Error(t, svc.EnforceVPCs(t.Context(), nc, "000000000002", 1)) + require.Error(t, svc.EnforceSubnets(t.Context(), nc, "000000000002", 1)) + require.Error(t, svc.EnforceEIPs(t.Context(), nc, "000000000002", 1)) + require.Error(t, svc.EnforceLoadBalancers(t.Context(), nc, "000000000002", 1)) + require.Error(t, svc.EnforceRDSInstances(t.Context(), nc, "000000000002", 1)) + require.Error(t, svc.EnforceVolumeCreate(t.Context(), nc, "000000000002", 1)) +} diff --git a/spinifex/handlers/quota/override_kv_test.go b/spinifex/handlers/quota/override_kv_test.go index 62fb664d9..a39c77975 100644 --- a/spinifex/handlers/quota/override_kv_test.go +++ b/spinifex/handlers/quota/override_kv_test.go @@ -1,3 +1,4 @@ +//test:in-package — exercises the unexported limitsFor resolver against a real KV bucket. package handlers_quota import ( diff --git a/spinifex/handlers/quota/override_test.go b/spinifex/handlers/quota/override_test.go index 77ce8ae0f..4cf5b83d5 100644 --- a/spinifex/handlers/quota/override_test.go +++ b/spinifex/handlers/quota/override_test.go @@ -1,3 +1,4 @@ +//test:in-package — exercises the unexported resolver (apply, limitsFor) and the shared exceeds comparison. package handlers_quota import ( From f41f5530a0068674b91029c8d3686a298d1d9db6 Mon Sep 17 00:00:00 2001 From: Ben Duncan <5178243+benduncan@users.noreply.github.com> Date: Thu, 20 Aug 2026 02:26:27 +1000 Subject: [PATCH 3/8] test: add an e2e suite proving quotas gate every capped resource --- .github/scripts/suite-sets.sh | 8 +- cmd/spinifex/cmd/admin_account_quota_test.go | 1 + spinifex/gateway/admin_accountquota_test.go | 1 + tests/e2e/Makefile | 5 +- tests/e2e/harness/quota.go | 318 +++++++++++++++++++ tests/e2e/quota/helpers_test.go | 245 ++++++++++++++ tests/e2e/quota/main_test.go | 172 ++++++++++ tests/e2e/quota/tests_test.go | 308 ++++++++++++++++++ 8 files changed, 1054 insertions(+), 4 deletions(-) create mode 100644 tests/e2e/harness/quota.go create mode 100644 tests/e2e/quota/helpers_test.go create mode 100644 tests/e2e/quota/main_test.go create mode 100644 tests/e2e/quota/tests_test.go diff --git a/.github/scripts/suite-sets.sh b/.github/scripts/suite-sets.sh index c39207631..2acbfadca 100644 --- a/.github/scripts/suite-sets.sh +++ b/.github/scripts/suite-sets.sh @@ -24,10 +24,14 @@ # behaviour, so a second node adds no assertion. Its DB VMs are capped at four # concurrent by the suite's own semaphore, which is what keeps it inside the # job's 50-minute budget alongside the suites above. -E2E_SUITES_SINGLE="single iam cert eks ecs storagegrowth partialblock rds" +# quota belongs to both topologies because the gate it asserts is gateway code +# that runs identically on either, and because a quota left switched on would +# cap every suite sharing the environment — so it must be proved to restore +# itself wherever it runs, not only on one node. +E2E_SUITES_SINGLE="single iam cert eks ecs storagegrowth partialblock rds quota" # Suites runnable against a multi-node environment. -E2E_SUITES_MULTI="multinode lb cert" +E2E_SUITES_MULTI="multinode lb cert quota" # grep -xE alternation form, for narrowing a requested set to the eligible one. E2E_SUITES_SINGLE_RE="$(printf '%s' "${E2E_SUITES_SINGLE}" | tr ' ' '|')" diff --git a/cmd/spinifex/cmd/admin_account_quota_test.go b/cmd/spinifex/cmd/admin_account_quota_test.go index 86713d63d..76f188368 100644 --- a/cmd/spinifex/cmd/admin_account_quota_test.go +++ b/cmd/spinifex/cmd/admin_account_quota_test.go @@ -1,3 +1,4 @@ +//test:in-package — drives the unexported flag table, parser and printer the quota subcommands are built from. package cmd import ( diff --git a/spinifex/gateway/admin_accountquota_test.go b/spinifex/gateway/admin_accountquota_test.go index 00302384c..5e50a4ec6 100644 --- a/spinifex/gateway/admin_accountquota_test.go +++ b/spinifex/gateway/admin_accountquota_test.go @@ -1,3 +1,4 @@ +//test:in-package — drives the unexported admin handlers and the request identity they read from the context. package gateway import ( diff --git a/tests/e2e/Makefile b/tests/e2e/Makefile index e14ea83bd..8f6610830 100644 --- a/tests/e2e/Makefile +++ b/tests/e2e/Makefile @@ -18,9 +18,9 @@ SUITE_TAG ?= $(notdir $(patsubst %/...,%,$(firstword $(PKG)))) # BIN_DIR/.test so CI can ship binaries (not source + go toolchain) to # the remote VM. BIN_DIR ?= $(CURDIR)/_bin -SUITES_BUILD ?= cert lb eks ecs single iam ecr bedrock gpu natuplink multinode storagegrowth partialblock rds +SUITES_BUILD ?= cert lb eks ecs single iam ecr bedrock gpu natuplink multinode storagegrowth partialblock rds quota -.PHONY: e2e e2e-cert e2e-lb e2e-eks e2e-ecs e2e-single e2e-iam e2e-ecr e2e-bedrock e2e-multinode e2e-bm e2e-reboot e2e-tofu e2e-gpu e2e-natuplink e2e-storagegrowth e2e-partialblock e2e-rds e2e-all clean e2e-build +.PHONY: e2e e2e-quota e2e-cert e2e-lb e2e-eks e2e-ecs e2e-single e2e-iam e2e-ecr e2e-bedrock e2e-multinode e2e-bm e2e-reboot e2e-tofu e2e-gpu e2e-natuplink e2e-storagegrowth e2e-partialblock e2e-rds e2e-all clean e2e-build RUN_FLAG = $(if $(RUN),-run '$(RUN)',) @@ -61,6 +61,7 @@ e2e-partialblock: ; $(MAKE) e2e PKG=./partialblock/... # minutes, so a single wedged create eats most of a 30m package timeout before # the tests still running get to report. e2e-rds: ; $(MAKE) e2e PKG=./rds/... TIMEOUT=45m +e2e-quota: ; $(MAKE) e2e PKG=./quota/... e2e-all: ; $(MAKE) e2e e2e-build: diff --git a/tests/e2e/harness/quota.go b/tests/e2e/harness/quota.go new file mode 100644 index 000000000..23212b80e --- /dev/null +++ b/tests/e2e/harness/quota.go @@ -0,0 +1,318 @@ +//go:build e2e + +package harness + +import ( + "context" + "crypto/tls" + "encoding/base64" + "fmt" + "net" + "os" + "os/exec" + "path/filepath" + "regexp" + "strconv" + "strings" + "testing" + "time" + + handlers_quota "github.com/mulgadc/spinifex/spinifex/handlers/quota" +) + +// QuotaLimits is the [quota] baseline a suite installs on the cluster. Every +// field is written, so a zero value denies that dimension outright rather than +// inheriting anything — pass a complete set. +type QuotaLimits struct { + VCPUs int + VPCs int + Subnets int + EIPs int + Volumes int + VolumesGiB int + RDSInstances int + LoadBalancers int +} + +// SandboxQuotaLimits mirrors the baseline shipped in the awsgw.toml template, +// which is what a self-service account gets in production. A suite asserting +// against these numbers is asserting against what a real tenant sees. +func SandboxQuotaLimits() QuotaLimits { + return QuotaLimits{ + VCPUs: 16, VPCs: 4, Subnets: 16, EIPs: 4, + Volumes: 16, VolumesGiB: 200, RDSInstances: 2, LoadBalancers: 2, + } +} + +// QuotaConfigPath returns the awsgw config file holding the [quota] block. +func QuotaConfigPath(env *Env) string { + return filepath.Join(env.ConfigDir, "awsgw", "awsgw.toml") +} + +// awsgwUnit is the systemd unit read the [quota] block at startup. +const awsgwUnit = "spinifex-awsgw" + +// EnableQuota turns per-account quotas on across the cluster and restarts every +// gateway so the new block takes effect. The returned function puts the +// original config back and restarts again — a suite that leaves quotas on caps +// every suite that follows it on the same VM, so it must always run. +// +// Restore reports through an error rather than the *testing.T: it belongs in +// TestMain, after every test has finished, where logging to a t would panic. +func EnableQuota(t *testing.T, env *Env, limits QuotaLimits) func() error { + t.Helper() + if env.ConfigDir == "" { + t.Fatalf("EnableQuota: no config dir located; set SPINIFEX_CONFIG_DIR") + } + path := QuotaConfigPath(env) + hosts := quotaHosts(env) + + saved := make(map[string]string, len(hosts)) + for _, h := range hosts { + body, err := readNodeFile(h, path) + if err != nil { + t.Fatalf("EnableQuota: read %s on %s: %v", path, hostLabel(h), err) + } + saved[h] = body + } + + for _, h := range hosts { + if err := writeNodeFile(h, path, withQuotaBlock(saved[h], limits)); err != nil { + t.Fatalf("EnableQuota: write %s on %s: %v", path, hostLabel(h), err) + } + if err := restartGateway(env, h); err != nil { + t.Fatalf("EnableQuota: restart gateway on %s: %v", hostLabel(h), err) + } + } + + return func() error { + // Every host is attempted even after one fails: a cluster left with the + // suite's quota block on some nodes and not others is worse than either. + var failures []string + for _, h := range hosts { + if err := writeNodeFile(h, path, saved[h]); err != nil { + failures = append(failures, fmt.Sprintf("write %s on %s: %v", path, hostLabel(h), err)) + continue + } + if err := restartGateway(env, h); err != nil { + failures = append(failures, fmt.Sprintf("restart gateway on %s: %v", hostLabel(h), err)) + } + } + if len(failures) > 0 { + return fmt.Errorf("restore quota config: %s", strings.Join(failures, "; ")) + } + return nil + } +} + +// quotaHosts returns the hosts whose gateway config must be patched. The empty +// string means this machine, which is where the single-node suites run. +func quotaHosts(env *Env) []string { + if env.Mode != ModeMultinode { + return []string{""} + } + cluster, err := ClusterFromEnv() + if err != nil { + return []string{""} + } + hosts := make([]string, 0, len(cluster.Nodes)) + for _, n := range cluster.Nodes { + hosts = append(hosts, n.Addr) + } + return hosts +} + +func hostLabel(host string) string { + if host == "" { + return "localhost" + } + return host +} + +// runNodeShell runs script on host, or on this machine when host is empty. +func runNodeShell(host, script string) ([]byte, error) { + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) + defer cancel() + if host != "" { + return NewPeerSSH().Run(ctx, host, script) + } + out, err := exec.CommandContext(ctx, "bash", "-c", script).CombinedOutput() + if err != nil { + return out, fmt.Errorf("local shell: %w: %s", err, strings.TrimSpace(string(out))) + } + return out, nil +} + +func readNodeFile(host, path string) (string, error) { + out, err := runNodeShell(host, "sudo -n cat -- "+ShellQuote(path)) + return string(out), err +} + +// writeNodeFile replaces path's contents. The body travels base64-encoded so a +// single command string carries it intact over SSH, quoting and newlines alike. +func writeNodeFile(host, path, body string) error { + script := fmt.Sprintf("printf %%s %s | base64 -d | sudo -n tee -- %s >/dev/null", + ShellQuote(base64.StdEncoding.EncodeToString([]byte(body))), ShellQuote(path)) + _, err := runNodeShell(host, script) + return err +} + +func restartGateway(env *Env, host string) error { + if _, err := runNodeShell(host, "sudo -n systemctl restart "+awsgwUnit); err != nil { + return err + } + addr := host + if addr == "" { + addr = gatewayDialHost(env) + } + return waitGatewayListening(addr, env.AWSGWPort, 90*time.Second) +} + +// gatewayDialHost picks an address the local gateway answers on. ServiceIPs[0] +// is the awsgw bind IP, so it is correct even where the listener is not on +// loopback. +func gatewayDialHost(env *Env) string { + if len(env.ServiceIPs) > 0 && env.ServiceIPs[0] != "" { + return env.ServiceIPs[0] + } + return "127.0.0.1" +} + +// waitGatewayListening polls until the gateway completes a TLS handshake. The +// certificate is not checked: this is a readiness probe, and the cert suite is +// what asserts the chain. +func waitGatewayListening(host string, port int, timeout time.Duration) error { + deadline := time.Now().Add(timeout) + dialer := &net.Dialer{Timeout: 5 * time.Second} + target := net.JoinHostPort(host, strconv.Itoa(port)) + var last error + for time.Now().Before(deadline) { + conn, err := tls.DialWithDialer(dialer, "tcp", target, + &tls.Config{InsecureSkipVerify: true}) //nolint:gosec // readiness probe, not a trust decision + if err == nil { + _ = conn.Close() + return nil + } + last = err + time.Sleep(time.Second) + } + return fmt.Errorf("gateway %s did not accept connections within %s: %w", target, timeout, last) +} + +// withQuotaBlock returns cfg with its [quota] table replaced by limits. The old +// table is dropped whole — editing keys in place would leave a stale key behind +// whenever the block being replaced carries one the new block does not. +func withQuotaBlock(cfg string, limits QuotaLimits) string { + quotaTable := regexp.MustCompile(`^\s*\[quota(\.[^\]]+)?\]\s*$`) + + var kept []string + skipping := false + for _, line := range strings.Split(cfg, "\n") { + if strings.HasPrefix(strings.TrimSpace(line), "[") { + skipping = quotaTable.MatchString(line) + } + if !skipping { + kept = append(kept, line) + } + } + + return strings.TrimRight(strings.Join(kept, "\n"), "\n") + "\n\n" + fmt.Sprintf( + `[quota] +enabled = true +vcpus = %d +vpcs = %d +subnets = %d +eips = %d +volumes = %d +volumes_gib = %d +rds_instances = %d +load_balancers = %d +`, + limits.VCPUs, limits.VPCs, limits.Subnets, limits.EIPs, + limits.Volumes, limits.VolumesGiB, limits.RDSInstances, limits.LoadBalancers) +} + +// spxAdminEnv adds the AWS profile the /admin surface signs with. The quota +// subcommands reach the gateway over SigV4 rather than NATS, so they need a +// credential where the account commands need none. +func spxAdminEnv() []string { + profile := os.Getenv("AWS_PROFILE") + if profile == "" { + profile = "spinifex" + } + return append(spxChildEnv(), "AWS_PROFILE="+profile) +} + +// SpxAdminQuotaSet runs `spx admin account quota set` for one account. args are +// the dimension flags, e.g. "--vcpus", "8". +func SpxAdminQuotaSet(t *testing.T, accountID string, args ...string) string { + t.Helper() + return spxAdminQuota(t, append([]string{"set", accountID}, args...)...) +} + +// SpxAdminQuotaClear drops every override, returning the account to the +// configured baseline. +func SpxAdminQuotaClear(t *testing.T, accountID string) string { + t.Helper() + return spxAdminQuota(t, "set", accountID, "--clear") +} + +// ClearAccountQuota is SpxAdminQuotaClear for teardown that outlives the test +// that registered it, where reporting through a *testing.T would panic. +func ClearAccountQuota(accountID string) error { + _, err := runSpxAdminQuota("set", accountID, "--clear") + return err +} + +// quotaRow matches one line of the table `spx admin account quota get` prints: +// dimension, limit, and the layer the limit came from. +var quotaRow = regexp.MustCompile(`^([a-z_]+)\s+(\S+)\s+(config|override)\s*$`) + +// SpxAdminQuotaGet returns the limits in force for an account and, per +// dimension, whether each was inherited from the config or set as an override. +func SpxAdminQuotaGet(t *testing.T, accountID string) (limits map[string]int, source map[string]string) { + t.Helper() + out := spxAdminQuota(t, "get", accountID) + + limits, source = map[string]int{}, map[string]string{} + for _, line := range strings.Split(out, "\n") { + m := quotaRow.FindStringSubmatch(strings.TrimSpace(line)) + if m == nil { + continue + } + value := handlers_quota.Unlimited + if m[2] != "unlimited" { + parsed, err := strconv.Atoi(m[2]) + if err != nil { + t.Fatalf("quota get %s: unparseable limit %q for %s", accountID, m[2], m[1]) + } + value = parsed + } + limits[m[1]], source[m[1]] = value, m[3] + } + if len(limits) == 0 { + t.Fatalf("quota get %s: no dimensions parsed from:\n%s", accountID, out) + } + return limits, source +} + +func spxAdminQuota(t *testing.T, args ...string) string { + t.Helper() + out, err := runSpxAdminQuota(args...) + if err != nil { + t.Fatalf("%v", err) + } + return out +} + +func runSpxAdminQuota(args ...string) (string, error) { + full := append([]string{"admin", "account", "quota"}, args...) + cmd := exec.Command(SpxBin(), full...) + cmd.Env = spxAdminEnv() + out, err := cmd.CombinedOutput() + if err != nil { + return string(out), fmt.Errorf("spx %s failed: %w\noutput:\n%s", + strings.Join(full, " "), err, out) + } + return string(out), nil +} diff --git a/tests/e2e/quota/helpers_test.go b/tests/e2e/quota/helpers_test.go new file mode 100644 index 000000000..4ce2e3032 --- /dev/null +++ b/tests/e2e/quota/helpers_test.go @@ -0,0 +1,245 @@ +//go:build e2e + +package quota + +import ( + "fmt" + "strconv" + "testing" + "time" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/mulgadc/spinifex/tests/e2e/harness" + "github.com/stretchr/testify/require" +) + +// quotaExceeded is the code every enforced dimension refuses with. Asserting on +// it rather than on "an error" is what separates a quota denial from a create +// that failed for an unrelated reason and would otherwise read as a pass. +const quotaExceeded = "ResourceLimitExceeded" + +// setLimit overrides one dimension on an account for the duration of the test +// and clears every override afterwards, so each test starts from the baseline +// regardless of the order they run in. +func setLimit(t *testing.T, accountID, dimension string, value int) { + t.Helper() + harness.SpxAdminQuotaSet(t, accountID, "--"+dimension, strconv.Itoa(value)) + t.Cleanup(func() { harness.SpxAdminQuotaClear(t, accountID) }) +} + +// headroom resolves what an account already holds and caps the dimension one +// above it. Every limit test is then about the boundary rather than about the +// state the suite happened to inherit. +func headroom(t *testing.T, accountID, dimension string, used int) { + t.Helper() + harness.Detail(t, "dimension", dimension, "used", used, "limit", used+1) + setLimit(t, accountID, dimension, used+1) +} + +func itoa(v int) string { return strconv.Itoa(v) } + +func countVPCs(t *testing.T, c *harness.AWSClient) int { + t.Helper() + out, err := c.EC2.DescribeVpcs(&ec2.DescribeVpcsInput{}) + require.NoError(t, err, "describe-vpcs") + return len(out.Vpcs) +} + +func countSubnets(t *testing.T, c *harness.AWSClient) int { + t.Helper() + out, err := c.EC2.DescribeSubnets(&ec2.DescribeSubnetsInput{}) + require.NoError(t, err, "describe-subnets") + return len(out.Subnets) +} + +func countAddresses(t *testing.T, c *harness.AWSClient) int { + t.Helper() + out, err := c.EC2.DescribeAddresses(&ec2.DescribeAddressesInput{}) + require.NoError(t, err, "describe-addresses") + return len(out.Addresses) +} + +// volumeUsage returns the account's volume count and the GiB they sum to, the +// two independently capped facts about EBS. +func volumeUsage(t *testing.T, c *harness.AWSClient) (count, gib int) { + t.Helper() + out, err := c.EC2.DescribeVolumes(&ec2.DescribeVolumesInput{}) + require.NoError(t, err, "describe-volumes") + for _, v := range out.Volumes { + gib += int(aws.Int64Value(v.Size)) + } + return len(out.Volumes), gib +} + +// createVPC creates a VPC and registers its removal. The caller gets the ID so +// it can delete early, which is how the release assertions are written. +func createVPC(t *testing.T, c *harness.AWSClient, cidr string) string { + t.Helper() + // The create path is the subject under test, so it cannot route through a + // fixture that would hide the call being counted. e2e:allow-create + out, err := c.EC2.CreateVpc(&ec2.CreateVpcInput{CidrBlock: aws.String(cidr)}) + require.NoError(t, err, "create-vpc %s", cidr) + id := aws.StringValue(out.Vpc.VpcId) + require.NotEmpty(t, id, "create-vpc returned no VpcId") + t.Cleanup(func() { deleteVPC(c, id) }) + return id +} + +func deleteVPC(c *harness.AWSClient, vpcID string) { + _, _ = c.EC2.DeleteVpc(&ec2.DeleteVpcInput{VpcId: aws.String(vpcID)}) +} + +// createVolume creates one EBS volume and registers its removal. +func createVolume(t *testing.T, c *harness.AWSClient, az string, sizeGiB int64) string { + t.Helper() + // e2e:allow-create — the create path is the subject under test. + out, err := c.EC2.CreateVolume(&ec2.CreateVolumeInput{ + AvailabilityZone: aws.String(az), + Size: aws.Int64(sizeGiB), + }) + require.NoError(t, err, "create-volume %d GiB", sizeGiB) + id := aws.StringValue(out.VolumeId) + require.NotEmpty(t, id, "create-volume returned no VolumeId") + t.Cleanup(func() { + _, _ = c.EC2.DeleteVolume(&ec2.DeleteVolumeInput{VolumeId: aws.String(id)}) + }) + return id +} + +// tenantSubnet returns a subnet the tenant can launch into, preferring one of +// the default VPC's so no test pays for a topology it does not assert on. +func tenantSubnet(t *testing.T, c *harness.AWSClient) string { + t.Helper() + out, err := c.EC2.DescribeSubnets(&ec2.DescribeSubnetsInput{}) + require.NoError(t, err, "describe-subnets") + require.NotEmpty(t, out.Subnets, "account has no subnet to launch into") + return aws.StringValue(out.Subnets[0].SubnetId) +} + +// tenantKeyPair creates a key pair for the account and registers its removal. +func tenantKeyPair(t *testing.T, c *harness.AWSClient) string { + t.Helper() + name := fmt.Sprintf("e2e-quota-%d", time.Now().UnixNano()) + // The fixture key pair belongs to the super-admin account; a launch by this + // tenant needs one of its own. e2e:allow-create + _, err := c.EC2.CreateKeyPair(&ec2.CreateKeyPairInput{KeyName: aws.String(name)}) + require.NoError(t, err, "create-key-pair") + t.Cleanup(func() { + _, _ = c.EC2.DeleteKeyPair(&ec2.DeleteKeyPairInput{KeyName: aws.String(name)}) + }) + return name +} + +// launchSpec is everything a tenant launch needs that is fixed for the run. +type launchSpec struct { + amiID string + keyName string + subnet string +} + +// runInstances issues one launch of count instances. It returns the error +// unwrapped so the caller can assert on the code rather than on a wrapper. +func (s launchSpec) runInstances(c *harness.AWSClient, instanceType string, count int64) (*ec2.Reservation, error) { + // The launch is the checked operation; a fixture would memoize it and the + // second, refused launch would never be issued. e2e:allow-create + return c.EC2.RunInstances(&ec2.RunInstancesInput{ + ImageId: aws.String(s.amiID), + InstanceType: aws.String(instanceType), + KeyName: aws.String(s.keyName), + SubnetId: aws.String(s.subnet), + MinCount: aws.Int64(1), + MaxCount: aws.Int64(count), + }) +} + +// launchOne runs a single instance and registers its termination. +func (s launchSpec) launchOne(t *testing.T, c *harness.AWSClient, instanceType string) (string, error) { + t.Helper() + out, err := s.runInstances(c, instanceType, 1) + if err != nil { + return "", err + } + require.NotEmpty(t, out.Instances, "RunInstances returned no instance") + id := aws.StringValue(out.Instances[0].InstanceId) + t.Cleanup(func() { terminateInstance(c, id) }) + return id, nil +} + +func terminateInstance(c *harness.AWSClient, instanceID string) { + _, _ = c.EC2.TerminateInstances(&ec2.TerminateInstancesInput{ + InstanceIds: []*string{aws.String(instanceID)}, + }) +} + +// waitTerminated blocks until the instance reaches terminated. The vCPU counter +// is released by the gateway's reconcile sweep, which reads instance state, so +// polling the state first keeps the release assertion honest about what it is +// waiting for. +func waitTerminated(t *testing.T, c *harness.AWSClient, instanceID string) { + t.Helper() + harness.EventuallyErr(t, func() error { + out, err := c.EC2.DescribeInstances(&ec2.DescribeInstancesInput{ + InstanceIds: []*string{aws.String(instanceID)}, + }) + if err != nil { + return err + } + for _, r := range out.Reservations { + for _, inst := range r.Instances { + if state := aws.StringValue(inst.State.Name); state != "terminated" { + return fmt.Errorf("instance %s state=%s", instanceID, state) + } + } + } + return nil + }, 5*time.Minute, 5*time.Second) +} + +func countInstances(t *testing.T, c *harness.AWSClient) int { + t.Helper() + out, err := c.EC2.DescribeInstances(&ec2.DescribeInstancesInput{}) + require.NoError(t, err, "describe-instances") + live := 0 + for _, r := range out.Reservations { + for _, inst := range r.Instances { + if aws.StringValue(inst.State.Name) != "terminated" { + live++ + } + } + } + return live +} + +// instanceTypeVCPUs returns the vCPUs one instance of instanceType reserves. +func instanceTypeVCPUs(t *testing.T, c *harness.AWSClient, instanceType string) int { + t.Helper() + out, err := c.EC2.DescribeInstanceTypes(&ec2.DescribeInstanceTypesInput{ + InstanceTypes: []*string{aws.String(instanceType)}, + }) + require.NoError(t, err, "describe-instance-types %s", instanceType) + require.NotEmpty(t, out.InstanceTypes, "instance type %s not offered", instanceType) + require.NotNil(t, out.InstanceTypes[0].VCpuInfo, "instance type %s reports no VCpuInfo", instanceType) + return int(aws.Int64Value(out.InstanceTypes[0].VCpuInfo.DefaultVCpus)) +} + +// widestInstanceType returns the offered type with the most vCPUs, which is how +// a tenant would try to consume its whole allowance in one launch. +func widestInstanceType(t *testing.T, c *harness.AWSClient) (string, int) { + t.Helper() + out, err := c.EC2.DescribeInstanceTypes(&ec2.DescribeInstanceTypesInput{}) + require.NoError(t, err, "describe-instance-types") + + var name string + var vcpus int + for _, it := range out.InstanceTypes { + if it.VCpuInfo == nil { + continue + } + if v := int(aws.Int64Value(it.VCpuInfo.DefaultVCpus)); v > vcpus { + name, vcpus = aws.StringValue(it.InstanceType), v + } + } + require.NotEmpty(t, name, "no instance type reports a vCPU count") + return name, vcpus +} diff --git a/tests/e2e/quota/main_test.go b/tests/e2e/quota/main_test.go new file mode 100644 index 000000000..c09ee21f8 --- /dev/null +++ b/tests/e2e/quota/main_test.go @@ -0,0 +1,172 @@ +//go:build e2e + +// Package quota is the per-account service-quota E2E suite. It turns quotas on +// for the duration of the run, gives the super-admin account an unlimited +// override so nothing else on the box is capped, then drives each enforced +// dimension from a freshly created tenant account: the account a self-service +// signup produces, holding the baseline a production sandbox user gets. +// +// Every test resolves the account's current usage before it sets a limit, so an +// assertion is about the boundary rather than about how much the account +// happened to hold when the suite started. +package quota + +import ( + "fmt" + "os" + "sync" + "testing" + "time" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/aws/aws-sdk-go/service/sts" + "github.com/mulgadc/spinifex/tests/e2e/harness" +) + +// Package-scoped singleton fixture, built lazily by requireQuotaFixture so a +// run with SPINIFEX_E2E unset neither dials AWS nor rewrites any config. +var ( + pkgFixOnce sync.Once + pkgFix *Fixture + pkgFixErr error +) + +// TestMain owns process-level lifecycle. Teardown unwinds LIFO, and the order +// is load-bearing: turning quotas back off closes the override bucket, so an +// override still to be removed must go first or its removal cannot be stored. +func TestMain(m *testing.M) { + code := m.Run() + if pkgFix != nil { + if pkgFix.Harness != nil { + if err := pkgFix.Harness.Close(); err != nil { + fmt.Fprintf(os.Stderr, "e2e teardown: %v\n", err) + code = 1 + } + } + for i := len(pkgFix.restore) - 1; i >= 0; i-- { + if err := pkgFix.restore[i](); err != nil { + fmt.Fprintf(os.Stderr, "e2e teardown: %v\n", err) + code = 1 + } + } + } + os.Exit(code) +} + +// Fixture carries the per-process state shared by every Test* in the package. +type Fixture struct { + Env *harness.Env + + // Admin is the super-admin account: the credential the /admin surface + // accepts, and the account whose quota is lifted for the run. + Admin *harness.AWSClient + AdminAccountID string + + // Harness memoizes the AMI, instance type and AZ discovery. Bound to the + // admin client because gold images live in the super-admin account. + Harness *harness.Fixture + + // Baseline is the [quota] block installed on the cluster, which is what an + // account with no override of its own resolves to. + Baseline harness.QuotaLimits + + // Tenant is the account under test; Peer proves an override applies to one + // account and not to its neighbour. + Tenant *harness.Profile + Peer *harness.Profile + + // restore undoes the cluster-level changes the fixture made, in the order + // registered. Each reports an error rather than failing a test: they run + // from TestMain, after the last test has finished. + restore []func() error +} + +// requireQuotaFixture returns the package-scoped Fixture, building it on first +// call. Skips when SPINIFEX_E2E is unset; runs in both single and multinode +// topologies, because the enforcement path is the same gateway code either way. +func requireQuotaFixture(t *testing.T) *Fixture { + t.Helper() + pkgFixOnce.Do(func() { + if os.Getenv("SPINIFEX_E2E") == "" { + return + } + env := harness.LoadEnv(t) + admin := harness.NewAWSClient(t, env) + + ident, err := admin.STS.GetCallerIdentity(&sts.GetCallerIdentityInput{}) + if err != nil { + pkgFixErr = fmt.Errorf("resolve super-admin account: %w", err) + return + } + fx, err := harness.NewProcessFixture(admin) + if err != nil { + pkgFixErr = err + return + } + + baseline := harness.SandboxQuotaLimits() + fix := &Fixture{ + Env: env, + Admin: admin, + AdminAccountID: aws.StringValue(ident.Account), + Harness: fx, + Baseline: baseline, + } + + // Registered before anything can fail below, so a half-built fixture + // still puts the cluster back the way it found it. + fix.restore = append(fix.restore, harness.EnableQuota(t, env, baseline)) + waitAdminSurface(t, fix) + + // Suites share this VM and most of them run as the super-admin account, + // which the baseline would cap at 16 vCPUs. Lifting it is also how a + // production operator exempts an account, so the path is worth using. + liftAdminQuota(t, fix) + + fix.Tenant = newTenant(t, env, "quota-tenant") + fix.Peer = newTenant(t, env, "quota-peer") + pkgFix = fix + }) + if pkgFixErr != nil { + t.Fatalf("quota fixture init failed: %v", pkgFixErr) + } + if pkgFix == nil { + t.Skip("quota fixture unavailable (SPINIFEX_E2E unset)") + } + return pkgFix +} + +// waitAdminSurface polls until the restarted gateway answers a signed request. +// A TLS handshake only proves the listener is back; the quota buckets are +// opened during startup and the first call must not race them. +func waitAdminSurface(t *testing.T, fix *Fixture) { + t.Helper() + harness.EventuallyErr(t, func() error { + _, err := fix.Admin.EC2.DescribeVpcs(&ec2.DescribeVpcsInput{}) + return err + }, 90*time.Second, 2*time.Second) +} + +// liftAdminQuota gives the super-admin account an unlimited override on every +// dimension and registers its removal. +func liftAdminQuota(t *testing.T, fix *Fixture) { + t.Helper() + harness.SpxAdminQuotaSet(t, fix.AdminAccountID, + "--vcpus", "-1", "--vpcs", "-1", "--subnets", "-1", "--eips", "-1", + "--volumes", "-1", "--volumes-gib", "-1", "--rds-instances", "-1", + "--load-balancers", "-1") + fix.restore = append(fix.restore, func() error { + return harness.ClearAccountQuota(fix.AdminAccountID) + }) +} + +// newTenant creates an account the way self-service signup does and returns a +// client scoped to it. Its quota is whatever the account inherits, which is the +// point: a sandbox user is never configured, only defaulted. +func newTenant(t *testing.T, env *harness.Env, label string) *harness.Profile { + t.Helper() + name := fmt.Sprintf("E2E %s %d", label, time.Now().UnixNano()) + info := harness.SpxAdminAccountCreate(t, name, "") + return harness.NewAccountCarousel().Add(t, env, label, info) +} diff --git a/tests/e2e/quota/tests_test.go b/tests/e2e/quota/tests_test.go new file mode 100644 index 000000000..d291fca69 --- /dev/null +++ b/tests/e2e/quota/tests_test.go @@ -0,0 +1,308 @@ +//go:build e2e + +package quota + +import ( + "testing" + "time" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/aws/aws-sdk-go/service/elbv2" + "github.com/aws/aws-sdk-go/service/rds" + "github.com/mulgadc/spinifex/tests/e2e/harness" + "github.com/stretchr/testify/require" +) + +// A sandbox account is never configured, only defaulted, so the numbers it +// inherits are the ones production actually enforces. Every dimension must +// report as inherited: a value that silently arrived as an override would mean +// account creation is writing limits nobody asked it to write. +func TestQuotaBaselineIsInherited(t *testing.T) { + fix := requireQuotaFixture(t) + harness.Phase(t, "Quota — a new account inherits the configured baseline") + + limits, source := harness.SpxAdminQuotaGet(t, fix.Tenant.AccountID) + + want := map[string]int{ + "vcpus": fix.Baseline.VCPUs, + "vpcs": fix.Baseline.VPCs, + "subnets": fix.Baseline.Subnets, + "eips": fix.Baseline.EIPs, + "volumes": fix.Baseline.Volumes, + "volumes_gib": fix.Baseline.VolumesGiB, + "rds_instances": fix.Baseline.RDSInstances, + "load_balancers": fix.Baseline.LoadBalancers, + } + require.Len(t, limits, len(want), "every dimension must be reported") + for dimension, value := range want { + require.Equal(t, value, limits[dimension], "%s limit", dimension) + require.Equal(t, "config", source[dimension], "%s must be inherited, not set", dimension) + } +} + +// The VPC gate, and with it the property the whole live-counted tier rests on: +// deleting a resource releases its allowance with no uncharge path to go wrong. +func TestQuotaVPCLimit(t *testing.T) { + fix := requireQuotaFixture(t) + harness.Phase(t, "Quota — VPCs") + tenant := fix.Tenant.Client + + headroom(t, fix.Tenant.AccountID, "vpcs", countVPCs(t, tenant)) + + harness.Step(t, "the one VPC the account has room for is created") + vpcID := createVPC(t, tenant, "10.180.0.0/16") + + harness.Step(t, "the next is refused") + // e2e:allow-create — the refusal is the assertion. + _, err := tenant.EC2.CreateVpc(&ec2.CreateVpcInput{CidrBlock: aws.String("10.181.0.0/16")}) + harness.AssertAWSError(t, err, quotaExceeded) + + harness.Step(t, "deleting releases the allowance") + deleteVPC(tenant, vpcID) + released := createVPC(t, tenant, "10.182.0.0/16") + require.NotEmpty(t, released, "a VPC deleted under the cap must free room for another") +} + +// Subnets are capped per account rather than per VPC, so the count that matters +// spans every VPC the account holds. +func TestQuotaSubnetLimit(t *testing.T) { + fix := requireQuotaFixture(t) + harness.Phase(t, "Quota — subnets") + tenant := fix.Tenant.Client + + vpcID := createVPC(t, tenant, "10.183.0.0/16") + headroom(t, fix.Tenant.AccountID, "subnets", countSubnets(t, tenant)) + + // e2e:allow-create — the create path is the subject under test. + out, err := tenant.EC2.CreateSubnet(&ec2.CreateSubnetInput{ + VpcId: aws.String(vpcID), + CidrBlock: aws.String("10.183.1.0/24"), + }) + require.NoError(t, err, "the subnet the account has room for") + subnetID := aws.StringValue(out.Subnet.SubnetId) + t.Cleanup(func() { + _, _ = tenant.EC2.DeleteSubnet(&ec2.DeleteSubnetInput{SubnetId: aws.String(subnetID)}) + }) + + // e2e:allow-create — the refusal is the assertion. + _, err = tenant.EC2.CreateSubnet(&ec2.CreateSubnetInput{ + VpcId: aws.String(vpcID), + CidrBlock: aws.String("10.183.2.0/24"), + }) + harness.AssertAWSError(t, err, quotaExceeded) +} + +// Elastic IPs are the scarcest thing a cluster hands out — the pool is finite +// and shared — so this gate is the one that decides how many tenants fit. +func TestQuotaEIPLimit(t *testing.T) { + fix := requireQuotaFixture(t) + harness.Phase(t, "Quota — Elastic IPs") + tenant := fix.Tenant.Client + + headroom(t, fix.Tenant.AccountID, "eips", countAddresses(t, tenant)) + + // e2e:allow-create — the allocation is the checked operation. + alloc, err := tenant.EC2.AllocateAddress(&ec2.AllocateAddressInput{Domain: aws.String("vpc")}) + require.NoError(t, err, "the address the account has room for") + allocID := aws.StringValue(alloc.AllocationId) + released := false + t.Cleanup(func() { + if !released { + _, _ = tenant.EC2.ReleaseAddress(&ec2.ReleaseAddressInput{AllocationId: aws.String(allocID)}) + } + }) + + // e2e:allow-create — the refusal is the assertion. + _, err = tenant.EC2.AllocateAddress(&ec2.AllocateAddressInput{Domain: aws.String("vpc")}) + harness.AssertAWSError(t, err, quotaExceeded) + + harness.Step(t, "releasing the address frees the allowance") + _, err = tenant.EC2.ReleaseAddress(&ec2.ReleaseAddressInput{AllocationId: aws.String(allocID)}) + require.NoError(t, err, "release-address") + released = true + + // e2e:allow-create — proves the release, not the allocation. + regained, err := tenant.EC2.AllocateAddress(&ec2.AllocateAddressInput{Domain: aws.String("vpc")}) + require.NoError(t, err, "a released address must free room for another") + t.Cleanup(func() { + _, _ = tenant.EC2.ReleaseAddress(&ec2.ReleaseAddressInput{AllocationId: regained.AllocationId}) + }) +} + +// EBS is capped twice over, and the two caps catch different abuse: a tenant can +// exhaust per-volume overhead long before it exhausts capacity. Each must be +// able to refuse a create on its own. +func TestQuotaVolumeLimits(t *testing.T) { + fix := requireQuotaFixture(t) + harness.Phase(t, "Quota — EBS volumes") + tenant := fix.Tenant.Client + az := harness.DiscoverDefaultAZ(t, fix.Harness) + + t.Run("count", func(t *testing.T) { + count, _ := volumeUsage(t, tenant) + harness.SpxAdminQuotaSet(t, fix.Tenant.AccountID, + "--volumes", itoa(count+1), "--volumes-gib", "-1") + t.Cleanup(func() { harness.SpxAdminQuotaClear(t, fix.Tenant.AccountID) }) + + volID := createVolume(t, tenant, az, 1) + require.NotEmpty(t, volID) + + // e2e:allow-create — the refusal is the assertion. + _, err := tenant.EC2.CreateVolume(&ec2.CreateVolumeInput{ + AvailabilityZone: aws.String(az), Size: aws.Int64(1), + }) + harness.AssertAWSError(t, err, quotaExceeded) + }) + + t.Run("capacity", func(t *testing.T) { + _, gib := volumeUsage(t, tenant) + harness.SpxAdminQuotaSet(t, fix.Tenant.AccountID, + "--volumes", "-1", "--volumes-gib", itoa(gib+1)) + t.Cleanup(func() { harness.SpxAdminQuotaClear(t, fix.Tenant.AccountID) }) + + require.NotEmpty(t, createVolume(t, tenant, az, 1), "the GiB the account has room for") + + // e2e:allow-create — the refusal is the assertion. + _, err := tenant.EC2.CreateVolume(&ec2.CreateVolumeInput{ + AvailabilityZone: aws.String(az), Size: aws.Int64(1), + }) + harness.AssertAWSError(t, err, quotaExceeded) + }) +} + +// The headline gate. A sandbox account is bounded by total vCPUs rather than by +// instance count, so it may spend its 16 however it likes — but not in one +// oversized launch, and not past the cap one nano at a time. +func TestQuotaVCPULimit(t *testing.T) { + fix := requireQuotaFixture(t) + harness.Phase(t, "Quota — EC2 vCPUs") + tenant := fix.Tenant.Client + + nanoType, arch := harness.DiscoverNanoInstanceType(t, fix.Harness) + spec := launchSpec{ + amiID: harness.DiscoverUbuntuAMI(t, fix.Harness, arch), + keyName: tenantKeyPair(t, tenant), + subnet: tenantSubnet(t, tenant), + } + + t.Run("one launch cannot exceed the cap", func(t *testing.T) { + wideType, wideVCPUs := widestInstanceType(t, tenant) + count := int64(fix.Baseline.VCPUs/wideVCPUs) + 1 + harness.Detail(t, "type", wideType, "vcpus_each", wideVCPUs, + "count", count, "cap", fix.Baseline.VCPUs) + + setLimit(t, fix.Tenant.AccountID, "vcpus", fix.Baseline.VCPUs) + before := countInstances(t, tenant) + + _, err := spec.runInstances(tenant, wideType, count) + harness.AssertAWSError(t, err, quotaExceeded) + + require.Equal(t, before, countInstances(t, tenant), + "a refused launch must not leave an instance behind") + }) + + t.Run("the cap is reached one instance at a time", func(t *testing.T) { + nanoVCPUs := instanceTypeVCPUs(t, tenant, nanoType) + setLimit(t, fix.Tenant.AccountID, "vcpus", nanoVCPUs) + + instID, err := spec.launchOne(t, tenant, nanoType) + require.NoError(t, err, "the instance the account has room for") + + _, err = spec.runInstances(tenant, nanoType, 1) + harness.AssertAWSError(t, err, quotaExceeded) + + harness.Step(t, "terminating releases the reserved vCPUs") + terminateInstance(tenant, instID) + waitTerminated(t, tenant, instID) + + // The counter is released by the gateway's periodic reconcile rather + // than by the terminate itself, so the retry window spans a sweep. A + // refused attempt launches nothing, so polling costs only the denials. + harness.EventuallyErr(t, func() error { + _, rerr := spec.launchOne(t, tenant, nanoType) + return rerr + }, 3*time.Minute, 10*time.Second) + }) +} + +// Load balancers are refused at the gateway before any subnet is resolved, so +// the two halves of this test differ only in the limit: at zero the quota is +// what refuses, and uncapped the same call gets far enough to fail on its own +// bad input. That contrast is what proves the quota was the cause. +func TestQuotaLoadBalancerLimit(t *testing.T) { + fix := requireQuotaFixture(t) + harness.Phase(t, "Quota — load balancers") + tenant := fix.Tenant.Client + + create := func() error { + // The gate runs before the input is validated, which is what lets this + // assert the limit without provisioning a balancer. e2e:allow-create + _, err := tenant.ELBv2.CreateLoadBalancer(&elbv2.CreateLoadBalancerInput{ + Name: aws.String("e2e-quota-lb"), + Type: aws.String("network"), + Subnets: []*string{aws.String("subnet-00000000000000000")}, + }) + return err + } + + setLimit(t, fix.Tenant.AccountID, "load-balancers", 0) + harness.AssertAWSError(t, create(), quotaExceeded) + + harness.SpxAdminQuotaSet(t, fix.Tenant.AccountID, "--load-balancers", "-1") + err := create() + require.Error(t, err, "the bogus subnet must still be refused") + require.False(t, harness.ErrorCodeIs(err, quotaExceeded), + "an uncapped account must get past the quota gate, got: %v", err) +} + +// RDS is capped by instance count rather than by vCPUs: the engine VM runs in +// the system account, so a tenant's database consumes none of the tenant's own +// vCPU allowance and the count is the only thing bounding it. +func TestQuotaRDSInstanceLimit(t *testing.T) { + fix := requireQuotaFixture(t) + harness.Phase(t, "Quota — RDS instances") + tenant := fix.Tenant.Client + + create := func() error { + // The gate runs before the input is validated, so the limit is asserted + // without booting a database. + _, err := tenant.RDS.CreateDBInstance(&rds.CreateDBInstanceInput{ //nolint:staticcheck // e2e:allow-create + DBInstanceIdentifier: aws.String("e2e-quota-db"), + DBInstanceClass: aws.String("db.t3.micro"), + Engine: aws.String("mariadb"), + MasterUsername: aws.String("admin"), + MasterUserPassword: aws.String("e2e-quota-password"), + AllocatedStorage: aws.Int64(1), + DBSubnetGroupName: aws.String("e2e-quota-missing-subnet-group"), + }) + return err + } + + setLimit(t, fix.Tenant.AccountID, "rds-instances", 0) + harness.AssertAWSError(t, create(), quotaExceeded) + + harness.SpxAdminQuotaSet(t, fix.Tenant.AccountID, "--rds-instances", "-1") + err := create() + require.Error(t, err, "the missing subnet group must still be refused") + require.False(t, harness.ErrorCodeIs(err, quotaExceeded), + "an uncapped account must get past the quota gate, got: %v", err) +} + +// A limit is a property of one account, not of the gateway. Two tenants served +// by the same gateway, one capped and one not, is the whole reason overrides +// exist: a special customer is raised without loosening anybody else. +func TestQuotaOverrideIsPerAccount(t *testing.T) { + fix := requireQuotaFixture(t) + harness.Phase(t, "Quota — an override applies to one account only") + + setLimit(t, fix.Tenant.AccountID, "vpcs", 0) + + // e2e:allow-create — the refusal is the assertion. + _, err := fix.Tenant.Client.EC2.CreateVpc(&ec2.CreateVpcInput{CidrBlock: aws.String("10.184.0.0/16")}) + harness.AssertAWSError(t, err, quotaExceeded) + + harness.Step(t, "the neighbouring account is untouched") + require.NotEmpty(t, createVPC(t, fix.Peer.Client, "10.185.0.0/16"), + "one account's override must not cap another") +} From 19ec127e2a791b660559f2bd89aa6669fbe00ff6 Mon Sep 17 00:00:00 2001 From: Ben Duncan <5178243+benduncan@users.noreply.github.com> Date: Thu, 20 Aug 2026 02:43:44 +1000 Subject: [PATCH 4/8] test: charge a multi-instance launch for every instance it asks for --- tests/e2e/quota/helpers_test.go | 14 ++++++++++ tests/e2e/quota/tests_test.go | 48 +++++++++++++++++++++++---------- 2 files changed, 48 insertions(+), 14 deletions(-) diff --git a/tests/e2e/quota/helpers_test.go b/tests/e2e/quota/helpers_test.go index 4ce2e3032..d343d219e 100644 --- a/tests/e2e/quota/helpers_test.go +++ b/tests/e2e/quota/helpers_test.go @@ -166,6 +166,20 @@ func (s launchSpec) launchOne(t *testing.T, c *harness.AWSClient, instanceType s return id, nil } +// registerTerminate queues every instance in a reservation for termination and +// returns their IDs. +func registerTerminate(t *testing.T, c *harness.AWSClient, reservation *ec2.Reservation) []string { + t.Helper() + ids := make([]string, 0, len(reservation.Instances)) + for _, inst := range reservation.Instances { + id := aws.StringValue(inst.InstanceId) + require.NotEmpty(t, id, "RunInstances returned an instance with no ID") + ids = append(ids, id) + t.Cleanup(func() { terminateInstance(c, id) }) + } + return ids +} + func terminateInstance(c *harness.AWSClient, instanceID string) { _, _ = c.EC2.TerminateInstances(&ec2.TerminateInstancesInput{ InstanceIds: []*string{aws.String(instanceID)}, diff --git a/tests/e2e/quota/tests_test.go b/tests/e2e/quota/tests_test.go index d291fca69..2eae70618 100644 --- a/tests/e2e/quota/tests_test.go +++ b/tests/e2e/quota/tests_test.go @@ -172,49 +172,69 @@ func TestQuotaVolumeLimits(t *testing.T) { } // The headline gate. A sandbox account is bounded by total vCPUs rather than by -// instance count, so it may spend its 16 however it likes — but not in one -// oversized launch, and not past the cap one nano at a time. +// instance count, so it may spend its 16 however it likes — but not on one +// instance too wide to fit, not on a count of instances that sums past the cap, +// and not past the cap one at a time. func TestQuotaVCPULimit(t *testing.T) { fix := requireQuotaFixture(t) harness.Phase(t, "Quota — EC2 vCPUs") tenant := fix.Tenant.Client nanoType, arch := harness.DiscoverNanoInstanceType(t, fix.Harness) + nanoVCPUs := instanceTypeVCPUs(t, tenant, nanoType) spec := launchSpec{ amiID: harness.DiscoverUbuntuAMI(t, fix.Harness, arch), keyName: tenantKeyPair(t, tenant), subnet: tenantSubnet(t, tenant), } - t.Run("one launch cannot exceed the cap", func(t *testing.T) { + t.Run("one instance wider than the cap is refused", func(t *testing.T) { wideType, wideVCPUs := widestInstanceType(t, tenant) - count := int64(fix.Baseline.VCPUs/wideVCPUs) + 1 - harness.Detail(t, "type", wideType, "vcpus_each", wideVCPUs, - "count", count, "cap", fix.Baseline.VCPUs) + require.Greater(t, wideVCPUs, fix.Baseline.VCPUs, + "the widest offered type must exceed the cap or this asserts nothing") + harness.Detail(t, "type", wideType, "vcpus", wideVCPUs, "cap", fix.Baseline.VCPUs) setLimit(t, fix.Tenant.AccountID, "vcpus", fix.Baseline.VCPUs) before := countInstances(t, tenant) - _, err := spec.runInstances(tenant, wideType, count) + _, err := spec.runInstances(tenant, wideType, 1) harness.AssertAWSError(t, err, quotaExceeded) require.Equal(t, before, countInstances(t, tenant), "a refused launch must not leave an instance behind") }) - t.Run("the cap is reached one instance at a time", func(t *testing.T) { - nanoVCPUs := instanceTypeVCPUs(t, tenant, nanoType) - setLimit(t, fix.Tenant.AccountID, "vcpus", nanoVCPUs) + // The count is multiplied by the type's vCPUs before the cap is consulted, + // so a type that fits on its own must still be refused in bulk. Asserting + // only the refusal would pass on a gateway that refuses everything, which is + // why the same type is then launched at a count that does fit. + t.Run("a launch is charged for every instance it asks for", func(t *testing.T) { + const room = 2 + require.Zero(t, countInstances(t, tenant), "the tenant must start this subtest idle") + harness.Detail(t, "type", nanoType, "vcpus_each", nanoVCPUs, + "cap", room*nanoVCPUs, "refused_count", room+1) - instID, err := spec.launchOne(t, tenant, nanoType) - require.NoError(t, err, "the instance the account has room for") + setLimit(t, fix.Tenant.AccountID, "vcpus", room*nanoVCPUs) + _, err := spec.runInstances(tenant, nanoType, room+1) + harness.AssertAWSError(t, err, quotaExceeded) + + out, err := spec.runInstances(tenant, nanoType, room) + require.NoError(t, err, "a request that fits must be allowed") + require.Len(t, out.Instances, room, "a permitted launch must deliver every instance") + launched := registerTerminate(t, tenant, out) + + harness.Step(t, "the account is now at its cap") _, err = spec.runInstances(tenant, nanoType, 1) harness.AssertAWSError(t, err, quotaExceeded) harness.Step(t, "terminating releases the reserved vCPUs") - terminateInstance(tenant, instID) - waitTerminated(t, tenant, instID) + for _, id := range launched { + terminateInstance(tenant, id) + } + for _, id := range launched { + waitTerminated(t, tenant, id) + } // The counter is released by the gateway's periodic reconcile rather // than by the terminate itself, so the retry window spans a sweep. A From 0d8425ef51430dac17c719ab535ccd81cca43f5b Mon Sep 17 00:00:00 2001 From: Ben Duncan <5178243+benduncan@users.noreply.github.com> Date: Thu, 20 Aug 2026 03:31:40 +1000 Subject: [PATCH 5/8] test: skip the EIP quota leg where a cell does not serve EIPs from every node --- tests/e2e/quota/helpers_test.go | 12 ++++++++++++ tests/e2e/quota/tests_test.go | 25 +++++++++++++++++++------ 2 files changed, 31 insertions(+), 6 deletions(-) diff --git a/tests/e2e/quota/helpers_test.go b/tests/e2e/quota/helpers_test.go index d343d219e..d857a5949 100644 --- a/tests/e2e/quota/helpers_test.go +++ b/tests/e2e/quota/helpers_test.go @@ -39,6 +39,18 @@ func headroom(t *testing.T, accountID, dimension string, used int) { func itoa(v int) string { return strconv.Itoa(v) } +// skipIfEIPUnsupported turns the disabled-EIP stub's refusal into a skip. A node +// with no external IPAM answers allocations with UnsupportedOperation and +// describes with an empty list, so where a cell's nodes do not all hold a pool +// there is no count for the cap to be asserted against. On a cluster that does +// serve EIPs the quota refuses first and this never fires. +func skipIfEIPUnsupported(t *testing.T, err error) { + t.Helper() + if harness.ErrorCodeIs(err, "UnsupportedOperation") { + t.Skip("external IPAM is not served by every node in this cell; EIP allocation is unsupported") + } +} + func countVPCs(t *testing.T, c *harness.AWSClient) int { t.Helper() out, err := c.EC2.DescribeVpcs(&ec2.DescribeVpcsInput{}) diff --git a/tests/e2e/quota/tests_test.go b/tests/e2e/quota/tests_test.go index 2eae70618..b16ad1598 100644 --- a/tests/e2e/quota/tests_test.go +++ b/tests/e2e/quota/tests_test.go @@ -95,15 +95,28 @@ func TestQuotaSubnetLimit(t *testing.T) { // Elastic IPs are the scarcest thing a cluster hands out — the pool is finite // and shared — so this gate is the one that decides how many tenants fit. +// +// Both AllocateAddress and DescribeAddresses are queue-group requests answered +// by whichever node picks them up, and a node with no external IPAM answers +// from the disabled stub: an empty address list, and UnsupportedOperation on a +// mutation. Where a cell's nodes do not all hold a pool the count this cap reads +// is whichever node replied, so the boundary is not assertable — hence the skip +// rather than a failure. A cluster with one static pool, which is what +// production runs, answers identically from every node. func TestQuotaEIPLimit(t *testing.T) { fix := requireQuotaFixture(t) harness.Phase(t, "Quota — Elastic IPs") tenant := fix.Tenant.Client + allocate := func() (*ec2.AllocateAddressOutput, error) { + // e2e:allow-create — the allocation is the checked operation. + return tenant.EC2.AllocateAddress(&ec2.AllocateAddressInput{Domain: aws.String("vpc")}) + } + headroom(t, fix.Tenant.AccountID, "eips", countAddresses(t, tenant)) - // e2e:allow-create — the allocation is the checked operation. - alloc, err := tenant.EC2.AllocateAddress(&ec2.AllocateAddressInput{Domain: aws.String("vpc")}) + alloc, err := allocate() + skipIfEIPUnsupported(t, err) require.NoError(t, err, "the address the account has room for") allocID := aws.StringValue(alloc.AllocationId) released := false @@ -113,8 +126,8 @@ func TestQuotaEIPLimit(t *testing.T) { } }) - // e2e:allow-create — the refusal is the assertion. - _, err = tenant.EC2.AllocateAddress(&ec2.AllocateAddressInput{Domain: aws.String("vpc")}) + _, err = allocate() + skipIfEIPUnsupported(t, err) harness.AssertAWSError(t, err, quotaExceeded) harness.Step(t, "releasing the address frees the allowance") @@ -122,8 +135,8 @@ func TestQuotaEIPLimit(t *testing.T) { require.NoError(t, err, "release-address") released = true - // e2e:allow-create — proves the release, not the allocation. - regained, err := tenant.EC2.AllocateAddress(&ec2.AllocateAddressInput{Domain: aws.String("vpc")}) + regained, err := allocate() + skipIfEIPUnsupported(t, err) require.NoError(t, err, "a released address must free room for another") t.Cleanup(func() { _, _ = tenant.EC2.ReleaseAddress(&ec2.ReleaseAddressInput{AllocationId: regained.AllocationId}) From 4508193180288af270911b5f0af01381f976c47b Mon Sep 17 00:00:00 2001 From: Ben Duncan <5178243+benduncan@users.noreply.github.com> Date: Thu, 20 Aug 2026 03:48:45 +1000 Subject: [PATCH 6/8] fix: fail daemon start when a node with public pools cannot build external IPAM --- spinifex/daemon/daemon.go | 116 +++++++++++++++++--------------- tests/e2e/quota/helpers_test.go | 15 ++--- tests/e2e/quota/tests_test.go | 16 ++--- 3 files changed, 75 insertions(+), 72 deletions(-) diff --git a/spinifex/daemon/daemon.go b/spinifex/daemon/daemon.go index e7e9d46da..27666dce9 100644 --- a/spinifex/daemon/daemon.go +++ b/spinifex/daemon/daemon.go @@ -1402,6 +1402,33 @@ func publicExternalPools(pools []config.ExternalPool) []config.ExternalPool { return public } +// externalPoolConfigs converts the node's public pools into IPAM pool configs +// and reports whether any of them leases from DHCP. The transit pool never +// enters IPAM — its addresses are gateway-LRP plumbing, not EIPs. +func (d *Daemon) externalPoolConfigs() (pools []external.ExternalPoolConfig, anyDHCP bool) { + for _, p := range publicExternalPools(d.clusterConfig.Network.ExternalPools) { + pools = append(pools, external.ExternalPoolConfig{ + Name: p.Name, + Source: p.Source, + BindBridge: p.BindBridge, + DHCPMAC: p.DHCPMAC, + RangeStart: p.RangeStart, + RangeEnd: p.RangeEnd, + Gateway: p.Gateway, + GatewayIP: p.GatewayIP, + PrefixLen: p.PrefixLen, + Region: p.Region, + AZ: p.AZ, + GwLrpRangeStart: p.GwLrpRangeStart, + GwLrpRangeEnd: p.GwLrpRangeEnd, + }) + if p.Source == "dhcp" { + anyDHCP = true + } + } + return pools, anyDHCP +} + // hasPublicIPPools reports whether the cluster can allocate routable public // IPs: pool mode always, nat mode only with a public pool beside the transit. func (d *Daemon) hasPublicIPPools() bool { @@ -1615,74 +1642,57 @@ func (d *Daemon) startCluster() error { return fmt.Errorf("failed to initialize NatGateway service: %w", err) } - // Initialize external IPAM when public IP pools exist (pool mode, or nat - // mode with a public pool alongside the transit segment). The transit pool - // never enters IPAM — its addresses are gateway-LRP plumbing, not EIPs. + // A node declaring public pools must serve EIPs or refuse to start: EIP + // subjects are a queue group, so one node silently on the disabled stub + // answers cluster-wide with an empty address list on the requests it wins. if d.hasPublicIPPools() { - js, jsErr := jetstream.New(d.natsConn) - if jsErr != nil { - slog.Warn("Failed to get JetStream for external IPAM", "err", jsErr) - } else { - var pools []external.ExternalPoolConfig - anyDHCP := false - for _, p := range publicExternalPools(d.clusterConfig.Network.ExternalPools) { - pools = append(pools, external.ExternalPoolConfig{ - Name: p.Name, - Source: p.Source, - BindBridge: p.BindBridge, - DHCPMAC: p.DHCPMAC, - RangeStart: p.RangeStart, - RangeEnd: p.RangeEnd, - Gateway: p.Gateway, - GatewayIP: p.GatewayIP, - PrefixLen: p.PrefixLen, - Region: p.Region, - AZ: p.AZ, - GwLrpRangeStart: p.GwLrpRangeStart, - GwLrpRangeEnd: p.GwLrpRangeEnd, - }) - if p.Source == "dhcp" { - anyDHCP = true + pools, anyDHCP := d.externalPoolConfigs() + d.externalIPAM, err = initServiceWithRetry("external IPAM", func() (*handlers_ec2_vpc.ExternalIPAM, error) { + js, jsErr := jetstream.New(d.natsConn) + if jsErr != nil { + return nil, fmt.Errorf("jetstream handle: %w", jsErr) + } + ipam, ipamErr := handlers_ec2_vpc.NewExternalIPAM(d.ctx, js, pools) + if ipamErr != nil { + return nil, ipamErr + } + if anyDHCP { + if dhcpErr := ipam.EnableDHCP(dhcp.NewNATSClient(d.natsConn, 0)); dhcpErr != nil { + return nil, fmt.Errorf("enable DHCP allocator: %w", dhcpErr) } } - d.externalIPAM, err = handlers_ec2_vpc.NewExternalIPAM(d.ctx, js, pools) - if err != nil { - slog.Warn("Failed to initialize external IPAM", "err", err) - } else { - if anyDHCP { - dhcpClient := dhcp.NewNATSClient(d.natsConn, 0) - if dhcpErr := d.externalIPAM.EnableDHCP(dhcpClient); dhcpErr != nil { - slog.Warn("Failed to enable DHCP allocator on external IPAM", "err", dhcpErr) - } - } - slog.Info("External IPAM initialized", "mode", d.clusterConfig.Network.ExternalMode, "pools", len(pools), "dhcp", anyDHCP) - } + return ipam, nil + }) + if err != nil { + return fmt.Errorf("failed to initialize external IPAM: %w", err) } + slog.Info("External IPAM initialized", "mode", d.clusterConfig.Network.ExternalMode, "pools", len(pools), "dhcp", anyDHCP) } // Initialize EIP service if external IPAM is available if d.externalIPAM != nil && d.vpcService != nil { - eipSvc, eipErr := handlers_ec2_eip.NewEIPServiceImpl(d.ctx, d.natsConn, d.externalIPAM, d.vpcService) + eipSvc, eipErr := initServiceWithRetry("EIP service", func() (*handlers_ec2_eip.EIPServiceImpl, error) { + return handlers_ec2_eip.NewEIPServiceImpl(d.ctx, d.natsConn, d.externalIPAM, d.vpcService) + }) if eipErr != nil { - slog.Warn("Failed to initialize EIP service", "err", eipErr) - } else { - d.eipService = eipSvc - slog.Info("EIP service initialized") + return fmt.Errorf("failed to initialize EIP service: %w", eipErr) } + d.eipService = eipSvc + slog.Info("EIP service initialized") // Inject external IPAM + EIP KV into VPC service so DeleteNetworkInterface // can release auto-assigned public IPs and NAT rules. - eipJS, eipJSErr := jetstream.New(d.natsConn) - if eipJSErr != nil { - slog.Warn("Failed to get JetStream for VPC external IPAM injection", "err", eipJSErr) - } else { - eipKV, eipKVErr := kvutil.GetOrCreateBucket(d.ctx, eipJS, handlers_ec2_eip.KVBucketEIPs, 10) - if eipKVErr != nil { - slog.Warn("Failed to get EIP KV bucket for VPC service", "err", eipKVErr) - } else { - d.vpcService.SetExternalIPAM(d.externalIPAM, eipKV) + eipKV, eipKVErr := initServiceWithRetry("EIP KV bucket", func() (jetstream.KeyValue, error) { + eipJS, jsErr := jetstream.New(d.natsConn) + if jsErr != nil { + return nil, fmt.Errorf("jetstream handle: %w", jsErr) } + return kvutil.GetOrCreateBucket(d.ctx, eipJS, handlers_ec2_eip.KVBucketEIPs, 10) + }) + if eipKVErr != nil { + return fmt.Errorf("failed to get EIP KV bucket for VPC service: %w", eipKVErr) } + d.vpcService.SetExternalIPAM(d.externalIPAM, eipKV) } // Without external IPAM (nat mode or external disabled) serve EIP requests diff --git a/tests/e2e/quota/helpers_test.go b/tests/e2e/quota/helpers_test.go index d857a5949..4deba91a9 100644 --- a/tests/e2e/quota/helpers_test.go +++ b/tests/e2e/quota/helpers_test.go @@ -39,16 +39,13 @@ func headroom(t *testing.T, accountID, dimension string, used int) { func itoa(v int) string { return strconv.Itoa(v) } -// skipIfEIPUnsupported turns the disabled-EIP stub's refusal into a skip. A node -// with no external IPAM answers allocations with UnsupportedOperation and -// describes with an empty list, so where a cell's nodes do not all hold a pool -// there is no count for the cap to be asserted against. On a cluster that does -// serve EIPs the quota refuses first and this never fires. -func skipIfEIPUnsupported(t *testing.T, err error) { +// requireEIPSupported fails rather than skips when a node answers from the +// disabled-EIP stub. A cell whose nodes disagree about whether EIPs exist +// cannot count addresses, and the cap under test is what exposes it. +func requireEIPSupported(t *testing.T, err error) { t.Helper() - if harness.ErrorCodeIs(err, "UnsupportedOperation") { - t.Skip("external IPAM is not served by every node in this cell; EIP allocation is unsupported") - } + require.False(t, harness.ErrorCodeIs(err, "UnsupportedOperation"), + "a node answered from the disabled-EIP stub: this cell does not serve external IPAM from every node") } func countVPCs(t *testing.T, c *harness.AWSClient) int { diff --git a/tests/e2e/quota/tests_test.go b/tests/e2e/quota/tests_test.go index b16ad1598..e2c2043b8 100644 --- a/tests/e2e/quota/tests_test.go +++ b/tests/e2e/quota/tests_test.go @@ -96,13 +96,9 @@ func TestQuotaSubnetLimit(t *testing.T) { // Elastic IPs are the scarcest thing a cluster hands out — the pool is finite // and shared — so this gate is the one that decides how many tenants fit. // -// Both AllocateAddress and DescribeAddresses are queue-group requests answered -// by whichever node picks them up, and a node with no external IPAM answers -// from the disabled stub: an empty address list, and UnsupportedOperation on a -// mutation. Where a cell's nodes do not all hold a pool the count this cap reads -// is whichever node replied, so the boundary is not assertable — hence the skip -// rather than a failure. A cluster with one static pool, which is what -// production runs, answers identically from every node. +// Both calls are queue-group requests answered by whichever node picks them up, +// so this leg also proves every node serves external IPAM: one that does not +// answers an empty address list the cap cannot count against. func TestQuotaEIPLimit(t *testing.T) { fix := requireQuotaFixture(t) harness.Phase(t, "Quota — Elastic IPs") @@ -116,7 +112,7 @@ func TestQuotaEIPLimit(t *testing.T) { headroom(t, fix.Tenant.AccountID, "eips", countAddresses(t, tenant)) alloc, err := allocate() - skipIfEIPUnsupported(t, err) + requireEIPSupported(t, err) require.NoError(t, err, "the address the account has room for") allocID := aws.StringValue(alloc.AllocationId) released := false @@ -127,7 +123,7 @@ func TestQuotaEIPLimit(t *testing.T) { }) _, err = allocate() - skipIfEIPUnsupported(t, err) + requireEIPSupported(t, err) harness.AssertAWSError(t, err, quotaExceeded) harness.Step(t, "releasing the address frees the allowance") @@ -136,7 +132,7 @@ func TestQuotaEIPLimit(t *testing.T) { released = true regained, err := allocate() - skipIfEIPUnsupported(t, err) + requireEIPSupported(t, err) require.NoError(t, err, "a released address must free room for another") t.Cleanup(func() { _, _ = tenant.EC2.ReleaseAddress(&ec2.ReleaseAddressInput{AllocationId: regained.AllocationId}) From cfb0abc8a8bceac3dd9377eab46f7674ba75f157 Mon Sep 17 00:00:00 2001 From: Ben Duncan <5178243+benduncan@users.noreply.github.com> Date: Thu, 20 Aug 2026 04:01:11 +1000 Subject: [PATCH 7/8] chore: drop a local settings temp file committed by mistake --- ...ttings.local.json.tmp.2466042.87acb86176ea | 25 ------------------- 1 file changed, 25 deletions(-) delete mode 100644 .claude/settings.local.json.tmp.2466042.87acb86176ea diff --git a/.claude/settings.local.json.tmp.2466042.87acb86176ea b/.claude/settings.local.json.tmp.2466042.87acb86176ea deleted file mode 100644 index 8d1eb5d6c..000000000 --- a/.claude/settings.local.json.tmp.2466042.87acb86176ea +++ /dev/null @@ -1,25 +0,0 @@ -{ - "permissions": { - "allow": [ - "Bash(timeout 30 bd ready)", - "Bash(python3 -c ' *)", - "Bash(timeout 30 bd label list)", - "Bash(timeout 30 bd labels)", - "Bash(git fetch *)", - "Bash(git checkout *)", - "Bash(grep -n \"^#\\\\{2,3\\\\} \" docs/development/improvements/new-multi-node-networking.md)", - "Bash(go version *)", - "Read(//home/tf-user/go/pkg/mod/golang.org/**)", - "Read(//usr/local/go/**)", - "Bash(go build *)", - "Bash(go test *)", - "Bash(go vet *)", - "Bash(python3 -)", - "Bash(bash -n scripts/iso-builder/build/spinifex-init.sh)", - "Bash(timeout 900 make preflight)", - "Bash(timeout 300 golangci-lint run ./cmd/installer/...)", - "Bash(timeout 300 golangci-lint run ./...)", - "Bash(git stash *)" - ] - } -} From 4dadce8ed529e1c27f5787d33822d73a6dd2509a Mon Sep 17 00:00:00 2001 From: Ben Duncan <5178243+benduncan@users.noreply.github.com> Date: Thu, 20 Aug 2026 04:07:19 +1000 Subject: [PATCH 8/8] test: cover the external pool projection fed to IPAM --- spinifex/daemon/external_pools_test.go | 88 ++++++++++++++++++++++++++ 1 file changed, 88 insertions(+) create mode 100644 spinifex/daemon/external_pools_test.go diff --git a/spinifex/daemon/external_pools_test.go b/spinifex/daemon/external_pools_test.go new file mode 100644 index 000000000..2bb2a9392 --- /dev/null +++ b/spinifex/daemon/external_pools_test.go @@ -0,0 +1,88 @@ +//test:in-package — drives the unexported pool projection startCluster feeds to external IPAM. +package daemon + +import ( + "testing" + + "github.com/mulgadc/spinifex/spinifex/config" + "github.com/mulgadc/spinifex/spinifex/network/host" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func daemonWithPools(pools ...config.ExternalPool) *Daemon { + d := &Daemon{clusterConfig: &config.ClusterConfig{}} + d.clusterConfig.Network.ExternalPools = pools + return d +} + +func TestExternalPoolConfigsCopiesEveryField(t *testing.T) { + d := daemonWithPools(config.ExternalPool{ + Name: "wan", + Source: "static", + BindBridge: "br-wan", + DHCPMAC: "derived", + RangeStart: "192.168.1.150", + RangeEnd: "192.168.1.250", + Gateway: "192.168.1.1", + GatewayIP: "192.168.1.149", + PrefixLen: 24, + Region: "us-west-1", + AZ: "us-west-1-az2", + GwLrpRangeStart: "192.168.1.100", + GwLrpRangeEnd: "192.168.1.110", + }) + + pools, anyDHCP := d.externalPoolConfigs() + require.Len(t, pools, 1) + assert.False(t, anyDHCP, "a static pool must not ask for the DHCP allocator") + + got := pools[0] + assert.Equal(t, "wan", got.Name) + assert.Equal(t, "static", got.Source) + assert.Equal(t, "br-wan", got.BindBridge) + assert.Equal(t, "derived", got.DHCPMAC) + assert.Equal(t, "192.168.1.150", got.RangeStart) + assert.Equal(t, "192.168.1.250", got.RangeEnd) + assert.Equal(t, "192.168.1.1", got.Gateway) + assert.Equal(t, "192.168.1.149", got.GatewayIP) + assert.Equal(t, 24, got.PrefixLen) + assert.Equal(t, "us-west-1", got.Region) + assert.Equal(t, "us-west-1-az2", got.AZ) + assert.Equal(t, "192.168.1.100", got.GwLrpRangeStart) + assert.Equal(t, "192.168.1.110", got.GwLrpRangeEnd) +} + +// The transit pool's addresses are gateway-LRP plumbing. Handing them to IPAM +// would let a tenant allocate one as an EIP and break the router. +func TestExternalPoolConfigsExcludesTheTransitPool(t *testing.T) { + d := daemonWithPools( + config.ExternalPool{Name: host.NATTransitPoolName, Source: "static"}, + config.ExternalPool{Name: "wan", Source: "static"}, + ) + + pools, _ := d.externalPoolConfigs() + require.Len(t, pools, 1) + assert.Equal(t, "wan", pools[0].Name) +} + +func TestExternalPoolConfigsReportsDHCP(t *testing.T) { + d := daemonWithPools( + config.ExternalPool{Name: "wan", Source: "static"}, + config.ExternalPool{Name: "wan2", Source: "dhcp", BindBridge: "br-wan"}, + ) + + pools, anyDHCP := d.externalPoolConfigs() + require.Len(t, pools, 2) + assert.True(t, anyDHCP, "one DHCP pool must enable the allocator for the node") +} + +// A transit-only config yields no pools, which is what keeps nat mode off the +// EIP path without a separate mode check at the call site. +func TestExternalPoolConfigsTransitOnlyYieldsNothing(t *testing.T) { + d := daemonWithPools(config.ExternalPool{Name: host.NATTransitPoolName, Source: "static"}) + + pools, anyDHCP := d.externalPoolConfigs() + assert.Empty(t, pools) + assert.False(t, anyDHCP) +}