From e03d15227d7d1ff404ec1918e0e499406283f991 Mon Sep 17 00:00:00 2001 From: Josh Siviour Date: Wed, 16 Sep 2026 00:06:21 +1000 Subject: [PATCH] fix: carry a handler's refusal message to the client without leaking wrapper context (#1036) * fix: carry a handler's own refusal message to the client without leaking wrapper context A daemon that explains a refusal had that explanation discarded one step short of the client, so every NATS-backed error surfaced as the generic ErrorLookup blurb for its code. The message crossed NATS intact; resolveErrorDetail then dropped it because decodedNATSError was not the shape it recognises. Rebuild the decoded response through awserrors.Errorf so the walk finds the message, and send only ResolveErrorDetail's curated message from the two encode sites instead of err.Error(). The latter rendered every %w layer, including context a call site added purely for its own logs, so restoring the message without also fixing the encode side would have turned a fidelity bug into an information disclosure. Two existing tests pinned that wrapper context onto the wire, directly contradicting the gateway's own no-leak assertion; both are rewritten to pin the sanitized contract instead. Also name the rejected filter in the twenty-four ParseFilters sites that logged the detail and returned a bare code, and give the ACM force-renew refusals a registered code so their reason still reaches the caller. * fix: stop the filter rejection repeating its own error code to the client ParseFilters spelled the code into its message text, so once the code began travelling separately the client saw it twice: "InvalidParameterValue: InvalidParameterValue: The filter 'x' is invalid". Give ParseFilters the code directly via awserrors.Errorf and have its twenty-five callers return that error unwrapped rather than re-wrapping it. All of them already mapped it to InvalidParameterValue, so nothing changes but the duplication. * fix: surface why a security group rule was rejected ipPermissionsToSGRules already names the reason, "IPv6 rules are not supported" or "IpPermission must specify at least one IpRange or UserIdGroupPair", and both Authorize call sites logged it and returned a bare InvalidParameterValue, so the caller saw only the canned blurb. This is the case mulga-e9qqx names specifically. It does not make IPv6 rules work; it makes the refusal legible. --- spinifex/awserrors/awserrors_test.go | 20 ++++++ spinifex/daemon/daemon_handlers.go | 3 +- spinifex/daemon/daemon_handlers_error_test.go | 48 +++++++++++--- spinifex/filterutil/filterutil.go | 6 +- spinifex/gateway/bedrock/invoke_llama_test.go | 4 +- spinifex/gateway/bedrock/vllm_test.go | 2 +- spinifex/handlers/acm/renewal.go | 4 +- spinifex/handlers/ec2/eigw/service_impl.go | 2 +- spinifex/handlers/ec2/eip/service_impl.go | 2 +- spinifex/handlers/ec2/igw/service_impl.go | 2 +- spinifex/handlers/ec2/image/service_impl.go | 2 +- .../handlers/ec2/image/service_impl_test.go | 5 +- .../handlers/ec2/instance/service_impl.go | 4 +- .../ec2/instance/service_impl_test.go | 5 +- .../handlers/ec2/instance/status_selection.go | 4 +- spinifex/handlers/ec2/key/service_impl.go | 2 +- .../ec2/launchtemplate/service_impl.go | 4 +- spinifex/handlers/ec2/natgw/service_impl.go | 2 +- .../ec2/placementgroup/service_impl.go | 2 +- .../handlers/ec2/routetable/service_impl.go | 2 +- .../handlers/ec2/snapshot/service_impl.go | 2 +- .../handlers/ec2/spotinstance/service_impl.go | 2 +- .../ec2/spotinstance/service_impl_test.go | 5 +- spinifex/handlers/ec2/tags/service_impl.go | 2 +- spinifex/handlers/ec2/volume/service_impl.go | 6 +- .../handlers/ec2/volume/service_impl_test.go | 10 ++- .../ec2/vpc/GetSecurityGroupsForVpc_test.go | 5 +- spinifex/handlers/ec2/vpc/eni.go | 2 +- spinifex/handlers/ec2/vpc/security_group.go | 10 +-- spinifex/handlers/ec2/vpc/service_impl.go | 4 +- spinifex/utils/nats.go | 20 +----- spinifex/utils/nats_test.go | 65 +++++++++++++++++-- 32 files changed, 184 insertions(+), 74 deletions(-) diff --git a/spinifex/awserrors/awserrors_test.go b/spinifex/awserrors/awserrors_test.go index f7299ced9..40cad9804 100644 --- a/spinifex/awserrors/awserrors_test.go +++ b/spinifex/awserrors/awserrors_test.go @@ -115,6 +115,26 @@ func TestResolveErrorDetail_ErrorfMessageWins(t *testing.T) { } } +// TestResolveErrorDetail_ErrorfMessageSurvivesOuterWrapper is the core +// anti-leak assertion: a call site's own %w wrapper, added purely for its +// logs, must not replace or blend with the curated message Errorf recorded. +func TestResolveErrorDetail_ErrorfMessageSurvivesOuterWrapper(t *testing.T) { + inner := Errorf(ErrorInsufficientAddressCapacity, "no capacity for m5.large on any eligible node") + outer := fmt.Errorf("dial predastore 10.0.1.5:8443: %w", inner) + + code, message, ok := ResolveErrorDetail(outer) + if !ok || code != ErrorInsufficientAddressCapacity { + t.Fatalf("ResolveErrorDetail() code = (%q, %v), want (%q, true)", code, ok, ErrorInsufficientAddressCapacity) + } + want := "no capacity for m5.large on any eligible node" + if message != want { + t.Errorf("ResolveErrorDetail() message = %q, want %q", message, want) + } + if strings.Contains(message, "dial predastore") { + t.Errorf("ResolveErrorDetail() message %q leaked the outer wrapper's context", message) + } +} + // 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. diff --git a/spinifex/daemon/daemon_handlers.go b/spinifex/daemon/daemon_handlers.go index 1536c20ba..df77e7035 100644 --- a/spinifex/daemon/daemon_handlers.go +++ b/spinifex/daemon/daemon_handlers.go @@ -73,7 +73,8 @@ func respondWithError(nodeID string, msg *nats.Msg, errCode string) { // an opaque ServerInternal, leaving the reason visible only in the daemon log. // Mirrors utils.ServeNATSRequestCtx, which has always preserved the message. func respondWithServiceError(nodeID string, msg *nats.Msg, err error) { - payload := utils.GenerateErrorPayloadWithMessage(awserrors.ValidErrorCodeFromError(err), err.Error()) + _, message, _ := awserrors.ResolveErrorDetail(err) + payload := utils.GenerateErrorPayloadWithMessage(awserrors.ValidErrorCodeFromError(err), message) respondNATSMsg(nodeID, msg, payload) } diff --git a/spinifex/daemon/daemon_handlers_error_test.go b/spinifex/daemon/daemon_handlers_error_test.go index e933a1a2d..8b491a139 100644 --- a/spinifex/daemon/daemon_handlers_error_test.go +++ b/spinifex/daemon/daemon_handlers_error_test.go @@ -44,31 +44,61 @@ func requestErrorEnvelope(t *testing.T, subject string, fn func(context.Context, return errResp } -// An unregistered error sanitizes to ServerInternal, but its reason must still -// reach the caller. This is the ACM force-renew refusal: without the message the -// CLI prints a bare "ServerInternal" and the operator has to read the daemon log -// to learn that only PRIVATE_CA certificates can be force-renewed. -func TestHandleNATSRequest_UncodedErrorCarriesMessage(t *testing.T) { +// A plain errors.New carries no client-facing message: only awserrors.Errorf +// records one. Falling back to err.Error() here would put whatever context a +// caller wrapped in for its own logs onto the wire, which is the disclosure +// risk this contract exists to prevent. +func TestHandleNATSRequest_UncodedErrorOmitsMessage(t *testing.T) { const reason = "acm renewal: arn:aws:acm:ap-southeast-2:000000000001:certificate/abc is a AMAZON_ISSUED certificate; only PRIVATE_CA certificates can be force-renewed" errResp := requestErrorEnvelope(t, "test.err.msg.uncoded", func(context.Context, *testInput, string) (*testOutput, error) { return nil, errors.New(reason) }) + assert.Equal(t, awserrors.ErrorServerInternal, errResp["Code"]) + assert.Nil(t, errResp["Message"]) +} + +// The ACM force-renew refusal this test mirrors now builds its error with +// awserrors.Errorf specifically so the operator sees the actionable reason +// instead of a bare "ServerInternal", without relying on the rejected +// err.Error() fallback. +func TestHandleNATSRequest_ErrorfMessageOnUnregisteredCodeCarriesMessage(t *testing.T) { + const reason = "acm renewal: arn:aws:acm:ap-southeast-2:000000000001:certificate/abc is a AMAZON_ISSUED certificate; only PRIVATE_CA certificates can be force-renewed" + + errResp := requestErrorEnvelope(t, "test.err.msg.uncoded.errorf", func(context.Context, *testInput, string) (*testOutput, error) { + return nil, awserrors.Errorf(awserrors.ErrorServerInternal, "%s", reason) + }) + assert.Equal(t, awserrors.ErrorServerInternal, errResp["Code"]) assert.Equal(t, reason, errResp["Message"], "the actionable reason must survive the transport, not only the daemon log") } -// A wrapped coded error keeps the real code (so the gateway maps the right HTTP -// status) and carries the wrapping context alongside it. -func TestHandleNATSRequest_WrappedCodedErrorCarriesMessage(t *testing.T) { +// A bare coded error wrapped only for the handler's own log line keeps the +// real code (so the gateway maps the right HTTP status) but must not leak the +// wrapping context onto the wire — that context is internal detail, not a +// message the producing call site attached via awserrors.Errorf. +func TestHandleNATSRequest_WrappedCodedErrorOmitsWrapperContext(t *testing.T) { errResp := requestErrorEnvelope(t, "test.err.msg.wrapped", func(context.Context, *testInput, string) (*testOutput, error) { return nil, fmt.Errorf("vol-123 is attached to i-456: %w", errors.New(awserrors.ErrorVolumeInUse)) }) assert.Equal(t, awserrors.ErrorVolumeInUse, errResp["Code"]) - assert.Contains(t, errResp["Message"], "vol-123 is attached to i-456") + assert.Nil(t, errResp["Message"]) +} + +// A wrapped Errorf-built error keeps its curated message across the wire, but +// drops an outer wrapper added purely for the handler's own log line. +func TestHandleNATSRequest_WrappedErrorfCarriesMessageNotWrapperContext(t *testing.T) { + const curated = "vol-123 is attached to i-456" + errResp := requestErrorEnvelope(t, "test.err.msg.wrapped.errorf", func(context.Context, *testInput, string) (*testOutput, error) { + cause := awserrors.Errorf(awserrors.ErrorVolumeInUse, "%s", curated) + return nil, fmt.Errorf("detach: %w", cause) + }) + + assert.Equal(t, awserrors.ErrorVolumeInUse, errResp["Code"]) + assert.Equal(t, curated, errResp["Message"]) } // A bare code carries no message. Handlers returning errors.New(awserrors.X) are diff --git a/spinifex/filterutil/filterutil.go b/spinifex/filterutil/filterutil.go index 95d4d693a..56bf0b003 100644 --- a/spinifex/filterutil/filterutil.go +++ b/spinifex/filterutil/filterutil.go @@ -1,11 +1,11 @@ package filterutil import ( - "fmt" "log/slog" "strings" "github.com/aws/aws-sdk-go/service/ec2" + "github.com/mulgadc/spinifex/spinifex/awserrors" ) // ParseFilters converts AWS SDK filter types to map[string][]string. @@ -23,8 +23,10 @@ func ParseFilters(filters []*ec2.Filter, validNames map[string]bool) (map[string } name := *f.Name + // Carries the code itself so callers return it unwrapped; wrapping it + // again would reach the client stuttered, code repeated in the message. if !strings.HasPrefix(name, "tag:") && !validNames[name] { - return nil, fmt.Errorf("InvalidParameterValue: The filter '%s' is invalid", name) + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "The filter '%s' is invalid", name) } for _, v := range f.Values { diff --git a/spinifex/gateway/bedrock/invoke_llama_test.go b/spinifex/gateway/bedrock/invoke_llama_test.go index ae172c2b4..61fa1b998 100644 --- a/spinifex/gateway/bedrock/invoke_llama_test.go +++ b/spinifex/gateway/bedrock/invoke_llama_test.go @@ -106,8 +106,8 @@ func TestLlamaInvokeAdapter_InvokeModel_UnresolvedEndpointReturnsModelNotReady(t } // TestLlamaInvokeAdapter_InvokeModel_EndpointErrorPropagatesCode proves a -// resolver err carrying an admission-refusal code (wrapped the way -// decodedNATSError's Unwrap tree does) surfaces faithfully rather than being +// resolver err carrying an admission-refusal code (wrapped the way a decoded +// NATS error's Unwrap tree does) surfaces faithfully rather than being // flattened to ServiceUnavailableException. func TestLlamaInvokeAdapter_InvokeModel_EndpointErrorPropagatesCode(t *testing.T) { wrapped := fmt.Errorf("endpoint resolve: %w", errors.New(awserrors.ErrorModelNotReadyException)) diff --git a/spinifex/gateway/bedrock/vllm_test.go b/spinifex/gateway/bedrock/vllm_test.go index 8250813eb..f20b2a2ca 100644 --- a/spinifex/gateway/bedrock/vllm_test.go +++ b/spinifex/gateway/bedrock/vllm_test.go @@ -79,7 +79,7 @@ func TestVLLMProvider_Converse_UnresolvedEndpointReturnsModelNotReady(t *testing } // TestVLLMProvider_Converse_EndpointErrorPropagatesCode proves a resolver err -// carrying an admission-refusal code (wrapped the way decodedNATSError's +// carrying an admission-refusal code (wrapped the way a decoded NATS error's // Unwrap tree does) surfaces faithfully rather than being flattened to // ServiceUnavailableException. func TestVLLMProvider_Converse_EndpointErrorPropagatesCode(t *testing.T) { diff --git a/spinifex/handlers/acm/renewal.go b/spinifex/handlers/acm/renewal.go index d933633b2..9e0c3f87e 100644 --- a/spinifex/handlers/acm/renewal.go +++ b/spinifex/handlers/acm/renewal.go @@ -376,7 +376,7 @@ func (w *Worker) ForceRenewCertificate(ctx context.Context, input *ForceRenewCer return nil, err } if rec.Type != acm.CertificateTypePrivate { - return nil, fmt.Errorf("acm renewal: %s is a %s certificate; only PRIVATE_CA certificates can be force-renewed", input.CertificateArn, rec.Type) + return nil, awserrors.Errorf(awserrors.ErrorServerInternal, "acm renewal: %s is a %s certificate; only PRIVATE_CA certificates can be force-renewed", input.CertificateArn, rec.Type) } acquired, err := w.renewWithLease(ctx, input.CertificateArn) @@ -384,7 +384,7 @@ func (w *Worker) ForceRenewCertificate(ctx context.Context, input *ForceRenewCer return nil, err } if !acquired { - return nil, fmt.Errorf("acm renewal: %s is currently leased by another node; try again shortly", input.CertificateArn) + return nil, awserrors.Errorf(awserrors.ErrorServerInternal, "acm renewal: %s is currently leased by another node; try again shortly", input.CertificateArn) } // Re-read for the post-renewal outcome only: serial, validity and renewal diff --git a/spinifex/handlers/ec2/eigw/service_impl.go b/spinifex/handlers/ec2/eigw/service_impl.go index 5ea73e37d..acc77b1a8 100644 --- a/spinifex/handlers/ec2/eigw/service_impl.go +++ b/spinifex/handlers/ec2/eigw/service_impl.go @@ -163,7 +163,7 @@ func (s *EgressOnlyIGWServiceImpl) DescribeEgressOnlyInternetGateways(ctx contex parsedFilters, err := filterutil.ParseFilters(input.Filters, describeEIGWValidFilters) if err != nil { slog.WarnContext(ctx, "DescribeEgressOnlyInternetGateways: invalid filter", "err", err) - return nil, errors.New(awserrors.ErrorInvalidParameterValue) + return nil, err } prefix := accountID + "." diff --git a/spinifex/handlers/ec2/eip/service_impl.go b/spinifex/handlers/ec2/eip/service_impl.go index 73f17e35a..868d798f1 100644 --- a/spinifex/handlers/ec2/eip/service_impl.go +++ b/spinifex/handlers/ec2/eip/service_impl.go @@ -449,7 +449,7 @@ func (s *EIPServiceImpl) DescribeAddresses(ctx context.Context, input *ec2.Descr parsedFilters, err := filterutil.ParseFilters(input.Filters, describeAddressesValidFilters) if err != nil { slog.WarnContext(ctx, "DescribeAddresses: invalid filter", "err", err) - return nil, errors.New(awserrors.ErrorInvalidParameterValue) + return nil, err } prefix := accountID + "." diff --git a/spinifex/handlers/ec2/igw/service_impl.go b/spinifex/handlers/ec2/igw/service_impl.go index a9ab45b69..0957c8379 100644 --- a/spinifex/handlers/ec2/igw/service_impl.go +++ b/spinifex/handlers/ec2/igw/service_impl.go @@ -203,7 +203,7 @@ func (s *IGWServiceImpl) DescribeInternetGateways(ctx context.Context, input *ec parsedFilters, err := filterutil.ParseFilters(input.Filters, describeIGWValidFilters) if err != nil { slog.WarnContext(ctx, "DescribeInternetGateways: invalid filter", "err", err) - return nil, errors.New(awserrors.ErrorInvalidParameterValue) + return nil, err } prefix := accountID + "." diff --git a/spinifex/handlers/ec2/image/service_impl.go b/spinifex/handlers/ec2/image/service_impl.go index 3c275c8af..4afd2e710 100644 --- a/spinifex/handlers/ec2/image/service_impl.go +++ b/spinifex/handlers/ec2/image/service_impl.go @@ -132,7 +132,7 @@ func (s *ImageServiceImpl) DescribeImages(ctx context.Context, input *ec2.Descri parsedFilters, err := filterutil.ParseFilters(input.Filters, describeImagesValidFilters) if err != nil { slog.WarnContext(ctx, "DescribeImages: invalid filter", "err", err) - return nil, errors.New(awserrors.ErrorInvalidParameterValue) + return nil, err } return s.describeImages(ctx, input, accountID, parsedFilters) diff --git a/spinifex/handlers/ec2/image/service_impl_test.go b/spinifex/handlers/ec2/image/service_impl_test.go index e57a2c728..eb061d834 100644 --- a/spinifex/handlers/ec2/image/service_impl_test.go +++ b/spinifex/handlers/ec2/image/service_impl_test.go @@ -1181,7 +1181,10 @@ func TestDescribeImages_FilterUnknownName_Error(t *testing.T) { }, }, testAccountID) require.Error(t, err) - assert.Equal(t, awserrors.ErrorInvalidParameterValue, err.Error()) + code, message, ok := awserrors.ResolveErrorDetail(err) + assert.True(t, ok) + assert.Equal(t, awserrors.ErrorInvalidParameterValue, code) + assert.Contains(t, message, "bogus-filter") } func TestDescribeImages_FilterWildcard(t *testing.T) { diff --git a/spinifex/handlers/ec2/instance/service_impl.go b/spinifex/handlers/ec2/instance/service_impl.go index f21ad3635..afd33de76 100644 --- a/spinifex/handlers/ec2/instance/service_impl.go +++ b/spinifex/handlers/ec2/instance/service_impl.go @@ -1778,7 +1778,7 @@ func (s *InstanceServiceImpl) DescribeInstances(ctx context.Context, input *ec2. parsedFilters, err := filterutil.ParseFilters(input.Filters, DescribeInstancesValidFilters) if err != nil { slog.WarnContext(ctx, "DescribeInstances: invalid filter", "err", err) - return nil, errors.New(awserrors.ErrorInvalidParameterValue) + return nil, err } reservationMap := make(map[string]*ec2.Reservation) @@ -1906,7 +1906,7 @@ func (s *InstanceServiceImpl) describeInstancesFromKV(ctx context.Context, input parsedFilters, filterErr := filterutil.ParseFilters(input.Filters, DescribeInstancesValidFilters) if filterErr != nil { slog.WarnContext(ctx, opName+": invalid filter", "err", filterErr) - return nil, errors.New(awserrors.ErrorInvalidParameterValue) + return nil, filterErr } instances, err := listFn() diff --git a/spinifex/handlers/ec2/instance/service_impl_test.go b/spinifex/handlers/ec2/instance/service_impl_test.go index f2482ed57..b01953c23 100644 --- a/spinifex/handlers/ec2/instance/service_impl_test.go +++ b/spinifex/handlers/ec2/instance/service_impl_test.go @@ -4199,7 +4199,10 @@ func TestDescribeInstanceStatus_UnknownFilter(t *testing.T) { }}, }, owner) require.Error(t, err) - assert.Equal(t, awserrors.ErrorInvalidParameterValue, err.Error()) + code, message, ok := awserrors.ResolveErrorDetail(err) + assert.True(t, ok) + assert.Equal(t, awserrors.ErrorInvalidParameterValue, code) + assert.Contains(t, message, "event.code") } func TestDescribeInstanceStatus_StateNameFilter(t *testing.T) { diff --git a/spinifex/handlers/ec2/instance/status_selection.go b/spinifex/handlers/ec2/instance/status_selection.go index ba53142fb..29c5827f8 100644 --- a/spinifex/handlers/ec2/instance/status_selection.go +++ b/spinifex/handlers/ec2/instance/status_selection.go @@ -1,12 +1,10 @@ package handlers_ec2_instance import ( - "errors" "log/slog" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/ec2" - "github.com/mulgadc/spinifex/spinifex/awserrors" "github.com/mulgadc/spinifex/spinifex/filterutil" "github.com/mulgadc/spinifex/spinifex/vm" ) @@ -41,7 +39,7 @@ func ParseStatusSelection(input *ec2.DescribeInstanceStatusInput, accountID stri filters, err := filterutil.ParseFilters(input.Filters, DescribeInstanceStatusValidFilters) if err != nil { slog.Warn("DescribeInstanceStatus: invalid filter", "err", err) - return StatusSelection{}, errors.New(awserrors.ErrorInvalidParameterValue) + return StatusSelection{}, err } includedStates := describeInstanceStatusRunningOnly diff --git a/spinifex/handlers/ec2/key/service_impl.go b/spinifex/handlers/ec2/key/service_impl.go index 5d7699448..a822b7bdd 100644 --- a/spinifex/handlers/ec2/key/service_impl.go +++ b/spinifex/handlers/ec2/key/service_impl.go @@ -605,7 +605,7 @@ func (s *KeyServiceImpl) DescribeKeyPairs(ctx context.Context, input *ec2.Descri parsedFilters, err := filterutil.ParseFilters(input.Filters, describeKeyPairsValidFilters) if err != nil { slog.WarnContext(ctx, "DescribeKeyPairs: invalid filter", "err", err) - return nil, errors.New(awserrors.ErrorInvalidParameterValue) + return nil, err } prefix := fmt.Sprintf("keys/%s/", accountID) diff --git a/spinifex/handlers/ec2/launchtemplate/service_impl.go b/spinifex/handlers/ec2/launchtemplate/service_impl.go index 5e635a46b..3d4424793 100644 --- a/spinifex/handlers/ec2/launchtemplate/service_impl.go +++ b/spinifex/handlers/ec2/launchtemplate/service_impl.go @@ -599,7 +599,7 @@ func (s *LaunchTemplateServiceImpl) DescribeLaunchTemplates(ctx context.Context, filters, err := filterutil.ParseFilters(input.Filters, describeLaunchTemplatesValidFilters) if err != nil { slog.WarnContext(ctx, "DescribeLaunchTemplates: invalid filter", "err", err) - return nil, errors.New(awserrors.ErrorInvalidParameterValue) + return nil, err } for _, id := range input.LaunchTemplateIds { @@ -718,7 +718,7 @@ func (s *LaunchTemplateServiceImpl) DescribeLaunchTemplateVersions(ctx context.C filters, err := filterutil.ParseFilters(input.Filters, describeLaunchTemplateVersionsValidFilters) if err != nil { slog.WarnContext(ctx, "DescribeLaunchTemplateVersions: invalid filter", "err", err) - return nil, errors.New(awserrors.ErrorInvalidParameterValue) + return nil, err } header, _, err := s.resolveHeader(ctx, accountID, input.LaunchTemplateId, input.LaunchTemplateName) diff --git a/spinifex/handlers/ec2/natgw/service_impl.go b/spinifex/handlers/ec2/natgw/service_impl.go index 04a3f6a2c..9af49933a 100644 --- a/spinifex/handlers/ec2/natgw/service_impl.go +++ b/spinifex/handlers/ec2/natgw/service_impl.go @@ -376,7 +376,7 @@ func (s *NatGatewayServiceImpl) DescribeNatGateways(ctx context.Context, input * parsedFilters, err := filterutil.ParseFilters(input.Filter, describeNatGatewaysValidFilters) if err != nil { slog.WarnContext(ctx, "DescribeNatGateways: invalid filter", "err", err) - return nil, errors.New(awserrors.ErrorInvalidParameterValue) + return nil, err } natgwIDs := make(map[string]bool) diff --git a/spinifex/handlers/ec2/placementgroup/service_impl.go b/spinifex/handlers/ec2/placementgroup/service_impl.go index 1d4e42b4d..150f81549 100644 --- a/spinifex/handlers/ec2/placementgroup/service_impl.go +++ b/spinifex/handlers/ec2/placementgroup/service_impl.go @@ -179,7 +179,7 @@ func (s *PlacementGroupServiceImpl) DescribePlacementGroups(ctx context.Context, parsedFilters, err := filterutil.ParseFilters(input.Filters, describePlacementGroupsValidFilters) if err != nil { slog.WarnContext(ctx, "DescribePlacementGroups: invalid filter", "err", err) - return nil, errors.New(awserrors.ErrorInvalidParameterValue) + return nil, err } // Build filter maps for GroupNames/GroupIds parameters diff --git a/spinifex/handlers/ec2/routetable/service_impl.go b/spinifex/handlers/ec2/routetable/service_impl.go index 8a9a4590b..6afd78132 100644 --- a/spinifex/handlers/ec2/routetable/service_impl.go +++ b/spinifex/handlers/ec2/routetable/service_impl.go @@ -613,7 +613,7 @@ func (s *RouteTableServiceImpl) DescribeRouteTables(ctx context.Context, input * parsedFilters, err := filterutil.ParseFilters(input.Filters, describeRouteTablesValidFilters) if err != nil { slog.WarnContext(ctx, "DescribeRouteTables: invalid filter", "err", err) - return nil, errors.New(awserrors.ErrorInvalidParameterValue) + return nil, err } prefix := accountID + "." diff --git a/spinifex/handlers/ec2/snapshot/service_impl.go b/spinifex/handlers/ec2/snapshot/service_impl.go index bebe86f6b..23fb9a676 100644 --- a/spinifex/handlers/ec2/snapshot/service_impl.go +++ b/spinifex/handlers/ec2/snapshot/service_impl.go @@ -442,7 +442,7 @@ func (s *SnapshotServiceImpl) describeSnapshots(ctx context.Context, input *ec2. parsedFilters, err := filterutil.ParseFilters(input.Filters, describeSnapshotsValidFilters) if err != nil { slog.WarnContext(ctx, "DescribeSnapshots: invalid filter", "err", err) - return nil, errors.New(awserrors.ErrorInvalidParameterValue) + return nil, err } // The caller's prefix is the isolation and the whole answer: no other diff --git a/spinifex/handlers/ec2/spotinstance/service_impl.go b/spinifex/handlers/ec2/spotinstance/service_impl.go index 20ccfaf4a..715924bae 100644 --- a/spinifex/handlers/ec2/spotinstance/service_impl.go +++ b/spinifex/handlers/ec2/spotinstance/service_impl.go @@ -130,7 +130,7 @@ func (s *SpotInstanceServiceImpl) DescribeSpotInstanceRequests(ctx context.Conte parsedFilters, err := filterutil.ParseFilters(input.Filters, describeSpotRequestsValidFilters) if err != nil { slog.WarnContext(ctx, "DescribeSpotInstanceRequests: invalid filter", "err", err) - return nil, errors.New(awserrors.ErrorInvalidParameterValue) + return nil, err } idSet := make(map[string]bool) diff --git a/spinifex/handlers/ec2/spotinstance/service_impl_test.go b/spinifex/handlers/ec2/spotinstance/service_impl_test.go index 61701a072..e4e1d1c7c 100644 --- a/spinifex/handlers/ec2/spotinstance/service_impl_test.go +++ b/spinifex/handlers/ec2/spotinstance/service_impl_test.go @@ -223,7 +223,10 @@ func TestDescribe_InvalidFilter(t *testing.T) { Filters: []*ec2.Filter{{Name: aws.String("bogus-filter"), Values: []*string{aws.String("x")}}}, }, testAccountID) require.Error(t, err) - assert.Equal(t, awserrors.ErrorInvalidParameterValue, err.Error()) + code, message, ok := awserrors.ResolveErrorDetail(err) + assert.True(t, ok) + assert.Equal(t, awserrors.ErrorInvalidParameterValue, code) + assert.Contains(t, message, "bogus-filter") } // --- Cancel --- diff --git a/spinifex/handlers/ec2/tags/service_impl.go b/spinifex/handlers/ec2/tags/service_impl.go index d9e01cb37..ab84505c8 100644 --- a/spinifex/handlers/ec2/tags/service_impl.go +++ b/spinifex/handlers/ec2/tags/service_impl.go @@ -218,7 +218,7 @@ func (s *TagsServiceImpl) DescribeTags(ctx context.Context, input *ec2.DescribeT filters, err = filterutil.ParseFilters(input.Filters, describeTagsValidFilters) if err != nil { slog.WarnContext(ctx, "DescribeTags: invalid filter", "err", err) - return nil, errors.New(awserrors.ErrorInvalidParameterValue) + return nil, err } } diff --git a/spinifex/handlers/ec2/volume/service_impl.go b/spinifex/handlers/ec2/volume/service_impl.go index c05ca2e26..1b16c8159 100644 --- a/spinifex/handlers/ec2/volume/service_impl.go +++ b/spinifex/handlers/ec2/volume/service_impl.go @@ -297,7 +297,7 @@ func (s *VolumeServiceImpl) DescribeVolumes(ctx context.Context, input *ec2.Desc parsedFilters, err := filterutil.ParseFilters(input.Filters, describeVolumesValidFilters) if err != nil { slog.WarnContext(ctx, "DescribeVolumes: invalid filter", "err", err) - return nil, errors.New(awserrors.ErrorInvalidParameterValue) + return nil, err } // Fast path: specific volume IDs requested. Fetch each document @@ -441,7 +441,7 @@ func (s *VolumeServiceImpl) DescribeVolumeStatus(ctx context.Context, input *ec2 parsedFilters, err := filterutil.ParseFilters(input.Filters, describeVolumeStatusValidFilters) if err != nil { slog.WarnContext(ctx, "DescribeVolumeStatus: invalid filter", "err", err) - return nil, errors.New(awserrors.ErrorInvalidParameterValue) + return nil, err } return s.describeVolumeStatus(ctx, input, accountID, parsedFilters) @@ -659,7 +659,7 @@ func (s *VolumeServiceImpl) DescribeVolumesModifications(ctx context.Context, in parsedFilters, err := filterutil.ParseFilters(input.Filters, describeVolumesModificationsValidFilters) if err != nil { slog.WarnContext(ctx, "DescribeVolumesModifications: invalid filter", "err", err) - return nil, errors.New(awserrors.ErrorInvalidParameterValue) + return nil, err } var modifications []*ec2.VolumeModification diff --git a/spinifex/handlers/ec2/volume/service_impl_test.go b/spinifex/handlers/ec2/volume/service_impl_test.go index a9e6b9c07..9540169c8 100644 --- a/spinifex/handlers/ec2/volume/service_impl_test.go +++ b/spinifex/handlers/ec2/volume/service_impl_test.go @@ -2138,7 +2138,10 @@ func TestDescribeVolumes_FilterUnknownName_Error(t *testing.T) { }, }, "000000000001") require.Error(t, err) - assert.Equal(t, awserrors.ErrorInvalidParameterValue, err.Error()) + code, message, ok := awserrors.ResolveErrorDetail(err) + assert.True(t, ok) + assert.Equal(t, awserrors.ErrorInvalidParameterValue, code) + assert.Contains(t, message, "bogus-filter") } func TestDescribeVolumes_FilterNoResults(t *testing.T) { @@ -2670,5 +2673,8 @@ func TestDescribeVolumesModifications_UnknownFilter(t *testing.T) { }, }, testVolAccountID) require.Error(t, err) - assert.Equal(t, awserrors.ErrorInvalidParameterValue, err.Error()) + code, message, ok := awserrors.ResolveErrorDetail(err) + assert.True(t, ok) + assert.Equal(t, awserrors.ErrorInvalidParameterValue, code) + assert.Contains(t, message, "not-a-real-filter") } diff --git a/spinifex/handlers/ec2/vpc/GetSecurityGroupsForVpc_test.go b/spinifex/handlers/ec2/vpc/GetSecurityGroupsForVpc_test.go index c3e376858..47b5388f4 100644 --- a/spinifex/handlers/ec2/vpc/GetSecurityGroupsForVpc_test.go +++ b/spinifex/handlers/ec2/vpc/GetSecurityGroupsForVpc_test.go @@ -223,7 +223,10 @@ func TestGetSecurityGroupsForVpc_Filters(t *testing.T) { Filters: []*ec2.Filter{{Name: aws.String("vpc-id"), Values: []*string{aws.String(vpcID)}}}, }, testAccountID) require.Error(t, err) - assert.Equal(t, awserrors.ErrorInvalidParameterValue, err.Error()) + code, message, ok := awserrors.ResolveErrorDetail(err) + assert.True(t, ok) + assert.Equal(t, awserrors.ErrorInvalidParameterValue, code) + assert.Contains(t, message, "vpc-id") out, err = svc.GetSecurityGroupsForVpc(context.Background(), &ec2.GetSecurityGroupsForVpcInput{ VpcId: aws.String(vpcID), diff --git a/spinifex/handlers/ec2/vpc/eni.go b/spinifex/handlers/ec2/vpc/eni.go index f99a2eb21..d1b4cd26f 100644 --- a/spinifex/handlers/ec2/vpc/eni.go +++ b/spinifex/handlers/ec2/vpc/eni.go @@ -476,7 +476,7 @@ func (s *VPCServiceImpl) DescribeNetworkInterfaces(ctx context.Context, input *e parsedFilters, err := filterutil.ParseFilters(input.Filters, describeNetworkInterfacesValidFilters) if err != nil { slog.WarnContext(ctx, "DescribeNetworkInterfaces: invalid filter", "err", err) - return nil, errors.New(awserrors.ErrorInvalidParameterValue) + return nil, err } enis := make([]*ec2.NetworkInterface, 0) diff --git a/spinifex/handlers/ec2/vpc/security_group.go b/spinifex/handlers/ec2/vpc/security_group.go index c998f30a8..ccee476f3 100644 --- a/spinifex/handlers/ec2/vpc/security_group.go +++ b/spinifex/handlers/ec2/vpc/security_group.go @@ -426,7 +426,7 @@ func (s *VPCServiceImpl) DescribeSecurityGroups(ctx context.Context, input *ec2. parsedFilters, err := filterutil.ParseFilters(input.Filters, describeSecurityGroupsValidFilters) if err != nil { slog.WarnContext(ctx, "DescribeSecurityGroups: invalid filter", "err", err) - return nil, errors.New(awserrors.ErrorInvalidParameterValue) + return nil, err } prefix := accountID + "." @@ -570,7 +570,7 @@ func (s *VPCServiceImpl) GetSecurityGroupsForVpc(ctx context.Context, input *ec2 parsedFilters, err := filterutil.ParseFilters(input.Filters, getSecurityGroupsForVpcValidFilters) if err != nil { slog.WarnContext(ctx, "GetSecurityGroupsForVpc: invalid filter", "err", err) - return nil, errors.New(awserrors.ErrorInvalidParameterValue) + return nil, err } prefix := accountID + "." @@ -726,7 +726,7 @@ func (s *VPCServiceImpl) DescribeSecurityGroupRules(ctx context.Context, input * parsedFilters, err := filterutil.ParseFilters(filters, describeSecurityGroupRulesValidFilters) if err != nil { slog.WarnContext(ctx, "DescribeSecurityGroupRules: invalid filter", "err", err) - return nil, errors.New(awserrors.ErrorInvalidParameterValue) + return nil, err } prefix := accountID + "." @@ -876,7 +876,7 @@ func (s *VPCServiceImpl) AuthorizeSecurityGroupIngress(ctx context.Context, inpu newRules, err := ipPermissionsToSGRules(input.IpPermissions, sgParseAuthorize) if err != nil { slog.WarnContext(ctx, "AuthorizeSecurityGroupIngress: invalid rule", "groupId", groupId, "err", err) - return nil, errors.New(awserrors.ErrorInvalidParameterValue) + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "%s", err) } if err := s.validateSGRuleReferences(ctx, accountID, record.VpcId, newRules); err != nil { return nil, err @@ -951,7 +951,7 @@ func (s *VPCServiceImpl) AuthorizeSecurityGroupEgress(ctx context.Context, input newRules, err := ipPermissionsToSGRules(input.IpPermissions, sgParseAuthorize) if err != nil { slog.WarnContext(ctx, "AuthorizeSecurityGroupEgress: invalid rule", "groupId", groupId, "err", err) - return nil, errors.New(awserrors.ErrorInvalidParameterValue) + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "%s", err) } if err := s.validateSGRuleReferences(ctx, accountID, record.VpcId, newRules); err != nil { return nil, err diff --git a/spinifex/handlers/ec2/vpc/service_impl.go b/spinifex/handlers/ec2/vpc/service_impl.go index bf4e29720..1d0256134 100644 --- a/spinifex/handlers/ec2/vpc/service_impl.go +++ b/spinifex/handlers/ec2/vpc/service_impl.go @@ -578,7 +578,7 @@ func (s *VPCServiceImpl) DescribeVpcs(ctx context.Context, input *ec2.DescribeVp parsedFilters, err := filterutil.ParseFilters(input.Filters, describeVpcsValidFilters) if err != nil { slog.WarnContext(ctx, "DescribeVpcs: invalid filter", "err", err) - return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "%s", err) + return nil, err } prefix := accountID + "." @@ -947,7 +947,7 @@ func (s *VPCServiceImpl) DescribeSubnets(ctx context.Context, input *ec2.Describ parsedFilters, err := filterutil.ParseFilters(input.Filters, describeSubnetsValidFilters) if err != nil { slog.WarnContext(ctx, "DescribeSubnets: invalid filter", "err", err) - return nil, errors.New(awserrors.ErrorInvalidParameterValue) + return nil, err } prefix := accountID + "." diff --git a/spinifex/utils/nats.go b/spinifex/utils/nats.go index 2beac16b2..bf04c576d 100644 --- a/spinifex/utils/nats.go +++ b/spinifex/utils/nats.go @@ -267,21 +267,6 @@ func PrincipalARNFromMsg(msg *nats.Msg) string { return msg.Header.Get(PrincipalARNHeader) } -// decodedNATSError retains a response's diagnostic message while exposing its -// sanitized AWS wire code to unwrap-aware classifiers. -type decodedNATSError struct { - code error - message string -} - -func (e *decodedNATSError) Error() string { - return e.message -} - -func (e *decodedNATSError) Unwrap() error { - return e.code -} - // NATSRequest performs a NATS request-response with JSON marshaling. It sends // with X-Account-ID (plus any extra headers), unmarshals the successful response // into Out, and carries ctx's trace context onto the wire: it opens a client span @@ -325,7 +310,7 @@ func NATSRequest[Out any](ctx context.Context, conn *nats.Conn, subject string, responseError, err := ValidateErrorPayload(msg.Data) if err != nil { if responseError.Message != nil && *responseError.Message != "" { - return nil, &decodedNATSError{code: errors.New(*responseError.Code), message: *responseError.Message} + return nil, awserrors.Errorf(*responseError.Code, "%s", *responseError.Message) } return nil, errors.New(*responseError.Code) } @@ -361,7 +346,8 @@ func ServeNATSRequestCtx[I any, O any](msg *nats.Msg, fn func(context.Context, * out, err := fn(ctx, input) if err != nil { MarkSpanError(span, err) - respondNATS(msg, GenerateErrorPayloadWithMessage(awserrors.ValidErrorCodeFromError(err), err.Error())) + _, message, _ := awserrors.ResolveErrorDetail(err) + respondNATS(msg, GenerateErrorPayloadWithMessage(awserrors.ValidErrorCodeFromError(err), message)) return false } data, err := json.Marshal(out) diff --git a/spinifex/utils/nats_test.go b/spinifex/utils/nats_test.go index 32676dbff..4fe057f66 100644 --- a/spinifex/utils/nats_test.go +++ b/spinifex/utils/nats_test.go @@ -286,6 +286,30 @@ func TestNATSRequest_ErrorResponse(t *testing.T) { assert.Equal(t, awserrors.ErrorInvalidParameterValue, code) } +// TestNATSRequest_DecodedErrorContainsCode pins the shape strings.Contains(err.Error(), code) +// classifiers depend on (e.g. handlers/iam/provision.go's isAlreadyExists, +// gateway/auth.go's IAM-not-found branch): the decoded error's Error() text +// must still contain the wire code even when a message rides alongside it. +func TestNATSRequest_DecodedErrorContainsCode(t *testing.T) { + ns := startTestNATSServer(t) + + nc, err := nats.Connect(ns.ClientURL()) + require.NoError(t, err) + defer nc.Close() + + const code = "EntityAlreadyExists" + const reason = "access key AKIA... already exists for user provisioning-bot" + _, err = nc.Subscribe("test.fail.classify", func(msg *nats.Msg) { + msg.Respond(GenerateErrorPayloadWithMessage(code, reason)) + }) + require.NoError(t, err) + + type Resp struct{} + _, err = NATSRequest[Resp](context.Background(), nc, "test.fail.classify", struct{}{}, 2*time.Second, "") + require.Error(t, err) + assert.Contains(t, err.Error(), code) +} + func TestNATSRequest_ErrorResponseSurfacesMessage(t *testing.T) { ns := startTestNATSServer(t) @@ -304,20 +328,22 @@ func TestNATSRequest_ErrorResponseSurfacesMessage(t *testing.T) { type Resp struct{} _, err = NATSRequest[Resp](context.Background(), nc, "test.fail.msg", struct{}{}, 2*time.Second, "") assert.Error(t, err) - assert.Equal(t, reason, err.Error()) - code, ok := awserrors.ResolveErrorCode(err) + assert.Contains(t, err.Error(), reason) + code, message, ok := awserrors.ResolveErrorDetail(err) assert.True(t, ok) assert.Equal(t, awserrors.ErrorServerInternal, code) + assert.Equal(t, reason, message) } -func TestServeNATSRequest_WrappedErrorPreservesCodeAndMessage(t *testing.T) { +func TestServeNATSRequest_WrappedErrorPreservesCodeButDropsWrapperContext(t *testing.T) { ns := startTestNATSServer(t) nc, err := nats.Connect(ns.ClientURL()) require.NoError(t, err) defer nc.Close() - const message = "launch on node-1: InsufficientAddressCapacity" + // A bare errors.New(code) wrapped only for the handler's own logs carries + // no client-facing message, so the wrapper text must not reach the wire. _, err = nc.Subscribe("test.serve.error", func(msg *nats.Msg) { ServeNATSRequest(msg, func(_ *struct{}) (*struct{}, error) { cause := errors.New(awserrors.ErrorInsufficientAddressCapacity) @@ -329,12 +355,41 @@ func TestServeNATSRequest_WrappedErrorPreservesCodeAndMessage(t *testing.T) { _, err = NATSRequest[struct{}](context.Background(), nc, "test.serve.error", struct{}{}, 2*time.Second, "") require.Error(t, err) - assert.Equal(t, message, err.Error()) + assert.Equal(t, awserrors.ErrorInsufficientAddressCapacity, err.Error()) + assert.NotContains(t, err.Error(), "launch on node-1") code, ok := awserrors.ResolveErrorCode(err) assert.True(t, ok) assert.Equal(t, awserrors.ErrorInsufficientAddressCapacity, code) } +func TestServeNATSRequest_ErrorfMessageSurvivesOuterWrapperContext(t *testing.T) { + ns := startTestNATSServer(t) + + nc, err := nats.Connect(ns.ClientURL()) + require.NoError(t, err) + defer nc.Close() + + // The curated message from awserrors.Errorf must round-trip over NATS, + // but an outer %w wrapper added only for the handler's own logs must not. + const curated = "no capacity for m5.large on any eligible node" + _, err = nc.Subscribe("test.serve.error.curated", func(msg *nats.Msg) { + ServeNATSRequest(msg, func(_ *struct{}) (*struct{}, error) { + cause := awserrors.Errorf(awserrors.ErrorInsufficientAddressCapacity, "%s", curated) + return nil, fmt.Errorf("launch on node-1: %w", cause) + }) + }) + require.NoError(t, err) + require.NoError(t, nc.Flush()) + + _, err = NATSRequest[struct{}](context.Background(), nc, "test.serve.error.curated", struct{}{}, 2*time.Second, "") + require.Error(t, err) + assert.NotContains(t, err.Error(), "launch on node-1") + code, message, ok := awserrors.ResolveErrorDetail(err) + assert.True(t, ok) + assert.Equal(t, awserrors.ErrorInsufficientAddressCapacity, code) + assert.Equal(t, curated, message) +} + func TestNATSRequest_NoResponders(t *testing.T) { ns := startTestNATSServer(t)