fix(aws/rds): refuse the 'SQL Server' engine spelling in the licensed-engine check

[IAmJSD]

Root cause

lookupEngineName (providers/aws/services/rds/client.go) refuses Oracle and SQL Server engines, because their reserved-offering ProductDescription includes a license model ((li) / (byol)) that the recommendation doesn't provide. It only checked for the prefixes oracle, sqlserver and sql-server. Cost Explorer spells the engine SQL Server, with a space. normalizeEngineName lowercases that to sql server and returns it unchanged, since it only rejects the bare sqlserver / sql-server. So it got past the refusal, went out to DescribeReservedDBInstancesOfferings as the ProductDescription, and failed later with a misleading "no offerings found".

Fix

I moved the prefix list into licensedEnginePrefixes and added "sql server". The check now lives in a small isLicensedEngine helper. Any engine starting with sql server (for example SQL Server or SQL Server Enterprise) now gets the same license model is not known; refusing to guess error, before any API call. normalizeEngineName is unchanged.

Closes #302

Verification (unit tests with mocked RDS client only)

These are unit tests with a mocked RDSAPI (MockRDSClient). No real AWS calls were made.

• Regression test: TestFindOfferingID_LicensedEnginesRefusedBeforeAPICall gains the cases SQL Server, sql server and SQL Server Enterprise. Each one asserts a license model error and that DescribeReservedDBInstancesOfferings is never called.
  • Before the fix: the SQL_Server subtest fails. The mock panics with mock: I don't know what to return because the method call was unexpected ... DescribeReservedDBInstancesOfferings(...), and the log shows engine=sql server reaching the offering lookup.
  • After the fix: all subtests pass.
• Commands, run from providers/aws with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null. This machine's default AWS profile is invalid, so it is masked out; the tests themselves need no credentials.
  • go build ./...: OK
  • go vet ./services/rds/: OK
  • go test -race -short ./services/rds/: ok
  • go test -short ./... (whole AWS module): all ok
  • golangci-lint run --allow-parallel-runners ./services/rds/... (v2.10.1): 0 issues
  • gocyclo -over 10 services/rds/client.go: silent
  • gofmt -l services/rds/: clean

Not covered: a live Cost Explorer → RDS run against real AWS.

Interaction with #430 (fix/issue-303)

Draft PR #430 changes the same package (offering pagination in client.go, and new tests appended at the end of client_test.go). To keep the two apart, this PR only touches lookupEngineName and the existing TestFindOfferingID_LicensedEnginesRefusedBeforeAPICall case list. It does not touch the pagination code or the end of the test file.

• git merge-tree --write-tree HEAD <fork/fix/issue-303 = dbac61d> exits 0 with no conflicts.
• I also ran a trial merge in a throwaway worktree. Both files auto-merged, and go test -short ./services/rds/ passes on the merged tree. The two PRs should merge cleanly in either order.

Out of scope

• normalizeEngineName still passes sql server through instead of rejecting the bare family name as ambiguous, the way it does for sqlserver. Since lookupEngineName now refuses it, there is no behavior gap on the purchase path. I left it alone so the error stays the license-model one the issue asks for.
• client.go was already over the repo's 500-line guideline before this change; it is now 757 lines.

Maintainer: please mirror the issue's triage labels onto this PR: triaged, priority/p3, severity/low, urgency/eventually, impact/few, effort/xs, type/bug.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 2 billable files and costs up to $0.50.}

Or wait 29 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-go/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: 24e9dd4f-cfe5-40cc-a943-da421aad8cc3

📥 Commits

Reviewing files that changed from the base of the PR and between fc2e542 and d9d1154.

📒 Files selected for processing: 2

• providers/aws/services/rds/client.go
• providers/aws/services/rds/client_test.go

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

Independent adversarial review: CLEAN at 807eb2a079d8ab9f93c6f08c845fb64f8f30f39f (claude-go-pr-merge-2, non-author reviewer). No blocking findings.

• SQL Server, sql server, SQL Server Enterprise refused before any API call; oracle*, sqlserver*, sql-server* still refused; aurora/mysql/postgresql/mariadb are mapped by normalizeEngineName first and unaffected. Cost Explorer spells it SQL Server (recommendations/coverage.go:50).
• Fail-before: with origin/main's client.go, TestFindOfferingID_LicensedEnginesRefusedBeforeAPICall/SQL_Server fails. Mutation: removing "sql server" from licensedEnginePrefixes fails the same subtest.
• Non-blocking: prefix-only with no trim, so SQL Server or Amazon RDS for SQL Server would still fall through to "no offerings found" (misleading error, nothing purchased; no known producer). No explicit postgresql/mysql negative case in the new table.
• Local verification (macOS, unit/mock, no AWS): (cd providers/aws && go test -race -short ./services/rds/... ./recommendations/...) pass.

Still needed before merge: owner approval of the fork CI runs and green CI on this SHA, and the author marking the PR ready. Labels mirrored from the linked issue. CodeRabbit skipped this draft; CR skipped: quota/draft, so the merge comment will carry CR waived: quota, adversarial review + local verification + green CI once CI is green.

[IAmJSD]

@cristim the Security Scanning job on this head failed on a network error, not a finding. govulncheck found 0 vulnerabilities affecting the code. The job then died while downloading a module: read "https://proxy.golang.org/go.opentelemetry.io/otel/@v/v1.45.0.zip": stream error: stream ID 37; INTERNAL_ERROR; received from peer. Could you re-run that job?

🤖 Generated with Claude Code

[IAmJSD]

Merged main in 8cd2945 (auto-merged providers/aws/services/rds/client.go, no conflicts). This picks up #485's scripts/run-gosec.sh and starts a fresh CI run. The previous Security Scanning failure was a Go proxy stream error (proxy.golang.org ... otel@v1.45.0.zip: stream error ... INTERNAL_ERROR), not a finding. go vet and go test -race ./services/rds/ pass on the merged tree.

[cristim]

Independent delta review verdict: CLEAN at 8cd2945ff69716de409f54ce928fc698d0e3dffe (merge driver; a reviewer who did not write the code; delta 807eb2a0..8cd2945f)

• The delta is one commit, 8cd2945, a pure merge of main (86b86a0). git merge-tree --write-tree 807eb2a 86b86a0 gives the same tree as the head, so it contains no hand edits or conflict resolutions and no new author changes.
• The PR's net change in providers/aws/services/rds/client.go is unchanged: isLicensedEngine and licensedEnginePrefixes (adds "sql server"). It merges cleanly with main's single-attempt and empty-token purchase changes, which are in separate hunks. The licensed-engine refusal still runs in lookupEngineName, before any offering API call.
• Mutation probes, both killed:
  • Removing "sql server" fails TestFindOfferingID_LicensedEnginesRefusedBeforeAPICall/SQL_Server.
  • Breaking the prefix match fails the oracle-se2 case.
• Local verification on macOS (fixture/mock evidence): go vet ./... and go test -race -short ./... pass in all five modules with GOWORK=off.
• Non-blocking notes from the earlier review still apply: matching is prefix-only and does not trim, and there is no explicit postgresql/mysql negative case.

[cristim]

Merge gate record at d9d11541fffab05881ddda5b858981a01e06da15 (merge driver)

• Independent review CLEAN at 8cd2945f (verdict above). Each first-parent commit after it is a merge of main, and each merge's tree equals git merge-tree --write-tree of its parents. The PR's own change is therefore unchanged and the verdict stands. The latest merge is GitHub's Update branch, which the author consented to on fix(azure): attribute shared-scope vm reservations to their billing subscription #423.
• Local verification on macOS at this head (fixture/mock evidence, no live cloud calls): cd providers/aws && go test -race -short ./... && go vet ./... passes.
• CI green on this head: CI - Build & Test 39052646629, pre-commit 39052646493. mergeState CLEAN, no unresolved threads, labels mirrored from fix(aws/rds): refuse the 'SQL Server' engine spelling in the licensed-engine check #302. CR waived (quota): adversarial review + local verification + green CI.

Squash-merging with --match-head-commit d9d11541.