From ee0bf68d71162dc12c4dd4cceb9fd257653f41ed Mon Sep 17 00:00:00 2001 From: Josh Siviour Date: Sun, 2 Aug 2026 08:02:34 +1000 Subject: [PATCH] fix(gateway): prefer a call site's own error message over the generic lookup (#630) gateway.ErrorHandler unconditionally overwrote every error with ErrorLookup's static per-code text, discarding both a caller's own formatted message and any per-service distinction for wire codes shared across services (e.g. ACM and EKS both use ResourceInUseException). ResolveErrorDetail now also returns a message when the producing call site used awserrors.Errorf, and LookupErrorMessage resolves service-scoped wording before falling back to the global default. DeleteVpc's DependencyViolation call sites now name the blocking subnet or security group. --- spinifex/awserrors/awserrors.go | 86 ++++++++++++++---- spinifex/awserrors/awserrors_test.go | 61 +++++++++++++ spinifex/gateway/gateway.go | 11 ++- spinifex/gateway/gateway_test.go | 88 +++++++++++++++++++ .../handlers/ec2/vpc/security_group_test.go | 5 +- spinifex/handlers/ec2/vpc/service_impl.go | 6 +- .../handlers/ec2/vpc/service_impl_test.go | 5 +- 7 files changed, 240 insertions(+), 22 deletions(-) diff --git a/spinifex/awserrors/awserrors.go b/spinifex/awserrors/awserrors.go index b0ecd7424..11da4fceb 100644 --- a/spinifex/awserrors/awserrors.go +++ b/spinifex/awserrors/awserrors.go @@ -383,10 +383,9 @@ var ( // wire code "ResourceInUseException", so this is a distinct, self-documenting // name for ACM call sites rather than a second ErrorLookup entry — ErrorLookup // is keyed by wire code across every service, so it can only hold one message - // per code. Naming it ErrorACMResourceInUse stops an ACM call site reading - // "EKS" while making no claim that the returned message text is ACM-flavored; - // it is still EKS's "cluster already exists" wording until ErrorLookup (or its - // caller in gateway.ErrorHandler) is keyed per-service instead of globally. + // per code. LookupErrorMessage resolves the ACM-flavored wording from + // errorLookupByService instead, so an ACM call site is not stuck with EKS's + // "cluster already exists" text. ErrorACMResourceInUse = ErrorEKSResourceInUse // ErrorACMRequestInProgress is what GetCertificate returns for a certificate // that exists but has not been issued yet (still PENDING_VALIDATION), so a @@ -561,34 +560,89 @@ func ValidErrorCode(code string) string { // ResolveErrorCode returns the first registered AWS error code in err's unwrap tree. func ResolveErrorCode(err error) (string, bool) { + code, _, ok := resolveErrorDetail(err) + return code, ok +} + +// ResolveErrorDetail resolves the same registered code as ResolveErrorCode, +// plus the message the producing call site attached via Errorf, if any. A +// generic %w wrapper added purely for internal context carries no message. +func ResolveErrorDetail(err error) (code, message string, ok bool) { + return resolveErrorDetail(err) +} + +// resolveErrorDetail is the shared unwrap-tree walk behind ResolveErrorCode +// and ResolveErrorDetail. +func resolveErrorDetail(err error) (code, message string, ok bool) { if err == nil { - return "", false + return "", "", false } - code := err.Error() - if _, ok := ErrorLookup[code]; ok { - return code, true + text := err.Error() + if _, exists := ErrorLookup[text]; exists { + var cause *codedError + if errors.As(err, &cause) { + return text, cause.message, true + } + return text, "", true } - if joined, ok := err.(interface{ Unwrap() []error }); ok { + if joined, isJoined := err.(interface{ Unwrap() []error }); isJoined { for _, inner := range joined.Unwrap() { - if code, found := ResolveErrorCode(inner); found { - return code, true + if c, m, found := resolveErrorDetail(inner); found { + return c, m, true } } - return "", false + return "", "", false } - if wrapped, ok := err.(interface{ Unwrap() error }); ok { - return ResolveErrorCode(wrapped.Unwrap()) + if wrapped, isWrapped := err.(interface{ Unwrap() error }); isWrapped { + return resolveErrorDetail(wrapped.Unwrap()) } - return "", false + return "", "", false } +// codedError is the leaf Errorf wraps around a registered code, letting the +// resolved layer distinguish a client-facing message from an unrelated %w +// wrapper. Error() returns only the code, so code resolution is unaffected. +type codedError struct { + code string + message string +} + +func (c *codedError) Error() string { return c.code } + // Errorf returns an error carrying an AWS error code where ResolveErrorCode can // find it, alongside a message for the logs and the traces. Formatting the code // into the message with %s instead leaves it unresolvable, so a handler's 400 // reaches the client as a 500 with its own code stripped. func Errorf(code, format string, args ...any) error { - return fmt.Errorf(format+": %w", append(args, errors.New(code))...) + cause := &codedError{code: code} + outer := fmt.Errorf(format+": %w", append(args, cause)...) + // Derived from the already-formatted outer text, not a second + // fmt.Sprintf(format, args...) call, so a caller's own %w renders via + // fmt.Errorf's %w support instead of failing under Sprintf, which has none. + cause.message = strings.TrimSuffix(outer.Error(), ": "+code) + return outer +} + +// errorLookupByService overrides ErrorLookup's message and HTTP status for a +// (service, code) pair whose wire code is shared by services with different +// canonical wording — e.g. ACM and EKS both use "ResourceInUseException". +var errorLookupByService = map[string]map[string]ErrorMessage{ + "acm": { + ErrorACMResourceInUse: {HTTPCode: 400, Message: "The certificate is in use by another AWS resource in this account. Remove the reference to the certificate before deleting it."}, + }, +} + +// LookupErrorMessage returns the ErrorMessage for code, scoped to service +// where errorLookupByService has an override, otherwise ErrorLookup's global +// default. +func LookupErrorMessage(service, code string) ErrorMessage { + if svcMsgs, ok := errorLookupByService[service]; ok { + if msg, ok := svcMsgs[code]; ok { + return msg + } + } + return ErrorLookup[code] } // ValidErrorCodeFromError resolves err or returns ErrorServerInternal. diff --git a/spinifex/awserrors/awserrors_test.go b/spinifex/awserrors/awserrors_test.go index fcdacf404..2cfbd1726 100644 --- a/spinifex/awserrors/awserrors_test.go +++ b/spinifex/awserrors/awserrors_test.go @@ -98,6 +98,67 @@ func TestResolveErrorCode(t *testing.T) { } } +// TestResolveErrorDetail_ErrorfMessageWins covers the message half of the +// gateway fidelity fix: a call site that used Errorf gets its own formatted +// wording back, not just the resolved code. +func TestResolveErrorDetail_ErrorfMessageWins(t *testing.T) { + err := Errorf(ErrorDependencyViolation, "the VPC has a dependent subnet %s", "subnet-1") + code, message, ok := ResolveErrorDetail(err) + if !ok || code != ErrorDependencyViolation { + t.Fatalf("ResolveErrorDetail() code = (%q, %v), want (%q, true)", code, ok, ErrorDependencyViolation) + } + want := "the VPC has a dependent subnet subnet-1" + if message != want { + t.Errorf("ResolveErrorDetail() message = %q, want %q", message, want) + } +} + +// TestResolveErrorDetail_PlainWrapCarriesNoMessage is the compatibility case: +// a generic %w wrapper not produced by Errorf must not be mistaken for a +// client-facing message, even though it sits right next to the bare code. +func TestResolveErrorDetail_PlainWrapCarriesNoMessage(t *testing.T) { + err := fmt.Errorf("launch on node-1: %w", errors.New(ErrorInsufficientAddressCapacity)) + code, message, ok := ResolveErrorDetail(err) + if !ok || code != ErrorInsufficientAddressCapacity { + t.Fatalf("ResolveErrorDetail() code = (%q, %v), want (%q, true)", code, ok, ErrorInsufficientAddressCapacity) + } + if message != "" { + t.Errorf("ResolveErrorDetail() message = %q, want empty (not Errorf-produced)", message) + } +} + +// TestResolveErrorDetail_BareCodeCarriesNoMessage is the compatibility case for +// the overwhelming majority of call sites: a bare errors.New(code) resolves the +// code with no message, exactly as ResolveErrorCode always has. +func TestResolveErrorDetail_BareCodeCarriesNoMessage(t *testing.T) { + code, message, ok := ResolveErrorDetail(errors.New(ErrorInvalidParameterValue)) + if !ok || code != ErrorInvalidParameterValue || message != "" { + t.Errorf("ResolveErrorDetail() = (%q, %q, %v), want (%q, \"\", true)", code, message, ok, ErrorInvalidParameterValue) + } +} + +// TestLookupErrorMessage covers per-service scoping: ACM must not inherit +// EKS's wording for the shared ResourceInUseException code, and an unscoped +// pair must fall back to ErrorLookup's existing global default unchanged. +func TestLookupErrorMessage(t *testing.T) { + acm := LookupErrorMessage("acm", ErrorACMResourceInUse) + eks := LookupErrorMessage("eks", ErrorEKSResourceInUse) + global := ErrorLookup[ErrorEKSResourceInUse] + + if acm.Message == global.Message { + t.Errorf("LookupErrorMessage(acm, ResourceInUseException) = %q, want distinct ACM wording", acm.Message) + } + if eks.Message != global.Message || eks.HTTPCode != global.HTTPCode { + t.Errorf("LookupErrorMessage(eks, ResourceInUseException) = %+v, want unchanged global %+v", eks, global) + } + + // A service/code pair with no override falls back to the global default. + unscoped := LookupErrorMessage("ec2", ErrorInvalidParameterValue) + if unscoped != ErrorLookup[ErrorInvalidParameterValue] { + t.Errorf("LookupErrorMessage(ec2, InvalidParameterValue) = %+v, want unchanged global %+v", unscoped, ErrorLookup[ErrorInvalidParameterValue]) + } +} + func TestValidErrorCodeFromError(t *testing.T) { wrapped := fmt.Errorf("launch: %w", errors.New(ErrorInsufficientInstanceCapacity)) if got := ValidErrorCodeFromError(wrapped); got != ErrorInsufficientInstanceCapacity { diff --git a/spinifex/gateway/gateway.go b/spinifex/gateway/gateway.go index 31848b7c4..d5d106b99 100644 --- a/spinifex/gateway/gateway.go +++ b/spinifex/gateway/gateway.go @@ -555,13 +555,20 @@ func (gw *GatewayConfig) ErrorHandler(w http.ResponseWriter, r *http.Request, er slog.Debug("ErrorHandler", "service", svc, "error", err.Error()) var requestId = uuid.NewString() - code, exists := awserrors.ResolveErrorCode(err) + code, message, exists := awserrors.ResolveErrorDetail(err) if !exists { slog.Warn("Unknown error code", "error", err.Error()) code = awserrors.ErrorInternalError + message = "" } - errorMsg := awserrors.ErrorLookup[code] + // LookupErrorMessage resolves per-service wording first (e.g. ACM vs EKS + // both using "ResourceInUseException"); a message the producing call site + // supplied via awserrors.Errorf then takes priority over either default. + errorMsg := awserrors.LookupErrorMessage(svc, code) + if message != "" { + errorMsg.Message = message + } if errorMsg.HTTPCode == 0 { errorMsg.HTTPCode = 500 } diff --git a/spinifex/gateway/gateway_test.go b/spinifex/gateway/gateway_test.go index 4b04944e3..f02218c34 100644 --- a/spinifex/gateway/gateway_test.go +++ b/spinifex/gateway/gateway_test.go @@ -220,6 +220,94 @@ func TestErrorHandler_WrappedErrorCode(t *testing.T) { assert.NotContains(t, xmlStr, "launch on node-1") } +// TestErrorHandler_PrefersCallSiteMessage covers the DeleteVpc DependencyViolation +// fidelity gap: a call site that names the blocking resource via awserrors.Errorf +// must have that wording reach the client instead of the generic ErrorLookup text. +func TestErrorHandler_PrefersCallSiteMessage(t *testing.T) { + gw := &GatewayConfig{DisableLogging: true} + + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + ctx := context.WithValue(r.Context(), ctxService, "ec2") + r = r.WithContext(ctx) + err := awserrors.Errorf(awserrors.ErrorDependencyViolation, + "the VPC has a dependent subnet %s that must be deleted first", "subnet-abc123") + gw.ErrorHandler(w, r, err) + }) + + req := httptest.NewRequest(http.MethodPost, "/", nil) + resp := doRequest(handler, req) + assert.Equal(t, 400, resp.StatusCode) + + body, _ := io.ReadAll(resp.Body) + xmlStr := string(body) + assert.Contains(t, xmlStr, ""+awserrors.ErrorDependencyViolation+"") + assert.Contains(t, xmlStr, "subnet-abc123") + assert.NotContains(t, xmlStr, awserrors.ErrorLookup[awserrors.ErrorDependencyViolation].Message) +} + +// TestErrorHandler_ACMResourceInUse_UsesACMWording is one direction of the +// ResourceInUseException collision check: ACM's DeleteCertificate must not +// surface EKS's "cluster already exists" wording for the shared wire code. +func TestErrorHandler_ACMResourceInUse_UsesACMWording(t *testing.T) { + gw := &GatewayConfig{DisableLogging: true} + req := httptest.NewRequest(http.MethodPost, "/", nil) + ctx := context.WithValue(req.Context(), ctxService, "acm") + req = req.WithContext(ctx) + w := httptest.NewRecorder() + + gw.ErrorHandler(w, req, errors.New(awserrors.ErrorACMResourceInUse)) + + var env struct { + Type string `json:"__type"` + Message string `json:"message"` + } + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &env)) + assert.NotEqual(t, awserrors.ErrorLookup[awserrors.ErrorEKSResourceInUse].Message, env.Message) + assert.NotContains(t, env.Message, "cluster") +} + +// TestErrorHandler_EKSResourceInUse_UsesEKSWording is the other direction: EKS's +// own ResourceInUseException must keep its existing wording, unaffected by the +// ACM-specific override. +func TestErrorHandler_EKSResourceInUse_UsesEKSWording(t *testing.T) { + gw := &GatewayConfig{DisableLogging: true} + req := httptest.NewRequest(http.MethodPost, "/", nil) + ctx := context.WithValue(req.Context(), ctxService, "eks") + req = req.WithContext(ctx) + w := httptest.NewRecorder() + + gw.ErrorHandler(w, req, errors.New(awserrors.ErrorEKSResourceInUse)) + + var env struct { + Type string `json:"__type"` + Message string `json:"message"` + } + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &env)) + assert.Equal(t, awserrors.ErrorLookup[awserrors.ErrorEKSResourceInUse].Message, env.Message) +} + +// TestErrorHandler_NoMessageSupplied_MatchesErrorLookup is the compatibility +// case: an error path that supplies no message (the vast majority of call +// sites) must keep rendering exactly today's ErrorLookup text, unchanged. +func TestErrorHandler_NoMessageSupplied_MatchesErrorLookup(t *testing.T) { + gw := &GatewayConfig{DisableLogging: true} + + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + ctx := context.WithValue(r.Context(), ctxService, "ec2") + r = r.WithContext(ctx) + gw.ErrorHandler(w, r, errors.New(awserrors.ErrorInvalidParameterValue)) + }) + + req := httptest.NewRequest(http.MethodPost, "/", nil) + resp := doRequest(handler, req) + assert.Equal(t, 400, resp.StatusCode) + + body, _ := io.ReadAll(resp.Body) + xmlStr := string(body) + assert.Contains(t, xmlStr, ""+awserrors.ErrorInvalidParameterValue+"") + assert.Contains(t, xmlStr, awserrors.ErrorLookup[awserrors.ErrorInvalidParameterValue].Message) +} + func TestErrorHandler_ELBv2Service(t *testing.T) { gw := &GatewayConfig{DisableLogging: true} diff --git a/spinifex/handlers/ec2/vpc/security_group_test.go b/spinifex/handlers/ec2/vpc/security_group_test.go index 2fdadab93..9c6f8d33d 100644 --- a/spinifex/handlers/ec2/vpc/security_group_test.go +++ b/spinifex/handlers/ec2/vpc/security_group_test.go @@ -1228,11 +1228,14 @@ func TestDeleteSecurityGroup_RejectsReferencedFromOtherSG(t *testing.T) { func TestDeleteVpc_BlocksOnNonDefaultSG(t *testing.T) { svc := setupTestVPCService(t) vpcID := createTestVPC(t, svc, "10.0.0.0/16") - createTestSG(t, svc, vpcID, "user-sg") + sgID := createTestSG(t, svc, vpcID, "user-sg") _, err := svc.DeleteVpc(context.Background(), &ec2.DeleteVpcInput{VpcId: aws.String(vpcID)}, testAccountID) require.Error(t, err) assert.Contains(t, err.Error(), "DependencyViolation") + // The message must name the blocking security group so the caller knows + // what to delete first, not just that something is blocking. + assert.Contains(t, err.Error(), sgID) } func TestDeleteVpc_CascadesDefaultSG(t *testing.T) { diff --git a/spinifex/handlers/ec2/vpc/service_impl.go b/spinifex/handlers/ec2/vpc/service_impl.go index acbf547ac..e5ff57cb5 100644 --- a/spinifex/handlers/ec2/vpc/service_impl.go +++ b/spinifex/handlers/ec2/vpc/service_impl.go @@ -360,7 +360,8 @@ func (s *VPCServiceImpl) DeleteVpc(ctx context.Context, input *ec2.DeleteVpcInpu return nil, errors.New(awserrors.ErrorServerInternal) } if subnet.VpcId == vpcID { - return nil, errors.New(awserrors.ErrorDependencyViolation) + return nil, awserrors.Errorf(awserrors.ErrorDependencyViolation, + "the VPC has a dependent subnet %s that must be deleted first", subnet.SubnetId) } } @@ -396,7 +397,8 @@ func (s *VPCServiceImpl) DeleteVpc(ctx context.Context, input *ec2.DeleteVpcInpu continue } if !sg.IsDefault { - return nil, errors.New(awserrors.ErrorDependencyViolation) + return nil, awserrors.Errorf(awserrors.ErrorDependencyViolation, + "the VPC has a dependent security group %s that must be deleted first", sg.GroupId) } defaultSGId = sg.GroupId } diff --git a/spinifex/handlers/ec2/vpc/service_impl_test.go b/spinifex/handlers/ec2/vpc/service_impl_test.go index 95dddeaff..2059f5a9a 100644 --- a/spinifex/handlers/ec2/vpc/service_impl_test.go +++ b/spinifex/handlers/ec2/vpc/service_impl_test.go @@ -199,13 +199,16 @@ func TestDeleteVpc_MissingID(t *testing.T) { func TestDeleteVpc_WithSubnets(t *testing.T) { svc := setupTestVPCService(t) vpcID := createTestVPC(t, svc, "10.0.0.0/16") - createTestSubnet(t, svc, vpcID, "10.0.1.0/24") + subnetID := createTestSubnet(t, svc, vpcID, "10.0.1.0/24") // Should fail because VPC has subnets _, err := svc.DeleteVpc(context.Background(), &ec2.DeleteVpcInput{ VpcId: aws.String(vpcID), }, testAccountID) assert.ErrorContains(t, err, "DependencyViolation") + // The message must name the blocking subnet so the caller knows what to + // delete first, not just that something is blocking. + assert.ErrorContains(t, err, subnetID) } func TestDescribeVpcs_All(t *testing.T) {