diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1a32a4d..c3da385 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -70,3 +70,24 @@ jobs: with: version: latest args: --timeout=5m + + windows: + name: Windows build + test + runs-on: windows-latest + steps: + - name: Check out code + uses: actions/checkout@v4 + + - name: Set up Go + uses: actions/setup-go@v5 + with: + go-version: '1.26' + + - name: Get dependencies + run: go mod download + + - name: Build + run: go build ./... + + - name: Test + run: go test ./... diff --git a/CHANGELOG.md b/CHANGELOG.md index 5251ded..7ea7f29 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,29 @@ ## [Unreleased] +### Fixed +- Windows build+test coverage in CI, plus a README Windows quick-start +- Config keys (`provider`, `timeout`, `exclude.tags`) that were parsed but never + wired to scan behavior now actually take effect +- Explicit CLI flags now correctly override config file values when the flag + is set to its own default (e.g. `--idle-days=14`) +- Text reporter now sorts findings by severity instead of scan order +- GCP unused-read-replica finding no longer reports a confirmed monthly-waste + figure without a real usage signal +- `selectReporter`'s output file handle is now closed, fixing a Windows CI + failure where an open handle blocked temp-directory cleanup + +### Changed +- Deduplicated exclusion-check and progress-reporting logic shared by the + AWS and GCP scanners + +## [0.1.1] - 2026-02-28 + +### Fixed +- SpectreHub reporter now uses the `spectre/v1` schema + +## [0.1.0] - 2026-02-28 + ### Added - AWS RDS scanner with 11 finding types - GCP Cloud SQL scanner with 6 config-based findings diff --git a/README.md b/README.md index efc5e9c..508aef4 100644 --- a/README.md +++ b/README.md @@ -37,6 +37,16 @@ cd rdsspectre make build ``` +### Windows + +Download the `rdsspectre__windows_.zip` archive from the +[latest release](https://github.com/ppiankov/rdsspectre/releases/latest), +extract it, and run the binary from PowerShell or Command Prompt: + +```powershell +.\rdsspectre.exe aws --region us-east-1 --format json +``` + ### Usage ```sh diff --git a/internal/cloudsql/client.go b/internal/cloudsql/client.go index c57e6ce..2484e94 100644 --- a/internal/cloudsql/client.go +++ b/internal/cloudsql/client.go @@ -32,6 +32,8 @@ type Instance struct { MasterInstanceName string CreateTime time.Time SelfLink string + // WO-7: user labels, for tag-based exclusion matching. + Labels map[string]string } // Client wraps a Cloud SQL Admin API service. @@ -91,6 +93,8 @@ func convertInstance(db *sqladmin.DatabaseInstance) Instance { inst.DataDiskSizeGB = db.Settings.DataDiskSizeGb inst.DataDiskType = db.Settings.DataDiskType inst.DeletionProtection = db.Settings.DeletionProtectionEnabled + // WO-7: populate labels for tag-based exclusion. + inst.Labels = db.Settings.UserLabels if db.Settings.BackupConfiguration != nil { inst.BackupEnabled = db.Settings.BackupConfiguration.Enabled diff --git a/internal/cloudsql/scanner.go b/internal/cloudsql/scanner.go index 41b52c6..97929b8 100644 --- a/internal/cloudsql/scanner.go +++ b/internal/cloudsql/scanner.go @@ -46,7 +46,12 @@ func (s *CloudSQLScanner) Scan(ctx context.Context, cfg database.ScanConfig, pro if inst.State != "RUNNABLE" { continue } - if cfg.Exclude.ResourceIDs[inst.Name] { + // WO-9: use shared ExcludeConfig.IsExcluded helper instead of inline map lookup. + if cfg.Exclude.IsExcluded(inst.Name) { + continue + } + // WO-7: skip instances matching a configured exclude.tags rule. + if len(cfg.Exclude.Tags) > 0 && cfg.Exclude.MatchesExcludedTags(inst.Labels) { continue } result.ResourcesScanned++ @@ -138,20 +143,25 @@ func (s *CloudSQLScanner) analyzeInstance(cfg database.ScanConfig, inst Instance }) } - // UNUSED_READ_REPLICA: config-based detection (no metrics) + // WO-11: UNUSED_READ_REPLICA config-based detection only, no connection signal + // available (Cloud Monitoring deferred). Unlike the AWS path + // (rds/scanner.go), which only fires on a confirmed zero-connection + // window, usage here is genuinely unknown, so this reports Low severity + // with no claimed EstimatedMonthlyWaste rather than presenting the full + // instance cost as confirmed savings. if inst.IsReplica { findings = append(findings, database.Finding{ - ID: database.FindingUnusedReadReplica, - Severity: database.SeverityHigh, - ResourceType: database.ResourceReplica, - ResourceID: inst.Name, - Region: region, - Message: "Read replica detected (connection metrics unavailable without Cloud Monitoring)", - EstimatedMonthlyWaste: monthlyCost, + ID: database.FindingUnusedReadReplica, + Severity: database.SeverityLow, + ResourceType: database.ResourceReplica, + ResourceID: inst.Name, + Region: region, + Message: "Read replica present; usage unknown (connection metrics unavailable without Cloud Monitoring) — verify before deleting", Metadata: map[string]any{ - "master_instance": inst.MasterInstanceName, - "database_version": inst.DatabaseVersion, - "tier": inst.Tier, + "master_instance": inst.MasterInstanceName, + "database_version": inst.DatabaseVersion, + "tier": inst.Tier, + "estimated_monthly_cost": monthlyCost, }, }) } @@ -170,12 +180,6 @@ func hasPublicAccess(inst Instance) bool { } func (s *CloudSQLScanner) reportProgress(progress func(database.ScanProgress), msg string) { - if progress != nil { - progress(database.ScanProgress{ - Region: s.project, - Scanner: "cloudsql", - Message: msg, - Timestamp: time.Now(), - }) - } + // WO-9: delegate to the shared database.ReportProgress helper. + database.ReportProgress(progress, "cloudsql", s.project, msg) } diff --git a/internal/cloudsql/scanner_test.go b/internal/cloudsql/scanner_test.go index 17b5915..2c2523a 100644 --- a/internal/cloudsql/scanner_test.go +++ b/internal/cloudsql/scanner_test.go @@ -181,8 +181,61 @@ func TestScanReadReplica(t *testing.T) { if len(hits) != 1 { t.Errorf("expected 1 UNUSED_READ_REPLICA finding, got %d", len(hits)) } - if hits[0].EstimatedMonthlyWaste <= 0 { - t.Error("replica finding should have cost estimate") + // WO-11: usage is unconfirmed without Cloud Monitoring data, so this must + // not report High severity or claim a definite EstimatedMonthlyWaste. + if hits[0].Severity != database.SeverityLow { + t.Errorf("replica finding severity = %q, want %q (unconfirmed usage)", hits[0].Severity, database.SeverityLow) + } + if hits[0].EstimatedMonthlyWaste != 0 { + t.Errorf("replica finding should not claim confirmed waste, got %.2f", hits[0].EstimatedMonthlyWaste) + } + if cost, ok := hits[0].Metadata["estimated_monthly_cost"]; !ok || cost.(float64) <= 0 { + t.Error("replica finding should surface an informational estimated_monthly_cost in metadata") + } +} + +// WO-7: exercises tag/label-based exclusion. +func TestScanExcludeByLabel(t *testing.T) { + mock := newMockClient() + mock.instances = []Instance{ + makeInstance("tagged-db", "db-f1-micro", "POSTGRES_17", func(i *Instance) { + i.Labels = map[string]string{"env": "temporary"} + i.DeletionProtection = false // would normally be flagged + }), + } + + cfg := defaultCfg() + cfg.Exclude.Tags = map[string]string{"env": "temporary"} + + s := newTestScanner(mock) + result := s.Scan(context.Background(), cfg, nil) + + if len(result.Findings) != 0 { + t.Errorf("expected 0 findings for label-excluded instance, got %d", len(result.Findings)) + } + if result.ResourcesScanned != 0 { + t.Errorf("ResourcesScanned = %d, want 0 (label-excluded)", result.ResourcesScanned) + } +} + +// WO-7: exercises tag/label-based exclusion. +func TestScanLabelExcludeNoMatch(t *testing.T) { + mock := newMockClient() + mock.instances = []Instance{ + makeInstance("keep-db", "db-f1-micro", "POSTGRES_17", func(i *Instance) { + i.Labels = map[string]string{"env": "production"} + i.DeletionProtection = false + }), + } + + cfg := defaultCfg() + cfg.Exclude.Tags = map[string]string{"env": "temporary"} + + s := newTestScanner(mock) + result := s.Scan(context.Background(), cfg, nil) + + if len(findByID(result.Findings, database.FindingNoDeletionProtect)) != 1 { + t.Error("expected instance to still be flagged (label does not match exclusion)") } } diff --git a/internal/commands/aws.go b/internal/commands/aws.go index 28cbb72..103dc15 100644 --- a/internal/commands/aws.go +++ b/internal/commands/aws.go @@ -3,6 +3,7 @@ package commands import ( "context" "fmt" + "io" "log/slog" "os" "strings" @@ -57,6 +58,18 @@ func init() { } func runAWS(cmd *cobra.Command, _ []string) error { + // WO-7: load config and apply defaults before building the timeout + // context, so a config-file timeout can fall back into effect. + cfg, err := config.Load(".") + if err != nil { + slog.Warn("Failed to load config file", "error", err) + } + // WO-7: reject a config provider that doesn't match the invoked subcommand. + if cfg.Provider != "" && cfg.Provider != "aws" { + return fmt.Errorf("config provider %q does not match the invoked \"aws\" subcommand", cfg.Provider) + } + applyAWSConfigDefaults(cmd, cfg) + ctx := cmd.Context() if awsFlags.timeout > 0 { var cancel context.CancelFunc @@ -64,13 +77,6 @@ func runAWS(cmd *cobra.Command, _ []string) error { defer cancel() } - // Load config and apply defaults - cfg, err := config.Load(".") - if err != nil { - slog.Warn("Failed to load config file", "error", err) - } - applyAWSConfigDefaults(cfg) - // Resolve profile and region profile := awsFlags.profile if profile == "" { @@ -94,10 +100,8 @@ func runAWS(cmd *cobra.Command, _ []string) error { slog.Info("Scanning RDS", "region", resolvedRegion) // Build scan config - excludeIDs := make(map[string]bool, len(cfg.Exclude.ResourceIDs)) - for _, id := range cfg.Exclude.ResourceIDs { - excludeIDs[id] = true - } + // WO-9: shared helper instead of an inline map-building loop. + excludeIDs := buildExcludeIDs(cfg.Exclude.ResourceIDs) excludeTags := parseExcludeTags(cfg.Exclude.Tags, awsFlags.excludeTags) scanCfg := database.ScanConfig{ @@ -153,55 +157,86 @@ func runAWS(cmd *cobra.Command, _ []string) error { } // Select and run reporter - reporter, err := selectReporter(awsFlags.format, awsFlags.outputFile) + // WO-12: close the output file after Generate so temp-dir cleanup + // (and any later read of the file) doesn't race an open handle, + // which is fatal on Windows. + reporter, closer, err := selectReporter(awsFlags.format, awsFlags.outputFile) if err != nil { return err } + if closer != nil { + defer func() { + if cerr := closer.Close(); cerr != nil { + slog.Warn("Failed to close output file", "error", cerr) + } + }() + } return reporter.Generate(data) } -func applyAWSConfigDefaults(cfg config.Config) { - if awsFlags.format == "text" && cfg.Format != "" { +// applyAWSConfigDefaults fills unset flags from the config file. +// An explicit CLI flag always wins over config, even when its value +// equals the flag's built-in default; only cmd.Flags().Changed() can tell +// "explicitly set to the default" apart from "never set". +func applyAWSConfigDefaults(cmd *cobra.Command, cfg config.Config) { + // WO-8: cmd.Flags() drives the Changed()-based precedence checks below. + flags := cmd.Flags() + // WO-8: cmd.Flags().Changed() replaces the old flag==default sentinel + // for every check below, so an explicit flag always wins over config. + if !flags.Changed("format") && cfg.Format != "" { awsFlags.format = cfg.Format } - if awsFlags.idleDays == 14 && cfg.IdleDays > 0 { + // WO-8: see above. + if !flags.Changed("idle-days") && cfg.IdleDays > 0 { awsFlags.idleDays = cfg.IdleDays } - if awsFlags.staleDays == 90 && cfg.StaleDays > 0 { + // WO-8: see above. + if !flags.Changed("stale-days") && cfg.StaleDays > 0 { awsFlags.staleDays = cfg.StaleDays } - if awsFlags.cpuThreshold == 20.0 && cfg.CPUThreshold > 0 { + // WO-8: see above. + if !flags.Changed("cpu-threshold") && cfg.CPUThreshold > 0 { awsFlags.cpuThreshold = cfg.CPUThreshold } - if awsFlags.metricDays == 14 && cfg.MetricDays > 0 { + // WO-8: see above. + if !flags.Changed("metric-days") && cfg.MetricDays > 0 { awsFlags.metricDays = cfg.MetricDays } - if awsFlags.minMonthlyCost == 0.10 && cfg.MinMonthlyCost > 0 { + // WO-8: see above. + if !flags.Changed("min-monthly-cost") && cfg.MinMonthlyCost > 0 { awsFlags.minMonthlyCost = cfg.MinMonthlyCost } + // WO-7: config-file timeout falls back into effect only if --timeout wasn't explicit. + if !flags.Changed("timeout") && cfg.TimeoutDuration() > 0 { + awsFlags.timeout = cfg.TimeoutDuration() + } } -func selectReporter(format, outputFile string) (report.Reporter, error) { - w := os.Stdout +// WO-12: also returns an io.Closer (nil for stdout) so callers can close the +// output file after Generate instead of leaking the handle until process exit. +func selectReporter(format, outputFile string) (report.Reporter, io.Closer, error) { + var w io.Writer = os.Stdout + var closer io.Closer if outputFile != "" { f, err := os.Create(outputFile) if err != nil { - return nil, fmt.Errorf("create output file: %w", err) + return nil, nil, fmt.Errorf("create output file: %w", err) } w = f + closer = f } switch format { case "json": - return &report.JSONReporter{Writer: w}, nil + return &report.JSONReporter{Writer: w}, closer, nil case "text": - return &report.TextReporter{Writer: w}, nil + return &report.TextReporter{Writer: w}, closer, nil case "sarif": - return &report.SARIFReporter{Writer: w}, nil + return &report.SARIFReporter{Writer: w}, closer, nil case "spectrehub": - return &report.SpectreHubReporter{Writer: w}, nil + return &report.SpectreHubReporter{Writer: w}, closer, nil default: - return nil, fmt.Errorf("unsupported format: %s (use text, json, sarif, or spectrehub)", format) + return nil, closer, fmt.Errorf("unsupported format: %s (use text, json, sarif, or spectrehub)", format) } } diff --git a/internal/commands/commands_test.go b/internal/commands/commands_test.go index 54c8d16..ca90541 100644 --- a/internal/commands/commands_test.go +++ b/internal/commands/commands_test.go @@ -7,6 +7,7 @@ import ( "path/filepath" "strings" "testing" + "time" "github.com/ppiankov/rdsspectre/internal/config" ) @@ -179,7 +180,8 @@ func TestSelectReporter(t *testing.T) { {"invalid", true}, } for _, tt := range tests { - r, err := selectReporter(tt.format, "") + // WO-12: selectReporter now returns an io.Closer alongside the reporter. + r, _, err := selectReporter(tt.format, "") if tt.wantErr { if err == nil { t.Errorf("selectReporter(%q) should error", tt.format) @@ -209,6 +211,25 @@ func TestParseExcludeTags(t *testing.T) { } } +// WO-9: exercises buildExcludeIDs. +func TestBuildExcludeIDs(t *testing.T) { + m := buildExcludeIDs([]string{"mydb-prod", "mydb-staging"}) + if !m["mydb-prod"] || !m["mydb-staging"] { + t.Errorf("expected both IDs present, got %+v", m) + } + if len(m) != 2 { + t.Errorf("len = %d, want 2", len(m)) + } +} + +// WO-9: exercises buildExcludeIDs. +func TestBuildExcludeIDsEmpty(t *testing.T) { + m := buildExcludeIDs(nil) + if len(m) != 0 { + t.Errorf("expected empty map, got %+v", m) + } +} + func TestParseExcludeTagsEmpty(t *testing.T) { tags := parseExcludeTags(nil, nil) if tags != nil { @@ -233,7 +254,8 @@ func TestApplyAWSConfigDefaults(t *testing.T) { MinMonthlyCost: 1.0, } - applyAWSConfigDefaults(cfg) + // WO-8: call site now passes cmd for Flags().Changed() precedence. + applyAWSConfigDefaults(awsCmd, cfg) if awsFlags.format != "json" { t.Errorf("format = %q, want json", awsFlags.format) @@ -254,19 +276,27 @@ func TestApplyAWSConfigDefaults(t *testing.T) { awsFlags.minMonthlyCost = 0.10 } +// WO-12: exercises the io.Closer returned for file-backed reporters. func TestSelectReporterOutputFile(t *testing.T) { f := filepath.Join(t.TempDir(), "out.json") - r, err := selectReporter("json", f) + r, closer, err := selectReporter("json", f) if err != nil { t.Fatalf("selectReporter() error: %v", err) } if r == nil { t.Fatal("selectReporter() returned nil") } + if closer == nil { + t.Fatal("selectReporter() should return a non-nil closer for a file output") + } + if err := closer.Close(); err != nil { + t.Errorf("closer.Close() error: %v", err) + } } func TestSelectReporterBadPath(t *testing.T) { - _, err := selectReporter("json", "/nonexistent/dir/file.json") + // WO-12: selectReporter now returns an io.Closer alongside the reporter. + _, _, err := selectReporter("json", "/nonexistent/dir/file.json") if err == nil { t.Error("expected error for bad output path") } @@ -284,7 +314,8 @@ func TestApplyAWSConfigDefaultsNoOverride(t *testing.T) { awsFlags.format = "text" awsFlags.idleDays = 14 cfg := config.Config{} // all zero - applyAWSConfigDefaults(cfg) + // WO-8: call site now passes cmd for Flags().Changed() precedence. + applyAWSConfigDefaults(awsCmd, cfg) if awsFlags.format != "text" { t.Errorf("format should remain text, got %q", awsFlags.format) } @@ -301,7 +332,8 @@ func TestApplyGCPConfigDefaults(t *testing.T) { Format: "json", MinMonthlyCost: 5.0, } - applyGCPConfigDefaults(cfg) + // WO-8: call site now passes cmd for Flags().Changed() precedence. + applyGCPConfigDefaults(gcpCmd, cfg) if gcpFlags.format != "json" { t.Errorf("format = %q, want json", gcpFlags.format) @@ -315,12 +347,64 @@ func TestApplyGCPConfigDefaults(t *testing.T) { gcpFlags.minMonthlyCost = 0.10 } +// WO-8: exercises explicit-flag-wins-over-config precedence. +func TestApplyAWSConfigDefaultsExplicitFlagWinsOverConfig(t *testing.T) { + // An explicit --idle-days=14 (equal to the built-in default) must + // win over a conflicting config file value, unlike the old sentinel check. + awsFlags.idleDays = 14 + if err := awsCmd.Flags().Set("idle-days", "14"); err != nil { + t.Fatalf("Set() error: %v", err) + } + defer func() { + awsCmd.Flags().Lookup("idle-days").Changed = false + awsFlags.idleDays = 14 + }() + + cfg := config.Config{IdleDays: 30} + applyAWSConfigDefaults(awsCmd, cfg) + + if awsFlags.idleDays != 14 { + t.Errorf("idleDays = %d, want 14 (explicit flag should win over config)", awsFlags.idleDays) + } +} + +// WO-8: exercises explicit-flag-wins-over-config precedence. +func TestApplyGCPConfigDefaultsExplicitFlagWinsOverConfig(t *testing.T) { + gcpFlags.minMonthlyCost = 0.10 + if err := gcpCmd.Flags().Set("min-monthly-cost", "0.10"); err != nil { + t.Fatalf("Set() error: %v", err) + } + defer func() { + gcpCmd.Flags().Lookup("min-monthly-cost").Changed = false + gcpFlags.minMonthlyCost = 0.10 + }() + + cfg := config.Config{MinMonthlyCost: 5.0} + applyGCPConfigDefaults(gcpCmd, cfg) + + if gcpFlags.minMonthlyCost != 0.10 { + t.Errorf("minMonthlyCost = %f, want 0.10 (explicit flag should win over config)", gcpFlags.minMonthlyCost) + } +} + +// WO-7: exercises config-file timeout fallback. +func TestApplyGCPConfigDefaultsTimeoutFallback(t *testing.T) { + gcpFlags.timeout = 10 * time.Minute + cfg := config.Config{Timeout: "5m"} + applyGCPConfigDefaults(gcpCmd, cfg) + if gcpFlags.timeout != 5*time.Minute { + t.Errorf("timeout = %v, want 5m (config fallback)", gcpFlags.timeout) + } + gcpFlags.timeout = 10 * time.Minute +} + func TestApplyGCPConfigDefaultsNoOverride(t *testing.T) { gcpFlags.format = "text" gcpFlags.minMonthlyCost = 0.10 cfg := config.Config{} // all zero - applyGCPConfigDefaults(cfg) + // WO-8: call site now passes cmd for Flags().Changed() precedence. + applyGCPConfigDefaults(gcpCmd, cfg) if gcpFlags.format != "text" { t.Errorf("format should remain text, got %q", gcpFlags.format) @@ -330,6 +414,69 @@ func TestApplyGCPConfigDefaultsNoOverride(t *testing.T) { } } +// WO-7: exercises config-provider-vs-subcommand validation. +func TestRunGCPProviderMismatch(t *testing.T) { + dir := t.TempDir() + cfgContent := "provider: aws\nproject: test-project\n" + if err := os.WriteFile(filepath.Join(dir, ".rdsspectre.yaml"), []byte(cfgContent), 0o644); err != nil { + t.Fatal(err) + } + chdir(t, dir) + + gcpFlags.project = "" + rootCmd.SetArgs([]string{"gcp"}) + err := rootCmd.Execute() + if err == nil || !strings.Contains(err.Error(), "does not match") { + t.Errorf("expected provider-mismatch error, got %v", err) + } +} + +// WO-7: exercises config-provider-vs-subcommand validation. +func TestRunAWSProviderMismatch(t *testing.T) { + dir := t.TempDir() + cfgContent := "provider: gcp\n" + if err := os.WriteFile(filepath.Join(dir, ".rdsspectre.yaml"), []byte(cfgContent), 0o644); err != nil { + t.Fatal(err) + } + chdir(t, dir) + + rootCmd.SetArgs([]string{"aws"}) + err := rootCmd.Execute() + if err == nil || !strings.Contains(err.Error(), "does not match") { + t.Errorf("expected provider-mismatch error, got %v", err) + } +} + +// WO-7: exercises config-file timeout fallback. +// WO-7: exercises config-file timeout fallback. +func TestApplyAWSConfigDefaultsTimeoutFallback(t *testing.T) { + awsFlags.timeout = 10 * time.Minute + cfg := config.Config{Timeout: "5m"} + applyAWSConfigDefaults(awsCmd, cfg) + if awsFlags.timeout != 5*time.Minute { + t.Errorf("timeout = %v, want 5m (config fallback)", awsFlags.timeout) + } + awsFlags.timeout = 10 * time.Minute +} + +// WO-7: exercises explicit --timeout winning over config fallback. +func TestApplyAWSConfigDefaultsTimeoutExplicitWins(t *testing.T) { + awsFlags.timeout = 10 * time.Minute + if err := awsCmd.Flags().Set("timeout", "10m"); err != nil { + t.Fatalf("Set() error: %v", err) + } + defer func() { + awsCmd.Flags().Lookup("timeout").Changed = false + awsFlags.timeout = 10 * time.Minute + }() + + cfg := config.Config{Timeout: "5m"} + applyAWSConfigDefaults(awsCmd, cfg) + if awsFlags.timeout != 10*time.Minute { + t.Errorf("timeout = %v, want 10m (explicit flag should win over config)", awsFlags.timeout) + } +} + func TestRunGCPMissingProject(t *testing.T) { gcpFlags.project = "" rootCmd.SetArgs([]string{"gcp"}) diff --git a/internal/commands/gcp.go b/internal/commands/gcp.go index c258627..cbbc3f6 100644 --- a/internal/commands/gcp.go +++ b/internal/commands/gcp.go @@ -44,6 +44,18 @@ func init() { } func runGCP(cmd *cobra.Command, _ []string) error { + // WO-7: load config and apply defaults before building the timeout + // context, so a config-file timeout can fall back into effect. + cfg, err := config.Load(".") + if err != nil { + slog.Warn("Failed to load config file", "error", err) + } + // WO-7: reject a config provider that doesn't match the invoked subcommand. + if cfg.Provider != "" && cfg.Provider != "gcp" { + return fmt.Errorf("config provider %q does not match the invoked \"gcp\" subcommand", cfg.Provider) + } + applyGCPConfigDefaults(cmd, cfg) + ctx := cmd.Context() if gcpFlags.timeout > 0 { var cancel context.CancelFunc @@ -51,13 +63,6 @@ func runGCP(cmd *cobra.Command, _ []string) error { defer cancel() } - // Load config and apply defaults - cfg, err := config.Load(".") - if err != nil { - slog.Warn("Failed to load config file", "error", err) - } - applyGCPConfigDefaults(cfg) - // Resolve project project := gcpFlags.project if project == "" { @@ -76,10 +81,8 @@ func runGCP(cmd *cobra.Command, _ []string) error { } // Build scan config - excludeIDs := make(map[string]bool, len(cfg.Exclude.ResourceIDs)) - for _, id := range cfg.Exclude.ResourceIDs { - excludeIDs[id] = true - } + // WO-9: shared helper instead of an inline map-building loop. + excludeIDs := buildExcludeIDs(cfg.Exclude.ResourceIDs) excludeTags := parseExcludeTags(cfg.Exclude.Tags, gcpFlags.excludeTags) scanCfg := database.ScanConfig{ @@ -127,18 +130,37 @@ func runGCP(cmd *cobra.Command, _ []string) error { } // Select and run reporter - reporter, err := selectReporter(gcpFlags.format, gcpFlags.outputFile) + // WO-12: close the output file after Generate; open handles are fatal + // to temp-dir cleanup on Windows. + reporter, closer, err := selectReporter(gcpFlags.format, gcpFlags.outputFile) if err != nil { return err } + if closer != nil { + defer func() { + if cerr := closer.Close(); cerr != nil { + slog.Warn("Failed to close output file", "error", cerr) + } + }() + } return reporter.Generate(data) } -func applyGCPConfigDefaults(cfg config.Config) { - if gcpFlags.format == "text" && cfg.Format != "" { +// applyGCPConfigDefaults fills unset flags from the config file. +// Mirrors applyAWSConfigDefaults's cmd.Flags().Changed() precedence fix. +func applyGCPConfigDefaults(cmd *cobra.Command, cfg config.Config) { + // WO-8: cmd.Flags() drives the Changed()-based precedence checks below. + flags := cmd.Flags() + // WO-8: cmd.Flags().Changed() replaces the old flag==default sentinel. + if !flags.Changed("format") && cfg.Format != "" { gcpFlags.format = cfg.Format } - if gcpFlags.minMonthlyCost == 0.10 && cfg.MinMonthlyCost > 0 { + // WO-8: see above. + if !flags.Changed("min-monthly-cost") && cfg.MinMonthlyCost > 0 { gcpFlags.minMonthlyCost = cfg.MinMonthlyCost } + // WO-7: config-file timeout falls back into effect only if --timeout wasn't explicit. + if !flags.Changed("timeout") && cfg.TimeoutDuration() > 0 { + gcpFlags.timeout = cfg.TimeoutDuration() + } } diff --git a/internal/commands/helpers.go b/internal/commands/helpers.go index 0a4917c..51a1a73 100644 --- a/internal/commands/helpers.go +++ b/internal/commands/helpers.go @@ -34,6 +34,16 @@ func enhanceError(action string, err error) error { return fmt.Errorf("%s: %w", action, err) } +// WO-9: shared by aws.go/gcp.go instead of each building the map inline. +// buildExcludeIDs converts a resource-ID list into the lookup map ScanConfig expects. +func buildExcludeIDs(ids []string) map[string]bool { + m := make(map[string]bool, len(ids)) + for _, id := range ids { + m[id] = true + } + return m +} + // computeTargetHash generates a SHA256 hash for the target URI. func computeTargetHash(provider string, regions []string, project string) string { input := fmt.Sprintf("provider:%s,regions:%s,project:%s", provider, strings.Join(regions, ","), project) diff --git a/internal/database/types.go b/internal/database/types.go index 4f403ce..9cfa6f4 100644 --- a/internal/database/types.go +++ b/internal/database/types.go @@ -76,6 +76,25 @@ type ExcludeConfig struct { Tags map[string]string `json:"tags,omitempty"` } +// WO-9: shared by rds/scanner.go and cloudsql/scanner.go instead of an inline lookup. +// IsExcluded reports whether id is in the resource-ID exclusion list. +func (e ExcludeConfig) IsExcluded(id string) bool { + return e.ResourceIDs[id] +} + +// WO-7: tag/label-based exclusion for both AWS and GCP scanners. +// MatchesExcludedTags reports whether tags matches any configured exclusion +// rule. A configured value of "" matches any value for that key (key-only +// exclusion, mirroring commands.parseExcludeTags's key-only tag syntax). +func (e ExcludeConfig) MatchesExcludedTags(tags map[string]string) bool { + for k, want := range e.Tags { + if got, ok := tags[k]; ok && (want == "" || got == want) { + return true + } + } + return false +} + // ScanProgress reports scanning progress. type ScanProgress struct { Region string `json:"region"` @@ -83,3 +102,17 @@ type ScanProgress struct { Message string `json:"message"` Timestamp time.Time `json:"timestamp"` } + +// WO-9: shared by rds/scanner.go and cloudsql/scanner.go instead of duplicated boilerplate. +// ReportProgress invokes progress with a ScanProgress if progress is non-nil. +// Shared by per-provider scanners to avoid re-implementing the same guard. +func ReportProgress(progress func(ScanProgress), scanner, region, msg string) { + if progress != nil { + progress(ScanProgress{ + Region: region, + Scanner: scanner, + Message: msg, + Timestamp: time.Now(), + }) + } +} diff --git a/internal/database/types_test.go b/internal/database/types_test.go index e2d2769..72d5d5e 100644 --- a/internal/database/types_test.go +++ b/internal/database/types_test.go @@ -2,6 +2,75 @@ package database import "testing" +// WO-9: exercises ExcludeConfig.IsExcluded. +func TestExcludeConfigIsExcluded(t *testing.T) { + e := ExcludeConfig{ResourceIDs: map[string]bool{"mydb-prod": true}} + if !e.IsExcluded("mydb-prod") { + t.Error("expected mydb-prod to be excluded") + } + if e.IsExcluded("mydb-dev") { + t.Error("expected mydb-dev to not be excluded") + } +} + +// WO-9: exercises ExcludeConfig.IsExcluded. +func TestExcludeConfigIsExcludedNilMap(t *testing.T) { + var e ExcludeConfig + if e.IsExcluded("anything") { + t.Error("nil ResourceIDs should exclude nothing") + } +} + +// WO-7: exercises ExcludeConfig.MatchesExcludedTags. +func TestMatchesExcludedTagsExactMatch(t *testing.T) { + e := ExcludeConfig{Tags: map[string]string{"env": "temporary"}} + if !e.MatchesExcludedTags(map[string]string{"env": "temporary"}) { + t.Error("expected exact key=value match to exclude") + } +} + +// WO-7: exercises ExcludeConfig.MatchesExcludedTags. +func TestMatchesExcludedTagsKeyOnlyWildcard(t *testing.T) { + e := ExcludeConfig{Tags: map[string]string{"temporary": ""}} + if !e.MatchesExcludedTags(map[string]string{"temporary": "anything"}) { + t.Error("empty configured value should match any value for that key") + } +} + +// WO-7: exercises ExcludeConfig.MatchesExcludedTags. +func TestMatchesExcludedTagsNoMatch(t *testing.T) { + e := ExcludeConfig{Tags: map[string]string{"env": "temporary"}} + if e.MatchesExcludedTags(map[string]string{"env": "production"}) { + t.Error("mismatched value should not exclude") + } + if e.MatchesExcludedTags(map[string]string{"other": "temporary"}) { + t.Error("missing key should not exclude") + } +} + +// WO-7: exercises ExcludeConfig.MatchesExcludedTags. +func TestMatchesExcludedTagsEmptyRules(t *testing.T) { + var e ExcludeConfig + if e.MatchesExcludedTags(map[string]string{"env": "production"}) { + t.Error("no configured rules should never exclude") + } +} + +// WO-9: exercises the shared ReportProgress helper. +func TestReportProgressNilCallback(t *testing.T) { + // Must not panic when progress is nil. + ReportProgress(nil, "rds", "us-east-1", "scanning") +} + +// WO-9: exercises the shared ReportProgress helper. +func TestReportProgressInvokesCallback(t *testing.T) { + var got ScanProgress + ReportProgress(func(p ScanProgress) { got = p }, "cloudsql", "my-project", "listing instances") + if got.Scanner != "cloudsql" || got.Region != "my-project" || got.Message != "listing instances" { + t.Errorf("unexpected progress: %+v", got) + } +} + func TestSeverityConstants(t *testing.T) { if SeverityCritical != "critical" { t.Error("SeverityCritical mismatch") diff --git a/internal/rds/cloudwatch.go b/internal/rds/cloudwatch.go index 7b8c574..f50e289 100644 --- a/internal/rds/cloudwatch.go +++ b/internal/rds/cloudwatch.go @@ -7,6 +7,7 @@ import ( "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/service/cloudwatch" cwtypes "github.com/aws/aws-sdk-go-v2/service/cloudwatch/types" + awsrds "github.com/aws/aws-sdk-go-v2/service/rds" ) // MetricStats holds summarized CloudWatch metric data. @@ -18,6 +19,26 @@ type MetricStats struct { DatapointCount int } +// WO-7: enables tag-based exclusion in rds/scanner.go. +// FetchTags retrieves resource tags for an RDS instance or snapshot ARN as a +// key-value map, for tag-based exclusion matching. +func FetchTags(ctx context.Context, client RDSAPI, arn string) (map[string]string, error) { + out, err := client.ListTagsForResource(ctx, &awsrds.ListTagsForResourceInput{ + ResourceName: aws.String(arn), + }) + if err != nil { + return nil, err + } + tags := make(map[string]string, len(out.TagList)) + for _, t := range out.TagList { + if t.Key == nil { + continue + } + tags[*t.Key] = deref(t.Value) + } + return tags, nil +} + // FetchInstanceMetrics retrieves CPU and connection metrics for an RDS instance. func FetchInstanceMetrics(ctx context.Context, cw CloudWatchAPI, instanceID string, now time.Time, days int) (*MetricStats, error) { start := now.AddDate(0, 0, -days) diff --git a/internal/rds/cloudwatch_test.go b/internal/rds/cloudwatch_test.go index ef9d181..33ac135 100644 --- a/internal/rds/cloudwatch_test.go +++ b/internal/rds/cloudwatch_test.go @@ -5,10 +5,40 @@ import ( "errors" "testing" + "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/service/cloudwatch" cwtypes "github.com/aws/aws-sdk-go-v2/service/cloudwatch/types" + rdstypes "github.com/aws/aws-sdk-go-v2/service/rds/types" ) +// WO-7: exercises FetchTags. +func TestFetchTags(t *testing.T) { + mock := newMockRDSClient() + mock.tagsForARN["arn:aws:rds:us-east-1:123456789012:db:mydb"] = []rdstypes.Tag{ + {Key: aws.String("env"), Value: aws.String("production")}, + {Key: aws.String("team"), Value: aws.String("platform")}, + } + + tags, err := FetchTags(context.Background(), mock, "arn:aws:rds:us-east-1:123456789012:db:mydb") + if err != nil { + t.Fatalf("error: %v", err) + } + if tags["env"] != "production" || tags["team"] != "platform" { + t.Errorf("unexpected tags: %+v", tags) + } +} + +// WO-7: exercises FetchTags. +func TestFetchTagsError(t *testing.T) { + mock := newMockRDSClient() + mock.listTagsErr = errors.New("boom") + + _, err := FetchTags(context.Background(), mock, "arn:aws:rds:us-east-1:123456789012:db:mydb") + if err == nil { + t.Error("expected error to propagate") + } +} + func TestFetchInstanceMetricsIdle(t *testing.T) { cw := newMockCWClient() cw.metrics["CPUUtilization"] = makeCPUDatapoints(2.0, 4.0, 14) diff --git a/internal/rds/scanner.go b/internal/rds/scanner.go index a884cc3..7a8ac82 100644 --- a/internal/rds/scanner.go +++ b/internal/rds/scanner.go @@ -3,6 +3,7 @@ package rds import ( "context" "fmt" + "log/slog" "strings" "time" @@ -48,9 +49,19 @@ func (s *RDSScanner) Scan(ctx context.Context, cfg database.ScanConfig, progress if inst.Status != "available" { continue } - if cfg.Exclude.ResourceIDs[inst.ID] { + // WO-9: use shared ExcludeConfig.IsExcluded helper instead of inline map lookup. + if cfg.Exclude.IsExcluded(inst.ID) { continue } + // WO-7: skip instances matching a configured exclude.tags rule. + if len(cfg.Exclude.Tags) > 0 { + tags, err := FetchTags(ctx, s.client, inst.ARN) + if err != nil { + slog.Warn("Failed to fetch tags for tag-based exclusion", "instance", inst.ID, "error", err) + } else if cfg.Exclude.MatchesExcludedTags(tags) { + continue + } + } result.ResourcesScanned++ findings := s.analyzeInstance(ctx, cfg, inst) result.Findings = append(result.Findings, findings...) @@ -64,7 +75,8 @@ func (s *RDSScanner) Scan(ctx context.Context, cfg database.ScanConfig, progress } else { s.reportProgress(progress, fmt.Sprintf("Found %d manual snapshots", len(snapshots))) for _, snap := range snapshots { - if cfg.Exclude.ResourceIDs[snap.ID] { + // WO-9: use shared ExcludeConfig.IsExcluded helper instead of inline map lookup. + if cfg.Exclude.IsExcluded(snap.ID) { continue } result.ResourcesScanned++ @@ -277,12 +289,6 @@ func (s *RDSScanner) analyzeSnapshot(cfg database.ScanConfig, snap Snapshot) []d } func (s *RDSScanner) reportProgress(progress func(database.ScanProgress), msg string) { - if progress != nil { - progress(database.ScanProgress{ - Region: s.region, - Scanner: "rds", - Message: msg, - Timestamp: time.Now(), - }) - } + // WO-9: delegate to the shared database.ReportProgress helper. + database.ReportProgress(progress, "rds", s.region, msg) } diff --git a/internal/rds/scanner_test.go b/internal/rds/scanner_test.go index dc1055c..1a5388f 100644 --- a/internal/rds/scanner_test.go +++ b/internal/rds/scanner_test.go @@ -394,6 +394,59 @@ func TestScanExcludeInstance(t *testing.T) { } } +// WO-7: exercises tag-based exclusion. +func TestScanExcludeByTag(t *testing.T) { + mock := newMockRDSClient() + mock.instances = []rdstypes.DBInstance{ + makeInstance("tagged-db", "db.t3.small", "postgres", "17.2", func(i *rdstypes.DBInstance) { + i.DBInstanceArn = aws.String("arn:aws:rds:us-east-1:123456789012:db:tagged-db") + i.StorageEncrypted = aws.Bool(false) // would normally be flagged + }), + } + mock.tagsForARN["arn:aws:rds:us-east-1:123456789012:db:tagged-db"] = []rdstypes.Tag{ + {Key: aws.String("env"), Value: aws.String("temporary")}, + } + cw := newMockCWClient() + + cfg := defaultCfg() + cfg.Exclude.Tags = map[string]string{"env": "temporary"} + + s := newTestScanner(mock, cw) + result := s.Scan(context.Background(), cfg, nil) + + if len(result.Findings) != 0 { + t.Errorf("expected 0 findings for tag-excluded instance, got %d", len(result.Findings)) + } + if result.ResourcesScanned != 0 { + t.Errorf("ResourcesScanned = %d, want 0 (tag-excluded)", result.ResourcesScanned) + } +} + +// WO-7: exercises tag-based exclusion. +func TestScanTagExcludeNoMatch(t *testing.T) { + mock := newMockRDSClient() + mock.instances = []rdstypes.DBInstance{ + makeInstance("keep-db", "db.t3.small", "postgres", "17.2", func(i *rdstypes.DBInstance) { + i.DBInstanceArn = aws.String("arn:aws:rds:us-east-1:123456789012:db:keep-db") + i.StorageEncrypted = aws.Bool(false) + }), + } + mock.tagsForARN["arn:aws:rds:us-east-1:123456789012:db:keep-db"] = []rdstypes.Tag{ + {Key: aws.String("env"), Value: aws.String("production")}, + } + cw := newMockCWClient() + + cfg := defaultCfg() + cfg.Exclude.Tags = map[string]string{"env": "temporary"} + + s := newTestScanner(mock, cw) + result := s.Scan(context.Background(), cfg, nil) + + if len(findByID(result.Findings, database.FindingUnencryptedStorage)) != 1 { + t.Error("expected instance to still be flagged (tag does not match exclusion)") + } +} + func TestScanSkipsNonAvailableInstances(t *testing.T) { mock := newMockRDSClient() mock.instances = []rdstypes.DBInstance{ diff --git a/internal/report/report_test.go b/internal/report/report_test.go index dab8d51..768419b 100644 --- a/internal/report/report_test.go +++ b/internal/report/report_test.go @@ -146,6 +146,44 @@ func TestSARIFRulesCount(t *testing.T) { } } +// WO-10: exercises severity-descending sort. +func TestTextReporterSortsBySeverityDescending(t *testing.T) { + data := sampleData() + data.Findings = []database.Finding{ + {ID: database.FindingIdleInstance, Severity: database.SeverityLow, ResourceID: "db-low", Message: "low"}, + {ID: database.FindingIdleInstance, Severity: database.SeverityMedium, ResourceID: "db-medium", Message: "medium"}, + {ID: database.FindingIdleInstance, Severity: database.SeverityCritical, ResourceID: "db-critical", Message: "critical"}, + {ID: database.FindingIdleInstance, Severity: database.SeverityHigh, ResourceID: "db-high", Message: "high"}, + } + var buf bytes.Buffer + r := &TextReporter{Writer: &buf} + if err := r.Generate(data); err != nil { + t.Fatalf("Generate() error: %v", err) + } + out := buf.String() + positions := map[string]int{ + "db-critical": strings.Index(out, "db-critical"), + "db-high": strings.Index(out, "db-high"), + "db-medium": strings.Index(out, "db-medium"), + "db-low": strings.Index(out, "db-low"), + } + for id, pos := range positions { + if pos == -1 { + t.Fatalf("missing resource %s in output", id) + } + } + if positions["db-critical"] >= positions["db-high"] || + positions["db-high"] >= positions["db-medium"] || + positions["db-medium"] >= positions["db-low"] { + t.Errorf("findings not sorted severity-descending, got positions %+v", positions) + } + + // data.Findings must remain unmutated by Generate (no in-place sort of caller's slice). + if data.Findings[0].ResourceID != "db-low" { + t.Errorf("Generate() mutated caller's Findings slice; first element = %s, want db-low", data.Findings[0].ResourceID) + } +} + func TestTextReporterWithErrors(t *testing.T) { data := sampleData() data.Errors = []string{"region us-west-2 failed: timeout"} diff --git a/internal/report/text.go b/internal/report/text.go index d22928a..966c44c 100644 --- a/internal/report/text.go +++ b/internal/report/text.go @@ -5,8 +5,27 @@ import ( "io" "sort" "text/tabwriter" + + "github.com/ppiankov/rdsspectre/internal/database" ) +// WO-10: severityRank orders findings critical-first when rendering text output. +// Unranked severities (should not occur) sort last, after low. +var severityRank = map[database.Severity]int{ + database.SeverityCritical: 0, + database.SeverityHigh: 1, + database.SeverityMedium: 2, + database.SeverityLow: 3, +} + +// WO-10: whole-function severity lookup, new for the severity-descending sort. +func rankSeverity(s database.Severity) int { + if rank, ok := severityRank[s]; ok { + return rank + } + return len(severityRank) +} + // TextReporter outputs human-readable text. type TextReporter struct { Writer io.Writer @@ -25,8 +44,15 @@ func (r *TextReporter) Generate(data Data) error { return w.Flush() } + // WO-10: sort a clone by severity descending; never mutate the caller's slice. + sorted := make([]database.Finding, len(data.Findings)) + copy(sorted, data.Findings) + sort.SliceStable(sorted, func(i, j int) bool { + return rankSeverity(sorted[i].Severity) < rankSeverity(sorted[j].Severity) + }) + r.printf(w, "SEVERITY\tTYPE\tRESOURCE\tREGION\tWASTE/MO\tMESSAGE\n") - for _, f := range data.Findings { + for _, f := range sorted { r.printf(w, "%s\t%s\t%s\t%s\t$%.2f\t%s\n", f.Severity, f.ID, f.ResourceID, f.Region, f.EstimatedMonthlyWaste, f.Message) }