From 64ccda8f57395f25e14e79f5e66c689bf2176b01 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 7 Oct 2026 14:53:07 +0200 Subject: [PATCH] fix(cli): signal unknown extended-support lifecycle instead of silent false isInExtendedSupport silently returned false when the engine:majorVersion key was missing from the lifecycle map, so a partial DescribeDBMajorEngineVersions response or an unmapped engine made the extended-support exclusion a silent no-op on a money path: instances accruing extended-support fees stayed in the RI/SP purchase count. - isInExtendedSupport now returns (extended, known); known=false when the major version can't be extracted or the lookup misses - adjustRecommendationForExcludedVersions treats unknown as a hard skip: the count stays untouched and a warning naming engine, version and region is logged once per engine:version pair - tests pin the new contract for the lookup-miss path, including the warning's content and per-version deduplication Closes #1313 --- cmd/multi_service_coverage_test.go | 10 +- cmd/multi_service_engine_versions.go | 86 ++++++++++------ cmd/multi_service_engine_versions_test.go | 119 ++++++++++++++++++---- 3 files changed, 161 insertions(+), 54 deletions(-) diff --git a/cmd/multi_service_coverage_test.go b/cmd/multi_service_coverage_test.go index 5151c4751..30cea2dfa 100644 --- a/cmd/multi_service_coverage_test.go +++ b/cmd/multi_service_coverage_test.go @@ -272,37 +272,43 @@ func TestIsInExtendedSupport_EdgeCases(t *testing.T) { engine string fullVersion string expectedExtended bool + expectedKnown bool }{ { name: "MySQL 5.7 in extended support", engine: "mysql", fullVersion: "5.7.44", expectedExtended: true, + expectedKnown: true, }, { name: "MySQL 8.0 in standard support", engine: "mysql", fullVersion: "8.0.35", expectedExtended: false, + expectedKnown: true, }, { name: "Unknown version", engine: "mysql", fullVersion: "9.0.0", expectedExtended: false, + expectedKnown: false, }, { name: "Empty version", engine: "mysql", fullVersion: "", expectedExtended: false, + expectedKnown: false, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := isInExtendedSupport(tt.engine, tt.fullVersion, versionInfo) - assert.Equal(t, tt.expectedExtended, result) + extended, known := isInExtendedSupport(tt.engine, tt.fullVersion, versionInfo) + assert.Equal(t, tt.expectedExtended, extended) + assert.Equal(t, tt.expectedKnown, known, "known flag mismatch") }) } } diff --git a/cmd/multi_service_engine_versions.go b/cmd/multi_service_engine_versions.go index 85c1bef00..0743e69f7 100644 --- a/cmd/multi_service_engine_versions.go +++ b/cmd/multi_service_engine_versions.go @@ -378,10 +378,14 @@ func extractNumericPrefix(s string) string { } // isInExtendedSupport checks if a version is currently in extended support based on lifecycle dates. -func isInExtendedSupport(engine, fullVersion string, versionInfo map[string]MajorEngineVersionInfo) bool { +// The second return value reports whether lifecycle data was available for the +// lookup: known is false when the major version cannot be extracted or the +// engine:majorVersion key is missing from versionInfo. Callers on money paths +// must not treat known=false as "not in extended support" (issue #1313). +func isInExtendedSupport(engine, fullVersion string, versionInfo map[string]MajorEngineVersionInfo) (extended, known bool) { majorVersion := extractMajorVersion(engine, fullVersion) if majorVersion == "" { - return false + return false, false } // Normalize engine name for lookup @@ -392,8 +396,7 @@ func isInExtendedSupport(engine, fullVersion string, versionInfo map[string]Majo key := fmt.Sprintf("%s:%s", normalizedEngine, majorVersion) info, exists := versionInfo[key] if !exists { - // If we don't have info, assume not in extended support - return false + return false, false } // Check if current date falls within extended support period @@ -409,11 +412,11 @@ func isInExtendedSupport(engine, fullVersion string, versionInfo map[string]Majo // Past the end date means extended support is over; a zero end date // is open-ended (issue #1182) if lifecycle.LifecycleSupportEndDate.IsZero() || now.Before(lifecycle.LifecycleSupportEndDate) { - return true + return true, true } } - return false + return false, true } // adjustRecommendationForExcludedVersions reduces the instance count in a recommendation @@ -445,33 +448,13 @@ func adjustRecommendationForExcludedVersions(rec common.Recommendation, instance // Count how many instances in this region are running versions in extended support excludedCount := 0 - for _, version := range versions { - // Only count instances in the same region - if version.Region != rec.Region { - continue - } - - // Match engine (normalize by removing spaces/hyphens and comparing lowercase) - normalizeEngine := func(engine string) string { - normalized := strings.ToLower(engine) - normalized = strings.ReplaceAll(normalized, "-", "") - normalized = strings.ReplaceAll(normalized, " ", "") - return normalized - } - - versionEngineNorm := normalizeEngine(version.Engine) - recEngineNorm := normalizeEngine(recEngine) - - if versionEngineNorm != recEngineNorm { - continue - } + // Warn once per engine:version when lifecycle data is missing instead of + // silently treating the instance as not on extended support (issue #1313). + unknownLogged := make(map[string]bool) - // Check if this version is in extended support - if isInExtendedSupport(version.Engine, version.EngineVersion, versionInfo) { - majorVersion := extractMajorVersion(version.Engine, version.EngineVersion) + for _, version := range versions { + if shouldExcludeForExtendedSupport(version, rec, recEngine, versionInfo, unknownLogged) { excludedCount++ - log.Printf("🚫 Found extended support instance: %s %s in %s running version %s (major version %s is in extended support)", - recEngine, rec.ResourceType, rec.Region, version.EngineVersion, majorVersion) } } @@ -489,3 +472,44 @@ func adjustRecommendationForExcludedVersions(rec common.Recommendation, instance return rec } + +// shouldExcludeForExtendedSupport reports whether a single running instance +// should be excluded from the recommendation because its engine version is in +// extended support. Instances in other regions or with other engines never +// match. When lifecycle data is missing the instance is kept (conservative +// skip) and a warning is logged once per engine:version via unknownLogged +// (issue #1313). Extracted from adjustRecommendationForExcludedVersions to +// keep both functions under the gocyclo cap. +func shouldExcludeForExtendedSupport(version InstanceEngineVersion, rec common.Recommendation, recEngine string, versionInfo map[string]MajorEngineVersionInfo, unknownLogged map[string]bool) bool { + // Only count instances in the same region + if version.Region != rec.Region { + return false + } + + // Match engine (normalize by removing spaces/hyphens and comparing lowercase) + if normalizeEngineNameForVersion(version.Engine) != normalizeEngineNameForVersion(recEngine) { + return false + } + + extended, known := isInExtendedSupport(version.Engine, version.EngineVersion, versionInfo) + if !known { + // Leave the instance in the count but make the gap visible so + // incomplete lifecycle data can't silently steer purchases (issue #1313). + warnKey := version.Engine + ":" + version.EngineVersion + if !unknownLogged[warnKey] { + unknownLogged[warnKey] = true + log.Printf("⚠️ No lifecycle data for %s %s in %s: extended-support status unknown, keeping instance in the count", + version.Engine, version.EngineVersion, version.Region) + } + return false + } + + if !extended { + return false + } + + majorVersion := extractMajorVersion(version.Engine, version.EngineVersion) + log.Printf("🚫 Found extended support instance: %s %s in %s running version %s (major version %s is in extended support)", + recEngine, rec.ResourceType, rec.Region, version.EngineVersion, majorVersion) + return true +} diff --git a/cmd/multi_service_engine_versions_test.go b/cmd/multi_service_engine_versions_test.go index ec55ba623..e431e9f6b 100644 --- a/cmd/multi_service_engine_versions_test.go +++ b/cmd/multi_service_engine_versions_test.go @@ -1,7 +1,10 @@ package main import ( + "bytes" "context" + "log" + "strings" "testing" "time" @@ -283,6 +286,69 @@ func TestAdjustRecommendationForExcludedVersions_TypedNilDetails(t *testing.T) { }) } +// TestAdjustRecommendationForExcludedVersions_UnknownLifecycle pins the +// issue #1313 contract: when an instance's engine/major version is missing +// from versionInfo, the instance must NOT be silently treated as "not on +// extended support". The count stays untouched (conservative skip) and a +// warning naming the engine, version, and region is logged, once per +// engine:version pair. +func TestAdjustRecommendationForExcludedVersions_UnknownLifecycle(t *testing.T) { + recommendation := common.Recommendation{ + Service: common.ServiceRDS, + Region: "us-east-1", + ResourceType: "db.r5.large", + Count: 4, + Details: &common.DatabaseDetails{ + Engine: "MySQL", + }, + } + + // Two instances of the same unknown version plus one of another: the + // warning must be deduplicated per engine:version, not per instance. + instanceVersions := map[string][]InstanceEngineVersion{ + "db.r5.large": { + {Engine: "mysql", EngineVersion: "9.0.1", InstanceClass: "db.r5.large", Region: "us-east-1"}, + {Engine: "mysql", EngineVersion: "9.0.1", InstanceClass: "db.r5.large", Region: "us-east-1"}, + {Engine: "mysql", EngineVersion: "10.1.0", InstanceClass: "db.r5.large", Region: "us-east-1"}, + }, + } + + // versionInfo has data for mysql:8.0 only, so 9.x and 10.x lookups miss. + versionInfo := map[string]MajorEngineVersionInfo{ + "mysql:8.0": { + Engine: "mysql", + MajorEngineVersion: "8.0", + SupportedEngineLifecycles: []EngineLifecycleInfo{ + { + LifecycleSupportName: string(rdstypes.LifecycleSupportNameOpenSourceRdsStandardSupport), + LifecycleSupportStartDate: time.Now().AddDate(-2, 0, 0), + LifecycleSupportEndDate: time.Now().AddDate(3, 0, 0), + }, + }, + }, + } + + var logBuf bytes.Buffer + origOutput := log.Writer() + origFlags := log.Flags() + log.SetOutput(&logBuf) + log.SetFlags(0) + defer func() { + log.SetOutput(origOutput) + log.SetFlags(origFlags) + }() + + result := adjustRecommendationForExcludedVersions(recommendation, instanceVersions, versionInfo) + + assert.Equal(t, 4, result.Count, "unknown lifecycle status must leave the count untouched") + + logged := logBuf.String() + assert.Contains(t, logged, "mysql 9.0.1", "warning must name the unknown engine and version") + assert.Contains(t, logged, "mysql 10.1.0", "warning must name the unknown engine and version") + assert.Contains(t, logged, "us-east-1", "warning must name the region") + assert.Equal(t, 2, strings.Count(logged, "No lifecycle data for"), "warning must be logged once per engine:version, not per instance") +} + func TestExtractMajorVersion_Additional(t *testing.T) { tests := []struct { name string @@ -531,11 +597,12 @@ func TestIsInExtendedSupport(t *testing.T) { futureDate := now.AddDate(3, 0, 0) tests := []struct { - versionInfo map[string]MajorEngineVersionInfo - name string - engine string - version string - expected bool + versionInfo map[string]MajorEngineVersionInfo + name string + engine string + version string + expected bool + expectedKnown bool }{ { name: "Version in extended support", @@ -554,7 +621,8 @@ func TestIsInExtendedSupport(t *testing.T) { }, }, }, - expected: true, + expected: true, + expectedKnown: true, }, { name: "Version not in extended support - still in standard support", @@ -573,10 +641,11 @@ func TestIsInExtendedSupport(t *testing.T) { }, }, }, - expected: false, + expected: false, + expectedKnown: true, }, { - name: "Version info not found", + name: "Version info not found - unknown, not silently false", engine: "mysql", version: "5.7.44", versionInfo: map[string]MajorEngineVersionInfo{ @@ -585,14 +654,16 @@ func TestIsInExtendedSupport(t *testing.T) { MajorEngineVersion: "13", }, }, - expected: false, + expected: false, + expectedKnown: false, }, { - name: "Empty version info", - engine: "mysql", - version: "5.7.44", - versionInfo: map[string]MajorEngineVersionInfo{}, - expected: false, + name: "Empty version info - unknown, not silently false", + engine: "mysql", + version: "5.7.44", + versionInfo: map[string]MajorEngineVersionInfo{}, + expected: false, + expectedKnown: false, }, { name: "Extended support not started yet", @@ -611,7 +682,8 @@ func TestIsInExtendedSupport(t *testing.T) { }, }, }, - expected: false, + expected: false, + expectedKnown: true, }, { name: "Extended support started on current date", @@ -630,7 +702,8 @@ func TestIsInExtendedSupport(t *testing.T) { }, }, }, - expected: true, + expected: true, + expectedKnown: true, }, { name: "Extended support already ended", @@ -649,7 +722,8 @@ func TestIsInExtendedSupport(t *testing.T) { }, }, }, - expected: false, + expected: false, + expectedKnown: true, }, { name: "Zero end date treated as open-ended", @@ -667,7 +741,8 @@ func TestIsInExtendedSupport(t *testing.T) { }, }, }, - expected: true, + expected: true, + expectedKnown: true, }, { name: "Engine name normalization with spaces", @@ -686,14 +761,16 @@ func TestIsInExtendedSupport(t *testing.T) { }, }, }, - expected: true, + expected: true, + expectedKnown: true, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := isInExtendedSupport(tt.engine, tt.version, tt.versionInfo) - assert.Equal(t, tt.expected, result) + extended, known := isInExtendedSupport(tt.engine, tt.version, tt.versionInfo) + assert.Equal(t, tt.expected, extended) + assert.Equal(t, tt.expectedKnown, known, "known flag mismatch") }) } }