feat: ELBv2 attributes API + DescribeTags (#128)

* feat: add attributes map to LB and TG records with defaults

Replace CrossZoneEnabled bool with Attributes map[string]string on
LoadBalancerRecord and TargetGroupRecord. Add DefaultLoadBalancerAttributes()
and DefaultTargetGroupAttributes() functions. ALBs seed cross-zone enabled
at create time.

* feat: add 4 attributes methods to ELBv2Service interface

* feat: implement ELBv2 attributes service methods and NATS wrappers

Add ModifyTargetGroupAttributes, DescribeTargetGroupAttributes,
ModifyLoadBalancerAttributes, and DescribeLoadBalancerAttributes
to service_impl.go with proper ARN validation and default merging.

Add corresponding NATS request wrappers in service_nats.go.

* feat: add gateway handlers for ELBv2 attributes API endpoints

Add ModifyTargetGroupAttributes, DescribeTargetGroupAttributes,
ModifyLoadBalancerAttributes, and DescribeLoadBalancerAttributes
gateway handlers with input validation and action map registration.

* feat: add daemon handlers & NATS subscriptions for elbv2 attributes

Add 4 handler functions in daemon_handlers_elbv2.go and 4 NATS
subscriptions in daemon.go for ModifyTargetGroupAttributes,
DescribeTargetGroupAttributes, ModifyLoadBalancerAttributes, and
DescribeLoadBalancerAttributes.

* fix: add missing accountID authorization check to ELBv2 attribute handlers

* fix: guard nil attributes and log skips in ELBv2 Modify*Attributes

ModifyTargetGroupAttributes and ModifyLoadBalancerAttributes silently
dropped attributes with nil Key or Value, and would panic on a nil
slice element when dereferencing attr.Key. Add an explicit nil-element
guard and log a warning whenever an attribute is skipped, so callers
and operators can see divergence between the submitted payload and the
persisted state.

* fix: sort ELBv2 attribute responses by key for deterministic output

DescribeTargetGroupAttributes and DescribeLoadBalancerAttributes iterated
a map and appended to a slice, producing random attribute order on each
call. Sort keys before building the response so Terraform plans stay
stable and snapshot tests can rely on consistent ordering.

Closes mulga-910

* feat: add ELBv2 DescribeTags handler (mulga-919)

Required by the Terraform AWS provider's post-create refresh on
aws_lb, aws_lb_target_group, and aws_lb_listener. Without it, the
gateway returns InvalidAction and apply fails after the resources
are already created.

Dispatches by ARN resource segment (loadbalancer/targetgroup/listener)
to the existing record stores. Enforces account scoping via per-type
not-found errors to avoid cross-account existence leaks. Listeners
return an empty Tags slice since ListenerRecord doesn't store tags yet
— that's the exact case the provider hits on every untagged listener
refresh. Output is sorted by key for deterministic responses.

* refactor: parameterize DefaultLoadBalancerAttributes by lbType (mulga-915)

Cross-zone default was split across three places: CreateLoadBalancer seeded
{load_balancing.cross_zone.enabled: true} for ALBs, DefaultLoadBalancerAttributes
returned false unconditionally, and tests asserted both sides. Make
DefaultLoadBalancerAttributes(lbType) the single source of truth: ALBs get
"true", NLBs (and any other type) get "false". CreateLoadBalancer no longer
seeds attributes — DescribeLoadBalancerAttributes derives per-type defaults
from lb.Type on read. Updated TestCreateLoadBalancer_CrossZoneAttributes to
assert via the Describe handler instead of poking stored record state.

* perf: direct KV lookup for ELBv2 *ByArn finders (mulga-913)

GetLoadBalancerByArn and GetTargetGroupByArn used to call ListLoadBalancers /
ListTargetGroups and linearly scan, doing one NATS round-trip per record in
the bucket. Terraform hits Describe*Attributes on every plan/refresh — a
20-ALB / 40-TG workspace was doing ~60 calls × ~60 records = ~3600 KV gets
per refresh where ~60 would suffice. The LB/TG ARN's final path segment IS
the KV key used by GetLoadBalancer(lbID)/GetTargetGroup(tgID), so parse the
short ID from the ARN and do an O(1) lookup, verifying the record's stored
ARN matches as defence-in-depth against any stale-key weirdness.

* fix: reject ELBv2 ModifyAttributes calls where every entry is invalid (mulga-911)

When every submitted attribute was rejected by the nil-guard (nil element,
nil Key, or nil Value), Modify{TargetGroup,LoadBalancer}Attributes still
called PutTargetGroup/PutLoadBalancer with an unchanged record and returned
a successful ModifyOutput with empty Attributes. Only a slog.Warn marked
the skip — the caller saw 200 OK and moved on. Return
ErrorInvalidParameterValue when len(input.Attributes) > 0 and every entry
was rejected, so the caller learns that nothing landed.

* fix: reject unknown ELBv2 ModifyAttributes keys with ValidationError (mulga-912)

ModifyTargetGroupAttributes and ModifyLoadBalancerAttributes had no whitelist
check — any caller-supplied key (typo like 'stickness.enabled', deprecated
keys, or cross-product keys sent to the wrong resource) was persisted verbatim
and merrily echoed back by Describe. AWS rejects unknown keys with
ValidationError; match that so Terraform surfaces the typo at plan time.
Validate each key against DefaultTargetGroupAttributes() /
DefaultLoadBalancerAttributes(lb.Type) and return ErrorValidationError on the
first unknown key, logging both the arn and the offending key name.

* perf: skip no-op Put in ELBv2 ModifyAttributes (mulga-914)

The unconditional PutTargetGroup/PutLoadBalancer fired on every Modify call,
even when the caller sent an empty attribute list or when every submitted
value already matched the stored value. Terraform re-applies the same
attribute set on every drift check, so the steady-state path was doing a
pointless NATS/KV round-trip per call and unnecessarily expanding the
read-modify-write race window.

Track a dirty flag per Modify call: compare each submitted key's value
against tg.Attributes/lb.Attributes and only write to the map (and allocate
the map lazily) when it actually differs. Skip the Put entirely if nothing
changed. Added regression tests that capture the KV entry revision before
and after a no-op modify to prove the write was skipped.

* test: assert exact ModifyAttributes echoed key/value pairs (mulga-917)

Both round-trip tests used assert.Len(t, modOut.Attributes, 2) and never
checked that the returned Attributes actually contained the submitted
keys/values. A regression that pointed at the wrong source, dropped Value,
or returned a copy of defaults would have passed the length check
unchallenged. Build a modMap from the echoed response and assert each
expected Key/Value pair explicitly.

* test: cover sequential ModifyAttributes merge and same-key overwrite (mulga-916)

All existing round-trip tests did one Modify then Describe. No test covered:
Modify {A}, Modify {B}, Describe, assert BOTH keys present; or Modify
overwriting the same key on a second call. A future refactor that replaced
tg.Attributes instead of mutating in place would pass every existing test
but silently wipe previous attributes on each Modify — the most likely
real-world ALB/TG bug. Add sequential-merge tests for both TG and LB
handlers that exercise multi-key merge, same-key overwrite, and
cross-survival of unrelated keys.
This commit is contained in:
Josh Siviour
2026-04-10 21:31:43 +10:00
committed by GitHub
parent 67bdc9d4af
commit c8ca746bf0
17 changed files with 1710 additions and 69 deletions
+6
View File
@@ -589,6 +589,12 @@ All ELBv2 resources are stored in the `spinifex-elbv2` JetStream KV bucket. Key
| `delete-listener` | `--listener-arn` | `--dry-run` | Listener must exist | Gateway validates ListenerArn (required) → NATS `elbv2.DeleteListener` → daemon verifies listener exists and belongs to account → deletes from KV → fetches parent LB → calls `pushConfig(lb)` to regenerate HAProxy config without this listener → returns success | 1. Delete listener<br>2. Not found (ListenerNotFound) | **DONE** |
| `describe-listeners` | `--load-balancer-arn`, `--listener-arns` | `--page-size`, `--marker`, `--dry-run` | None | NATS `elbv2.DescribeListeners` → daemon lists listeners → if LoadBalancerArn specified, filters to that LB → applies ARN filter → account isolation → returns Listener list | 1. Filter by LB ARN<br>2. Account isolation | **DONE** |
#### ELBv2 - Tags
| Command | Implemented Flags | Missing Flags | Prerequisites | Basic Logic | Test Cases | Status |
|---------|-------------------|---------------|---------------|-------------|------------|--------|
| `describe-tags` | `--resource-arns` (loadbalancer, targetgroup, listener) | — | Resource(s) must exist | Gateway validates non-empty ResourceArns → NATS `elbv2.DescribeTags` → daemon parses each ARN's resource segment to dispatch by type → looks up record (LB/TG/listener) → enforces account isolation (cross-account → not-found) → returns TagDescriptions in input order with sorted-by-key Tag list. Listener records don't store tags yet, so they always return an empty Tags slice (matches AWS behaviour for untagged resources). Required by Terraform AWS provider post-create refresh on `aws_lb`, `aws_lb_target_group`, and `aws_lb_listener`. | 1. LB tags round-trip<br>2. TG tags round-trip<br>3. Listener returns empty Tags (no error)<br>4. Multiple ARNs in one call<br>5. Unknown LB/TG/listener (per-type not-found)<br>6. Cross-account ARN (not-found, no leak)<br>7. Invalid/non-ELBv2 ARN (InvalidParameterValue)<br>8. Empty/nil ResourceArns (MissingParameter)<br>9. Untagged LB returns empty Tags | **DONE** |
#### ELBv2 - Not Implemented
| Feature | Description | Priority | Status |