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)