From c481c787c8bb281c6c8ae9f1c8c6f8513af11939 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 5 Jun 2026 13:18:51 +0200 Subject: [PATCH 1/2] fix(recommendations): wire MemoryDB NodeType from CE InstanceDetails (follow-up to #539) PR #539 made parseMemoryDBDetails read rec.ResourceType and error loudly when empty, with a doc comment claiming the AWS SDK has no MemoryDB sub-struct in ReservationPurchaseRecommendationDetail. That premise is wrong: the CE SDK (costexplorer v1.61.0) exposes details.InstanceDetails.MemoryDBInstanceDetails with NodeType and Region, exactly like ElastiCache and Redshift. Nothing upstream populates rec.ResourceType before parseMemoryDBDetails runs (parseRecommendationDetail only sets it inside the service parsers themselves), so every MemoryDB recommendation hit the empty-ResourceType error and was skipped. The happy-path branch and its unit tests (which injected ResourceType manually) were unreachable in production. Read NodeType and Region from MemoryDBInstanceDetails, mirroring the ElastiCache/Redshift parsers, so the happy path is reachable for real CE responses. Keep a loud error only when the sub-struct or its NodeType is genuinely absent. Rewrite the tests to exercise the real production path via the SDK struct rather than a manually injected ResourceType. --- .../aws/recommendations/parser_services.go | 33 +++++++---- .../recommendations/parser_services_test.go | 55 +++++++++++++++---- 2 files changed, 65 insertions(+), 23 deletions(-) diff --git a/providers/aws/recommendations/parser_services.go b/providers/aws/recommendations/parser_services.go index cb456c58e..946bc8ff0 100644 --- a/providers/aws/recommendations/parser_services.go +++ b/providers/aws/recommendations/parser_services.go @@ -191,22 +191,31 @@ func (c *Client) parseRedshiftDetails(_ context.Context, rec *common.Recommendat // parseMemoryDBDetails extracts MemoryDB-specific details. // -// Cost Explorer does not populate InstanceDetails for MemoryDB reserved nodes: -// the MemoryDB-specific sub-struct does not exist in the AWS SDK's -// ReservationPurchaseRecommendationDetail, so the instance type must come from -// rec.ResourceType, which the upstream caller should have populated from the -// recommendation summary fields before dispatching here. +// Cost Explorer exposes MemoryDB reserved-node offerings via +// details.InstanceDetails.MemoryDBInstanceDetails (NodeType, Region), the same +// shape ElastiCache and Redshift use. The node type comes from that struct, not +// from a hardcoded default, so we never substitute a wrong instance type. // -// If rec.ResourceType is empty, the function returns an error so the -// recommendation is skipped loudly (logged by parseRecommendations) rather -// than silently substituting a wrong default instance type. -func (c *Client) parseMemoryDBDetails(_ context.Context, rec *common.Recommendation, _ *types.ReservationPurchaseRecommendationDetail) error { - if rec.ResourceType == "" { - return fmt.Errorf("MemoryDB recommendation has no ResourceType; cannot determine offering - Cost Explorer did not populate instance details") +// If the MemoryDB sub-struct or its NodeType is absent, the function returns an +// error so the recommendation is skipped loudly (logged by parseRecommendations) +// rather than silently guessing an offering. +func (c *Client) parseMemoryDBDetails(rec *common.Recommendation, details *types.ReservationPurchaseRecommendationDetail) error { + if details == nil || details.InstanceDetails == nil || details.InstanceDetails.MemoryDBInstanceDetails == nil { + return fmt.Errorf("MemoryDB instance details not found; cannot determine offering - Cost Explorer did not populate instance details") + } + + mdbDetails := details.InstanceDetails.MemoryDBInstanceDetails + if mdbDetails.NodeType == nil || *mdbDetails.NodeType == "" { + return fmt.Errorf("MemoryDB recommendation has no NodeType; cannot determine offering") + } + + rec.ResourceType = *mdbDetails.NodeType + if mdbDetails.Region != nil { + rec.Region = normalizeRegionName(*mdbDetails.Region) } rec.Details = &common.CacheDetails{ Engine: "redis", - NodeType: rec.ResourceType, + NodeType: *mdbDetails.NodeType, } return nil } diff --git a/providers/aws/recommendations/parser_services_test.go b/providers/aws/recommendations/parser_services_test.go index e01c2bd94..ec11711e0 100644 --- a/providers/aws/recommendations/parser_services_test.go +++ b/providers/aws/recommendations/parser_services_test.go @@ -508,30 +508,63 @@ func TestParseRedshiftDetails(t *testing.T) { func TestParseMemoryDBDetails(t *testing.T) { client := &Client{} - details := &types.ReservationPurchaseRecommendationDetail{} - t.Run("empty ResourceType returns error", func(t *testing.T) { + t.Run("missing MemoryDB instance details returns error", func(t *testing.T) { rec := &common.Recommendation{} - err := client.parseMemoryDBDetails(context.Background(), rec, details) - require.Error(t, err, "should fail loudly when ResourceType is empty") - assert.Contains(t, err.Error(), "ResourceType") + err := client.parseMemoryDBDetails(rec, &types.ReservationPurchaseRecommendationDetail{}) + require.Error(t, err, "should fail loudly when MemoryDB instance details are absent") + assert.Contains(t, err.Error(), "instance details not found") assert.Nil(t, rec.Details, "Details should remain nil on error") + assert.Empty(t, rec.ResourceType, "ResourceType should remain empty on error") }) - t.Run("non-default instance type populates Details", func(t *testing.T) { - rec := &common.Recommendation{ResourceType: "db.r6g.large"} - err := client.parseMemoryDBDetails(context.Background(), rec, details) + t.Run("nil NodeType returns error", func(t *testing.T) { + rec := &common.Recommendation{} + details := &types.ReservationPurchaseRecommendationDetail{ + InstanceDetails: &types.InstanceDetails{ + MemoryDBInstanceDetails: &types.MemoryDBInstanceDetails{ + Region: aws.String("US East (N. Virginia)"), + }, + }, + } + err := client.parseMemoryDBDetails(rec, details) + require.Error(t, err, "should fail loudly when NodeType is absent") + assert.Contains(t, err.Error(), "NodeType") + assert.Nil(t, rec.Details, "Details should remain nil on error") + }) + + t.Run("populates Details, ResourceType and Region from CE node type", func(t *testing.T) { + rec := &common.Recommendation{} + details := &types.ReservationPurchaseRecommendationDetail{ + InstanceDetails: &types.InstanceDetails{ + MemoryDBInstanceDetails: &types.MemoryDBInstanceDetails{ + NodeType: aws.String("db.r6g.large"), + Region: aws.String("US East (N. Virginia)"), + }, + }, + } + err := client.parseMemoryDBDetails(rec, details) require.NoError(t, err) + assert.Equal(t, "db.r6g.large", rec.ResourceType) + assert.Equal(t, "us-east-1", rec.Region) cacheDetails, ok := rec.Details.(*common.CacheDetails) require.True(t, ok, "Details should be *common.CacheDetails") assert.Equal(t, "db.r6g.large", cacheDetails.NodeType) assert.Equal(t, "redis", cacheDetails.Engine) }) - t.Run("xlarge instance type populates Details", func(t *testing.T) { - rec := &common.Recommendation{ResourceType: "db.r6gd.xlarge"} - err := client.parseMemoryDBDetails(context.Background(), rec, details) + t.Run("xlarge node type populates Details", func(t *testing.T) { + rec := &common.Recommendation{} + details := &types.ReservationPurchaseRecommendationDetail{ + InstanceDetails: &types.InstanceDetails{ + MemoryDBInstanceDetails: &types.MemoryDBInstanceDetails{ + NodeType: aws.String("db.r6gd.xlarge"), + }, + }, + } + err := client.parseMemoryDBDetails(rec, details) require.NoError(t, err) + assert.Equal(t, "db.r6gd.xlarge", rec.ResourceType) cacheDetails, ok := rec.Details.(*common.CacheDetails) require.True(t, ok, "Details should be *common.CacheDetails") assert.Equal(t, "db.r6gd.xlarge", cacheDetails.NodeType) From e13e08366dadb1913f2d3563f21710bb25a43767 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 6 Jun 2026 08:48:06 +0200 Subject: [PATCH 2/2] fix(recommendations): restore context.Context param in parseMemoryDBDetails The serviceParserFunc type requires (context.Context, *Recommendation, *Detail) signature. The prior implementation correctly used _ context.Context; keep that to satisfy the function type while the new body reads from details directly. --- providers/aws/recommendations/parser_services.go | 2 +- providers/aws/recommendations/parser_services_test.go | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/providers/aws/recommendations/parser_services.go b/providers/aws/recommendations/parser_services.go index 946bc8ff0..ed4591cd2 100644 --- a/providers/aws/recommendations/parser_services.go +++ b/providers/aws/recommendations/parser_services.go @@ -199,7 +199,7 @@ func (c *Client) parseRedshiftDetails(_ context.Context, rec *common.Recommendat // If the MemoryDB sub-struct or its NodeType is absent, the function returns an // error so the recommendation is skipped loudly (logged by parseRecommendations) // rather than silently guessing an offering. -func (c *Client) parseMemoryDBDetails(rec *common.Recommendation, details *types.ReservationPurchaseRecommendationDetail) error { +func (c *Client) parseMemoryDBDetails(_ context.Context, rec *common.Recommendation, details *types.ReservationPurchaseRecommendationDetail) error { if details == nil || details.InstanceDetails == nil || details.InstanceDetails.MemoryDBInstanceDetails == nil { return fmt.Errorf("MemoryDB instance details not found; cannot determine offering - Cost Explorer did not populate instance details") } diff --git a/providers/aws/recommendations/parser_services_test.go b/providers/aws/recommendations/parser_services_test.go index ec11711e0..b70e14b31 100644 --- a/providers/aws/recommendations/parser_services_test.go +++ b/providers/aws/recommendations/parser_services_test.go @@ -511,7 +511,7 @@ func TestParseMemoryDBDetails(t *testing.T) { t.Run("missing MemoryDB instance details returns error", func(t *testing.T) { rec := &common.Recommendation{} - err := client.parseMemoryDBDetails(rec, &types.ReservationPurchaseRecommendationDetail{}) + err := client.parseMemoryDBDetails(context.Background(), rec, &types.ReservationPurchaseRecommendationDetail{}) require.Error(t, err, "should fail loudly when MemoryDB instance details are absent") assert.Contains(t, err.Error(), "instance details not found") assert.Nil(t, rec.Details, "Details should remain nil on error") @@ -527,7 +527,7 @@ func TestParseMemoryDBDetails(t *testing.T) { }, }, } - err := client.parseMemoryDBDetails(rec, details) + err := client.parseMemoryDBDetails(context.Background(), rec, details) require.Error(t, err, "should fail loudly when NodeType is absent") assert.Contains(t, err.Error(), "NodeType") assert.Nil(t, rec.Details, "Details should remain nil on error") @@ -543,7 +543,7 @@ func TestParseMemoryDBDetails(t *testing.T) { }, }, } - err := client.parseMemoryDBDetails(rec, details) + err := client.parseMemoryDBDetails(context.Background(), rec, details) require.NoError(t, err) assert.Equal(t, "db.r6g.large", rec.ResourceType) assert.Equal(t, "us-east-1", rec.Region) @@ -562,7 +562,7 @@ func TestParseMemoryDBDetails(t *testing.T) { }, }, } - err := client.parseMemoryDBDetails(rec, details) + err := client.parseMemoryDBDetails(context.Background(), rec, details) require.NoError(t, err) assert.Equal(t, "db.r6gd.xlarge", rec.ResourceType) cacheDetails, ok := rec.Details.(*common.CacheDetails)