From 0e87ccf12e99aa832dae7b1c805f749c646022bc Mon Sep 17 00:00:00 2001 From: Karthik Unnikrishnan Date: Fri, 7 Aug 2026 16:26:13 +0000 Subject: [PATCH 01/10] feat: implemented TODO and guardrail harness --- pkg/migrate/helpers.go | 44 ++++++++++++++++ pkg/migrate/helpers_test.go | 100 ++++++++++++++++++++++++++++++++++++ pkg/migrate/logger.go | 10 ++-- pkg/migrate/migrate.go | 22 ++++---- pkg/migrate/migrate_test.go | 20 ++++---- 5 files changed, 170 insertions(+), 26 deletions(-) diff --git a/pkg/migrate/helpers.go b/pkg/migrate/helpers.go index eb9334e6c0..fdf53e5321 100644 --- a/pkg/migrate/helpers.go +++ b/pkg/migrate/helpers.go @@ -46,6 +46,12 @@ const ( labelAddress = "__address__" ) +const ( + AnnotationTodoPrefix = "gmp.googleapis.com/todo-" + GuardrailLabelKey = "gmp.googleapis.com/migration-review-required" + GuardrailLabelValue = "true" +) + // Constants representing the supported ScrapeProtocol enum values defined in upstream Prometheus Operator. const ( scrapeProtocolOpenMetricsText100 = pomonitoringv1.ScrapeProtocol("OpenMetricsText1.0.0") @@ -127,6 +133,44 @@ func CopyObjectMeta(src metav1.ObjectMeta, targetNamespace string, logger *slog. return dst } +// AddMigrationTodo appends a sequential TODO annotation to the unstructured resource. +func AddMigrationTodo(u *unstructured.Unstructured, category, reason, action string) { + if u == nil || u.Object == nil { + return + } + annotations := u.GetAnnotations() + if annotations == nil { + annotations = make(map[string]string) + } + + todoNumber := 1 + for { + key := fmt.Sprintf("%s%d", AnnotationTodoPrefix, todoNumber) + if _, exists := annotations[key]; !exists { + annotations[key] = fmt.Sprintf("[%s] %s ACTION: %s", category, reason, action) + break + } + todoNumber++ + } + u.SetAnnotations(annotations) +} + +// InjectSafetyGuardrail adds a non-matching label to spec.selector.matchLabels to prevent accidental target scraping. +func InjectSafetyGuardrail(u *unstructured.Unstructured) error { + if u == nil || u.Object == nil { + return errors.New("cannot inject guardrail into nil unstructured resource") + } + labelsMap, found, err := unstructured.NestedStringMap(u.Object, "spec", "selector", "matchLabels") + if err != nil { + return fmt.Errorf("failed to read spec.selector.matchLabels: %w", err) + } + if !found || labelsMap == nil { + labelsMap = make(map[string]string) + } + labelsMap[GuardrailLabelKey] = GuardrailLabelValue + return unstructured.SetNestedStringMap(u.Object, labelsMap, "spec", "selector", "matchLabels") +} + // parseAndCleanNamespaces trims whitespace, filters out empty strings, and deduplicates namespaces. func parseAndCleanNamespaces(namespaces []string) []string { unique := make(map[string]bool) diff --git a/pkg/migrate/helpers_test.go b/pkg/migrate/helpers_test.go index f91b6f9324..e1ff6b03a6 100644 --- a/pkg/migrate/helpers_test.go +++ b/pkg/migrate/helpers_test.go @@ -1314,3 +1314,103 @@ func TestMakeUniqueResourceName(t *testing.T) { }) } } + +func TestAddMigrationTodo(t *testing.T) { + tests := []struct { + name string + initialObj map[string]any + category string + reason string + action string + expectedMap map[string]string + }{ + { + name: "single todo with action", + initialObj: map[string]any{ + "metadata": map[string]any{}, + }, + category: "WARNING", + reason: "Dropped unsupported 'annotationMatches' selector.", + action: "Verify 'spec.selector.matchLabels' on target pods and remove guardrail label.", + expectedMap: map[string]string{ + "gmp.googleapis.com/todo-1": "[WARNING] Dropped unsupported 'annotationMatches' selector. ACTION: Verify 'spec.selector.matchLabels' on target pods and remove guardrail label.", + }, + }, + { + name: "sequential todos preserving existing annotations", + initialObj: map[string]any{ + "metadata": map[string]any{ + "annotations": map[string]any{ + "existing.io/key": "existing-val", + "gmp.googleapis.com/todo-1": "[WARNING] First todo. ACTION: Fix first item.", + }, + }, + }, + category: "ERROR", + reason: "Invalid proxy URL.", + action: "Move credentials to Secret.", + expectedMap: map[string]string{ + "existing.io/key": "existing-val", + "gmp.googleapis.com/todo-1": "[WARNING] First todo. ACTION: Fix first item.", + "gmp.googleapis.com/todo-2": "[ERROR] Invalid proxy URL. ACTION: Move credentials to Secret.", + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + u := &unstructured.Unstructured{Object: tc.initialObj} + AddMigrationTodo(u, tc.category, tc.reason, tc.action) + if diff := cmp.Diff(tc.expectedMap, u.GetAnnotations()); diff != "" { + t.Errorf("AddMigrationTodo() mismatch (-want +got):\n%s", diff) + } + }) + } +} + +func TestInjectSafetyGuardrail(t *testing.T) { + tests := []struct { + name string + initialObj map[string]any + expectedMatch map[string]string + }{ + { + name: "inject into empty matchLabels", + initialObj: map[string]any{ + "spec": map[string]any{}, + }, + expectedMatch: map[string]string{ + "gmp.googleapis.com/migration-review-required": "true", + }, + }, + { + name: "preserve existing matchLabels", + initialObj: map[string]any{ + "spec": map[string]any{ + "selector": map[string]any{ + "matchLabels": map[string]any{ + "app": "frontend", + }, + }, + }, + }, + expectedMatch: map[string]string{ + "app": "frontend", + "gmp.googleapis.com/migration-review-required": "true", + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + u := &unstructured.Unstructured{Object: tc.initialObj} + if err := InjectSafetyGuardrail(u); err != nil { + t.Fatalf("InjectSafetyGuardrail() unexpected error: %v", err) + } + gotMatch, _, _ := unstructured.NestedStringMap(u.Object, "spec", "selector", "matchLabels") + if diff := cmp.Diff(tc.expectedMatch, gotMatch); diff != "" { + t.Errorf("InjectSafetyGuardrail() mismatch (-want +got):\n%s", diff) + } + }) + } +} diff --git a/pkg/migrate/logger.go b/pkg/migrate/logger.go index bcd2b1527b..47304e9b21 100644 --- a/pkg/migrate/logger.go +++ b/pkg/migrate/logger.go @@ -29,15 +29,15 @@ import ( type ResourceStatus int const ( - StatusSuccess ResourceStatus = iota // 0 (Migrated Successfully). - StatusSkipped // 1 (Skipped / Unsupported). - StatusWarning // 2 (Migrated with Warnings). - StatusFailed // 3 (Failed). + StatusSuccess ResourceStatus = iota // 0 (Migrated Successfully). + StatusSkipped // 1 (Skipped / Unsupported). + StatusActionItems // 2 (Migrated with Action Items). + StatusFailed // 3 (Failed). ) // statusLevels maps slog.Levels to their corresponding ResourceStatus. var statusLevels = map[slog.Level]ResourceStatus{ - slog.LevelWarn: StatusWarning, + slog.LevelWarn: StatusActionItems, slog.LevelError: StatusFailed, } diff --git a/pkg/migrate/migrate.go b/pkg/migrate/migrate.go index 4d19f340a8..3edb961ef1 100644 --- a/pkg/migrate/migrate.go +++ b/pkg/migrate/migrate.go @@ -37,11 +37,11 @@ import ( // MigrationReport accumulates the statistics and payloads of the migration run. type MigrationReport struct { - SuccessCount int // Successfully migrated with no warnings. - WarningCount int // Successfully migrated but had warnings. - SkippedCount int // Bypassed because resource is unsupported/out-of-scope. - FailedCount int // Fatal failure, resource skipped. - Outputs []*unstructured.Unstructured // Converted GMP manifests in-memory. + SuccessCount int // Successfully migrated with no action items. + ActionItemsCount int // Successfully migrated but had TODO annotations or guardrails. + SkippedCount int // Bypassed because resource is unsupported/out-of-scope. + FailedCount int // Fatal failure, resource skipped. + Outputs []*unstructured.Unstructured // Converted GMP manifests in-memory. } // Migrator orchestrates the migration process. @@ -127,8 +127,8 @@ func (m *Migrator) Run(inputPaths ...string) (*MigrationReport, error) { report.SuccessCount++ case StatusSkipped: report.SkippedCount++ - case StatusWarning: - report.WarningCount++ + case StatusActionItems: + report.ActionItemsCount++ case StatusFailed: report.FailedCount++ } @@ -141,10 +141,10 @@ func (m *Migrator) Run(inputPaths ...string) (*MigrationReport, error) { func (m *Migrator) PrintSummary(r *MigrationReport) { fmt.Fprintln(m.Stderr, "\n=========================================") fmt.Fprintln(m.Stderr, "Migration Complete Summary:") - fmt.Fprintf(m.Stderr, " Successfully Migrated: %d\n", r.SuccessCount) - fmt.Fprintf(m.Stderr, " Migrated with Warnings: %d\n", r.WarningCount) - fmt.Fprintf(m.Stderr, " Skipped (Unsupported): %d\n", r.SkippedCount) - fmt.Fprintf(m.Stderr, " Failed: %d\n", r.FailedCount) + fmt.Fprintf(m.Stderr, " Successfully Migrated: %d\n", r.SuccessCount) + fmt.Fprintf(m.Stderr, " Migrated with Action Items: %d\n", r.ActionItemsCount) + fmt.Fprintf(m.Stderr, " Skipped (Unsupported): %d\n", r.SkippedCount) + fmt.Fprintf(m.Stderr, " Failed: %d\n", r.FailedCount) fmt.Fprintln(m.Stderr, "=========================================") } diff --git a/pkg/migrate/migrate_test.go b/pkg/migrate/migrate_test.go index 78db6829eb..fc0321b54d 100644 --- a/pkg/migrate/migrate_test.go +++ b/pkg/migrate/migrate_test.go @@ -103,8 +103,8 @@ spec: if report.SuccessCount != 1 { t.Errorf("expected SuccessCount to be 1, got %d", report.SuccessCount) } - if report.WarningCount != 0 { - t.Errorf("expected WarningCount to be 0, got %d", report.WarningCount) + if report.ActionItemsCount != 0 { + t.Errorf("expected ActionItemsCount to be 0, got %d", report.ActionItemsCount) } if report.SkippedCount != 0 { t.Errorf("expected SkippedCount to be 0, got %d", report.SkippedCount) @@ -210,8 +210,8 @@ spec: if report.SuccessCount != 0 { t.Errorf("expected SuccessCount to be 0, got %d", report.SuccessCount) } - if report.WarningCount != 0 { - t.Errorf("expected WarningCount to be 0, got %d", report.WarningCount) + if report.ActionItemsCount != 0 { + t.Errorf("expected ActionItemsCount to be 0, got %d", report.ActionItemsCount) } if report.SkippedCount != 0 { t.Errorf("expected SkippedCount to be 0, got %d", report.SkippedCount) @@ -263,8 +263,8 @@ spec: if report.SuccessCount != 0 { t.Errorf("expected SuccessCount to be 0, got %d", report.SuccessCount) } - if report.WarningCount != 0 { - t.Errorf("expected WarningCount to be 0, got %d", report.WarningCount) + if report.ActionItemsCount != 0 { + t.Errorf("expected ActionItemsCount to be 0, got %d", report.ActionItemsCount) } if report.FailedCount != 0 { t.Errorf("expected FailedCount to be 0, got %d", report.FailedCount) @@ -333,8 +333,8 @@ spec: if report.SuccessCount != 1 { t.Errorf("expected SuccessCount to be 1, got %d", report.SuccessCount) } - if report.WarningCount != 0 { - t.Errorf("expected WarningCount to be 0, got %d", report.WarningCount) + if report.ActionItemsCount != 0 { + t.Errorf("expected ActionItemsCount to be 0, got %d", report.ActionItemsCount) } if report.SkippedCount != 0 { t.Errorf("expected SkippedCount to be 0, got %d", report.SkippedCount) @@ -399,8 +399,8 @@ items: if report.SuccessCount != 1 { t.Errorf("expected SuccessCount to be 1, got %d", report.SuccessCount) } - if report.WarningCount != 0 { - t.Errorf("expected WarningCount to be 0, got %d", report.WarningCount) + if report.ActionItemsCount != 0 { + t.Errorf("expected ActionItemsCount to be 0, got %d", report.ActionItemsCount) } if report.SkippedCount != 0 { t.Errorf("expected SkippedCount to be 0, got %d", report.SkippedCount) From e4030acf5073bbbdcaacee1c08fc718554328482 Mon Sep 17 00:00:00 2001 From: Karthik Unnikrishnan Date: Fri, 7 Aug 2026 17:54:59 +0000 Subject: [PATCH 02/10] feat: inject TODO annotations on scope expansion --- pkg/migrate/helpers.go | 66 +++++++++++++---- pkg/migrate/podmonitor.go | 14 ++++ pkg/migrate/podmonitor_test.go | 131 ++++++++++++++++++++++++++++++++- pkg/migrate/servicemonitor.go | 14 ++++ 4 files changed, 209 insertions(+), 16 deletions(-) diff --git a/pkg/migrate/helpers.go b/pkg/migrate/helpers.go index fdf53e5321..e41abb9871 100644 --- a/pkg/migrate/helpers.go +++ b/pkg/migrate/helpers.go @@ -101,6 +101,7 @@ type preScrapeRelabelingResult struct { MatchLabels map[string]string MatchExpressions []metav1.LabelSelectorRequirement PromotedRules []monitoringv1.RelabelingRule + Todos []todoItem } // ExtractedPreScrapeRules holds all translated rules, separated by where they belong in GMP. @@ -200,6 +201,13 @@ func determineNamespaceScoping(nsSel pomonitoringv1.NamespaceSelector, defaultNS return []string{defaultNS}, false, nil } +// todoItem represents an actionable TODO annotation to attach to generated resources. +type todoItem struct { + category string + reason string + action string +} + // commonMonitorSpec holds common fields extracted from Prometheus Operator monitor specs for building GMP resources. type commonMonitorSpec struct { endpoints []monitoringv1.ScrapeEndpoint @@ -209,6 +217,7 @@ type commonMonitorSpec struct { filterRunning *bool limits *monitoringv1.ScrapeLimits generatedSecrets []*unstructured.Unstructured + todos []todoItem } // conversionContext groups common parameters passed down to conversion helper functions. @@ -268,7 +277,15 @@ func convertPreScrapeRelabelings(logger *slog.Logger, configs []pomonitoringv1.R action = relabel.Replace } - if shouldSkipRelabelConfig(logger, config, action) { + if skip, scopeExpanded, details := shouldSkipRelabelConfig(logger, config, action); skip { + if scopeExpanded { + logger.Warn("Scraping scope expanded: targets previously excluded by this rule will now be scraped. Adjust pod selectors to compensate.") + res.Todos = append(res.Todos, todoItem{ + category: "WARNING", + reason: fmt.Sprintf("Dropped target filtering rule (%s).", details), + action: "Add equivalent pod label selector in 'spec.selector.matchLabels'.", + }) + } continue } @@ -347,6 +364,7 @@ func extractPreScrapeRelabelings(logger *slog.Logger, endpointsRelabelConfigs [] if r.Metadata != nil { rawMetadata = append(rawMetadata, *r.Metadata...) } + combined.Todos = append(combined.Todos, r.Todos...) } epResults = append(epResults, r) } @@ -781,7 +799,7 @@ func convertMetricRelabelings( action = relabel.Replace } - if shouldSkipRelabelConfig(logger, config, action) { + if skip, _, _ := shouldSkipRelabelConfig(logger, config, action); skip { continue } @@ -875,34 +893,38 @@ func convertTargetLabels(logger *slog.Logger, sourceLabels []string, jobLabel st } // shouldSkipRelabelConfig checks if the relabel config uses unsupported actions or references annotations. -func shouldSkipRelabelConfig(logger *slog.Logger, config pomonitoringv1.RelabelConfig, action relabel.Action) bool { +func shouldSkipRelabelConfig(logger *slog.Logger, config pomonitoringv1.RelabelConfig, action relabel.Action) (skip bool, scopeExpanded bool, dropDetails string) { switch action { case relabel.Replace, relabel.HashMod, "": if config.TargetLabel == "" { logger.Warn(fmt.Sprintf("Relabeling rule uses 'action: %s' with an empty 'targetLabel', which is invalid in Prometheus and has been dropped.", action)) - return true + return true, false, "" } case relabel.Keep, relabel.Drop, relabel.LabelDrop, relabel.LabelKeep: // Supported actions that do not require targetLabel. case relabel.LabelMap, relabel.Lowercase, relabel.Uppercase, relabel.KeepEqual, relabel.DropEqual: logger.Warn(fmt.Sprintf("Relabeling rule uses 'action: %s' which is not supported by GMP and has been dropped.", action)) - return true + return true, false, "" default: logger.Warn(fmt.Sprintf("Relabeling rule uses unknown 'action: %s' which is not supported by GMP and has been dropped.", action)) - return true + return true, false, "" } for _, sl := range config.SourceLabels { - if strings.HasPrefix(string(sl), "__meta_kubernetes_pod_annotation_") { - logger.Warn(fmt.Sprintf("Relabeling rule referencing pod annotation %q is unsupported in GMP. The rule has been dropped.", string(sl))) - return true + s := string(sl) + if strings.HasPrefix(s, "__meta_kubernetes_pod_annotation_") { + logger.Warn(fmt.Sprintf("Relabeling rule referencing pod annotation %q is unsupported in GMP. The rule has been dropped.", s)) + if action == relabel.Keep || action == relabel.Drop { + return true, true, fmt.Sprintf("'%s' on '%s'", action, s) + } + return true, false, "" } - if strings.HasPrefix(string(sl), "__meta_kubernetes_node_") && string(sl) != "__meta_kubernetes_node_name" { - logger.Warn(fmt.Sprintf("Relabeling rule referencing node metadata %q is unsupported in GMP (only node name is supported). The rule has been dropped.", string(sl))) - return true + if strings.HasPrefix(s, "__meta_kubernetes_node_") && s != "__meta_kubernetes_node_name" { + logger.Warn(fmt.Sprintf("Relabeling rule referencing node metadata %q is unsupported in GMP (only node name is supported). The rule has been dropped.", s)) + return true, false, "" } } - return false + return false, false, "" } // resolveSourceLabels resolves source labels to pod labels, metadata labels, and rewritten labels. @@ -1162,6 +1184,15 @@ func buildPodMonitoring( u.SetAPIVersion(GMPAPIVersion) u.SetKind(KindPodMonitoring) + for _, td := range spec.todos { + AddMigrationTodo(u, td.category, td.reason, td.action) + } + if len(spec.todos) > 0 { + if err := InjectSafetyGuardrail(u); err != nil { + return nil, err + } + } + return u, nil } @@ -1199,6 +1230,15 @@ func buildClusterPodMonitoring( u.SetAPIVersion(GMPAPIVersion) u.SetKind(KindClusterPodMonitoring) + for _, td := range spec.todos { + AddMigrationTodo(u, td.category, td.reason, td.action) + } + if len(spec.todos) > 0 { + if err := InjectSafetyGuardrail(u); err != nil { + return nil, err + } + } + return u, nil } diff --git a/pkg/migrate/podmonitor.go b/pkg/migrate/podmonitor.go index 48c34097d1..12d7e56558 100644 --- a/pkg/migrate/podmonitor.go +++ b/pkg/migrate/podmonitor.go @@ -186,11 +186,24 @@ func (c *PodMonitorConverter) convertMonitorSpec(pm *pomonitoringv1.PodMonitor, if err != nil { return nil, err } + var todos []todoItem + todos = append(todos, rules.ResourceCombined.Todos...) + if len(mergedSelector.MatchLabels) == 0 && len(mergedSelector.MatchExpressions) == 0 { if isCluster { logger.Warn("Resulting ClusterPodMonitoring selector is empty. It will select and scrape all pods across all namespaces. Verify if this is intended.") + todos = append(todos, todoItem{ + category: "WARNING", + reason: "Resulting ClusterPodMonitoring selector is empty and matches all pods across all namespaces.", + action: "Define explicit 'matchLabels' in 'spec.selector'.", + }) } else { logger.Warn("Resulting PodMonitoring selector is empty. It will select and scrape all pods in this namespace. Verify if this is intended.") + todos = append(todos, todoItem{ + category: "WARNING", + reason: "Resulting PodMonitoring selector is empty and matches all pods in this namespace.", + action: "Define explicit 'matchLabels' in 'spec.selector'.", + }) } } @@ -217,6 +230,7 @@ func (c *PodMonitorConverter) convertMonitorSpec(pm *pomonitoringv1.PodMonitor, filterRunning: filterRunning, limits: limits, generatedSecrets: convCtx.getGeneratedSecrets(), + todos: todos, }, nil } diff --git a/pkg/migrate/podmonitor_test.go b/pkg/migrate/podmonitor_test.go index 2a9f0ba12b..7d92b56f5c 100644 --- a/pkg/migrate/podmonitor_test.go +++ b/pkg/migrate/podmonitor_test.go @@ -414,6 +414,9 @@ func TestPodMonitorConversion(t *testing.T) { Namespace: "frontend", }, Spec: pomonitoringv1.PodMonitorSpec{ + Selector: metav1.LabelSelector{ + MatchLabels: map[string]string{"app": "label-app"}, + }, JobLabel: "app-name", PodTargetLabels: []string{"env", "instance", "version"}, PodMetricsEndpoints: []pomonitoringv1.PodMetricsEndpoint{ @@ -434,6 +437,9 @@ func TestPodMonitorConversion(t *testing.T) { Namespace: "frontend", }, Spec: monitoringv1.PodMonitoringSpec{ + Selector: metav1.LabelSelector{ + MatchLabels: map[string]string{"app": "label-app"}, + }, Endpoints: []monitoringv1.ScrapeEndpoint{ { Port: intstr.FromString("metrics"), @@ -1199,10 +1205,21 @@ func TestPodMonitorConversion(t *testing.T) { }, expected: []runtime.Object{ &monitoringv1.PodMonitoring{ - TypeMeta: BuildTypeMeta(KindPodMonitoring), - ObjectMeta: metav1.ObjectMeta{Name: "annotation-keep-monitor", Namespace: "default"}, + TypeMeta: BuildTypeMeta(KindPodMonitoring), + ObjectMeta: metav1.ObjectMeta{ + Name: "annotation-keep-monitor", + Namespace: "default", + Annotations: map[string]string{ + "gmp.googleapis.com/todo-1": "[WARNING] Dropped target filtering rule ('keep' on '__meta_kubernetes_pod_annotation_prometheus_io_scrape'). ACTION: Add equivalent pod label selector in 'spec.selector.matchLabels'.", + "gmp.googleapis.com/todo-2": "[WARNING] Resulting PodMonitoring selector is empty and matches all pods in this namespace. ACTION: Define explicit 'matchLabels' in 'spec.selector'.", + }, + }, Spec: monitoringv1.PodMonitoringSpec{ - Selector: metav1.LabelSelector{}, // Remains empty, selecting all pods. + Selector: metav1.LabelSelector{ + MatchLabels: map[string]string{ + "gmp.googleapis.com/migration-review-required": "true", + }, + }, Endpoints: []monitoringv1.ScrapeEndpoint{ { Port: intstr.FromString("metrics"), @@ -2232,6 +2249,114 @@ func TestPodMonitorConversion(t *testing.T) { "Endpoint-level configuration conflict detected", }, }, + { + name: "PodMonitor with dropped annotation relabeling injects guardrail label and TODO annotation", + input: &pomonitoringv1.PodMonitor{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "monitoring.coreos.com/v1", + Kind: KindPodMonitor, + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "annotated-relabel-monitor", + Namespace: "default", + }, + Spec: pomonitoringv1.PodMonitorSpec{ + Selector: metav1.LabelSelector{ + MatchLabels: map[string]string{"app": "annotated-app"}, + }, + PodMetricsEndpoints: []pomonitoringv1.PodMetricsEndpoint{ + { + Port: "web", + RelabelConfigs: []pomonitoringv1.RelabelConfig{ + { + Action: "keep", + SourceLabels: []pomonitoringv1.LabelName{"__meta_kubernetes_pod_annotation_prometheus_io_scrape"}, + Regex: "true", + }, + }, + }, + }, + }, + }, + expected: []runtime.Object{ + &monitoringv1.PodMonitoring{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "monitoring.googleapis.com/v1", + Kind: KindPodMonitoring, + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "annotated-relabel-monitor", + Namespace: "default", + Annotations: map[string]string{ + "gmp.googleapis.com/todo-1": "[WARNING] Dropped target filtering rule ('keep' on '__meta_kubernetes_pod_annotation_prometheus_io_scrape'). ACTION: Add equivalent pod label selector in 'spec.selector.matchLabels'.", + }, + }, + Spec: monitoringv1.PodMonitoringSpec{ + Selector: metav1.LabelSelector{ + MatchLabels: map[string]string{ + "app": "annotated-app", + "gmp.googleapis.com/migration-review-required": "true", + }, + }, + Endpoints: []monitoringv1.ScrapeEndpoint{ + { + Port: intstr.FromString("web"), + Interval: "30s", + }, + }, + }, + }, + }, + }, + { + name: "PodMonitor with empty selector injects guardrail label and TODO annotation", + input: &pomonitoringv1.PodMonitor{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "monitoring.coreos.com/v1", + Kind: KindPodMonitor, + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "empty-selector-monitor", + Namespace: "default", + }, + Spec: pomonitoringv1.PodMonitorSpec{ + Selector: metav1.LabelSelector{}, + PodMetricsEndpoints: []pomonitoringv1.PodMetricsEndpoint{ + { + Port: "web", + }, + }, + }, + }, + expected: []runtime.Object{ + &monitoringv1.PodMonitoring{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "monitoring.googleapis.com/v1", + Kind: KindPodMonitoring, + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "empty-selector-monitor", + Namespace: "default", + Annotations: map[string]string{ + "gmp.googleapis.com/todo-1": "[WARNING] Resulting PodMonitoring selector is empty and matches all pods in this namespace. ACTION: Define explicit 'matchLabels' in 'spec.selector'.", + }, + }, + Spec: monitoringv1.PodMonitoringSpec{ + Selector: metav1.LabelSelector{ + MatchLabels: map[string]string{ + "gmp.googleapis.com/migration-review-required": "true", + }, + }, + Endpoints: []monitoringv1.ScrapeEndpoint{ + { + Port: intstr.FromString("web"), + Interval: "30s", + }, + }, + }, + }, + }, + }, } converter := &PodMonitorConverter{} diff --git a/pkg/migrate/servicemonitor.go b/pkg/migrate/servicemonitor.go index c6431a1fa4..a3b3422a9f 100644 --- a/pkg/migrate/servicemonitor.go +++ b/pkg/migrate/servicemonitor.go @@ -280,11 +280,24 @@ func (c *ServiceMonitorConverter) buildSpecForGroup( if err != nil { return nil, err } + var todos []todoItem + todos = append(todos, rules.ResourceCombined.Todos...) + if len(mergedSelector.MatchLabels) == 0 && len(mergedSelector.MatchExpressions) == 0 { if isClusterScoped { logger.Warn("Resulting ClusterPodMonitoring selector is empty. It will select and scrape all pods across all namespaces. Verify if this is intended.") + todos = append(todos, todoItem{ + category: "WARNING", + reason: "Resulting ClusterPodMonitoring selector is empty and matches all pods across all namespaces.", + action: "Define explicit 'matchLabels' in 'spec.selector'.", + }) } else { logger.Warn("Resulting PodMonitoring selector is empty. It will select and scrape all pods in this namespace. Verify if this is intended.") + todos = append(todos, todoItem{ + category: "WARNING", + reason: "Resulting PodMonitoring selector is empty and matches all pods in this namespace.", + action: "Define explicit 'matchLabels' in 'spec.selector'.", + }) } } @@ -311,6 +324,7 @@ func (c *ServiceMonitorConverter) buildSpecForGroup( filterRunning: filterRunning, limits: limits, generatedSecrets: convCtx.getGeneratedSecrets(), + todos: todos, }, nil } From 5e65b36328b245a6cb693de413eea0c831b5e58d Mon Sep 17 00:00:00 2001 From: Karthik Unnikrishnan Date: Fri, 7 Aug 2026 19:55:43 +0000 Subject: [PATCH 03/10] feat: add draft generation with todo annotations for recoverable errors --- pkg/migrate/helpers.go | 216 ++++++++++++----- pkg/migrate/helpers_test.go | 359 +++++++++++++++++++---------- pkg/migrate/migrate.go | 6 + pkg/migrate/migrate_test.go | 45 ++++ pkg/migrate/podmonitor.go | 21 +- pkg/migrate/podmonitor_test.go | 269 ++++++++++++++++++++- pkg/migrate/servicemonitor.go | 88 +++++-- pkg/migrate/servicemonitor_test.go | 60 ++++- 8 files changed, 841 insertions(+), 223 deletions(-) diff --git a/pkg/migrate/helpers.go b/pkg/migrate/helpers.go index e41abb9871..5bcbad3964 100644 --- a/pkg/migrate/helpers.go +++ b/pkg/migrate/helpers.go @@ -20,6 +20,7 @@ import ( "fmt" "hash/fnv" "log/slog" + "net/url" "slices" "strings" @@ -233,6 +234,7 @@ type conversionContext struct { generatedSecrets map[string]*unstructured.Unstructured // isClusterScoped indicates if the target resource is cluster-scoped (ClusterPodMonitoring). isClusterScoped bool + todos []todoItem } // getGeneratedSecrets returns the generated secrets accumulated in the context as a slice. @@ -389,9 +391,9 @@ func extractPreScrapeRelabelings(logger *slog.Logger, endpointsRelabelConfigs [] } // mergeLabelSelector combines base selector requirements with extracted pre-scrape filtering rules. -func mergeLabelSelector(base metav1.LabelSelector, extraLabels map[string]string, extraExprs []metav1.LabelSelectorRequirement) (metav1.LabelSelector, error) { +func (c *conversionContext) mergeLabelSelector(base metav1.LabelSelector, extraLabels map[string]string, extraExprs []metav1.LabelSelectorRequirement) metav1.LabelSelector { if len(extraLabels) == 0 && len(extraExprs) == 0 { - return *base.DeepCopy(), nil + return *base.DeepCopy() } res := base.DeepCopy() if len(extraLabels) > 0 && res.MatchLabels == nil { @@ -399,12 +401,17 @@ func mergeLabelSelector(base metav1.LabelSelector, extraLabels map[string]string } for k, v := range extraLabels { if existing, exists := res.MatchLabels[k]; exists && existing != v { - return metav1.LabelSelector{}, fmt.Errorf("selector conflict: label %q has conflicting values %q (base selector) and %q (relabeling rule)", k, existing, v) + c.todos = append(c.todos, todoItem{ + category: "ERROR", + reason: fmt.Sprintf("Selector conflict: label %q has conflicting values %q (from base selector) and %q (from relabeling rule).", k, existing, v), + action: fmt.Sprintf("Reconcile 'spec.selector.matchLabels' for label %q with the intended target pods.", k), + }) + continue } res.MatchLabels[k] = v } res.MatchExpressions = append(res.MatchExpressions, extraExprs...) - return *res, nil + return *res } // mergeFromPod merges target label mappings and deduplicates by target label name. @@ -445,29 +452,43 @@ func mergeFromPod(logger *slog.Logger, base []monitoringv1.LabelMapping, extra [ } // extractResourceKey is a consolidated helper that fetches a key from a ConfigMap or Secret. -// It returns an error if the reference is malformed, missing, or corrupt. -func (c *conversionContext) extractResourceKey(kind, name, key string) (string, error) { +func (c *conversionContext) extractResourceKey(kind, name, key string) string { kindUpper := strings.ToUpper(kind) if name == "" && key == "" { - return "", nil + return "" } if name == "" { - return "", fmt.Errorf("%s reference has an empty name for key %q", kindUpper, key) + c.todos = append(c.todos, todoItem{ + category: "ERROR", + reason: fmt.Sprintf("Referenced %s has an empty name for key %q.", kindUpper, key), + action: fmt.Sprintf("Specify a valid %s name in the configuration.", kindUpper), + }) + return fmt.Sprintf("TODO_SET_%s_FROM_%s_EMPTY_NAME", strings.ToUpper(key), kindUpper) } if key == "" { - return "", fmt.Errorf("%s reference has an empty key for name %q", kindUpper, name) + c.todos = append(c.todos, todoItem{ + category: "ERROR", + reason: fmt.Sprintf("Referenced %s %q has an empty key.", kindUpper, name), + action: fmt.Sprintf("Specify a valid key in %s %q.", kindUpper, name), + }) + return fmt.Sprintf("TODO_SET_EMPTY_KEY_FROM_%s_%s", kindUpper, strings.ToUpper(name)) } obj, ok := c.cache.Get(kind, c.sourceNamespace, name) if !ok { - return "", fmt.Errorf("%s %q for key %q not found in namespace %q", kind, name, key, c.sourceNamespace) + c.todos = append(c.todos, todoItem{ + category: "ERROR", + reason: fmt.Sprintf("Referenced %s %q for key %q was not found in migration inputs.", kindUpper, name, key), + action: fmt.Sprintf("Verify %s %q exists in namespace %q or provide it in migration inputs.", kindUpper, name, c.sourceNamespace), + }) + return fmt.Sprintf("TODO_SET_%s_FROM_%s_%s", strings.ToUpper(key), kindUpper, strings.ToUpper(name)) } // Secrets support unencoded stringData. if kind == KindSecret { val, found, _ := unstructured.NestedString(obj.Object, "stringData", key) if found { - return val, nil + return val } } @@ -477,11 +498,16 @@ func (c *conversionContext) extractResourceKey(kind, name, key string) (string, if kind == KindSecret { decoded, err := base64.StdEncoding.DecodeString(strings.TrimSpace(val)) if err != nil { - return "", fmt.Errorf("failed to decode base64 data for key %q in secret %q: %w", key, name, err) + c.todos = append(c.todos, todoItem{ + category: "ERROR", + reason: fmt.Sprintf("Failed to base64-decode key %q in Secret %q.", key, name), + action: fmt.Sprintf("Ensure Secret %q contains valid base64 data (or use stringData).", name), + }) + return fmt.Sprintf("TODO_CORRUPT_SECRET_DATA_%s", strings.ToUpper(key)) } - return string(decoded), nil + return string(decoded) } - return val, nil + return val } // ConfigMaps can store base64 binaryData. @@ -490,29 +516,37 @@ func (c *conversionContext) extractResourceKey(kind, name, key string) (string, if found { decoded, err := base64.StdEncoding.DecodeString(strings.TrimSpace(val)) if err != nil { - return "", fmt.Errorf("failed to decode base64 binaryData for key %q in configmap %q: %w", key, name, err) + c.todos = append(c.todos, todoItem{ + category: "ERROR", + reason: fmt.Sprintf("Failed to base64-decode key %q in ConfigMap %q.", key, name), + action: fmt.Sprintf("Ensure ConfigMap %q contains valid base64 binaryData.", name), + }) + return fmt.Sprintf("TODO_CORRUPT_CONFIGMAP_DATA_%s", strings.ToUpper(key)) } - return string(decoded), nil + return string(decoded) } } - return "", fmt.Errorf("key %q not found in %s %q", key, kindUpper, name) + c.todos = append(c.todos, todoItem{ + category: "ERROR", + reason: fmt.Sprintf("Key %q was not found in referenced %s %q.", key, kindUpper, name), + action: fmt.Sprintf("Add the %q key to %s %q.", key, kindUpper, name), + }) + return fmt.Sprintf("TODO_MISSING_KEY_%s_IN_%s_%s", strings.ToUpper(key), kindUpper, strings.ToUpper(name)) } // extractSecretKey extracts a string value from a Secret. -// It returns an error if the reference or data is malformed, or a placeholder if the resource is not found. -func (c *conversionContext) extractSecretKey(sel corev1.SecretKeySelector) (string, error) { +func (c *conversionContext) extractSecretKey(sel corev1.SecretKeySelector) string { if sel.Name == "" && sel.Key == "" { - return "", nil + return "" } return c.extractResourceKey(KindSecret, sel.Name, sel.Key) } // extractConfigMapKey extracts a string value from a ConfigMap. -// It returns an error if the reference or data is malformed, or a placeholder if the resource is not found. -func (c *conversionContext) extractConfigMapKey(sel corev1.ConfigMapKeySelector) (string, error) { +func (c *conversionContext) extractConfigMapKey(sel corev1.ConfigMapKeySelector) string { if sel.Name == "" && sel.Key == "" { - return "", nil + return "" } return c.extractResourceKey(KindConfigMap, sel.Name, sel.Key) } @@ -523,15 +557,27 @@ func (c *conversionContext) convertConfigMapToSecretSelector(sel *corev1.ConfigM if sel == nil || (sel.Name == "" && sel.Key == "") { return nil, nil } - if sel.Name == "" { - return nil, fmt.Errorf("configmap reference has an empty name for key %q", sel.Key) + name := sel.Name + key := sel.Key + if name == "" { + c.todos = append(c.todos, todoItem{ + category: "ERROR", + reason: fmt.Sprintf("Referenced ConfigMap has an empty name for key %q.", key), + action: "Specify a valid ConfigMap name in the configuration.", + }) + name = "TODO_SET_CONFIGMAP_NAME" } - if sel.Key == "" { - return nil, fmt.Errorf("configmap reference has an empty key for name %q", sel.Name) + if key == "" { + c.todos = append(c.todos, todoItem{ + category: "ERROR", + reason: fmt.Sprintf("Referenced ConfigMap %q has an empty key.", name), + action: fmt.Sprintf("Specify a valid key in ConfigMap %q.", name), + }) + key = "TODO_SET_CONFIGMAP_KEY" } - secretName := "secret-" + sel.Name - secretKey := sel.Key + secretName := "secret-" + name + secretKey := key if sel.Optional != nil && *sel.Optional { c.logger.Warn("ConfigMap reference had 'optional: true'. GMP does not support optional secrets. The reference is now mandatory.", @@ -599,21 +645,33 @@ func (c *conversionContext) convertSecretSelector(sel *corev1.SecretKeySelector) if sel == nil || (sel.Name == "" && sel.Key == "") { return nil, nil } - if sel.Name == "" { - return nil, fmt.Errorf("secret reference has an empty name for key %q", sel.Key) + name := sel.Name + key := sel.Key + if name == "" { + c.todos = append(c.todos, todoItem{ + category: "ERROR", + reason: fmt.Sprintf("Referenced Secret has an empty name for key %q.", key), + action: "Specify a valid Secret name in the configuration.", + }) + name = "TODO_SET_SECRET_NAME" } - if sel.Key == "" { - return nil, fmt.Errorf("secret reference has an empty key for name %q", sel.Name) + if key == "" { + c.todos = append(c.todos, todoItem{ + category: "ERROR", + reason: fmt.Sprintf("Referenced Secret %q has an empty key.", name), + action: fmt.Sprintf("Specify a valid key in Secret %q.", name), + }) + key = "TODO_SET_SECRET_KEY" } if sel.Optional != nil && *sel.Optional { c.logger.Warn("Secret reference had 'optional: true'. GMP does not support optional secrets. The reference is now mandatory.", - slog.String("secret", sel.Name)) + slog.String("secret", name)) } var ns string if c.isClusterScoped { ns = c.targetNamespace } - secretRef := &monitoringv1.SecretKeySelector{Name: sel.Name, Key: sel.Key, Namespace: ns} + secretRef := &monitoringv1.SecretKeySelector{Name: name, Key: key, Namespace: ns} return &monitoringv1.SecretSelector{Secret: secretRef}, nil } @@ -623,10 +681,7 @@ func (c *conversionContext) convertBasicAuth(ba *pomonitoringv1.BasicAuth) (*mon if ba == nil { return nil, nil } - username, err := c.extractSecretKey(ba.Username) - if err != nil { - return nil, err - } + username := c.extractSecretKey(ba.Username) password, err := c.convertSecretSelector(&ba.Password) if err != nil { return nil, err @@ -681,16 +736,17 @@ func (c *conversionContext) convertOAuth2(oa *pomonitoringv1.OAuth2) (*monitorin return nil, nil } clientID := "" - var err error if oa.ClientID.Secret != nil { - clientID, err = c.extractSecretKey(*oa.ClientID.Secret) + clientID = c.extractSecretKey(*oa.ClientID.Secret) } else if oa.ClientID.ConfigMap != nil { - clientID, err = c.extractConfigMapKey(*oa.ClientID.ConfigMap) + clientID = c.extractConfigMapKey(*oa.ClientID.ConfigMap) } else { - return nil, errors.New("OAuth2 clientID must be defined as either Secret or ConfigMap") - } - if err != nil { - return nil, err + c.todos = append(c.todos, todoItem{ + category: "ERROR", + reason: "OAuth2 clientID must be defined as either Secret or ConfigMap.", + action: "Specify a valid Secret or ConfigMap reference for 'clientID'.", + }) + clientID = "TODO_SET_OAUTH2_CLIENT_ID" } clientSecret, err := c.convertSecretSelector(&oa.ClientSecret) @@ -980,7 +1036,12 @@ func convertRelabelingToSelector(logger *slog.Logger, data *relabelingData, isSi res.MatchLabels = make(map[string]string) } if existing, exists := res.MatchLabels[labelName]; exists && existing != parts[0] { - return false, fmt.Errorf("conflicting keep rules for label %q: cannot require both %q and %q simultaneously", labelName, existing, parts[0]) + res.Todos = append(res.Todos, todoItem{ + category: "ERROR", + reason: fmt.Sprintf("Conflicting relabeling keep rules for label %q: cannot require both %q and %q simultaneously.", labelName, existing, parts[0]), + action: fmt.Sprintf("Define the intended label value for %q in 'spec.selector.matchLabels'.", labelName), + }) + return true, nil } res.MatchLabels[labelName] = parts[0] logger.Info(fmt.Sprintf("Converted target filtering relabeling rule (%q -> %q) to Pod Selector (matchLabels).", source, parts[0])) @@ -1351,41 +1412,60 @@ func resolveFilterRunning(filterRunnings []*bool, logger *slog.Logger, isCluster } // resolveScrapeIntervalAndTimeout validates and caps timeout to interval if needed. -func resolveScrapeIntervalAndTimeout(logger *slog.Logger, interval, timeout string) (resolvedInterval, resolvedTimeout string, err error) { +func (c *conversionContext) resolveScrapeIntervalAndTimeout(interval, timeout string) (resolvedInterval, resolvedTimeout string) { // TODO(M2): Inherit global scrape interval from Prometheus CR if empty. if interval == "" { - logger.Warn("Scrape interval is empty. Defaulting to '30s' as GMP requires this field.") + c.logger.Warn("Scrape interval is empty. Defaulting to '30s' as GMP requires this field.") interval = "30s" } intDur, err := prommodel.ParseDuration(interval) if err != nil { - return "", "", fmt.Errorf("invalid interval %q: %w", interval, err) + c.todos = append(c.todos, todoItem{ + category: "ERROR", + reason: fmt.Sprintf("Invalid scrape interval %q.", interval), + action: "Specify a valid Prometheus duration string (e.g. '30s', '1m') in 'spec.endpoints[].interval'.", + }) + interval = "30s" + intDur, _ = prommodel.ParseDuration("30s") } // TODO(M2): Inherit global scrape timeout from Prometheus CR if empty. if timeout != "" { toDur, err := prommodel.ParseDuration(timeout) if err != nil { - return "", "", fmt.Errorf("invalid scrapeTimeout %q: %w", timeout, err) - } - if toDur > intDur { - logger.Warn("Scrape timeout is larger than scrape interval. Capping timeout to interval.", + c.todos = append(c.todos, todoItem{ + category: "ERROR", + reason: fmt.Sprintf("Invalid scrape timeout %q.", timeout), + action: "Specify a valid Prometheus duration string (e.g. '10s') in 'spec.endpoints[].timeout'.", + }) + timeout = "" + } else if toDur > intDur { + c.logger.Warn("Scrape timeout is larger than scrape interval. Capping timeout to interval.", slog.String("timeout", timeout), slog.String("interval", interval)) timeout = interval } } - return interval, timeout, nil + return interval, timeout } -// convertProxyURL verifies proxy URL credentials. -func convertProxyURL(proxyURL *string) (string, error) { +// convertProxyURL verifies proxy URL credentials and attaches a TODO if passwords are present. +func (c *conversionContext) convertProxyURL(proxyURL *string) (string, error) { if proxyURL == nil { return "", nil } - if strings.Contains(*proxyURL, "@") { - return "", errors.New("proxyUrl contains credentials (matches '@'), which is blocked by GMP API validation") + parsed, err := url.Parse(*proxyURL) + if err == nil && parsed.User != nil { + if _, hasPass := parsed.User.Password(); hasPass { + c.todos = append(c.todos, todoItem{ + category: "ERROR", + reason: "Proxy URL contains embedded plaintext credentials. Credentials were removed.", + action: "Configure proxy authentication via Kubernetes Secret or proxy server configuration.", + }) + parsed.User = nil + return parsed.String(), nil + } } return *proxyURL, nil } @@ -1444,15 +1524,19 @@ func endpointPortKey(ep pomonitoringv1.Endpoint) string { } // resolveServicePort resolves a Service port to the backing Pod's target port. -func resolveServicePort(logger *slog.Logger, svc *corev1.Service, portStr string) (intstr.IntOrString, error) { +func resolveServicePort(logger *slog.Logger, svc *corev1.Service, portStr string) (intstr.IntOrString, *todoItem) { if portStr == "" { - return intstr.IntOrString{}, errors.New("port string cannot be empty") + return intstr.FromString("TODO_SET_PORT"), nil } if svc == nil { - return intstr.IntOrString{}, errors.New("cannot resolve port on nil or uninitialized Service") + return intstr.FromString("TODO_RESOLVE_PORT"), nil } if len(svc.Spec.Ports) == 0 { - return intstr.IntOrString{}, errors.New("service has no ports defined in spec") + return intstr.FromString(fmt.Sprintf("TODO_RESOLVE_PORT_%s", strings.ToUpper(portStr))), &todoItem{ + category: "WARNING", + reason: fmt.Sprintf("Port %q could not be resolved because Service %q defines no ports in its spec.", portStr, svc.Name), + action: fmt.Sprintf("Verify that target pods expose port %q.", portStr), + } } for _, p := range svc.Spec.Ports { @@ -1472,7 +1556,11 @@ func resolveServicePort(logger *slog.Logger, svc *corev1.Service, portStr string } } - return intstr.IntOrString{}, fmt.Errorf("port %q not found in Service spec", portStr) + return intstr.FromString(fmt.Sprintf("TODO_RESOLVE_PORT_%s", strings.ToUpper(portStr))), &todoItem{ + category: "WARNING", + reason: fmt.Sprintf("Port %q was not found in Service %q spec.", portStr, svc.Name), + action: fmt.Sprintf("Verify that target pods expose port %q.", portStr), + } } const ( diff --git a/pkg/migrate/helpers_test.go b/pkg/migrate/helpers_test.go index e1ff6b03a6..4c71d24ea0 100644 --- a/pkg/migrate/helpers_test.go +++ b/pkg/migrate/helpers_test.go @@ -85,14 +85,16 @@ func TestExtractSecretKey(t *testing.T) { setupCache func(cache *ResourceCache) error selector corev1.SecretKeySelector expectedVal string + expectTodos int wantErr bool }{ { name: "Missing secret", setupCache: func(_ *ResourceCache) error { return nil }, // Empty cache. selector: corev1.SecretKeySelector{LocalObjectReference: corev1.LocalObjectReference{Name: "missing"}, Key: "user"}, - expectedVal: "", - wantErr: true, + expectedVal: "TODO_SET_USER_FROM_SECRET_MISSING", + expectTodos: 1, + wantErr: false, }, { name: "Secret with StringData", @@ -101,6 +103,7 @@ func TestExtractSecretKey(t *testing.T) { }, selector: corev1.SecretKeySelector{LocalObjectReference: corev1.LocalObjectReference{Name: "my-secret"}, Key: "user"}, expectedVal: "admin", + expectTodos: 0, wantErr: false, }, { @@ -110,21 +113,24 @@ func TestExtractSecretKey(t *testing.T) { }, selector: corev1.SecretKeySelector{LocalObjectReference: corev1.LocalObjectReference{Name: "my-secret-2"}, Key: "pass"}, expectedVal: "supersecret", + expectTodos: 0, wantErr: false, }, { name: "Secret reference with empty Name", setupCache: func(_ *ResourceCache) error { return nil }, selector: corev1.SecretKeySelector{Key: "user"}, - expectedVal: "", - wantErr: true, + expectedVal: "TODO_SET_USER_FROM_SECRET_EMPTY_NAME", + expectTodos: 1, + wantErr: false, }, { name: "Secret reference with empty Key", setupCache: func(_ *ResourceCache) error { return nil }, selector: corev1.SecretKeySelector{LocalObjectReference: corev1.LocalObjectReference{Name: "my-secret"}}, - expectedVal: "", - wantErr: true, + expectedVal: "TODO_SET_EMPTY_KEY_FROM_SECRET_MY-SECRET", + expectTodos: 1, + wantErr: false, }, { name: "Secret exists but key missing", @@ -132,8 +138,9 @@ func TestExtractSecretKey(t *testing.T) { return addSecretToCache(cache, "default", "my-secret", "user", "admin", true) }, selector: corev1.SecretKeySelector{LocalObjectReference: corev1.LocalObjectReference{Name: "my-secret"}, Key: "password"}, - expectedVal: "", - wantErr: true, + expectedVal: "TODO_MISSING_KEY_PASSWORD_IN_SECRET_MY-SECRET", + expectTodos: 1, + wantErr: false, }, { name: "Secret exists but data corrupted", @@ -154,8 +161,9 @@ func TestExtractSecretKey(t *testing.T) { return cache.Add(secret) }, selector: corev1.SecretKeySelector{LocalObjectReference: corev1.LocalObjectReference{Name: "corrupted-secret"}, Key: "pass"}, - expectedVal: "", - wantErr: true, + expectedVal: "TODO_CORRUPT_SECRET_DATA_PASS", + expectTodos: 1, + wantErr: false, }, } @@ -166,13 +174,13 @@ func TestExtractSecretKey(t *testing.T) { t.Fatalf("failed to setup cache: %v", err) } - val, err := ctx.extractSecretKey(tc.selector) - if (err != nil) != tc.wantErr { - t.Fatalf("extractSecretKey() error = %v, wantErr %v", err, tc.wantErr) - } - if !tc.wantErr && val != tc.expectedVal { + val := ctx.extractSecretKey(tc.selector) + if val != tc.expectedVal { t.Errorf("expected %s, got %s", tc.expectedVal, val) } + if len(ctx.todos) != tc.expectTodos { + t.Errorf("expected %d todos, got %d", tc.expectTodos, len(ctx.todos)) + } }) } } @@ -183,14 +191,16 @@ func TestExtractConfigMapKey(t *testing.T) { setupCache func(cache *ResourceCache) error selector corev1.ConfigMapKeySelector expectedVal string + expectTodos int wantErr bool }{ { name: "Missing configmap", setupCache: func(_ *ResourceCache) error { return nil }, // Empty cache. selector: corev1.ConfigMapKeySelector{LocalObjectReference: corev1.LocalObjectReference{Name: "missing"}, Key: "user"}, - expectedVal: "", - wantErr: true, + expectedVal: "TODO_SET_USER_FROM_CONFIGMAP_MISSING", + expectTodos: 1, + wantErr: false, }, { name: "Found configmap", @@ -199,21 +209,24 @@ func TestExtractConfigMapKey(t *testing.T) { }, selector: corev1.ConfigMapKeySelector{LocalObjectReference: corev1.LocalObjectReference{Name: "my-cm"}, Key: "id"}, expectedVal: "client-123", + expectTodos: 0, wantErr: false, }, { name: "Configmap reference with empty Name", setupCache: func(_ *ResourceCache) error { return nil }, selector: corev1.ConfigMapKeySelector{Key: "user"}, - expectedVal: "", - wantErr: true, + expectedVal: "TODO_SET_USER_FROM_CONFIGMAP_EMPTY_NAME", + expectTodos: 1, + wantErr: false, }, { name: "Configmap reference with empty Key", setupCache: func(_ *ResourceCache) error { return nil }, selector: corev1.ConfigMapKeySelector{LocalObjectReference: corev1.LocalObjectReference{Name: "my-cm"}}, - expectedVal: "", - wantErr: true, + expectedVal: "TODO_SET_EMPTY_KEY_FROM_CONFIGMAP_MY-CM", + expectTodos: 1, + wantErr: false, }, { name: "Configmap exists but key missing", @@ -221,8 +234,9 @@ func TestExtractConfigMapKey(t *testing.T) { return addConfigMapToCache(cache, "default", "my-cm", "id", "client-123") }, selector: corev1.ConfigMapKeySelector{LocalObjectReference: corev1.LocalObjectReference{Name: "my-cm"}, Key: "secret"}, - expectedVal: "", - wantErr: true, + expectedVal: "TODO_MISSING_KEY_SECRET_IN_CONFIGMAP_MY-CM", + expectTodos: 1, + wantErr: false, }, } @@ -233,13 +247,13 @@ func TestExtractConfigMapKey(t *testing.T) { t.Fatalf("failed to setup cache: %v", err) } - val, err := ctx.extractConfigMapKey(tc.selector) - if (err != nil) != tc.wantErr { - t.Fatalf("extractConfigMapKey() error = %v, wantErr %v", err, tc.wantErr) - } - if !tc.wantErr && val != tc.expectedVal { + val := ctx.extractConfigMapKey(tc.selector) + if val != tc.expectedVal { t.Errorf("expected %s, got %s", tc.expectedVal, val) } + if len(ctx.todos) != tc.expectTodos { + t.Errorf("expected %d todos, got %d", tc.expectTodos, len(ctx.todos)) + } }) } } @@ -281,19 +295,19 @@ func TestConvertConfigMapToSecretSelector(t *testing.T) { name: "Empty name reference", setupCache: func(_ *ResourceCache) error { return nil }, selector: &corev1.ConfigMapKeySelector{Key: "ca.crt"}, - expectedSecretName: "", - expectedSecretKey: "", + expectedSecretName: "secret-TODO_SET_CONFIGMAP_NAME", + expectedSecretKey: "ca.crt", expectGeneratedSecret: false, - wantErr: true, + wantErr: false, }, { name: "Empty key reference", setupCache: func(_ *ResourceCache) error { return nil }, selector: &corev1.ConfigMapKeySelector{LocalObjectReference: corev1.LocalObjectReference{Name: "tls-cm"}}, - expectedSecretName: "", - expectedSecretKey: "", + expectedSecretName: "secret-tls-cm", + expectedSecretKey: "TODO_SET_CONFIGMAP_KEY", expectGeneratedSecret: false, - wantErr: true, + wantErr: false, }, } @@ -368,13 +382,18 @@ func TestConvertBasicAuth(t *testing.T) { wantErr: false, }, { - name: "Malformed Username reference", - setupCache: func(_ *ResourceCache) error { return nil }, + name: "Malformed Username reference", + setupCache: func(cache *ResourceCache) error { + return addSecretToCache(cache, "default", "auth-secret", "pass", "pass123", true) + }, basicAuth: &pomonitoringv1.BasicAuth{ Username: corev1.SecretKeySelector{Key: "user"}, Password: corev1.SecretKeySelector{LocalObjectReference: corev1.LocalObjectReference{Name: "auth-secret"}, Key: "pass"}, }, - wantErr: true, + expectedUser: "TODO_SET_USER_FROM_SECRET_EMPTY_NAME", + expectedPassName: "auth-secret", + expectedPassKey: "pass", + wantErr: false, }, } @@ -461,7 +480,9 @@ func TestConvertSafeTLSConfig(t *testing.T) { ConfigMap: &corev1.ConfigMapKeySelector{Key: "ca.crt"}, }, }, - wantErr: true, + expectedCAName: "secret-TODO_SET_CONFIGMAP_NAME", + expectedCAKey: "ca.crt", + wantErr: false, }, } @@ -487,11 +508,15 @@ func TestConvertSafeTLSConfig(t *testing.T) { return } - if gmpTLS.CA.Secret.Name != tc.expectedCAName || gmpTLS.CA.Secret.Key != tc.expectedCAKey { - t.Errorf("unexpected CA selector: %+v", gmpTLS.CA) + if tc.expectedCAName != "" { + if gmpTLS.CA == nil || gmpTLS.CA.Secret == nil || gmpTLS.CA.Secret.Name != tc.expectedCAName || gmpTLS.CA.Secret.Key != tc.expectedCAKey { + t.Errorf("unexpected CA selector: %+v", gmpTLS.CA) + } } - if gmpTLS.Cert.Secret.Name != tc.expectedCertName || gmpTLS.Cert.Secret.Key != tc.expectedCertKey { - t.Errorf("unexpected Cert selector: %+v", gmpTLS.Cert) + if tc.expectedCertName != "" { + if gmpTLS.Cert == nil || gmpTLS.Cert.Secret == nil || gmpTLS.Cert.Secret.Name != tc.expectedCertName || gmpTLS.Cert.Secret.Key != tc.expectedCertKey { + t.Errorf("unexpected Cert selector: %+v", gmpTLS.Cert) + } } if gmpTLS.InsecureSkipVerify != tc.expectedSkipVerify { t.Errorf("expected InsecureSkipVerify %v, got %v", tc.expectedSkipVerify, gmpTLS.InsecureSkipVerify) @@ -503,6 +528,98 @@ func TestConvertSafeTLSConfig(t *testing.T) { } } +func TestConvertOAuth2(t *testing.T) { + tests := []struct { + name string + setupCache func(cache *ResourceCache) error + oauth2 *pomonitoringv1.OAuth2 + expectedClientID string + expectedSecName string + expectedSecKey string + expectTodos int + wantErr bool + }{ + { + name: "Nil OAuth2 returns nil", + setupCache: func(_ *ResourceCache) error { return nil }, + oauth2: nil, + wantErr: false, + }, + { + name: "Valid OAuth2 from Secret", + setupCache: func(cache *ResourceCache) error { + return addSecretToCache(cache, "default", "oauth-sec", "client_id", "my-client", true) + }, + oauth2: &pomonitoringv1.OAuth2{ + ClientID: pomonitoringv1.SecretOrConfigMap{ + Secret: &corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{Name: "oauth-sec"}, + Key: "client_id", + }, + }, + ClientSecret: corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{Name: "oauth-sec"}, + Key: "client_secret", + }, + TokenURL: "https://auth.example.com/token", + }, + expectedClientID: "my-client", + expectedSecName: "oauth-sec", + expectedSecKey: "client_secret", + expectTodos: 0, + wantErr: false, + }, + { + name: "Empty ClientID generates placeholder and TODO", + setupCache: func(_ *ResourceCache) error { return nil }, + oauth2: &pomonitoringv1.OAuth2{ + ClientSecret: corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{Name: "oauth-sec"}, + Key: "client_secret", + }, + TokenURL: "https://auth.example.com/token", + }, + expectedClientID: "TODO_SET_OAUTH2_CLIENT_ID", + expectedSecName: "oauth-sec", + expectedSecKey: "client_secret", + expectTodos: 1, + wantErr: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + ctx := newTestConversionContext() + if err := tc.setupCache(ctx.cache); err != nil { + t.Fatalf("failed to setup cache: %v", err) + } + + res, err := ctx.convertOAuth2(tc.oauth2) + if (err != nil) != tc.wantErr { + t.Fatalf("convertOAuth2() error = %v, wantErr %v", err, tc.wantErr) + } + if tc.oauth2 == nil { + if res != nil { + t.Errorf("expected nil result for nil OAuth2, got %+v", res) + } + return + } + if tc.wantErr { + return + } + if res.ClientID != tc.expectedClientID { + t.Errorf("expected ClientID %q, got %q", tc.expectedClientID, res.ClientID) + } + if res.ClientSecret.Secret == nil || res.ClientSecret.Secret.Name != tc.expectedSecName || res.ClientSecret.Secret.Key != tc.expectedSecKey { + t.Errorf("unexpected ClientSecret selector: %+v", res.ClientSecret) + } + if len(ctx.todos) != tc.expectTodos { + t.Errorf("expected %d todos, got %d", tc.expectTodos, len(ctx.todos)) + } + }) + } +} + func TestConvertConfigMapToSecretSelectorDeduplication(t *testing.T) { ctx := newTestConversionContext() err := addConfigMapToCache(ctx.cache, "default", "tls-cm", "ca.crt", "cert-data") @@ -670,14 +787,13 @@ func TestDetermineNamespaceScoping(t *testing.T) { } func TestResolveScrapeIntervalAndTimeout(t *testing.T) { - logger := slog.New(slog.NewTextHandler(os.Stdout, nil)) tests := []struct { name string interval string timeout string expectedInt string expectedTimeout string - expectErr bool + expectTodos int }{ { name: "empty defaults to 30s", @@ -685,7 +801,7 @@ func TestResolveScrapeIntervalAndTimeout(t *testing.T) { timeout: "", expectedInt: "30s", expectedTimeout: "", - expectErr: false, + expectTodos: 0, }, { name: "valid interval and timeout", @@ -693,7 +809,7 @@ func TestResolveScrapeIntervalAndTimeout(t *testing.T) { timeout: "10s", expectedInt: "15s", expectedTimeout: "10s", - expectErr: false, + expectTodos: 0, }, { name: "timeout larger than interval is capped", @@ -701,36 +817,38 @@ func TestResolveScrapeIntervalAndTimeout(t *testing.T) { timeout: "20s", expectedInt: "10s", expectedTimeout: "10s", - expectErr: false, + expectTodos: 0, }, { - name: "invalid interval duration", - interval: "invalid", - expectErr: true, + name: "invalid interval duration defaults to 30s with todo", + interval: "invalid", + expectedInt: "30s", + expectedTimeout: "", + expectTodos: 1, }, { - name: "invalid timeout duration", - interval: "15s", - timeout: "invalid", - expectErr: true, + name: "invalid timeout duration is dropped with todo", + interval: "15s", + timeout: "invalid", + expectedInt: "15s", + expectedTimeout: "", + expectTodos: 1, }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - intVal, toVal, err := resolveScrapeIntervalAndTimeout(logger, tc.interval, tc.timeout) - if (err != nil) != tc.expectErr { - t.Fatalf("resolveScrapeIntervalAndTimeout() error = %v, expectErr = %v", err, tc.expectErr) - } - if tc.expectErr { - return - } + ctx := newTestConversionContext() + intVal, toVal := ctx.resolveScrapeIntervalAndTimeout(tc.interval, tc.timeout) if intVal != tc.expectedInt { t.Errorf("resolveScrapeIntervalAndTimeout() interval = %v, want %v", intVal, tc.expectedInt) } if toVal != tc.expectedTimeout { t.Errorf("resolveScrapeIntervalAndTimeout() timeout = %v, want %v", toVal, tc.expectedTimeout) } + if len(ctx.todos) != tc.expectTodos { + t.Errorf("expected %d todos, got %d", tc.expectTodos, len(ctx.todos)) + } }) } } @@ -740,30 +858,36 @@ func TestConvertProxyURL(t *testing.T) { name string proxyURL *string expectedURL string + expectTodos int expectErr bool }{ { name: "nil proxyURL", proxyURL: nil, expectedURL: "", + expectTodos: 0, expectErr: false, }, { name: "valid proxyURL without credentials", proxyURL: ptrTo("http://proxy.example.com"), expectedURL: "http://proxy.example.com", + expectTodos: 0, expectErr: false, }, { - name: "proxyURL with credentials returns error", - proxyURL: ptrTo("http://user:pass@proxy.example.com"), - expectErr: true, + name: "proxyURL with credentials sanitizes password and adds todo", + proxyURL: ptrTo("http://user:pass@proxy.example.com:8080"), + expectedURL: "http://proxy.example.com:8080", + expectTodos: 1, + expectErr: false, }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - url, err := convertProxyURL(tc.proxyURL) + convCtx := &conversionContext{} + url, err := convCtx.convertProxyURL(tc.proxyURL) if (err != nil) != tc.expectErr { t.Fatalf("convertProxyURL() error = %v, expectErr = %v", err, tc.expectErr) } @@ -773,6 +897,9 @@ func TestConvertProxyURL(t *testing.T) { if url != tc.expectedURL { t.Errorf("convertProxyURL() = %v, want %v", url, tc.expectedURL) } + if len(convCtx.todos) != tc.expectTodos { + t.Errorf("expected %d todos, got %d", tc.expectTodos, len(convCtx.todos)) + } }) } } @@ -936,10 +1063,7 @@ func TestDecoupledNamespaces(t *testing.T) { LocalObjectReference: corev1.LocalObjectReference{Name: "my-secret"}, Key: "user", } - val, err := ctx.extractSecretKey(sel) - if err != nil { - t.Fatalf("extractSecretKey() unexpected error: %v", err) - } + val := ctx.extractSecretKey(sel) if val != "admin" { t.Errorf("extractSecretKey() = %q, want %q", val, "admin") } @@ -1082,81 +1206,81 @@ func TestFindServicesBySelector(t *testing.T) { func TestResolveServicePort(t *testing.T) { tests := []struct { - name string - service *corev1.Service - portStr string - expected intstr.IntOrString - wantErr bool + name string + service *corev1.Service + portStr string + expected intstr.IntOrString + expectTodo bool }{ { name: "Resolve by name to int", service: makeTestTypedService("default", "my-svc", nil, []corev1.ServicePort{ {Name: "web", Port: 80, TargetPort: intstr.FromInt32(8080)}, }), - portStr: "web", - expected: intstr.FromInt32(8080), - wantErr: false, + portStr: "web", + expected: intstr.FromInt32(8080), + expectTodo: false, }, { name: "Resolve by name to string", service: makeTestTypedService("default", "my-svc", nil, []corev1.ServicePort{ {Name: "web", Port: 80, TargetPort: intstr.FromString("http-web")}, }), - portStr: "web", - expected: intstr.FromString("http-web"), - wantErr: false, + portStr: "web", + expected: intstr.FromString("http-web"), + expectTodo: false, }, { name: "Resolve by port number to targetPort int", service: makeTestTypedService("default", "my-svc", nil, []corev1.ServicePort{ {Name: "web", Port: 80, TargetPort: intstr.FromInt32(8080)}, }), - portStr: "80", - expected: intstr.FromInt32(8080), - wantErr: false, + portStr: "80", + expected: intstr.FromInt32(8080), + expectTodo: false, }, { name: "Resolve by targetPort string when port name omitted", service: makeTestTypedService("default", "my-svc", nil, []corev1.ServicePort{ {Name: "web", Port: 80, TargetPort: intstr.FromString("http-metrics")}, }), - portStr: "http-metrics", - expected: intstr.FromString("http-metrics"), - wantErr: false, + portStr: "http-metrics", + expected: intstr.FromString("http-metrics"), + expectTodo: false, }, { name: "Resolve by port number", service: makeTestTypedService("default", "my-svc", nil, []corev1.ServicePort{ {Name: "web", Port: 80, TargetPort: intstr.FromString("http-web")}, }), - portStr: "80", - expected: intstr.FromString("http-web"), - wantErr: false, + portStr: "80", + expected: intstr.FromString("http-web"), + expectTodo: false, }, { name: "Resolve with omitted targetPort", service: makeTestTypedService("default", "my-svc", nil, []corev1.ServicePort{ {Name: "web", Port: 80}, }), - portStr: "web", - expected: intstr.FromInt32(80), - wantErr: false, + portStr: "web", + expected: intstr.FromInt32(80), + expectTodo: false, }, { name: "Port not found", service: makeTestTypedService("default", "my-svc", nil, []corev1.ServicePort{ {Name: "web", Port: 80}, }), - portStr: "admin", - expected: intstr.IntOrString{}, - wantErr: true, + portStr: "admin", + expected: intstr.FromString("TODO_RESOLVE_PORT_ADMIN"), + expectTodo: true, }, { - name: "Nil service", - service: nil, - portStr: "web", - expected: intstr.IntOrString{}, - wantErr: true, + name: "Nil service", + service: nil, + portStr: "web", + expected: intstr.FromString("TODO_RESOLVE_PORT"), + expectTodo: false, }, { name: "Skip malformed port entry and resolve valid later entry", @@ -1169,12 +1293,12 @@ func TestResolveServicePort(t *testing.T) { }, }, }, - portStr: "web", - expected: intstr.FromInt32(8080), - wantErr: false, + portStr: "web", + expected: intstr.FromInt32(8080), + expectTodo: false, }, { - name: "All ports malformed returns error", + name: "All ports malformed returns todo placeholder", service: &corev1.Service{ ObjectMeta: metav1.ObjectMeta{Name: "my-svc", Namespace: "default"}, Spec: corev1.ServiceSpec{ @@ -1184,9 +1308,9 @@ func TestResolveServicePort(t *testing.T) { }, }, }, - portStr: "web", - expected: intstr.IntOrString{}, - wantErr: true, + portStr: "web", + expected: intstr.FromString("TODO_RESOLVE_PORT_WEB"), + expectTodo: true, }, { name: "Out of range port number is rejected", @@ -1200,18 +1324,18 @@ func TestResolveServicePort(t *testing.T) { }, }, }, - portStr: "web", - expected: intstr.FromInt32(8080), - wantErr: false, + portStr: "web", + expected: intstr.FromInt32(8080), + expectTodo: false, }, { name: "Resolve with empty string targetPort defaults to port number", service: makeTestTypedService("default", "my-svc", nil, []corev1.ServicePort{ {Name: "web", Port: 80, TargetPort: intstr.FromString("")}, }), - portStr: "web", - expected: intstr.FromInt32(80), - wantErr: false, + portStr: "web", + expected: intstr.FromInt32(80), + expectTodo: false, }, } @@ -1219,12 +1343,9 @@ func TestResolveServicePort(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - got, err := resolveServicePort(logger, tc.service, tc.portStr) - if (err != nil) != tc.wantErr { - t.Fatalf("resolveServicePort() error = %v, wantErr %v", err, tc.wantErr) - } - if tc.wantErr { - return + got, todo := resolveServicePort(logger, tc.service, tc.portStr) + if (todo != nil) != tc.expectTodo { + t.Fatalf("resolveServicePort() todo = %v, expectTodo %v", todo, tc.expectTodo) } if got != tc.expected { t.Errorf("expected %+v, got %+v", tc.expected, got) diff --git a/pkg/migrate/migrate.go b/pkg/migrate/migrate.go index 3edb961ef1..bd85f2518d 100644 --- a/pkg/migrate/migrate.go +++ b/pkg/migrate/migrate.go @@ -146,6 +146,12 @@ func (m *Migrator) PrintSummary(r *MigrationReport) { fmt.Fprintf(m.Stderr, " Skipped (Unsupported): %d\n", r.SkippedCount) fmt.Fprintf(m.Stderr, " Failed: %d\n", r.FailedCount) fmt.Fprintln(m.Stderr, "=========================================") + if r.ActionItemsCount > 0 { + fmt.Fprintln(m.Stderr, "\nNOTE: Some resources were migrated with action items and contain TODO annotations.") + fmt.Fprintln(m.Stderr, "These resources include the safety guardrail label:") + fmt.Fprintln(m.Stderr, " 'gmp.googleapis.com/migration-review-required: \"true\"'") + fmt.Fprintln(m.Stderr, "Review the TODO annotations in the generated manifests and remove this label when ready to activate scraping.") + } } // WriteOutputs serializes and writes the converted manifests to the migrator's Stdout stream. diff --git a/pkg/migrate/migrate_test.go b/pkg/migrate/migrate_test.go index fc0321b54d..90cd1face7 100644 --- a/pkg/migrate/migrate_test.go +++ b/pkg/migrate/migrate_test.go @@ -415,3 +415,48 @@ items: t.Errorf("expected reference to be successfully resolved inside list, got logs: %q", stderrLogs) } } + +func TestMigratorPrintSummary(t *testing.T) { + tests := []struct { + name string + report *MigrationReport + wantContains []string + }{ + { + name: "Clean report without action items", + report: &MigrationReport{ + SuccessCount: 2, + }, + wantContains: []string{ + "Successfully Migrated: 2", + "Migrated with Action Items: 0", + }, + }, + { + name: "Report with action items includes guidance note", + report: &MigrationReport{ + SuccessCount: 1, + ActionItemsCount: 1, + }, + wantContains: []string{ + "Migrated with Action Items: 1", + "gmp.googleapis.com/migration-review-required", + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + var buf bytes.Buffer + m := NewMigrator() + m.Stderr = &buf + m.PrintSummary(tc.report) + output := buf.String() + for _, s := range tc.wantContains { + if !strings.Contains(output, s) { + t.Errorf("PrintSummary output missing %q, got:\n%s", s, output) + } + } + }) + } +} diff --git a/pkg/migrate/podmonitor.go b/pkg/migrate/podmonitor.go index 12d7e56558..f098ddb80e 100644 --- a/pkg/migrate/podmonitor.go +++ b/pkg/migrate/podmonitor.go @@ -117,7 +117,12 @@ func (c *PodMonitorConverter) convertEndpoints( } else if ep.TargetPort != nil { // nolint:staticcheck // Map deprecated TargetPort for backwards compatibility. gmpEp.Port = *ep.TargetPort // nolint:staticcheck // Map deprecated TargetPort for backwards compatibility. } else { - return nil, fmt.Errorf("endpoint [%d]: port or targetPort must be set", i) + convCtx.todos = append(convCtx.todos, todoItem{ + category: "ERROR", + reason: fmt.Sprintf("Endpoint [%d] does not specify a 'port' or 'targetPort'.", i), + action: "Specify a valid port name or number in 'spec.endpoints[].port'.", + }) + gmpEp.Port = intstr.FromString("TODO_SET_PORT") } // 2. Basic Fields. @@ -126,10 +131,7 @@ func (c *PodMonitorConverter) convertEndpoints( gmpEp.Params = ep.Params // 3. Scrape Intervals & Timeouts. - interval, timeout, err := resolveScrapeIntervalAndTimeout(convCtx.logger, string(ep.Interval), string(ep.ScrapeTimeout)) - if err != nil { - return nil, fmt.Errorf("endpoint [%d]: %w", i, err) - } + interval, timeout := convCtx.resolveScrapeIntervalAndTimeout(string(ep.Interval), string(ep.ScrapeTimeout)) gmpEp.Interval = interval gmpEp.Timeout = timeout @@ -137,7 +139,7 @@ func (c *PodMonitorConverter) convertEndpoints( gmpEp.MetricRelabeling = combineAndConvertRelabelings(convCtx.logger, epResults[i].PromotedRules, ep.MetricRelabelConfigs) // Proxy Settings. - proxyURL, err := convertProxyURL(ep.ProxyURL) + proxyURL, err := convCtx.convertProxyURL(ep.ProxyURL) if err != nil { return nil, fmt.Errorf("endpoint [%d]: %w", i, err) } @@ -182,10 +184,7 @@ func (c *PodMonitorConverter) convertMonitorSpec(pm *pomonitoringv1.PodMonitor, } mergedFromPod := mergeFromPod(logger, convertTargetLabels(logger, pm.Spec.PodTargetLabels, pm.Spec.JobLabel, "Pod"), rules.ResourceCombined.FromPod) - mergedSelector, err := mergeLabelSelector(pm.Spec.Selector, rules.ResourceCombined.MatchLabels, rules.ResourceCombined.MatchExpressions) - if err != nil { - return nil, err - } + mergedSelector := convCtx.mergeLabelSelector(pm.Spec.Selector, rules.ResourceCombined.MatchLabels, rules.ResourceCombined.MatchExpressions) var todos []todoItem todos = append(todos, rules.ResourceCombined.Todos...) @@ -230,7 +229,7 @@ func (c *PodMonitorConverter) convertMonitorSpec(pm *pomonitoringv1.PodMonitor, filterRunning: filterRunning, limits: limits, generatedSecrets: convCtx.getGeneratedSecrets(), - todos: todos, + todos: append(todos, convCtx.todos...), }, nil } diff --git a/pkg/migrate/podmonitor_test.go b/pkg/migrate/podmonitor_test.go index 7d92b56f5c..44ccff62aa 100644 --- a/pkg/migrate/podmonitor_test.go +++ b/pkg/migrate/podmonitor_test.go @@ -1622,10 +1622,35 @@ func TestPodMonitorConversion(t *testing.T) { }, }, }, - wantErr: "selector conflict: label \"app\" has conflicting values \"frontend\" (base selector) and \"backend\" (relabeling rule)", + expected: []runtime.Object{ + &monitoringv1.PodMonitoring{ + TypeMeta: BuildTypeMeta(KindPodMonitoring), + ObjectMeta: metav1.ObjectMeta{ + Name: "conflict-selector-monitor", + Namespace: "default", + Annotations: map[string]string{ + "gmp.googleapis.com/todo-1": "[ERROR] Selector conflict: label \"app\" has conflicting values \"frontend\" (from base selector) and \"backend\" (from relabeling rule). ACTION: Reconcile 'spec.selector.matchLabels' for label \"app\" with the intended target pods.", + }, + }, + Spec: monitoringv1.PodMonitoringSpec{ + Selector: metav1.LabelSelector{ + MatchLabels: map[string]string{ + "app": "frontend", + "gmp.googleapis.com/migration-review-required": "true", + }, + }, + Endpoints: []monitoringv1.ScrapeEndpoint{ + { + Port: intstr.FromString("metrics"), + Interval: "30s", + }, + }, + }, + }, + }, }, { - name: "Pre-Scrape Relabelings: conflicting keep rules on same pod label return error", + name: "Pre-Scrape Relabelings: conflicting keep rules on same pod label injects guardrail label and TODO annotation", input: &pomonitoringv1.PodMonitor{ TypeMeta: metav1.TypeMeta{ APIVersion: "monitoring.coreos.com/v1", @@ -1658,7 +1683,33 @@ func TestPodMonitorConversion(t *testing.T) { }, }, }, - wantErr: "conflicting keep rules for label \"env\": cannot require both \"production\" and \"staging\" simultaneously", + expected: []runtime.Object{ + &monitoringv1.PodMonitoring{ + TypeMeta: BuildTypeMeta(KindPodMonitoring), + ObjectMeta: metav1.ObjectMeta{ + Name: "conflicting-keep-monitor", + Namespace: "default", + Annotations: map[string]string{ + "gmp.googleapis.com/todo-1": "[ERROR] Conflicting relabeling keep rules for label \"env\": cannot require both \"production\" and \"staging\" simultaneously. ACTION: Define the intended label value for \"env\" in 'spec.selector.matchLabels'.", + }, + }, + Spec: monitoringv1.PodMonitoringSpec{ + Selector: metav1.LabelSelector{ + MatchLabels: map[string]string{ + "app": "test", + "env": "production", + "gmp.googleapis.com/migration-review-required": "true", + }, + }, + Endpoints: []monitoringv1.ScrapeEndpoint{ + { + Port: intstr.FromString("metrics"), + Interval: "30s", + }, + }, + }, + }, + }, }, { name: "BearerTokenSecret with empty Name returns validation error", @@ -1683,7 +1734,92 @@ func TestPodMonitorConversion(t *testing.T) { }, }, }, - wantErr: "bearerTokenSecret: secret reference has an empty name for key \"token\"", + expected: []runtime.Object{ + &monitoringv1.PodMonitoring{ + TypeMeta: BuildTypeMeta(KindPodMonitoring), + ObjectMeta: metav1.ObjectMeta{ + Name: "bearer-token-secret-err", + Namespace: "default", + Annotations: map[string]string{ + "gmp.googleapis.com/todo-1": "[ERROR] Referenced Secret has an empty name for key \"token\". ACTION: Specify a valid Secret name in the configuration.", + }, + }, + Spec: monitoringv1.PodMonitoringSpec{ + Selector: metav1.LabelSelector{ + MatchLabels: map[string]string{ + "app": "frontend", + "gmp.googleapis.com/migration-review-required": "true", + }, + }, + Endpoints: []monitoringv1.ScrapeEndpoint{ + { + Port: intstr.FromString("metrics"), + Interval: "30s", + HTTPClientConfig: monitoringv1.HTTPClientConfig{ + Authorization: &monitoringv1.Auth{ + Credentials: &monitoringv1.SecretSelector{ + Secret: &monitoringv1.SecretKeySelector{ + Name: "TODO_SET_SECRET_NAME", + Key: "token", + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + { + name: "PodMetricsEndpoint missing port and targetPort generates draft with TODO", + input: &pomonitoringv1.PodMonitor{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "monitoring.coreos.com/v1", + Kind: KindPodMonitor, + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "missing-port-monitor", + Namespace: "default", + }, + Spec: pomonitoringv1.PodMonitorSpec{ + Selector: metav1.LabelSelector{ + MatchLabels: map[string]string{"app": "frontend"}, + }, + PodMetricsEndpoints: []pomonitoringv1.PodMetricsEndpoint{ + { + Path: "/metrics", + }, + }, + }, + }, + expected: []runtime.Object{ + &monitoringv1.PodMonitoring{ + TypeMeta: BuildTypeMeta(KindPodMonitoring), + ObjectMeta: metav1.ObjectMeta{ + Name: "missing-port-monitor", + Namespace: "default", + Annotations: map[string]string{ + "gmp.googleapis.com/todo-1": "[ERROR] Endpoint [0] does not specify a 'port' or 'targetPort'. ACTION: Specify a valid port name or number in 'spec.endpoints[].port'.", + }, + }, + Spec: monitoringv1.PodMonitoringSpec{ + Selector: metav1.LabelSelector{ + MatchLabels: map[string]string{ + "app": "frontend", + "gmp.googleapis.com/migration-review-required": "true", + }, + }, + Endpoints: []monitoringv1.ScrapeEndpoint{ + { + Port: intstr.FromString("TODO_SET_PORT"), + Path: "/metrics", + Interval: "30s", + }, + }, + }, + }, + }, }, { name: "Pre-Scrape Relabelings: drop action rule on pod label falls through to metricRelabelings", @@ -2357,6 +2493,131 @@ func TestPodMonitorConversion(t *testing.T) { }, }, }, + { + name: "PodMonitor with proxyUrl containing password injects guardrail label and TODO annotation", + input: &pomonitoringv1.PodMonitor{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "monitoring.coreos.com/v1", + Kind: KindPodMonitor, + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "proxy-pass-monitor", + Namespace: "default", + }, + Spec: pomonitoringv1.PodMonitorSpec{ + Selector: metav1.LabelSelector{ + MatchLabels: map[string]string{"app": "proxy-app"}, + }, + PodMetricsEndpoints: []pomonitoringv1.PodMetricsEndpoint{ + { + Port: "web", + ProxyURL: ptrTo("http://user:secret123@proxy.example.com:8080"), + }, + }, + }, + }, + expected: []runtime.Object{ + &monitoringv1.PodMonitoring{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "monitoring.googleapis.com/v1", + Kind: KindPodMonitoring, + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "proxy-pass-monitor", + Namespace: "default", + Annotations: map[string]string{ + "gmp.googleapis.com/todo-1": "[ERROR] Proxy URL contains embedded plaintext credentials. Credentials were removed. ACTION: Configure proxy authentication via Kubernetes Secret or proxy server configuration.", + }, + }, + Spec: monitoringv1.PodMonitoringSpec{ + Selector: metav1.LabelSelector{ + MatchLabels: map[string]string{ + "app": "proxy-app", + "gmp.googleapis.com/migration-review-required": "true", + }, + }, + Endpoints: []monitoringv1.ScrapeEndpoint{ + { + Port: intstr.FromString("web"), + Interval: "30s", + HTTPClientConfig: monitoringv1.HTTPClientConfig{ + ProxyConfig: monitoringv1.ProxyConfig{ + ProxyURL: "http://proxy.example.com:8080", + }, + }, + }, + }, + }, + }, + }, + }, + { + name: "PodMonitor with missing BasicAuth secret username injects placeholder and TODO annotation", + input: &pomonitoringv1.PodMonitor{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "monitoring.coreos.com/v1", + Kind: KindPodMonitor, + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "missing-secret-monitor", + Namespace: "default", + }, + Spec: pomonitoringv1.PodMonitorSpec{ + Selector: metav1.LabelSelector{ + MatchLabels: map[string]string{"app": "missing-app"}, + }, + PodMetricsEndpoints: []pomonitoringv1.PodMetricsEndpoint{ + { + Port: "web", + BasicAuth: &pomonitoringv1.BasicAuth{ + Username: corev1.SecretKeySelector{LocalObjectReference: corev1.LocalObjectReference{Name: "missing-auth"}, Key: "user"}, + Password: corev1.SecretKeySelector{LocalObjectReference: corev1.LocalObjectReference{Name: "missing-auth"}, Key: "pass"}, + }, + }, + }, + }, + }, + expected: []runtime.Object{ + &monitoringv1.PodMonitoring{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "monitoring.googleapis.com/v1", + Kind: KindPodMonitoring, + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "missing-secret-monitor", + Namespace: "default", + Annotations: map[string]string{ + "gmp.googleapis.com/todo-1": "[ERROR] Referenced SECRET \"missing-auth\" for key \"user\" was not found in migration inputs. ACTION: Verify SECRET \"missing-auth\" exists in namespace \"default\" or provide it in migration inputs.", + }, + }, + Spec: monitoringv1.PodMonitoringSpec{ + Selector: metav1.LabelSelector{ + MatchLabels: map[string]string{ + "app": "missing-app", + "gmp.googleapis.com/migration-review-required": "true", + }, + }, + Endpoints: []monitoringv1.ScrapeEndpoint{ + { + Port: intstr.FromString("web"), + Interval: "30s", + HTTPClientConfig: monitoringv1.HTTPClientConfig{ + BasicAuth: &monitoringv1.BasicAuth{ + Username: "TODO_SET_USER_FROM_SECRET_MISSING-AUTH", + Password: &monitoringv1.SecretSelector{ + Secret: &monitoringv1.SecretKeySelector{ + Name: "missing-auth", + Key: "pass", + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, } converter := &PodMonitorConverter{} diff --git a/pkg/migrate/servicemonitor.go b/pkg/migrate/servicemonitor.go index a3b3422a9f..d89e8b2c51 100644 --- a/pkg/migrate/servicemonitor.go +++ b/pkg/migrate/servicemonitor.go @@ -41,6 +41,7 @@ type serviceGroup struct { portMap map[string]intstr.IntOrString targetLabels map[string]string services []*corev1.Service + todos []todoItem } // namespaces returns a sorted slice of unique namespaces where Services in this group reside. @@ -131,6 +132,9 @@ func (c *ServiceMonitorConverter) convertToMonitoringResources( if err != nil { return nil, nil, fmt.Errorf("failed to search for Services matching selector: %w", err) } + if len(groups) == 0 { + return nil, nil, nil + } if len(groups) > 1 { targetKind := "PodMonitoring" @@ -208,16 +212,44 @@ func (c *ServiceMonitorConverter) findAndGroupServices( return nil, fmt.Errorf("failed to search for Services matching selector: %w", err) } if len(svcs) == 0 { - return nil, errors.New("corresponding Kubernetes Service was not found. Selector and port mappings cannot be resolved") + logger.Warn("Corresponding Kubernetes Service was not found. Emitting draft PodMonitoring with placeholder selector and ports.", + slog.String("servicemonitor", sm.Name)) + portMap := make(map[string]intstr.IntOrString) + for _, ep := range sm.Spec.Endpoints { + k := endpointPortKey(ep) + if k == "" { + k = "TODO_SET_PORT" + } + portMap[k] = intstr.FromString("TODO_RESOLVE_PORT") + } + dummySvc := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: sm.Name, + Namespace: sm.Namespace, + }, + } + return []*serviceGroup{ + { + selector: map[string]string{ + "app": "TODO_SET_POD_SELECTOR", + }, + portMap: portMap, + services: []*corev1.Service{dummySvc}, + todos: []todoItem{ + { + category: "ERROR", + reason: "Corresponding Kubernetes Service was not found. Selector and port mappings could not be resolved.", + action: "Define target pod selector in 'spec.selector.matchLabels' and verify endpoint ports.", + }, + }, + }, + }, nil } groups, err := groupServices(logger, sm.Spec.TargetLabels, sm, svcs) if err != nil { return nil, fmt.Errorf("failed to group Services: %w", err) } - if len(groups) == 0 { - return nil, errors.New("no valid Kubernetes Service groups found. Selector and port mappings cannot be resolved") - } return groups, nil } @@ -243,6 +275,7 @@ func (c *ServiceMonitorConverter) buildSpecForGroup( targetNamespace: sm.Namespace, isClusterScoped: isClusterScoped, } + convCtx.todos = append(convCtx.todos, group.todos...) // Extract pre-scrape relabelings. var relabelConfigs [][]pomonitoringv1.RelabelConfig @@ -276,10 +309,7 @@ func (c *ServiceMonitorConverter) buildSpecForGroup( mergedFromPod := mergeFromPod(logger, convertTargetLabels(logger, sm.Spec.PodTargetLabels, "", "Pod"), rules.ResourceCombined.FromPod) baseSelector := metav1.LabelSelector{MatchLabels: group.selector} - mergedSelector, err := mergeLabelSelector(baseSelector, rules.ResourceCombined.MatchLabels, rules.ResourceCombined.MatchExpressions) - if err != nil { - return nil, err - } + mergedSelector := convCtx.mergeLabelSelector(baseSelector, rules.ResourceCombined.MatchLabels, rules.ResourceCombined.MatchExpressions) var todos []todoItem todos = append(todos, rules.ResourceCombined.Todos...) @@ -324,7 +354,7 @@ func (c *ServiceMonitorConverter) buildSpecForGroup( filterRunning: filterRunning, limits: limits, generatedSecrets: convCtx.getGeneratedSecrets(), - todos: todos, + todos: append(todos, convCtx.todos...), }, nil } @@ -345,9 +375,12 @@ func (c *ServiceMonitorConverter) convertEndpointsForGroup( // 1. Port mapping (Use pre-resolved port from group). portKey := endpointPortKey(ep) + if portKey == "" { + portKey = "TODO_SET_PORT" + } resolvedPort, exists := group.portMap[portKey] if !exists { - return nil, fmt.Errorf("endpoint [%d]: port %q was not resolved for this group", i, portKey) + resolvedPort = intstr.FromString("TODO_RESOLVE_PORT") } gmpEp.Port = resolvedPort @@ -357,10 +390,7 @@ func (c *ServiceMonitorConverter) convertEndpointsForGroup( gmpEp.Params = ep.Params // 3. Scrape Intervals & Timeouts. - interval, timeout, err := resolveScrapeIntervalAndTimeout(convCtx.logger, string(ep.Interval), string(ep.ScrapeTimeout)) - if err != nil { - return nil, fmt.Errorf("endpoint [%d]: %w", i, err) - } + interval, timeout := convCtx.resolveScrapeIntervalAndTimeout(string(ep.Interval), string(ep.ScrapeTimeout)) gmpEp.Interval = interval gmpEp.Timeout = timeout @@ -368,7 +398,7 @@ func (c *ServiceMonitorConverter) convertEndpointsForGroup( gmpEp.MetricRelabeling = combineAndConvertRelabelings(convCtx.logger, epResults[i].PromotedRules, ep.MetricRelabelConfigs) // Proxy Settings. - proxyURL, err := convertProxyURL(ep.ProxyURL) + proxyURL, err := convCtx.convertProxyURL(ep.ProxyURL) if err != nil { return nil, fmt.Errorf("endpoint [%d]: %w", i, err) } @@ -423,21 +453,33 @@ func groupServices( // 1. Extract and validate selector. selectorString := svc.Spec.Selector if len(selectorString) == 0 { - return nil, fmt.Errorf("service %q has no selector (targets external or static endpoints). GMP Managed Collection only supports scraping in-cluster Pod targets", svc.GetName()) + logger.Info("Service targets external endpoints without a pod selector. GMP Managed Collection only supports in-cluster Pods. Skipping resource.", + slog.String("migration_status", "skipped"), + slog.String("service", svc.GetName()), + ) + continue } // 2. Resolve ports for this Service. + var groupTodos []todoItem portMap := make(map[string]intstr.IntOrString) for i, ep := range sm.Spec.Endpoints { portKey := endpointPortKey(ep) if portKey == "" { - return nil, fmt.Errorf("endpoint [%d]: port or targetPort must be set", i) - } - resolvedPort, err := resolveServicePort(logger, svc, portKey) - if err != nil { - return nil, fmt.Errorf("service %q: failed to resolve port %q: %w", svc.Name, portKey, err) + portKey = "TODO_SET_PORT" + groupTodos = append(groupTodos, todoItem{ + category: "ERROR", + reason: fmt.Sprintf("Endpoint [%d] does not specify a 'port' or 'targetPort'.", i), + action: "Specify a valid port name or number in 'spec.endpoints[].port'.", + }) + portMap[portKey] = intstr.FromString("TODO_SET_PORT") + } else { + resolvedPort, todo := resolveServicePort(logger, svc, portKey) + if todo != nil { + groupTodos = append(groupTodos, *todo) + } + portMap[portKey] = resolvedPort } - portMap[portKey] = resolvedPort } // 3. Resolve target labels for this Service. @@ -477,12 +519,14 @@ func groupServices( // Merge complementary mappings. maps.Copy(matchedGroup.portMap, portMap) maps.Copy(matchedGroup.targetLabels, resolvedLabels) + matchedGroup.todos = append(matchedGroup.todos, groupTodos...) } else { groups = append(groups, &serviceGroup{ selector: svc.Spec.Selector, portMap: portMap, targetLabels: resolvedLabels, services: []*corev1.Service{svc}, + todos: groupTodos, }) } } diff --git a/pkg/migrate/servicemonitor_test.go b/pkg/migrate/servicemonitor_test.go index 9f53c5f734..fc5d6ea43d 100644 --- a/pkg/migrate/servicemonitor_test.go +++ b/pkg/migrate/servicemonitor_test.go @@ -672,7 +672,33 @@ func TestServiceMonitorConverter_Convert(t *testing.T) { }, }, }, - wantErr: true, + expected: []runtime.Object{ + &monitoringv1.PodMonitoring{ + TypeMeta: BuildTypeMeta(KindPodMonitoring), + ObjectMeta: metav1.ObjectMeta{ + Name: "my-monitor", + Namespace: "default", + Annotations: map[string]string{ + "gmp.googleapis.com/todo-1": "[ERROR] Corresponding Kubernetes Service was not found. Selector and port mappings could not be resolved. ACTION: Define target pod selector in 'spec.selector.matchLabels' and verify endpoint ports.", + }, + }, + Spec: monitoringv1.PodMonitoringSpec{ + Selector: metav1.LabelSelector{ + MatchLabels: map[string]string{ + "app": "TODO_SET_POD_SELECTOR", + "gmp.googleapis.com/migration-review-required": "true", + }, + }, + Endpoints: []monitoringv1.ScrapeEndpoint{ + { + Port: intstr.FromString("TODO_RESOLVE_PORT"), + Interval: "30s", + }, + }, + }, + }, + }, + wantErr: false, }, { name: "Backing Service has no selector", @@ -698,7 +724,8 @@ func TestServiceMonitorConverter_Convert(t *testing.T) { }, }, }, - wantErr: true, + expected: nil, + wantErr: false, }, { name: "Endpoint missing port and targetPort", @@ -724,7 +751,34 @@ func TestServiceMonitorConverter_Convert(t *testing.T) { }, }, }, - wantErr: true, + expected: []runtime.Object{ + &monitoringv1.PodMonitoring{ + TypeMeta: BuildTypeMeta(KindPodMonitoring), + ObjectMeta: metav1.ObjectMeta{ + Name: "my-monitor", + Namespace: "default", + Annotations: map[string]string{ + "gmp.googleapis.com/todo-1": "[ERROR] Endpoint [0] does not specify a 'port' or 'targetPort'. ACTION: Specify a valid port name or number in 'spec.endpoints[].port'.", + }, + }, + Spec: monitoringv1.PodMonitoringSpec{ + Selector: metav1.LabelSelector{ + MatchLabels: map[string]string{ + "app": "foo-pod", + "gmp.googleapis.com/migration-review-required": "true", + }, + }, + Endpoints: []monitoringv1.ScrapeEndpoint{ + { + Port: intstr.FromString("TODO_SET_PORT"), + Path: "/metrics", + Interval: "30s", + }, + }, + }, + }, + }, + wantErr: false, }, { name: "JobLabel conversion from Service", From 868ac094b22fd1c331e4c0bcc585e2cb91fe6194 Mon Sep 17 00:00:00 2001 From: Karthik Unnikrishnan Date: Fri, 7 Aug 2026 20:37:11 +0000 Subject: [PATCH 04/10] fix: remove dead errors, placeholders for proxyURL and OAuth token --- pkg/migrate/helpers.go | 152 +++++++++++++--------------------- pkg/migrate/helpers_test.go | 96 ++++++++++----------- pkg/migrate/podmonitor.go | 11 +-- pkg/migrate/servicemonitor.go | 11 +-- 4 files changed, 105 insertions(+), 165 deletions(-) diff --git a/pkg/migrate/helpers.go b/pkg/migrate/helpers.go index 5bcbad3964..ad8981934b 100644 --- a/pkg/migrate/helpers.go +++ b/pkg/migrate/helpers.go @@ -552,10 +552,9 @@ func (c *conversionContext) extractConfigMapKey(sel corev1.ConfigMapKeySelector) } // convertConfigMapToSecretSelector translates a ConfigMapKeySelector to a SecretSelector. -// It returns an error if the reference is malformed. -func (c *conversionContext) convertConfigMapToSecretSelector(sel *corev1.ConfigMapKeySelector) (*monitoringv1.SecretSelector, error) { +func (c *conversionContext) convertConfigMapToSecretSelector(sel *corev1.ConfigMapKeySelector) *monitoringv1.SecretSelector { if sel == nil || (sel.Name == "" && sel.Key == "") { - return nil, nil + return nil } name := sel.Name key := sel.Key @@ -622,12 +621,11 @@ func (c *conversionContext) convertConfigMapToSecretSelector(sel *corev1.ConfigM ns = c.targetNamespace } secretRef := &monitoringv1.SecretKeySelector{Name: secretName, Key: secretKey, Namespace: ns} - return &monitoringv1.SecretSelector{Secret: secretRef}, nil + return &monitoringv1.SecretSelector{Secret: secretRef} } // convertSecretOrConfigMapToSecretSelector translates a SecretOrConfigMap to a SecretSelector. -// It returns an error if the selected configuration reference is malformed. -func (c *conversionContext) convertSecretOrConfigMapToSecretSelector(sel pomonitoringv1.SecretOrConfigMap) (*monitoringv1.SecretSelector, error) { +func (c *conversionContext) convertSecretOrConfigMapToSecretSelector(sel pomonitoringv1.SecretOrConfigMap) *monitoringv1.SecretSelector { if sel.Secret != nil { return c.convertSecretSelector(sel.Secret) } @@ -636,14 +634,13 @@ func (c *conversionContext) convertSecretOrConfigMapToSecretSelector(sel pomonit return c.convertConfigMapToSecretSelector(sel.ConfigMap) } - return nil, nil + return nil } // convertSecretSelector translates a SecretKeySelector to a SecretSelector. -// It returns an error if the reference is malformed. -func (c *conversionContext) convertSecretSelector(sel *corev1.SecretKeySelector) (*monitoringv1.SecretSelector, error) { +func (c *conversionContext) convertSecretSelector(sel *corev1.SecretKeySelector) *monitoringv1.SecretSelector { if sel == nil || (sel.Name == "" && sel.Key == "") { - return nil, nil + return nil } name := sel.Name key := sel.Key @@ -672,31 +669,26 @@ func (c *conversionContext) convertSecretSelector(sel *corev1.SecretKeySelector) ns = c.targetNamespace } secretRef := &monitoringv1.SecretKeySelector{Name: name, Key: key, Namespace: ns} - return &monitoringv1.SecretSelector{Secret: secretRef}, nil + return &monitoringv1.SecretSelector{Secret: secretRef} } // convertBasicAuth maps PO BasicAuth to GMP BasicAuth, extracting the username string. -// It returns an error if either the username or password secret reference is malformed or invalid. -func (c *conversionContext) convertBasicAuth(ba *pomonitoringv1.BasicAuth) (*monitoringv1.BasicAuth, error) { +func (c *conversionContext) convertBasicAuth(ba *pomonitoringv1.BasicAuth) *monitoringv1.BasicAuth { if ba == nil { - return nil, nil + return nil } username := c.extractSecretKey(ba.Username) - password, err := c.convertSecretSelector(&ba.Password) - if err != nil { - return nil, err - } + password := c.convertSecretSelector(&ba.Password) return &monitoringv1.BasicAuth{ Username: username, Password: password, - }, nil + } } // convertSafeTLSConfig maps PO SafeTLSConfig to GMP TLS, wrapping ConfigMaps into Secrets. -// It returns an error if any referenced certificate secret or configmap is malformed. -func (c *conversionContext) convertSafeTLSConfig(tls *pomonitoringv1.SafeTLSConfig) (*monitoringv1.TLS, error) { +func (c *conversionContext) convertSafeTLSConfig(tls *pomonitoringv1.SafeTLSConfig) *monitoringv1.TLS { if tls == nil { - return nil, nil + return nil } gmpTLS := &monitoringv1.TLS{} if tls.InsecureSkipVerify != nil { @@ -706,34 +698,21 @@ func (c *conversionContext) convertSafeTLSConfig(tls *pomonitoringv1.SafeTLSConf gmpTLS.ServerName = *tls.ServerName } if tls.CA.Secret != nil || tls.CA.ConfigMap != nil { - ca, err := c.convertSecretOrConfigMapToSecretSelector(tls.CA) - if err != nil { - return nil, err - } - gmpTLS.CA = ca + gmpTLS.CA = c.convertSecretOrConfigMapToSecretSelector(tls.CA) } if tls.Cert.Secret != nil || tls.Cert.ConfigMap != nil { - cert, err := c.convertSecretOrConfigMapToSecretSelector(tls.Cert) - if err != nil { - return nil, err - } - gmpTLS.Cert = cert + gmpTLS.Cert = c.convertSecretOrConfigMapToSecretSelector(tls.Cert) } if tls.KeySecret != nil { - key, err := c.convertSecretSelector(tls.KeySecret) - if err != nil { - return nil, err - } - gmpTLS.Key = key + gmpTLS.Key = c.convertSecretSelector(tls.KeySecret) } - return gmpTLS, nil + return gmpTLS } // convertOAuth2 maps PO OAuth2 to GMP OAuth2, extracting the clientID string. -// It returns an error if any secret or configmap reference is malformed or invalid. -func (c *conversionContext) convertOAuth2(oa *pomonitoringv1.OAuth2) (*monitoringv1.OAuth2, error) { +func (c *conversionContext) convertOAuth2(oa *pomonitoringv1.OAuth2) *monitoringv1.OAuth2 { if oa == nil { - return nil, nil + return nil } clientID := "" if oa.ClientID.Secret != nil { @@ -749,40 +728,40 @@ func (c *conversionContext) convertOAuth2(oa *pomonitoringv1.OAuth2) (*monitorin clientID = "TODO_SET_OAUTH2_CLIENT_ID" } - clientSecret, err := c.convertSecretSelector(&oa.ClientSecret) - if err != nil { - return nil, err + clientSecret := c.convertSecretSelector(&oa.ClientSecret) + + tokenURL := oa.TokenURL + if tokenURL == "" { + c.todos = append(c.todos, todoItem{ + category: "ERROR", + reason: "OAuth2 tokenURL is empty.", + action: "Specify a valid token endpoint URL in 'spec.endpoints[].oauth2.tokenUrl'.", + }) + tokenURL = "TODO_SET_OAUTH2_TOKEN_URL" } return &monitoringv1.OAuth2{ ClientID: clientID, ClientSecret: clientSecret, - TokenURL: oa.TokenURL, + TokenURL: tokenURL, Scopes: oa.Scopes, EndpointParams: oa.EndpointParams, - }, nil + } } // convertAuthorization maps PO SafeAuthorization to GMP Auth. -// It returns an error if the credentials secret reference is malformed. -func (c *conversionContext) convertAuthorization(auth *pomonitoringv1.SafeAuthorization) (*monitoringv1.Auth, error) { +func (c *conversionContext) convertAuthorization(auth *pomonitoringv1.SafeAuthorization) *monitoringv1.Auth { if auth == nil { - return nil, nil + return nil } - var ( - credentials *monitoringv1.SecretSelector - err error - ) + var credentials *monitoringv1.SecretSelector if auth.Credentials != nil { - credentials, err = c.convertSecretSelector(auth.Credentials) - if err != nil { - return nil, err - } + credentials = c.convertSecretSelector(auth.Credentials) } return &monitoringv1.Auth{ Type: auth.Type, Credentials: credentials, - }, nil + } } // applyAuthAndTLS converts credentials and TLS settings for a generic endpoint. @@ -793,38 +772,18 @@ func (c *conversionContext) applyAuthAndTLS( tlsConfig *pomonitoringv1.SafeTLSConfig, authorization *pomonitoringv1.SafeAuthorization, bearerTokenSecret corev1.SecretKeySelector, -) error { - if gmpEp == nil { - return errors.New("scrape endpoint cannot be nil") - } - +) { if basicAuth != nil { - ba, err := c.convertBasicAuth(basicAuth) - if err != nil { - return fmt.Errorf("basicAuth: %w", err) - } - gmpEp.BasicAuth = ba + gmpEp.BasicAuth = c.convertBasicAuth(basicAuth) } if oAuth2 != nil { - oa, err := c.convertOAuth2(oAuth2) - if err != nil { - return fmt.Errorf("oAuth2: %w", err) - } - gmpEp.OAuth2 = oa + gmpEp.OAuth2 = c.convertOAuth2(oAuth2) } if tlsConfig != nil { - tls, err := c.convertSafeTLSConfig(tlsConfig) - if err != nil { - return fmt.Errorf("tlsConfig: %w", err) - } - gmpEp.TLS = tls + gmpEp.TLS = c.convertSafeTLSConfig(tlsConfig) } if authorization != nil { - auth, err := c.convertAuthorization(authorization) - if err != nil { - return fmt.Errorf("authorization: %w", err) - } - gmpEp.Authorization = auth + gmpEp.Authorization = c.convertAuthorization(authorization) } // Handle deprecated BearerTokenSecret -> Authorization. @@ -833,14 +792,9 @@ func (c *conversionContext) applyAuthAndTLS( c.logger.Warn("Endpoint has both 'bearerTokenSecret' and 'authorization' defined. Dropping 'bearerTokenSecret'.") } else { tokenSecret := bearerTokenSecret // nolint:staticcheck // Map deprecated BearerTokenSecret for backwards compatibility. - auth, err := c.convertAuthorization(&pomonitoringv1.SafeAuthorization{Credentials: &tokenSecret}) - if err != nil { - return fmt.Errorf("bearerTokenSecret: %w", err) - } - gmpEp.Authorization = auth + gmpEp.Authorization = c.convertAuthorization(&pomonitoringv1.SafeAuthorization{Credentials: &tokenSecret}) } } - return nil } func convertMetricRelabelings( @@ -1450,13 +1404,21 @@ func (c *conversionContext) resolveScrapeIntervalAndTimeout(interval, timeout st return interval, timeout } -// convertProxyURL verifies proxy URL credentials and attaches a TODO if passwords are present. -func (c *conversionContext) convertProxyURL(proxyURL *string) (string, error) { +// convertProxyURL verifies proxy URL credentials and attaches a TODO if passwords are present or malformed. +func (c *conversionContext) convertProxyURL(proxyURL *string) string { if proxyURL == nil { - return "", nil + return "" } parsed, err := url.Parse(*proxyURL) - if err == nil && parsed.User != nil { + if err != nil { + c.todos = append(c.todos, todoItem{ + category: "ERROR", + reason: fmt.Sprintf("Proxy URL %q is invalid or malformed.", *proxyURL), + action: "Specify a valid proxy URL (e.g. 'http://proxy.example.com:8080').", + }) + return "TODO_SET_VALID_PROXY_URL" + } + if parsed.User != nil { if _, hasPass := parsed.User.Password(); hasPass { c.todos = append(c.todos, todoItem{ category: "ERROR", @@ -1464,10 +1426,10 @@ func (c *conversionContext) convertProxyURL(proxyURL *string) (string, error) { action: "Configure proxy authentication via Kubernetes Secret or proxy server configuration.", }) parsed.User = nil - return parsed.String(), nil + return parsed.String() } } - return *proxyURL, nil + return *proxyURL } // warnUnsupportedEndpointFields logs warnings for fields that GMP does not support. diff --git a/pkg/migrate/helpers_test.go b/pkg/migrate/helpers_test.go index 4c71d24ea0..a0712581f0 100644 --- a/pkg/migrate/helpers_test.go +++ b/pkg/migrate/helpers_test.go @@ -318,10 +318,7 @@ func TestConvertConfigMapToSecretSelector(t *testing.T) { t.Fatalf("failed to setup cache: %v", err) } - gmpSel, err := ctx.convertConfigMapToSecretSelector(tc.selector) - if (err != nil) != tc.wantErr { - t.Fatalf("convertConfigMapToSecretSelector() error = %v, wantErr %v", err, tc.wantErr) - } + gmpSel := ctx.convertConfigMapToSecretSelector(tc.selector) if tc.selector == nil { if gmpSel != nil { @@ -329,9 +326,6 @@ func TestConvertConfigMapToSecretSelector(t *testing.T) { } return } - if tc.wantErr { - return - } if gmpSel == nil || gmpSel.Secret == nil || gmpSel.Secret.Name != tc.expectedSecretName || gmpSel.Secret.Key != tc.expectedSecretKey { t.Errorf("unexpected secret selector: %+v", gmpSel) @@ -404,10 +398,7 @@ func TestConvertBasicAuth(t *testing.T) { t.Fatalf("failed to setup cache: %v", err) } - gmpBA, err := ctx.convertBasicAuth(tc.basicAuth) - if (err != nil) != tc.wantErr { - t.Fatalf("convertBasicAuth() error = %v, wantErr %v", err, tc.wantErr) - } + gmpBA := ctx.convertBasicAuth(tc.basicAuth) if tc.basicAuth == nil { if gmpBA != nil { @@ -415,9 +406,6 @@ func TestConvertBasicAuth(t *testing.T) { } return } - if tc.wantErr { - return - } if gmpBA.Username != tc.expectedUser { t.Errorf("expected username %s, got %s", tc.expectedUser, gmpBA.Username) @@ -493,10 +481,7 @@ func TestConvertSafeTLSConfig(t *testing.T) { t.Fatalf("failed to setup cache: %v", err) } - gmpTLS, err := ctx.convertSafeTLSConfig(tc.tlsConfig) - if (err != nil) != tc.wantErr { - t.Fatalf("convertSafeTLSConfig() error = %v, wantErr %v", err, tc.wantErr) - } + gmpTLS := ctx.convertSafeTLSConfig(tc.tlsConfig) if tc.tlsConfig == nil { if gmpTLS != nil { @@ -504,9 +489,6 @@ func TestConvertSafeTLSConfig(t *testing.T) { } return } - if tc.wantErr { - return - } if tc.expectedCAName != "" { if gmpTLS.CA == nil || gmpTLS.CA.Secret == nil || gmpTLS.CA.Secret.Name != tc.expectedCAName || gmpTLS.CA.Secret.Key != tc.expectedCAKey { @@ -536,14 +518,13 @@ func TestConvertOAuth2(t *testing.T) { expectedClientID string expectedSecName string expectedSecKey string + expectedTokenURL string expectTodos int - wantErr bool }{ { name: "Nil OAuth2 returns nil", setupCache: func(_ *ResourceCache) error { return nil }, oauth2: nil, - wantErr: false, }, { name: "Valid OAuth2 from Secret", @@ -566,8 +547,8 @@ func TestConvertOAuth2(t *testing.T) { expectedClientID: "my-client", expectedSecName: "oauth-sec", expectedSecKey: "client_secret", + expectedTokenURL: "https://auth.example.com/token", expectTodos: 0, - wantErr: false, }, { name: "Empty ClientID generates placeholder and TODO", @@ -582,8 +563,32 @@ func TestConvertOAuth2(t *testing.T) { expectedClientID: "TODO_SET_OAUTH2_CLIENT_ID", expectedSecName: "oauth-sec", expectedSecKey: "client_secret", + expectedTokenURL: "https://auth.example.com/token", + expectTodos: 1, + }, + { + name: "Empty TokenURL generates placeholder and TODO", + setupCache: func(cache *ResourceCache) error { + return addSecretToCache(cache, "default", "oauth-sec", "client_id", "my-client", true) + }, + oauth2: &pomonitoringv1.OAuth2{ + ClientID: pomonitoringv1.SecretOrConfigMap{ + Secret: &corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{Name: "oauth-sec"}, + Key: "client_id", + }, + }, + ClientSecret: corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{Name: "oauth-sec"}, + Key: "client_secret", + }, + TokenURL: "", + }, + expectedClientID: "my-client", + expectedSecName: "oauth-sec", + expectedSecKey: "client_secret", + expectedTokenURL: "TODO_SET_OAUTH2_TOKEN_URL", expectTodos: 1, - wantErr: false, }, } @@ -594,22 +599,19 @@ func TestConvertOAuth2(t *testing.T) { t.Fatalf("failed to setup cache: %v", err) } - res, err := ctx.convertOAuth2(tc.oauth2) - if (err != nil) != tc.wantErr { - t.Fatalf("convertOAuth2() error = %v, wantErr %v", err, tc.wantErr) - } + res := ctx.convertOAuth2(tc.oauth2) if tc.oauth2 == nil { if res != nil { t.Errorf("expected nil result for nil OAuth2, got %+v", res) } return } - if tc.wantErr { - return - } if res.ClientID != tc.expectedClientID { t.Errorf("expected ClientID %q, got %q", tc.expectedClientID, res.ClientID) } + if res.TokenURL != tc.expectedTokenURL { + t.Errorf("expected TokenURL %q, got %q", tc.expectedTokenURL, res.TokenURL) + } if res.ClientSecret.Secret == nil || res.ClientSecret.Secret.Name != tc.expectedSecName || res.ClientSecret.Secret.Key != tc.expectedSecKey { t.Errorf("unexpected ClientSecret selector: %+v", res.ClientSecret) } @@ -633,19 +635,13 @@ func TestConvertConfigMapToSecretSelectorDeduplication(t *testing.T) { } // Call first time. - gmpSel1, err := ctx.convertConfigMapToSecretSelector(selector) - if err != nil { - t.Fatalf("first call failed with error: %v", err) - } + gmpSel1 := ctx.convertConfigMapToSecretSelector(selector) if gmpSel1 == nil || gmpSel1.Secret.Name != "secret-tls-cm" { t.Fatal("first call failed to translate selector") } // Call second time. - gmpSel2, err := ctx.convertConfigMapToSecretSelector(selector) - if err != nil { - t.Fatalf("second call failed with error: %v", err) - } + gmpSel2 := ctx.convertConfigMapToSecretSelector(selector) if gmpSel2 == nil || gmpSel2.Secret.Name != "secret-tls-cm" { t.Fatal("second call failed to translate selector") } @@ -880,20 +876,19 @@ func TestConvertProxyURL(t *testing.T) { proxyURL: ptrTo("http://user:pass@proxy.example.com:8080"), expectedURL: "http://proxy.example.com:8080", expectTodos: 1, - expectErr: false, + }, + { + name: "malformed proxyURL returns placeholder and adds todo", + proxyURL: ptrTo("://invalid-url"), + expectedURL: "TODO_SET_VALID_PROXY_URL", + expectTodos: 1, }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { convCtx := &conversionContext{} - url, err := convCtx.convertProxyURL(tc.proxyURL) - if (err != nil) != tc.expectErr { - t.Fatalf("convertProxyURL() error = %v, expectErr = %v", err, tc.expectErr) - } - if tc.expectErr { - return - } + url := convCtx.convertProxyURL(tc.proxyURL) if url != tc.expectedURL { t.Errorf("convertProxyURL() = %v, want %v", url, tc.expectedURL) } @@ -1076,10 +1071,7 @@ func TestDecoupledNamespaces(t *testing.T) { LocalObjectReference: corev1.LocalObjectReference{Name: "tls-cm"}, Key: "ca.crt", } - secretSel, err := ctx.convertConfigMapToSecretSelector(cmSel) - if err != nil { - t.Fatalf("convertConfigMapToSecretSelector() unexpected error: %v", err) - } + secretSel := ctx.convertConfigMapToSecretSelector(cmSel) if secretSel.Secret.Namespace != "target-ns" { t.Errorf("expected selector namespace %q, got %q", "target-ns", secretSel.Secret.Namespace) } diff --git a/pkg/migrate/podmonitor.go b/pkg/migrate/podmonitor.go index f098ddb80e..e3c3e3398d 100644 --- a/pkg/migrate/podmonitor.go +++ b/pkg/migrate/podmonitor.go @@ -139,20 +139,13 @@ func (c *PodMonitorConverter) convertEndpoints( gmpEp.MetricRelabeling = combineAndConvertRelabelings(convCtx.logger, epResults[i].PromotedRules, ep.MetricRelabelConfigs) // Proxy Settings. - proxyURL, err := convCtx.convertProxyURL(ep.ProxyURL) - if err != nil { - return nil, fmt.Errorf("endpoint [%d]: %w", i, err) - } - gmpEp.ProxyURL = proxyURL + gmpEp.ProxyURL = convCtx.convertProxyURL(ep.ProxyURL) // noProxy, proxyConnectHeader, and proxyFromEnvironment fields are silently dropped. // The pinned Prometheus Operator version lacks these fields, and GMP does not support them anyway. // Auth & TLS mappings. - err = convCtx.applyAuthAndTLS(&gmpEp, ep.BasicAuth, ep.OAuth2, ep.TLSConfig, ep.Authorization, ep.BearerTokenSecret) // nolint:staticcheck // Map deprecated BearerTokenSecret for backwards compatibility. - if err != nil { - return nil, fmt.Errorf("endpoint [%d]: %w", i, err) - } + convCtx.applyAuthAndTLS(&gmpEp, ep.BasicAuth, ep.OAuth2, ep.TLSConfig, ep.Authorization, ep.BearerTokenSecret) // nolint:staticcheck // Map deprecated BearerTokenSecret for backwards compatibility. // 5. Warnings for Unsupported Fields in Endpoint. warnUnsupportedEndpointFields(convCtx.logger, ep.FollowRedirects, ep.EnableHttp2, ep.HonorLabels, ep.HonorTimestamps, ep.TrackTimestampsStaleness, i) diff --git a/pkg/migrate/servicemonitor.go b/pkg/migrate/servicemonitor.go index d89e8b2c51..05c4555b42 100644 --- a/pkg/migrate/servicemonitor.go +++ b/pkg/migrate/servicemonitor.go @@ -398,11 +398,7 @@ func (c *ServiceMonitorConverter) convertEndpointsForGroup( gmpEp.MetricRelabeling = combineAndConvertRelabelings(convCtx.logger, epResults[i].PromotedRules, ep.MetricRelabelConfigs) // Proxy Settings. - proxyURL, err := convCtx.convertProxyURL(ep.ProxyURL) - if err != nil { - return nil, fmt.Errorf("endpoint [%d]: %w", i, err) - } - gmpEp.ProxyURL = proxyURL + gmpEp.ProxyURL = convCtx.convertProxyURL(ep.ProxyURL) // Auth & TLS mappings. var safeTLS *pomonitoringv1.SafeTLSConfig @@ -426,10 +422,7 @@ func (c *ServiceMonitorConverter) convertEndpointsForGroup( // nolint:staticcheck // Map deprecated BearerTokenSecret for backwards compatibility. bearerTokenSecret = *ep.BearerTokenSecret } - err = convCtx.applyAuthAndTLS(&gmpEp, ep.BasicAuth, ep.OAuth2, safeTLS, ep.Authorization, bearerTokenSecret) // nolint:staticcheck - if err != nil { - return nil, fmt.Errorf("endpoint [%d]: %w", i, err) - } + convCtx.applyAuthAndTLS(&gmpEp, ep.BasicAuth, ep.OAuth2, safeTLS, ep.Authorization, bearerTokenSecret) // nolint:staticcheck // Warnings for Unsupported Fields. warnUnsupportedEndpointFields(convCtx.logger, ep.FollowRedirects, ep.EnableHttp2, ep.HonorLabels, ep.HonorTimestamps, ep.TrackTimestampsStaleness, i) From 249532222903ca88a1e6de6c8b9666a53fce7891 Mon Sep 17 00:00:00 2001 From: Karthik Unnikrishnan Date: Mon, 10 Aug 2026 02:20:56 +0000 Subject: [PATCH 05/10] fix: add back warning logs --- pkg/migrate/helpers.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/pkg/migrate/helpers.go b/pkg/migrate/helpers.go index ad8981934b..05fde6fc09 100644 --- a/pkg/migrate/helpers.go +++ b/pkg/migrate/helpers.go @@ -129,7 +129,7 @@ func CopyObjectMeta(src metav1.ObjectMeta, targetNamespace string, logger *slog. } if len(src.Labels) > 0 || len(src.Annotations) > 0 { - logger.Warn("Stripped all metadata labels and annotations. Reconfigure them manually if needed") + logger.Info("Stripped all metadata labels and annotations. Reconfigure them manually if needed") } return dst @@ -1201,6 +1201,9 @@ func buildPodMonitoring( for _, td := range spec.todos { AddMigrationTodo(u, td.category, td.reason, td.action) + if logger != nil { + logger.Warn(td.reason, slog.String("action", td.action)) + } } if len(spec.todos) > 0 { if err := InjectSafetyGuardrail(u); err != nil { @@ -1247,6 +1250,9 @@ func buildClusterPodMonitoring( for _, td := range spec.todos { AddMigrationTodo(u, td.category, td.reason, td.action) + if logger != nil { + logger.Warn(td.reason, slog.String("action", td.action)) + } } if len(spec.todos) > 0 { if err := InjectSafetyGuardrail(u); err != nil { From 7c4156eada7f190f58996d591389e734d94ac69d Mon Sep 17 00:00:00 2001 From: Karthik Unnikrishnan Date: Mon, 10 Aug 2026 02:38:01 +0000 Subject: [PATCH 06/10] fix: make resource splitting a warning --- pkg/migrate/servicemonitor.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/migrate/servicemonitor.go b/pkg/migrate/servicemonitor.go index 05c4555b42..832d0c6ffe 100644 --- a/pkg/migrate/servicemonitor.go +++ b/pkg/migrate/servicemonitor.go @@ -141,7 +141,7 @@ func (c *ServiceMonitorConverter) convertToMonitoringResources( if isClusterScoped { targetKind = "ClusterPodMonitoring" } - logger.Warn(fmt.Sprintf("Services matched by selector have conflicts (different selectors, port mappings, or labels). Splitting into multiple %s resources.", targetKind), + logger.Info(fmt.Sprintf("Services matched by selector have conflicts (different selectors, port mappings, or labels). Splitting into multiple %s resources.", targetKind), slog.Int("total_groups", len(groups)), slog.String("servicemonitor", sm.Name)) } From 4b2762fd0be897faebfe3647a62d56d87b46c745 Mon Sep 17 00:00:00 2001 From: Karthik Unnikrishnan Date: Mon, 10 Aug 2026 15:29:31 +0000 Subject: [PATCH 07/10] fix: separate TODO vs non-TODO warnings --- pkg/migrate/helpers.go | 4 ++-- pkg/migrate/logger.go | 25 ++++++++++++++----------- pkg/migrate/migrate.go | 6 +++++- pkg/migrate/migrate_test.go | 6 +++++- 4 files changed, 26 insertions(+), 15 deletions(-) diff --git a/pkg/migrate/helpers.go b/pkg/migrate/helpers.go index 05fde6fc09..d9c9d4ec9e 100644 --- a/pkg/migrate/helpers.go +++ b/pkg/migrate/helpers.go @@ -1202,7 +1202,7 @@ func buildPodMonitoring( for _, td := range spec.todos { AddMigrationTodo(u, td.category, td.reason, td.action) if logger != nil { - logger.Warn(td.reason, slog.String("action", td.action)) + logger.Warn(td.reason, slog.String("action", td.action), slog.String("migration_status", "action_items")) } } if len(spec.todos) > 0 { @@ -1251,7 +1251,7 @@ func buildClusterPodMonitoring( for _, td := range spec.todos { AddMigrationTodo(u, td.category, td.reason, td.action) if logger != nil { - logger.Warn(td.reason, slog.String("action", td.action)) + logger.Warn(td.reason, slog.String("action", td.action), slog.String("migration_status", "action_items")) } } if len(spec.todos) > 0 { diff --git a/pkg/migrate/logger.go b/pkg/migrate/logger.go index 47304e9b21..008752c719 100644 --- a/pkg/migrate/logger.go +++ b/pkg/migrate/logger.go @@ -31,16 +31,11 @@ type ResourceStatus int const ( StatusSuccess ResourceStatus = iota // 0 (Migrated Successfully). StatusSkipped // 1 (Skipped / Unsupported). - StatusActionItems // 2 (Migrated with Action Items). - StatusFailed // 3 (Failed). + StatusWarnings // 2 (Migrated with Warnings). + StatusActionItems // 3 (Migrated with Action Items). + StatusFailed // 4 (Failed). ) -// statusLevels maps slog.Levels to their corresponding ResourceStatus. -var statusLevels = map[slog.Level]ResourceStatus{ - slog.LevelWarn: StatusActionItems, - slog.LevelError: StatusFailed, -} - // loggerState encapsulates the shared, thread-safe state across all handler clones. type loggerState struct { mu sync.Mutex @@ -164,15 +159,23 @@ func (h *ConsoleHandler) Handle(_ context.Context, r slog.Record) error { } if key != "" { - if r.Level == slog.LevelInfo { + switch r.Level { + case slog.LevelInfo: switch migrationStatus { case "skipped": h.trackStatus(key, StatusSkipped) case "success": h.trackStatus(key, StatusSuccess) } - } else if status, ok := statusLevels[r.Level]; ok { - h.trackStatus(key, status) + case slog.LevelWarn: + switch migrationStatus { + case "action_items": + h.trackStatus(key, StatusActionItems) + default: + h.trackStatus(key, StatusWarnings) + } + case slog.LevelError: + h.trackStatus(key, StatusFailed) } } diff --git a/pkg/migrate/migrate.go b/pkg/migrate/migrate.go index bd85f2518d..60fb747090 100644 --- a/pkg/migrate/migrate.go +++ b/pkg/migrate/migrate.go @@ -37,7 +37,8 @@ import ( // MigrationReport accumulates the statistics and payloads of the migration run. type MigrationReport struct { - SuccessCount int // Successfully migrated with no action items. + SuccessCount int // Successfully migrated with no warnings or action items. + WarningsCount int // Migrated with non-blocking warnings (e.g. dropped unsupported fields). ActionItemsCount int // Successfully migrated but had TODO annotations or guardrails. SkippedCount int // Bypassed because resource is unsupported/out-of-scope. FailedCount int // Fatal failure, resource skipped. @@ -127,6 +128,8 @@ func (m *Migrator) Run(inputPaths ...string) (*MigrationReport, error) { report.SuccessCount++ case StatusSkipped: report.SkippedCount++ + case StatusWarnings: + report.WarningsCount++ case StatusActionItems: report.ActionItemsCount++ case StatusFailed: @@ -142,6 +145,7 @@ func (m *Migrator) PrintSummary(r *MigrationReport) { fmt.Fprintln(m.Stderr, "\n=========================================") fmt.Fprintln(m.Stderr, "Migration Complete Summary:") fmt.Fprintf(m.Stderr, " Successfully Migrated: %d\n", r.SuccessCount) + fmt.Fprintf(m.Stderr, " Migrated with Warnings: %d\n", r.WarningsCount) fmt.Fprintf(m.Stderr, " Migrated with Action Items: %d\n", r.ActionItemsCount) fmt.Fprintf(m.Stderr, " Skipped (Unsupported): %d\n", r.SkippedCount) fmt.Fprintf(m.Stderr, " Failed: %d\n", r.FailedCount) diff --git a/pkg/migrate/migrate_test.go b/pkg/migrate/migrate_test.go index 90cd1face7..58898e2132 100644 --- a/pkg/migrate/migrate_test.go +++ b/pkg/migrate/migrate_test.go @@ -429,16 +429,20 @@ func TestMigratorPrintSummary(t *testing.T) { }, wantContains: []string{ "Successfully Migrated: 2", + "Migrated with Warnings: 0", "Migrated with Action Items: 0", }, }, { - name: "Report with action items includes guidance note", + name: "Report with warnings and action items includes guidance note", report: &MigrationReport{ SuccessCount: 1, + WarningsCount: 1, ActionItemsCount: 1, }, wantContains: []string{ + "Successfully Migrated: 1", + "Migrated with Warnings: 1", "Migrated with Action Items: 1", "gmp.googleapis.com/migration-review-required", }, From e0f81fffc9540eca110ab96e75f47025bcbbc13a Mon Sep 17 00:00:00 2001 From: Karthik Unnikrishnan Date: Tue, 11 Aug 2026 14:11:14 +0000 Subject: [PATCH 08/10] fix: modify to use --all --- cmd/gmp-migrate/main.go | 21 +++++++++++-- pkg/migrate/helpers.go | 28 ------------------ pkg/migrate/helpers_test.go | 47 ------------------------------ pkg/migrate/migrate.go | 40 ++++++++++++++++++++----- pkg/migrate/migrate_test.go | 29 ++++++++++++++++-- pkg/migrate/podmonitor_test.go | 19 ++---------- pkg/migrate/servicemonitor_test.go | 2 -- 7 files changed, 79 insertions(+), 107 deletions(-) diff --git a/cmd/gmp-migrate/main.go b/cmd/gmp-migrate/main.go index 72ae25bd1c..80e24df698 100644 --- a/cmd/gmp-migrate/main.go +++ b/cmd/gmp-migrate/main.go @@ -49,6 +49,10 @@ func main() { flag.Var(&inputFiles, "file", "Input source (YAML file, directory, or '-' for stdin) (Required)") flag.Var(&inputFiles, "f", "Input source (YAML file, directory, or '-' for stdin) (Required)") + var emitAll bool + flag.BoolVar(&emitAll, "all", false, "Emit all manifests, including best-effort draft configurations with TODO annotations") + flag.BoolVar(&emitAll, "a", false, "Emit all manifests, including best-effort draft configurations with TODO annotations") + flag.Usage = func() { fmt.Fprintf(os.Stderr, "Usage of %s:\n", os.Args[0]) fmt.Fprint(os.Stderr, "Migrate Prometheus Operator configurations to Google Managed Prometheus (GMP).\n\n") @@ -87,19 +91,30 @@ func main() { // If any resource failed to convert in-memory, we print summary and abort. if report.FailedCount > 0 { - migrator.PrintSummary(report) // Still print the diagnostic summary to Stderr. + migrator.PrintSummary(report, emitAll) // Still print the diagnostic summary to Stderr. slog.Error("Migration aborted: resources failed conversion. Zero manifests were written to Stdout.", slog.Int("failures", report.FailedCount), ) os.Exit(1) } + // Select outputs based on --all flag. + outputsToWrite := report.ReadyOutputs + if emitAll { + outputsToWrite = report.Outputs + } + // Write the converted GMP manifests using the migrator's Stdout stream. - if err := migrator.WriteOutputs(report.Outputs); err != nil { + if err := migrator.WriteOutputs(outputsToWrite); err != nil { slog.Error("Failed to write outputs", slog.Any("error", err)) os.Exit(1) } // Print the successful complete summary to Stderr. - migrator.PrintSummary(report) + migrator.PrintSummary(report, emitAll) + + // If any resource required action items or failed, exit with 1. + if report.ActionItemsCount > 0 || report.FailedCount > 0 { + os.Exit(1) + } } diff --git a/pkg/migrate/helpers.go b/pkg/migrate/helpers.go index d9c9d4ec9e..fe13c76c75 100644 --- a/pkg/migrate/helpers.go +++ b/pkg/migrate/helpers.go @@ -49,8 +49,6 @@ const ( const ( AnnotationTodoPrefix = "gmp.googleapis.com/todo-" - GuardrailLabelKey = "gmp.googleapis.com/migration-review-required" - GuardrailLabelValue = "true" ) // Constants representing the supported ScrapeProtocol enum values defined in upstream Prometheus Operator. @@ -157,22 +155,6 @@ func AddMigrationTodo(u *unstructured.Unstructured, category, reason, action str u.SetAnnotations(annotations) } -// InjectSafetyGuardrail adds a non-matching label to spec.selector.matchLabels to prevent accidental target scraping. -func InjectSafetyGuardrail(u *unstructured.Unstructured) error { - if u == nil || u.Object == nil { - return errors.New("cannot inject guardrail into nil unstructured resource") - } - labelsMap, found, err := unstructured.NestedStringMap(u.Object, "spec", "selector", "matchLabels") - if err != nil { - return fmt.Errorf("failed to read spec.selector.matchLabels: %w", err) - } - if !found || labelsMap == nil { - labelsMap = make(map[string]string) - } - labelsMap[GuardrailLabelKey] = GuardrailLabelValue - return unstructured.SetNestedStringMap(u.Object, labelsMap, "spec", "selector", "matchLabels") -} - // parseAndCleanNamespaces trims whitespace, filters out empty strings, and deduplicates namespaces. func parseAndCleanNamespaces(namespaces []string) []string { unique := make(map[string]bool) @@ -1205,11 +1187,6 @@ func buildPodMonitoring( logger.Warn(td.reason, slog.String("action", td.action), slog.String("migration_status", "action_items")) } } - if len(spec.todos) > 0 { - if err := InjectSafetyGuardrail(u); err != nil { - return nil, err - } - } return u, nil } @@ -1254,11 +1231,6 @@ func buildClusterPodMonitoring( logger.Warn(td.reason, slog.String("action", td.action), slog.String("migration_status", "action_items")) } } - if len(spec.todos) > 0 { - if err := InjectSafetyGuardrail(u); err != nil { - return nil, err - } - } return u, nil } diff --git a/pkg/migrate/helpers_test.go b/pkg/migrate/helpers_test.go index a0712581f0..fe08a820d7 100644 --- a/pkg/migrate/helpers_test.go +++ b/pkg/migrate/helpers_test.go @@ -1480,50 +1480,3 @@ func TestAddMigrationTodo(t *testing.T) { }) } } - -func TestInjectSafetyGuardrail(t *testing.T) { - tests := []struct { - name string - initialObj map[string]any - expectedMatch map[string]string - }{ - { - name: "inject into empty matchLabels", - initialObj: map[string]any{ - "spec": map[string]any{}, - }, - expectedMatch: map[string]string{ - "gmp.googleapis.com/migration-review-required": "true", - }, - }, - { - name: "preserve existing matchLabels", - initialObj: map[string]any{ - "spec": map[string]any{ - "selector": map[string]any{ - "matchLabels": map[string]any{ - "app": "frontend", - }, - }, - }, - }, - expectedMatch: map[string]string{ - "app": "frontend", - "gmp.googleapis.com/migration-review-required": "true", - }, - }, - } - - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - u := &unstructured.Unstructured{Object: tc.initialObj} - if err := InjectSafetyGuardrail(u); err != nil { - t.Fatalf("InjectSafetyGuardrail() unexpected error: %v", err) - } - gotMatch, _, _ := unstructured.NestedStringMap(u.Object, "spec", "selector", "matchLabels") - if diff := cmp.Diff(tc.expectedMatch, gotMatch); diff != "" { - t.Errorf("InjectSafetyGuardrail() mismatch (-want +got):\n%s", diff) - } - }) - } -} diff --git a/pkg/migrate/migrate.go b/pkg/migrate/migrate.go index 60fb747090..2d4316221b 100644 --- a/pkg/migrate/migrate.go +++ b/pkg/migrate/migrate.go @@ -39,10 +39,11 @@ import ( type MigrationReport struct { SuccessCount int // Successfully migrated with no warnings or action items. WarningsCount int // Migrated with non-blocking warnings (e.g. dropped unsupported fields). - ActionItemsCount int // Successfully migrated but had TODO annotations or guardrails. + ActionItemsCount int // Successfully migrated but had TODO annotations. SkippedCount int // Bypassed because resource is unsupported/out-of-scope. FailedCount int // Fatal failure, resource skipped. - Outputs []*unstructured.Unstructured // Converted GMP manifests in-memory. + Outputs []*unstructured.Unstructured // All converted GMP manifests in-memory. + ReadyOutputs []*unstructured.Unstructured // Only 100% ready manifests (0 TODO annotations). } // Migrator orchestrates the migration process. @@ -121,6 +122,14 @@ func (m *Migrator) Run(inputPaths ...string) (*MigrationReport, error) { outputs := m.convertResources() report.Outputs = outputs + var readyOutputs []*unstructured.Unstructured + for _, out := range outputs { + if !hasTodoAnnotations(out) { + readyOutputs = append(readyOutputs, out) + } + } + report.ReadyOutputs = readyOutputs + // 4. Calculate final statistics from the handler's tracked statuses. for _, status := range handler.ResourceStatuses() { switch status { @@ -140,8 +149,21 @@ func (m *Migrator) Run(inputPaths ...string) (*MigrationReport, error) { return report, nil } +// hasTodoAnnotations returns true if the resource contains any GMP migration TODO annotations. +func hasTodoAnnotations(u *unstructured.Unstructured) bool { + if u == nil { + return false + } + for k := range u.GetAnnotations() { + if strings.HasPrefix(k, AnnotationTodoPrefix) { + return true + } + } + return false +} + // PrintSummary formats and writes the standardized migration report summary. -func (m *Migrator) PrintSummary(r *MigrationReport) { +func (m *Migrator) PrintSummary(r *MigrationReport, emitAll bool) { fmt.Fprintln(m.Stderr, "\n=========================================") fmt.Fprintln(m.Stderr, "Migration Complete Summary:") fmt.Fprintf(m.Stderr, " Successfully Migrated: %d\n", r.SuccessCount) @@ -151,10 +173,14 @@ func (m *Migrator) PrintSummary(r *MigrationReport) { fmt.Fprintf(m.Stderr, " Failed: %d\n", r.FailedCount) fmt.Fprintln(m.Stderr, "=========================================") if r.ActionItemsCount > 0 { - fmt.Fprintln(m.Stderr, "\nNOTE: Some resources were migrated with action items and contain TODO annotations.") - fmt.Fprintln(m.Stderr, "These resources include the safety guardrail label:") - fmt.Fprintln(m.Stderr, " 'gmp.googleapis.com/migration-review-required: \"true\"'") - fmt.Fprintln(m.Stderr, "Review the TODO annotations in the generated manifests and remove this label when ready to activate scraping.") + if !emitAll { + fmt.Fprintf(m.Stderr, "\nNOTE: Emitted %d ready manifests to Stdout.\n", len(r.ReadyOutputs)) + fmt.Fprintf(m.Stderr, "%d manifests with action items were omitted from Stdout as they contain best-effort draft configurations with TODO annotations and placeholders.\n", r.ActionItemsCount) + fmt.Fprintln(m.Stderr, "Run with '--all' to output all manifests for review.") + } else { + fmt.Fprintf(m.Stderr, "\nNOTE: %d manifests contain best-effort draft configurations with TODO annotations and placeholders.\n", r.ActionItemsCount) + fmt.Fprintln(m.Stderr, "Review the inline 'gmp.googleapis.com/todo-*' annotations in the generated manifests before applying to a cluster.") + } } } diff --git a/pkg/migrate/migrate_test.go b/pkg/migrate/migrate_test.go index 58898e2132..b2ddc11f29 100644 --- a/pkg/migrate/migrate_test.go +++ b/pkg/migrate/migrate_test.go @@ -420,6 +420,7 @@ func TestMigratorPrintSummary(t *testing.T) { tests := []struct { name string report *MigrationReport + emitAll bool wantContains []string }{ { @@ -427,6 +428,7 @@ func TestMigratorPrintSummary(t *testing.T) { report: &MigrationReport{ SuccessCount: 2, }, + emitAll: false, wantContains: []string{ "Successfully Migrated: 2", "Migrated with Warnings: 0", @@ -434,17 +436,38 @@ func TestMigratorPrintSummary(t *testing.T) { }, }, { - name: "Report with warnings and action items includes guidance note", + name: "Default mode with action items notes omitted drafts", report: &MigrationReport{ SuccessCount: 1, WarningsCount: 1, ActionItemsCount: 1, + ReadyOutputs: make([]*unstructured.Unstructured, 2), }, + emitAll: false, wantContains: []string{ "Successfully Migrated: 1", "Migrated with Warnings: 1", "Migrated with Action Items: 1", - "gmp.googleapis.com/migration-review-required", + "NOTE: Emitted 2 ready manifests to Stdout.", + "1 manifests with action items were omitted from Stdout as they contain best-effort draft configurations with TODO annotations and placeholders.", + "Run with '--all' to output all manifests for review.", + }, + }, + { + name: "All mode with action items notes draft review", + report: &MigrationReport{ + SuccessCount: 1, + WarningsCount: 1, + ActionItemsCount: 1, + Outputs: make([]*unstructured.Unstructured, 3), + }, + emitAll: true, + wantContains: []string{ + "Successfully Migrated: 1", + "Migrated with Warnings: 1", + "Migrated with Action Items: 1", + "NOTE: 1 manifests contain best-effort draft configurations with TODO annotations and placeholders.", + "Review the inline 'gmp.googleapis.com/todo-*' annotations in the generated manifests before applying to a cluster.", }, }, } @@ -454,7 +477,7 @@ func TestMigratorPrintSummary(t *testing.T) { var buf bytes.Buffer m := NewMigrator() m.Stderr = &buf - m.PrintSummary(tc.report) + m.PrintSummary(tc.report, tc.emitAll) output := buf.String() for _, s := range tc.wantContains { if !strings.Contains(output, s) { diff --git a/pkg/migrate/podmonitor_test.go b/pkg/migrate/podmonitor_test.go index 44ccff62aa..bbe3fc55db 100644 --- a/pkg/migrate/podmonitor_test.go +++ b/pkg/migrate/podmonitor_test.go @@ -1215,11 +1215,7 @@ func TestPodMonitorConversion(t *testing.T) { }, }, Spec: monitoringv1.PodMonitoringSpec{ - Selector: metav1.LabelSelector{ - MatchLabels: map[string]string{ - "gmp.googleapis.com/migration-review-required": "true", - }, - }, + Selector: metav1.LabelSelector{}, Endpoints: []monitoringv1.ScrapeEndpoint{ { Port: intstr.FromString("metrics"), @@ -1636,7 +1632,6 @@ func TestPodMonitorConversion(t *testing.T) { Selector: metav1.LabelSelector{ MatchLabels: map[string]string{ "app": "frontend", - "gmp.googleapis.com/migration-review-required": "true", }, }, Endpoints: []monitoringv1.ScrapeEndpoint{ @@ -1698,7 +1693,6 @@ func TestPodMonitorConversion(t *testing.T) { MatchLabels: map[string]string{ "app": "test", "env": "production", - "gmp.googleapis.com/migration-review-required": "true", }, }, Endpoints: []monitoringv1.ScrapeEndpoint{ @@ -1748,7 +1742,6 @@ func TestPodMonitorConversion(t *testing.T) { Selector: metav1.LabelSelector{ MatchLabels: map[string]string{ "app": "frontend", - "gmp.googleapis.com/migration-review-required": "true", }, }, Endpoints: []monitoringv1.ScrapeEndpoint{ @@ -1807,7 +1800,6 @@ func TestPodMonitorConversion(t *testing.T) { Selector: metav1.LabelSelector{ MatchLabels: map[string]string{ "app": "frontend", - "gmp.googleapis.com/migration-review-required": "true", }, }, Endpoints: []monitoringv1.ScrapeEndpoint{ @@ -2431,7 +2423,6 @@ func TestPodMonitorConversion(t *testing.T) { Selector: metav1.LabelSelector{ MatchLabels: map[string]string{ "app": "annotated-app", - "gmp.googleapis.com/migration-review-required": "true", }, }, Endpoints: []monitoringv1.ScrapeEndpoint{ @@ -2478,11 +2469,7 @@ func TestPodMonitorConversion(t *testing.T) { }, }, Spec: monitoringv1.PodMonitoringSpec{ - Selector: metav1.LabelSelector{ - MatchLabels: map[string]string{ - "gmp.googleapis.com/migration-review-required": "true", - }, - }, + Selector: metav1.LabelSelector{}, Endpoints: []monitoringv1.ScrapeEndpoint{ { Port: intstr.FromString("web"), @@ -2533,7 +2520,6 @@ func TestPodMonitorConversion(t *testing.T) { Selector: metav1.LabelSelector{ MatchLabels: map[string]string{ "app": "proxy-app", - "gmp.googleapis.com/migration-review-required": "true", }, }, Endpoints: []monitoringv1.ScrapeEndpoint{ @@ -2594,7 +2580,6 @@ func TestPodMonitorConversion(t *testing.T) { Selector: metav1.LabelSelector{ MatchLabels: map[string]string{ "app": "missing-app", - "gmp.googleapis.com/migration-review-required": "true", }, }, Endpoints: []monitoringv1.ScrapeEndpoint{ diff --git a/pkg/migrate/servicemonitor_test.go b/pkg/migrate/servicemonitor_test.go index fc5d6ea43d..76c30cec25 100644 --- a/pkg/migrate/servicemonitor_test.go +++ b/pkg/migrate/servicemonitor_test.go @@ -686,7 +686,6 @@ func TestServiceMonitorConverter_Convert(t *testing.T) { Selector: metav1.LabelSelector{ MatchLabels: map[string]string{ "app": "TODO_SET_POD_SELECTOR", - "gmp.googleapis.com/migration-review-required": "true", }, }, Endpoints: []monitoringv1.ScrapeEndpoint{ @@ -765,7 +764,6 @@ func TestServiceMonitorConverter_Convert(t *testing.T) { Selector: metav1.LabelSelector{ MatchLabels: map[string]string{ "app": "foo-pod", - "gmp.googleapis.com/migration-review-required": "true", }, }, Endpoints: []monitoringv1.ScrapeEndpoint{ From 3c4fdac8fe43c245bcad53f1d6b35ba982c69501 Mon Sep 17 00:00:00 2001 From: Karthik Unnikrishnan Date: Wed, 12 Aug 2026 18:17:15 +0000 Subject: [PATCH 09/10] fix: resolve proxyURL, scopeExpanded, dummySvc --- pkg/migrate/helpers.go | 21 ++-- pkg/migrate/helpers_test.go | 6 ++ pkg/migrate/podmonitor_test.go | 55 +++++++++++ pkg/migrate/servicemonitor.go | 26 +++-- pkg/migrate/servicemonitor_test.go | 150 +++++++++++++++++++++++++---- 5 files changed, 222 insertions(+), 36 deletions(-) diff --git a/pkg/migrate/helpers.go b/pkg/migrate/helpers.go index fe13c76c75..5c7ff91f2d 100644 --- a/pkg/migrate/helpers.go +++ b/pkg/migrate/helpers.go @@ -913,6 +913,9 @@ func shouldSkipRelabelConfig(logger *slog.Logger, config pomonitoringv1.RelabelC } if strings.HasPrefix(s, "__meta_kubernetes_node_") && s != "__meta_kubernetes_node_name" { logger.Warn(fmt.Sprintf("Relabeling rule referencing node metadata %q is unsupported in GMP (only node name is supported). The rule has been dropped.", s)) + if action == relabel.Keep || action == relabel.Drop { + return true, true, fmt.Sprintf("'%s' on '%s'", action, s) + } return true, false, "" } } @@ -1382,7 +1385,7 @@ func (c *conversionContext) resolveScrapeIntervalAndTimeout(interval, timeout st return interval, timeout } -// convertProxyURL verifies proxy URL credentials and attaches a TODO if passwords are present or malformed. +// convertProxyURL verifies proxy URL credentials and attaches a TODO if credentials are present or malformed. func (c *conversionContext) convertProxyURL(proxyURL *string) string { if proxyURL == nil { return "" @@ -1397,15 +1400,13 @@ func (c *conversionContext) convertProxyURL(proxyURL *string) string { return "TODO_SET_VALID_PROXY_URL" } if parsed.User != nil { - if _, hasPass := parsed.User.Password(); hasPass { - c.todos = append(c.todos, todoItem{ - category: "ERROR", - reason: "Proxy URL contains embedded plaintext credentials. Credentials were removed.", - action: "Configure proxy authentication via Kubernetes Secret or proxy server configuration.", - }) - parsed.User = nil - return parsed.String() - } + c.todos = append(c.todos, todoItem{ + category: "ERROR", + reason: "Proxy URL contains embedded plaintext credentials. Credentials were removed.", + action: "Configure proxy authentication via Kubernetes Secret or proxy server configuration.", + }) + parsed.User = nil + return parsed.String() } return *proxyURL } diff --git a/pkg/migrate/helpers_test.go b/pkg/migrate/helpers_test.go index fe08a820d7..043ffee19f 100644 --- a/pkg/migrate/helpers_test.go +++ b/pkg/migrate/helpers_test.go @@ -877,6 +877,12 @@ func TestConvertProxyURL(t *testing.T) { expectedURL: "http://proxy.example.com:8080", expectTodos: 1, }, + { + name: "proxyURL with username only sanitizes credentials and adds todo", + proxyURL: ptrTo("http://user@proxy.example.com:8080"), + expectedURL: "http://proxy.example.com:8080", + expectTodos: 1, + }, { name: "malformed proxyURL returns placeholder and adds todo", proxyURL: ptrTo("://invalid-url"), diff --git a/pkg/migrate/podmonitor_test.go b/pkg/migrate/podmonitor_test.go index bbe3fc55db..8da1ac610b 100644 --- a/pkg/migrate/podmonitor_test.go +++ b/pkg/migrate/podmonitor_test.go @@ -2435,6 +2435,61 @@ func TestPodMonitorConversion(t *testing.T) { }, }, }, + { + name: "PodMonitor with dropped node metadata relabeling attaches TODO annotation", + input: &pomonitoringv1.PodMonitor{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "monitoring.coreos.com/v1", + Kind: KindPodMonitor, + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "node-keep-monitor", + Namespace: "default", + }, + Spec: pomonitoringv1.PodMonitorSpec{ + Selector: metav1.LabelSelector{ + MatchLabels: map[string]string{"app": "node-app"}, + }, + PodMetricsEndpoints: []pomonitoringv1.PodMetricsEndpoint{ + { + Port: "web", + RelabelConfigs: []pomonitoringv1.RelabelConfig{ + { + Action: "keep", + SourceLabels: []pomonitoringv1.LabelName{"__meta_kubernetes_node_label_topology_kubernetes_io_zone"}, + Regex: "us-central1-a", + }, + }, + }, + }, + }, + }, + expected: []runtime.Object{ + &monitoringv1.PodMonitoring{ + TypeMeta: BuildTypeMeta(KindPodMonitoring), + ObjectMeta: metav1.ObjectMeta{ + Name: "node-keep-monitor", + Namespace: "default", + Annotations: map[string]string{ + "gmp.googleapis.com/todo-1": "[WARNING] Dropped target filtering rule ('keep' on '__meta_kubernetes_node_label_topology_kubernetes_io_zone'). ACTION: Add equivalent pod label selector in 'spec.selector.matchLabels'.", + }, + }, + Spec: monitoringv1.PodMonitoringSpec{ + Selector: metav1.LabelSelector{ + MatchLabels: map[string]string{ + "app": "node-app", + }, + }, + Endpoints: []monitoringv1.ScrapeEndpoint{ + { + Port: intstr.FromString("web"), + Interval: "30s", + }, + }, + }, + }, + }, + }, { name: "PodMonitor with empty selector injects guardrail label and TODO annotation", input: &pomonitoringv1.PodMonitor{ diff --git a/pkg/migrate/servicemonitor.go b/pkg/migrate/servicemonitor.go index 832d0c6ffe..efa9b594f1 100644 --- a/pkg/migrate/servicemonitor.go +++ b/pkg/migrate/servicemonitor.go @@ -222,11 +222,25 @@ func (c *ServiceMonitorConverter) findAndGroupServices( } portMap[k] = intstr.FromString("TODO_RESOLVE_PORT") } - dummySvc := &corev1.Service{ - ObjectMeta: metav1.ObjectMeta{ - Name: sm.Name, - Namespace: sm.Namespace, - }, + var dummySvcs []*corev1.Service + if len(targetNamespaces) == 0 { + dummySvcs = []*corev1.Service{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: sm.Name, + Namespace: sm.Namespace, + }, + }, + } + } else { + for _, ns := range targetNamespaces { + dummySvcs = append(dummySvcs, &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: sm.Name, + Namespace: ns, + }, + }) + } } return []*serviceGroup{ { @@ -234,7 +248,7 @@ func (c *ServiceMonitorConverter) findAndGroupServices( "app": "TODO_SET_POD_SELECTOR", }, portMap: portMap, - services: []*corev1.Service{dummySvc}, + services: dummySvcs, todos: []todoItem{ { category: "ERROR", diff --git a/pkg/migrate/servicemonitor_test.go b/pkg/migrate/servicemonitor_test.go index 76c30cec25..449ca5c0a4 100644 --- a/pkg/migrate/servicemonitor_test.go +++ b/pkg/migrate/servicemonitor_test.go @@ -699,6 +699,118 @@ func TestServiceMonitorConverter_Convert(t *testing.T) { }, wantErr: false, }, + { + name: "Missing backing Service with multiple target namespaces", + setupCache: func(_ *ResourceCache) error { return nil }, + inputSM: &pomonitoringv1.ServiceMonitor{ + TypeMeta: metav1.TypeMeta{APIVersion: "monitoring.coreos.com/v1", Kind: "ServiceMonitor"}, + ObjectMeta: metav1.ObjectMeta{ + Name: "my-monitor", + Namespace: "default", + }, + Spec: pomonitoringv1.ServiceMonitorSpec{ + NamespaceSelector: pomonitoringv1.NamespaceSelector{ + MatchNames: []string{"ns-1", "ns-2"}, + }, + Selector: metav1.LabelSelector{MatchLabels: map[string]string{"app": "foo"}}, + Endpoints: []pomonitoringv1.Endpoint{ + {Port: "web"}, + }, + }, + }, + expected: []runtime.Object{ + &monitoringv1.PodMonitoring{ + TypeMeta: BuildTypeMeta(KindPodMonitoring), + ObjectMeta: metav1.ObjectMeta{ + Name: "my-monitor", + Namespace: "ns-1", + Annotations: map[string]string{ + "gmp.googleapis.com/todo-1": "[ERROR] Corresponding Kubernetes Service was not found. Selector and port mappings could not be resolved. ACTION: Define target pod selector in 'spec.selector.matchLabels' and verify endpoint ports.", + }, + }, + Spec: monitoringv1.PodMonitoringSpec{ + Selector: metav1.LabelSelector{ + MatchLabels: map[string]string{ + "app": "TODO_SET_POD_SELECTOR", + }, + }, + Endpoints: []monitoringv1.ScrapeEndpoint{ + { + Port: intstr.FromString("TODO_RESOLVE_PORT"), + Interval: "30s", + }, + }, + }, + }, + &monitoringv1.PodMonitoring{ + TypeMeta: BuildTypeMeta(KindPodMonitoring), + ObjectMeta: metav1.ObjectMeta{ + Name: "my-monitor", + Namespace: "ns-2", + Annotations: map[string]string{ + "gmp.googleapis.com/todo-1": "[ERROR] Corresponding Kubernetes Service was not found. Selector and port mappings could not be resolved. ACTION: Define target pod selector in 'spec.selector.matchLabels' and verify endpoint ports.", + }, + }, + Spec: monitoringv1.PodMonitoringSpec{ + Selector: metav1.LabelSelector{ + MatchLabels: map[string]string{ + "app": "TODO_SET_POD_SELECTOR", + }, + }, + Endpoints: []monitoringv1.ScrapeEndpoint{ + { + Port: intstr.FromString("TODO_RESOLVE_PORT"), + Interval: "30s", + }, + }, + }, + }, + }, + wantErr: false, + }, + { + name: "Missing backing Service with cluster scoping", + setupCache: func(_ *ResourceCache) error { return nil }, + inputSM: &pomonitoringv1.ServiceMonitor{ + TypeMeta: metav1.TypeMeta{APIVersion: "monitoring.coreos.com/v1", Kind: "ServiceMonitor"}, + ObjectMeta: metav1.ObjectMeta{ + Name: "my-cluster-monitor", + Namespace: "default", + }, + Spec: pomonitoringv1.ServiceMonitorSpec{ + NamespaceSelector: pomonitoringv1.NamespaceSelector{Any: true}, + Selector: metav1.LabelSelector{MatchLabels: map[string]string{"app": "foo"}}, + Endpoints: []pomonitoringv1.Endpoint{ + {Port: "web"}, + }, + }, + }, + expected: []runtime.Object{ + &monitoringv1.ClusterPodMonitoring{ + TypeMeta: BuildTypeMeta(KindClusterPodMonitoring), + ObjectMeta: metav1.ObjectMeta{ + Name: "my-cluster-monitor", + Annotations: map[string]string{ + "gmp.googleapis.com/todo-1": "[ERROR] Corresponding Kubernetes Service was not found. Selector and port mappings could not be resolved. ACTION: Define target pod selector in 'spec.selector.matchLabels' and verify endpoint ports.", + }, + }, + Spec: monitoringv1.ClusterPodMonitoringSpec{ + Selector: metav1.LabelSelector{ + MatchLabels: map[string]string{ + "app": "TODO_SET_POD_SELECTOR", + }, + }, + Endpoints: []monitoringv1.ScrapeEndpoint{ + { + Port: intstr.FromString("TODO_RESOLVE_PORT"), + Interval: "30s", + }, + }, + }, + }, + }, + wantErr: false, + }, { name: "Backing Service has no selector", setupCache: func(cache *ResourceCache) error { @@ -909,29 +1021,27 @@ func TestServiceMonitorConverter_Convert(t *testing.T) { return } - if tc.expected != nil { - if len(outputs) != len(tc.expected) { - t.Fatalf("expected %d outputs, got %d", len(tc.expected), len(outputs)) + if len(outputs) != len(tc.expected) { + t.Fatalf("expected %d outputs, got %d", len(tc.expected), len(outputs)) + } + for i := range tc.expected { + var gotObj runtime.Object + switch tc.expected[i].(type) { + case *monitoringv1.PodMonitoring: + gotObj = &monitoringv1.PodMonitoring{} + case *monitoringv1.ClusterPodMonitoring: + gotObj = &monitoringv1.ClusterPodMonitoring{} + default: + t.Fatalf("expected object at index %d must be a pointer to a recognized monitoring type, got %T", i, tc.expected[i]) } - for i := range tc.expected { - var gotObj runtime.Object - switch tc.expected[i].(type) { - case *monitoringv1.PodMonitoring: - gotObj = &monitoringv1.PodMonitoring{} - case *monitoringv1.ClusterPodMonitoring: - gotObj = &monitoringv1.ClusterPodMonitoring{} - default: - t.Fatalf("expected object at index %d must be a pointer to a recognized monitoring type, got %T", i, tc.expected[i]) - } - err := runtime.DefaultUnstructuredConverter.FromUnstructured(outputs[i].Object, gotObj) - if err != nil { - t.Fatalf("failed to convert actual to struct: %v", err) - } + err := runtime.DefaultUnstructuredConverter.FromUnstructured(outputs[i].Object, gotObj) + if err != nil { + t.Fatalf("failed to convert actual to struct: %v", err) + } - if diff := cmp.Diff(tc.expected[i], gotObj); diff != "" { - t.Errorf("mismatch at index %d (-want +got):\n%s", i, diff) - } + if diff := cmp.Diff(tc.expected[i], gotObj); diff != "" { + t.Errorf("mismatch at index %d (-want +got):\n%s", i, diff) } } }) From b7c792a72439e9f8244c25382e646ef825d89e3c Mon Sep 17 00:00:00 2001 From: Karthik Unnikrishnan Date: Thu, 13 Aug 2026 15:56:46 +0000 Subject: [PATCH 10/10] fix: resolve isClusterScoped, StatusSkipped, test names --- pkg/migrate/migrate.go | 4 + pkg/migrate/migrate_test.go | 136 +++++++++++++++++++++++++++++++++ pkg/migrate/podmonitor.go | 1 + pkg/migrate/podmonitor_test.go | 79 +++++++++++++++++-- pkg/migrate/servicemonitor.go | 11 ++- 5 files changed, 225 insertions(+), 6 deletions(-) diff --git a/pkg/migrate/migrate.go b/pkg/migrate/migrate.go index 2d4316221b..4d329e1eb4 100644 --- a/pkg/migrate/migrate.go +++ b/pkg/migrate/migrate.go @@ -395,6 +395,10 @@ func (m *Migrator) convertResources() []*unstructured.Unstructured { continue } + if len(outputs) == 0 { + continue + } + resourceLogger.Info("Converted successfully", slog.String("migration_status", "success")) } } diff --git a/pkg/migrate/migrate_test.go b/pkg/migrate/migrate_test.go index b2ddc11f29..f4f39c382a 100644 --- a/pkg/migrate/migrate_test.go +++ b/pkg/migrate/migrate_test.go @@ -487,3 +487,139 @@ func TestMigratorPrintSummary(t *testing.T) { }) } } + +func TestMigratorServiceMonitorWithSkippedChildService(t *testing.T) { + tmpDir := t.TempDir() + + // 1. Valid Service with a pod selector. + validServiceYAML := ` +apiVersion: v1 +kind: Service +metadata: + name: valid-service + namespace: default + labels: + app: my-app +spec: + selector: + app: my-app-pod + ports: + - name: metrics + port: 8080 +` + // 2. Service without a pod selector (external endpoints). + externalServiceYAML := ` +apiVersion: v1 +kind: Service +metadata: + name: external-service + namespace: default + labels: + app: my-app +spec: + ports: + - name: metrics + port: 8080 +` + // 3. ServiceMonitor matching both services via label selector. + smYAML := ` +apiVersion: monitoring.coreos.com/v1 +kind: ServiceMonitor +metadata: + name: my-servicemonitor + namespace: default +spec: + selector: + matchLabels: + app: my-app + endpoints: + - port: metrics + interval: 30s +` + if err := os.WriteFile(filepath.Join(tmpDir, "valid-svc.yaml"), []byte(validServiceYAML), 0644); err != nil { + t.Fatalf("failed to write valid-svc.yaml: %v", err) + } + if err := os.WriteFile(filepath.Join(tmpDir, "external-svc.yaml"), []byte(externalServiceYAML), 0644); err != nil { + t.Fatalf("failed to write external-svc.yaml: %v", err) + } + if err := os.WriteFile(filepath.Join(tmpDir, "sm.yaml"), []byte(smYAML), 0644); err != nil { + t.Fatalf("failed to write sm.yaml: %v", err) + } + + migrator := NewMigrator() + migrator.RegisterConverter(&ServiceMonitorConverter{}) + var stdoutBuf, stderrBuf bytes.Buffer + migrator.Stdout = &stdoutBuf + migrator.Stderr = &stderrBuf + + report, err := migrator.Run(tmpDir) + if err != nil { + t.Fatalf("Run failed: %v", err) + } + + // Verify that the ServiceMonitor is tracked as Success when at least one child service converts. + if report.SuccessCount != 1 { + t.Errorf("expected SuccessCount to be 1, got %d", report.SuccessCount) + } + if report.SkippedCount != 0 { + t.Errorf("expected SkippedCount to be 0, got %d", report.SkippedCount) + } +} + +func TestMigratorServiceMonitorAllServicesSkipped(t *testing.T) { + tmpDir := t.TempDir() + + // Service without a pod selector (external endpoints). + externalServiceYAML := ` +apiVersion: v1 +kind: Service +metadata: + name: external-service + namespace: default + labels: + app: my-app +spec: + ports: + - name: metrics + port: 8080 +` + // ServiceMonitor matching only the external service. + smYAML := ` +apiVersion: monitoring.coreos.com/v1 +kind: ServiceMonitor +metadata: + name: my-external-servicemonitor + namespace: default +spec: + selector: + matchLabels: + app: my-app + endpoints: + - port: metrics +` + if err := os.WriteFile(filepath.Join(tmpDir, "external-svc.yaml"), []byte(externalServiceYAML), 0644); err != nil { + t.Fatalf("failed to write external-svc.yaml: %v", err) + } + if err := os.WriteFile(filepath.Join(tmpDir, "sm.yaml"), []byte(smYAML), 0644); err != nil { + t.Fatalf("failed to write sm.yaml: %v", err) + } + + migrator := NewMigrator() + migrator.RegisterConverter(&ServiceMonitorConverter{}) + var stdoutBuf, stderrBuf bytes.Buffer + migrator.Stdout = &stdoutBuf + migrator.Stderr = &stderrBuf + + report, err := migrator.Run(tmpDir) + if err != nil { + t.Fatalf("Run failed: %v", err) + } + + // Verify that the ServiceMonitor is tracked as Skipped when all child services lack selectors. + if report.SkippedCount != 1 { + t.Errorf("expected SkippedCount to be 1, got %d", report.SkippedCount) + } + if report.SuccessCount != 0 { + t.Errorf("expected SuccessCount to be 0, got %d", report.SuccessCount) + } +} diff --git a/pkg/migrate/podmonitor.go b/pkg/migrate/podmonitor.go index e3c3e3398d..4cb7bd8389 100644 --- a/pkg/migrate/podmonitor.go +++ b/pkg/migrate/podmonitor.go @@ -162,6 +162,7 @@ func (c *PodMonitorConverter) convertMonitorSpec(pm *pomonitoringv1.PodMonitor, cache: cache, sourceNamespace: pm.Namespace, targetNamespace: targetNamespace, + isClusterScoped: isCluster, } var relabelConfigs [][]pomonitoringv1.RelabelConfig for _, ep := range pm.Spec.PodMetricsEndpoints { diff --git a/pkg/migrate/podmonitor_test.go b/pkg/migrate/podmonitor_test.go index 8da1ac610b..59ae115df7 100644 --- a/pkg/migrate/podmonitor_test.go +++ b/pkg/migrate/podmonitor_test.go @@ -83,6 +83,75 @@ func TestPodMonitorConversion(t *testing.T) { }, }, }, + { + name: "Cluster-Scoped with Secret References", + setupCache: func(cache *ResourceCache) error { + return addSecretToCache(cache, "monitoring-ns", "auth-secret", "user", "admin", true) + }, + input: &pomonitoringv1.PodMonitor{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "monitoring.coreos.com/v1", + Kind: KindPodMonitor, + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster-auth-monitor", + Namespace: "monitoring-ns", + }, + Spec: pomonitoringv1.PodMonitorSpec{ + NamespaceSelector: pomonitoringv1.NamespaceSelector{ + Any: true, + }, + Selector: metav1.LabelSelector{ + MatchLabels: map[string]string{"app": "cluster-app"}, + }, + PodMetricsEndpoints: []pomonitoringv1.PodMetricsEndpoint{ + { + Port: "metrics", + BasicAuth: &pomonitoringv1.BasicAuth{ + Username: corev1.SecretKeySelector{LocalObjectReference: corev1.LocalObjectReference{Name: "auth-secret"}, Key: "user"}, + Password: corev1.SecretKeySelector{LocalObjectReference: corev1.LocalObjectReference{Name: "auth-secret"}, Key: "pass"}, + }, + }, + }, + }, + }, + expected: []runtime.Object{ + &monitoringv1.ClusterPodMonitoring{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "monitoring.googleapis.com/v1", + Kind: KindClusterPodMonitoring, + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster-auth-monitor", + }, + Spec: monitoringv1.ClusterPodMonitoringSpec{ + Selector: metav1.LabelSelector{ + MatchLabels: map[string]string{ + "app": "cluster-app", + }, + }, + Endpoints: []monitoringv1.ScrapeEndpoint{ + { + Port: intstr.FromString("metrics"), + Interval: "30s", + HTTPClientConfig: monitoringv1.HTTPClientConfig{ + BasicAuth: &monitoringv1.BasicAuth{ + Username: "admin", + Password: &monitoringv1.SecretSelector{ + Secret: &monitoringv1.SecretKeySelector{ + Name: "auth-secret", + Key: "pass", + Namespace: "monitoring-ns", + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, { name: "Multi-Namespace Split", input: &pomonitoringv1.PodMonitor{ @@ -1645,7 +1714,7 @@ func TestPodMonitorConversion(t *testing.T) { }, }, { - name: "Pre-Scrape Relabelings: conflicting keep rules on same pod label injects guardrail label and TODO annotation", + name: "Pre-Scrape Relabelings: conflicting keep rules on same pod label attaches TODO annotation", input: &pomonitoringv1.PodMonitor{ TypeMeta: metav1.TypeMeta{ APIVersion: "monitoring.coreos.com/v1", @@ -1706,7 +1775,7 @@ func TestPodMonitorConversion(t *testing.T) { }, }, { - name: "BearerTokenSecret with empty Name returns validation error", + name: "BearerTokenSecret with empty Name generates draft with TODO annotation", input: &pomonitoringv1.PodMonitor{ TypeMeta: metav1.TypeMeta{ APIVersion: "monitoring.coreos.com/v1", @@ -2378,7 +2447,7 @@ func TestPodMonitorConversion(t *testing.T) { }, }, { - name: "PodMonitor with dropped annotation relabeling injects guardrail label and TODO annotation", + name: "PodMonitor with dropped annotation relabeling attaches TODO annotation", input: &pomonitoringv1.PodMonitor{ TypeMeta: metav1.TypeMeta{ APIVersion: "monitoring.coreos.com/v1", @@ -2491,7 +2560,7 @@ func TestPodMonitorConversion(t *testing.T) { }, }, { - name: "PodMonitor with empty selector injects guardrail label and TODO annotation", + name: "PodMonitor with empty selector attaches TODO annotation", input: &pomonitoringv1.PodMonitor{ TypeMeta: metav1.TypeMeta{ APIVersion: "monitoring.coreos.com/v1", @@ -2536,7 +2605,7 @@ func TestPodMonitorConversion(t *testing.T) { }, }, { - name: "PodMonitor with proxyUrl containing password injects guardrail label and TODO annotation", + name: "PodMonitor with proxyUrl containing password attaches TODO annotation", input: &pomonitoringv1.PodMonitor{ TypeMeta: metav1.TypeMeta{ APIVersion: "monitoring.coreos.com/v1", diff --git a/pkg/migrate/servicemonitor.go b/pkg/migrate/servicemonitor.go index efa9b594f1..3e18f52784 100644 --- a/pkg/migrate/servicemonitor.go +++ b/pkg/migrate/servicemonitor.go @@ -97,6 +97,11 @@ func (c *ServiceMonitorConverter) Convert(_ context.Context, logger *slog.Logger var outputs []*unstructured.Unstructured outputs = append(outputs, clusterPodMonitorings...) outputs = append(outputs, generatedSecrets...) + if len(outputs) == 0 { + logger.Info("All matched Services lack pod selectors (target external endpoints). GMP Managed Collection only supports in-cluster Pods. Skipping resource.", + slog.String("migration_status", "skipped"), + ) + } return outputs, nil } @@ -118,6 +123,11 @@ func (c *ServiceMonitorConverter) Convert(_ context.Context, logger *slog.Logger var outputs []*unstructured.Unstructured outputs = append(outputs, podMonitorings...) outputs = append(outputs, generatedSecrets...) + if len(outputs) == 0 { + logger.Info("All matched Services lack pod selectors (target external endpoints). GMP Managed Collection only supports in-cluster Pods. Skipping resource.", + slog.String("migration_status", "skipped"), + ) + } return outputs, nil } @@ -461,7 +471,6 @@ func groupServices( selectorString := svc.Spec.Selector if len(selectorString) == 0 { logger.Info("Service targets external endpoints without a pod selector. GMP Managed Collection only supports in-cluster Pods. Skipping resource.", - slog.String("migration_status", "skipped"), slog.String("service", svc.GetName()), ) continue