From de5ee4339a64e80b26acdae10a37631a5c09f7e8 Mon Sep 17 00:00:00 2001 From: Josh Siviour Date: Wed, 3 Jun 2026 15:06:35 +1000 Subject: [PATCH] fix(elbv2): render fixed-response listener defaults + DescribeTags listener-rules (#278) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Shared-ingress ALBs use a fixed-response default action (HTTP 404) with host-header rules forwarding to per-app target groups. Two gaps broke this: - The HAProxy generator read DefaultActions[0].TargetGroupArn unconditionally, so a fixed-response default (empty ARN) rendered `default_backend bk_`, a dangling reference that made HAProxy fail to start — the listener never bound :80 and the LB served no traffic. Store and render the fixed-response config as a synthetic backend (`http-request return status N ...`), with status, content-type, and body validated before interpolation. - ListenerAction dropped the fixed-response config on create/modify, so DescribeListeners read back an empty action and tofu saw a perpetual diff. Preserve it through CreateListener/ModifyListener and the SDK conversion. - DescribeTags returned InvalidParameterValue for listener-rule ARNs; resolve the rule and return an (untagged) descriptor instead. Adds generator tests for the fixed-response default (with host-header rules) and unsafe-body rejection, plus DescribeTags listener-rule coverage. --- spinifex/handlers/elbv2/haproxy.go | 94 +++++++++++++++++++- spinifex/handlers/elbv2/haproxy_test.go | 85 ++++++++++++++++++ spinifex/handlers/elbv2/service_impl.go | 94 ++++++++++++++------ spinifex/handlers/elbv2/service_impl_test.go | 49 ++++++++++ spinifex/handlers/elbv2/types.go | 14 ++- 5 files changed, 304 insertions(+), 32 deletions(-) diff --git a/spinifex/handlers/elbv2/haproxy.go b/spinifex/handlers/elbv2/haproxy.go index 92c487c82..865b9915e 100644 --- a/spinifex/handlers/elbv2/haproxy.go +++ b/spinifex/handlers/elbv2/haproxy.go @@ -6,6 +6,7 @@ import ( "os" "os/exec" "path/filepath" + "regexp" "strconv" "strings" "sync" @@ -44,12 +45,16 @@ frontend {{.Name}} {{end}} {{range .Backends}} backend {{.Name}} +{{- if .FixedResponse}} + http-request return status {{.FixedResponse.StatusCode}}{{if .FixedResponse.ContentType}} content-type "{{.FixedResponse.ContentType}}"{{end}}{{if .FixedResponse.Body}} string "{{.FixedResponse.Body}}"{{end}} +{{- else}} balance roundrobin{{if .HealthCheckPath}} option httpchk GET {{.HealthCheckPath}} http-check expect status {{.HealthCheckMatcher}}{{end}} {{- range .Servers}} server {{.Name}} {{.Addr}}:{{.Port}} check inter {{.CheckInterval}}s fall {{.Fall}} rise {{.Rise}} {{- end}} +{{- end}} {{end}} ` @@ -123,7 +128,8 @@ type HAProxyACL struct { Expr string } -// HAProxyBackend represents a target group backend. +// HAProxyBackend represents a target group backend, or a synthetic backend that +// emits a fixed response (when FixedResponse is set, the server fields are unused). type HAProxyBackend struct { Name string HealthCheckPath string @@ -131,6 +137,15 @@ type HAProxyBackend struct { TCPCheck bool HTTPCheck bool Servers []HAProxyServer + FixedResponse *HAProxyFixedResponse +} + +// HAProxyFixedResponse renders an `http-request return` directive for a +// fixed-response listener default action. All fields are validated before use. +type HAProxyFixedResponse struct { + StatusCode string + ContentType string + Body string } // HAProxyServer represents a single target server. @@ -238,13 +253,43 @@ func buildHAProxyConfig(lb *LoadBalancerRecord, listeners []*ListenerRecord, tgB backendAdded[backendName] = struct{}{} } + addFixedResponseBackend := func(name string, fr *FixedResponseAction) { + if _, ok := backendAdded[name]; ok { + return + } + hf := &HAProxyFixedResponse{StatusCode: "503"} + if fr != nil { + if validHTTPStatusCode(fr.StatusCode) { + hf.StatusCode = fr.StatusCode + } + if validContentType(fr.ContentType) { + hf.ContentType = fr.ContentType + } + if validFixedResponseBody(fr.MessageBody) { + hf.Body = fr.MessageBody + } + } + cfg.Backends = append(cfg.Backends, HAProxyBackend{Name: name, FixedResponse: hf}) + backendAdded[name] = struct{}{} + } + for _, l := range listeners { if len(l.DefaultActions) == 0 { continue } - tgArn := l.DefaultActions[0].TargetGroupArn - defaultBackendName := sanitizeName("bk", tgArn) + da := l.DefaultActions[0] + isForwardDefault := da.Type == ActionTypeForward && da.TargetGroupArn != "" + + var defaultBackendName string + if isForwardDefault { + defaultBackendName = sanitizeName("bk", da.TargetGroupArn) + } else { + // fixed-response (or any non-forward default, e.g. the shared-ingress + // 404): synthesize a backend that returns the canned reply so HAProxy + // has a valid default_backend instead of a dangling reference. + defaultBackendName = sanitizeName("bkdefault", l.ListenerArn) + } frontend := HAProxyFrontend{ Name: sanitizeName("ft", l.ListenerArn), @@ -270,7 +315,11 @@ func buildHAProxyConfig(lb *LoadBalancerRecord, listeners []*ListenerRecord, tgB } cfg.Frontends = append(cfg.Frontends, frontend) - addBackend(tgArn) + if isForwardDefault { + addBackend(da.TargetGroupArn) + } else { + addFixedResponseBackend(defaultBackendName, da.FixedResponse) + } } return cfg, nil @@ -395,6 +444,43 @@ func wildcardMatch(v string) (flag, literal string) { } } +// contentTypeRegex matches a conservative MIME type (type/subtype) for the +// fixed-response content-type header rendered into the HAProxy config. +var contentTypeRegex = regexp.MustCompile(`^[a-zA-Z0-9!#$&^_.+-]+/[a-zA-Z0-9!#$&^_.+-]+$`) + +// validHTTPStatusCode accepts a 3-digit 2xx–5xx HTTP status. +func validHTTPStatusCode(s string) bool { + if len(s) != 3 || s[0] < '2' || s[0] > '5' { + return false + } + for _, r := range s { + if r < '0' || r > '9' { + return false + } + } + return true +} + +// validContentType accepts a conservative type/subtype MIME string. +func validContentType(s string) bool { + return contentTypeRegex.MatchString(s) +} + +// validFixedResponseBody rejects bytes that would terminate the quoted HAProxy +// string or inject a directive. Spaces are allowed (the body renders inside +// double quotes). +func validFixedResponseBody(s string) bool { + if s == "" { + return false + } + for _, r := range s { + if r == '\n' || r == '\r' || r == '"' || r == '\\' || r == '#' { + return false + } + } + return true +} + // validateRenderSafe rejects bytes that could break ACL parsing or inject new // directives. Mirrors the write-time validators in service_impl_rules.go; // duplicated here so a future migration that bypasses the API cannot leak a diff --git a/spinifex/handlers/elbv2/haproxy_test.go b/spinifex/handlers/elbv2/haproxy_test.go index 4adaed7b4..2c89d2480 100644 --- a/spinifex/handlers/elbv2/haproxy_test.go +++ b/spinifex/handlers/elbv2/haproxy_test.go @@ -163,6 +163,91 @@ func TestGenerateHAProxyConfig_SharedTargetGroup(t *testing.T) { assert.Equal(t, 1, strings.Count(config, "balance roundrobin")) } +// TestGenerateHAProxyConfig_FixedResponseDefault mirrors the shared-ingress +// shape: a listener whose default action is fixed-response (no target group), +// with host-header rules forwarding to per-app target groups. The generator +// must synthesize a default backend instead of emitting a dangling +// `default_backend bk_`, which makes HAProxy fail to start. +func TestGenerateHAProxyConfig_FixedResponseDefault(t *testing.T) { + lb := &LoadBalancerRecord{LoadBalancerID: "lb-ingress"} + listenerArn := "arn:aws:elasticloadbalancing:ap-southeast-2:1:listener/app/wd-ingress/lb-ingress/lst-616760baf6a3031c3" + appTG := "arn:aws:elasticloadbalancing:ap-southeast-2:1:targetgroup/wd-identity/tg-app1" + + listeners := []*ListenerRecord{ + { + ListenerArn: listenerArn, + Port: 80, + Protocol: ProtocolHTTP, + DefaultActions: []ListenerAction{ + {Type: ActionTypeFixedResponse, FixedResponse: &FixedResponseAction{ + StatusCode: "404", + ContentType: "text/plain", + MessageBody: "no route", + }}, + }, + }, + } + + rulesByListener := map[string][]*RuleRecord{ + listenerArn: { + { + RuleID: "rule1", + ListenerArn: listenerArn, + Priority: 1, + Conditions: []RuleCondition{{Field: RuleFieldHostHeader, Values: []string{"identity.toc.spinifex.local"}}}, + Actions: []ListenerAction{{Type: ActionTypeForward, TargetGroupArn: appTG}}, + }, + }, + } + + tgByArn := map[string]*TargetGroupRecord{ + appTG: { + TargetGroupArn: appTG, + HealthCheck: DefaultHealthCheck(), + Targets: []Target{{Id: "i-app1", Port: 8080, PrivateIP: "10.42.0.9", HealthState: TargetHealthHealthy}}, + }, + } + + config, err := GenerateHAProxyConfig(lb, listeners, tgByArn, rulesByListener, "10.42.0.13") + require.NoError(t, err) + + // No dangling default backend. + assert.NotContains(t, config, "default_backend bk_\n") + // Synthetic default backend named off the listener ARN, returning the fixed response. + assert.Contains(t, config, "default_backend bkdefault_lst-616760baf6a3031c3") + assert.Contains(t, config, "backend bkdefault_lst-616760baf6a3031c3") + assert.Contains(t, config, `http-request return status 404 content-type "text/plain" string "no route"`) + // Host-header rule still routes to the app backend. + assert.Contains(t, config, "use_backend bk_tg-app1 if") + assert.Contains(t, config, "10.42.0.9:8080") +} + +// TestGenerateHAProxyConfig_FixedResponseRejectsUnsafeBody falls back to a bare +// status when the body or content-type contains injection bytes. +func TestGenerateHAProxyConfig_FixedResponseRejectsUnsafeBody(t *testing.T) { + lb := &LoadBalancerRecord{LoadBalancerID: "lb-evil"} + listeners := []*ListenerRecord{ + { + ListenerArn: "arn:lst-evil", + Port: 80, + DefaultActions: []ListenerAction{ + {Type: ActionTypeFixedResponse, FixedResponse: &FixedResponseAction{ + StatusCode: "200", + ContentType: "text/plain", + MessageBody: "evil\"\n acl x always_true", + }}, + }, + }, + } + + config, err := GenerateHAProxyConfig(lb, listeners, nil, nil, "0.0.0.0") + require.NoError(t, err) + + assert.Contains(t, config, "http-request return status 200") + assert.NotContains(t, config, "always_true") + assert.NotContains(t, config, `string "evil`) +} + func TestGenerateHAProxyConfig_NoListeners(t *testing.T) { lb := &LoadBalancerRecord{LoadBalancerID: "lb-empty"} config, err := GenerateHAProxyConfig(lb, nil, nil, nil, "0.0.0.0") diff --git a/spinifex/handlers/elbv2/service_impl.go b/spinifex/handlers/elbv2/service_impl.go index 8491c62a4..7bb7f796a 100644 --- a/spinifex/handlers/elbv2/service_impl.go +++ b/spinifex/handlers/elbv2/service_impl.go @@ -777,13 +777,15 @@ const ( elbv2ResourceLoadBalancer = "loadbalancer" elbv2ResourceTargetGroup = "targetgroup" elbv2ResourceListener = "listener" + elbv2ResourceListenerRule = "listener-rule" ) // elbv2ResourceTypeFromArn extracts the resource type from an ELBv2 ARN. // ELBv2 ARNs have the form // "arn:aws:elasticloadbalancing:{region}:{account}:{type}/...". Returns one -// of "loadbalancer", "targetgroup", "listener" — anything else yields -// ErrorInvalidParameterValue so callers can surface a proper API error. +// of "loadbalancer", "targetgroup", "listener", "listener-rule" — anything +// else yields ErrorInvalidParameterValue so callers can surface a proper API +// error. func elbv2ResourceTypeFromArn(arn string) (string, error) { parts := strings.SplitN(arn, ":", 6) if len(parts) < 6 || parts[0] != "arn" || parts[2] != "elasticloadbalancing" { @@ -796,7 +798,7 @@ func elbv2ResourceTypeFromArn(arn string) (string, error) { } resourceType := resourceSegment[:slash] switch resourceType { - case elbv2ResourceLoadBalancer, elbv2ResourceTargetGroup, elbv2ResourceListener: + case elbv2ResourceLoadBalancer, elbv2ResourceTargetGroup, elbv2ResourceListener, elbv2ResourceListenerRule: return resourceType, nil default: return "", errors.New(awserrors.ErrorInvalidParameterValue) @@ -1626,6 +1628,34 @@ func (s *ELBv2ServiceImpl) DescribeTargetHealth(input *elbv2.DescribeTargetHealt // --- Listener operations --- +// listenerActionFromSDK converts an SDK listener action into the stored form, +// preserving fixed-response config so HAProxy generation and tofu read-back see +// the full action (a forward-only conversion drops the fixed-response default +// and produces a dangling backend + perpetual plan diff). +func listenerActionFromSDK(a *elbv2.Action) ListenerAction { + action := ListenerAction{} + if a.Type != nil { + action.Type = *a.Type + } + if a.TargetGroupArn != nil { + action.TargetGroupArn = *a.TargetGroupArn + } + if a.FixedResponseConfig != nil { + fr := &FixedResponseAction{} + if a.FixedResponseConfig.StatusCode != nil { + fr.StatusCode = *a.FixedResponseConfig.StatusCode + } + if a.FixedResponseConfig.ContentType != nil { + fr.ContentType = *a.FixedResponseConfig.ContentType + } + if a.FixedResponseConfig.MessageBody != nil { + fr.MessageBody = *a.FixedResponseConfig.MessageBody + } + action.FixedResponse = fr + } + return action +} + func (s *ELBv2ServiceImpl) CreateListener(input *elbv2.CreateListenerInput, accountID string) (*elbv2.CreateListenerOutput, error) { if input.LoadBalancerArn == nil || *input.LoadBalancerArn == "" { return nil, errors.New(awserrors.ErrorMissingParameter) @@ -1705,14 +1735,7 @@ func (s *ELBv2ServiceImpl) CreateListener(input *elbv2.CreateListenerInput, acco var actions []ListenerAction for _, a := range input.DefaultActions { - action := ListenerAction{} - if a.Type != nil { - action.Type = *a.Type - } - if a.TargetGroupArn != nil { - action.TargetGroupArn = *a.TargetGroupArn - } - actions = append(actions, action) + actions = append(actions, listenerActionFromSDK(a)) } record := &ListenerRecord{ @@ -1857,13 +1880,7 @@ func (s *ELBv2ServiceImpl) ModifyListener(input *elbv2.ModifyListenerInput, acco if len(input.DefaultActions) > 0 { var actions []ListenerAction for _, a := range input.DefaultActions { - action := ListenerAction{} - if a.Type != nil { - action.Type = *a.Type - } - if a.TargetGroupArn != nil { - action.TargetGroupArn = *a.TargetGroupArn - } + action := listenerActionFromSDK(a) if action.Type == ActionTypeForward && action.TargetGroupArn != "" { tg, tgErr := s.store.GetTargetGroupByArn(action.TargetGroupArn) if tgErr != nil { @@ -1957,11 +1974,12 @@ func (s *ELBv2ServiceImpl) DescribeListeners(input *elbv2.DescribeListenersInput // --- Tag operations --- // DescribeTags returns tags for one or more ELBv2 resources (load balancers, -// target groups, listeners). Tag data is read from the existing record stores -// — Spinifex doesn't have a separate tag KV. Listeners currently never store -// tags, so they always return an empty Tags slice (matches AWS behaviour for -// untagged resources). Cross-account or unknown ARNs return the per-resource -// not-found error so existence isn't leaked across accounts. +// target groups, listeners, listener rules). Tag data is read from the existing +// record stores — Spinifex doesn't have a separate tag KV. Listeners and rules +// currently never store tags, so they always return an empty Tags slice +// (matches AWS behaviour for untagged resources). Cross-account or unknown ARNs +// return the per-resource not-found error so existence isn't leaked across +// accounts. func (s *ELBv2ServiceImpl) DescribeTags(input *elbv2.DescribeTagsInput, accountID string) (*elbv2.DescribeTagsOutput, error) { if input == nil || len(input.ResourceArns) == 0 { return nil, errors.New(awserrors.ErrorMissingParameter) @@ -2022,6 +2040,18 @@ func (s *ELBv2ServiceImpl) DescribeTags(input *elbv2.DescribeTagsInput, accountI ownerAccount = l.AccountID // Listeners don't store tags yet — leave map nil. } + case elbv2ResourceListenerRule: + notFoundError = awserrors.ErrorELBv2RuleNotFound + r, rErr := s.store.GetRuleByArn(arn) + if rErr != nil { + slog.Error("DescribeTags: failed to get rule", "arn", arn, "err", rErr) + return nil, errors.New(awserrors.ErrorServerInternal) + } + if r != nil { + found = true + ownerAccount = r.AccountID + // Rules don't store tags yet — leave map nil. + } } if !found || ownerAccount != accountID { @@ -2357,9 +2387,21 @@ func (s *ELBv2ServiceImpl) listenerRecordToSDK(r *ListenerRecord) *elbv2.Listene } for _, a := range r.DefaultActions { - action := &elbv2.Action{ - Type: aws.String(a.Type), - TargetGroupArn: aws.String(a.TargetGroupArn), + action := &elbv2.Action{Type: aws.String(a.Type)} + if a.TargetGroupArn != "" { + action.TargetGroupArn = aws.String(a.TargetGroupArn) + } + if a.FixedResponse != nil { + fr := &elbv2.FixedResponseActionConfig{ + StatusCode: aws.String(a.FixedResponse.StatusCode), + } + if a.FixedResponse.ContentType != "" { + fr.ContentType = aws.String(a.FixedResponse.ContentType) + } + if a.FixedResponse.MessageBody != "" { + fr.MessageBody = aws.String(a.FixedResponse.MessageBody) + } + action.FixedResponseConfig = fr } listener.DefaultActions = append(listener.DefaultActions, action) } diff --git a/spinifex/handlers/elbv2/service_impl_test.go b/spinifex/handlers/elbv2/service_impl_test.go index 3a9f08118..968b86a65 100644 --- a/spinifex/handlers/elbv2/service_impl_test.go +++ b/spinifex/handlers/elbv2/service_impl_test.go @@ -2980,6 +2980,55 @@ func TestDescribeTags_Listener_NoTags(t *testing.T) { assert.Empty(t, out.TagDescriptions[0].Tags) } +func TestDescribeTags_ListenerRule_NoTags(t *testing.T) { + svc := setupTestService(t) + lbOut, _ := svc.CreateLoadBalancer(&elbv2.CreateLoadBalancerInput{Name: aws.String("tags-rule-lb")}, testAccountID) + tgOut, _ := svc.CreateTargetGroup(&elbv2.CreateTargetGroupInput{Name: aws.String("tags-rule-tg")}, testAccountID) + lstOut, err := svc.CreateListener(&elbv2.CreateListenerInput{ + LoadBalancerArn: lbOut.LoadBalancers[0].LoadBalancerArn, + Protocol: aws.String("HTTP"), + Port: aws.Int64(80), + DefaultActions: []*elbv2.Action{ + {Type: aws.String("forward"), TargetGroupArn: tgOut.TargetGroups[0].TargetGroupArn}, + }, + }, testAccountID) + require.NoError(t, err) + + ruleOut, err := svc.CreateRule(&elbv2.CreateRuleInput{ + ListenerArn: lstOut.Listeners[0].ListenerArn, + Priority: aws.Int64(10), + Conditions: []*elbv2.RuleCondition{ + {Field: aws.String("host-header"), Values: aws.StringSlice([]string{"app.example.com"})}, + }, + Actions: []*elbv2.Action{ + {Type: aws.String("forward"), TargetGroupArn: tgOut.TargetGroups[0].TargetGroupArn}, + }, + }, testAccountID) + require.NoError(t, err) + + // Rules don't store tags yet — must still return a TagDescription (with an + // empty Tags slice), not an error. This is the case the Terraform AWS + // provider hits during post-create refresh of aws_lb_listener_rule. + out, err := svc.DescribeTags(&elbv2.DescribeTagsInput{ + ResourceArns: []*string{ruleOut.Rules[0].RuleArn}, + }, testAccountID) + require.NoError(t, err) + require.Len(t, out.TagDescriptions, 1) + assert.Equal(t, *ruleOut.Rules[0].RuleArn, *out.TagDescriptions[0].ResourceArn) + assert.Empty(t, out.TagDescriptions[0].Tags) +} + +func TestDescribeTags_RuleNotFound(t *testing.T) { + svc := setupTestService(t) + _, err := svc.DescribeTags(&elbv2.DescribeTagsInput{ + ResourceArns: []*string{ + aws.String("arn:aws:elasticloadbalancing:us-east-1:123456789012:listener-rule/app/missing/lb-x/lst-y/rule-deadbeef"), + }, + }, testAccountID) + require.Error(t, err) + assert.Contains(t, err.Error(), awserrors.ErrorELBv2RuleNotFound) +} + func TestDescribeTags_MultipleArns(t *testing.T) { svc := setupTestService(t) lbOut, _ := svc.CreateLoadBalancer(&elbv2.CreateLoadBalancerInput{ diff --git a/spinifex/handlers/elbv2/types.go b/spinifex/handlers/elbv2/types.go index f0ef8daa3..21fe3ae0f 100644 --- a/spinifex/handlers/elbv2/types.go +++ b/spinifex/handlers/elbv2/types.go @@ -34,7 +34,8 @@ const ( ProtocolTCPUDP = "TCP_UDP" // Listener action types - ActionTypeForward = "forward" + ActionTypeForward = "forward" + ActionTypeFixedResponse = "fixed-response" // Rule condition fields RuleFieldHostHeader = "host-header" @@ -196,8 +197,17 @@ type ListenerRecord struct { // ListenerAction defines a listener's default action. type ListenerAction struct { - Type string `json:"type"` // "forward" + Type string `json:"type"` // "forward" or "fixed-response" TargetGroupArn string `json:"target_group_arn"` + // FixedResponse is populated when Type == "fixed-response" (no target group). + FixedResponse *FixedResponseAction `json:"fixed_response,omitempty"` +} + +// FixedResponseAction holds the canned reply for a "fixed-response" action. +type FixedResponseAction struct { + StatusCode string `json:"status_code"` + ContentType string `json:"content_type,omitempty"` + MessageBody string `json:"message_body,omitempty"` } // RuleRecord represents a stored listener rule.