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) {