PO to GMP Migration Tool: Add Draft Generation with TODO Annotations for Errors

[karthunni]

This PR enhances the Prometheus Operator to GMP migration tool by implementing graceful draft generation for conversion errors instead of aborting the entire migration. The converter produces manifests with TODO annotations, placeholder values, and a safety guardrail label.

• Missing Secret / ConfigMap Keys & References:
  • Emits TODO_MISSING_KEY_<KEY>_IN_<KIND>_<NAME> and TODO_SET_SECRET_NAME placeholders with [ERROR] TODO annotations instead of returning hard errors.
• Duration Parsing & Scrape Timeouts:
  • Defaults invalid scrape intervals to "30s" with [ERROR] TODOs to preserve schema validity.
  • Drops invalid scrape timeouts and caps timeouts exceeding scrape intervals with actionable TODOs.
• Endpoint & Service Port Resolution:
  • Emits port: "TODO_SET_PORT" when an endpoint omits both port and targetPort.
  • Resolves unmapped Service ports to port: "TODO_RESOLVE_PORT_<PORT>" with [WARNING] TODOs.
• Missing Backing Services for ServiceMonitor:
  • When a ServiceMonitor has no matching backing Service in the inputs, generates a draft PodMonitoring with selector: {app: "TODO_SET_POD_SELECTOR"} and port: "TODO_RESOLVE_PORT".
• Selector & Keep Rule Conflicts:
  • Preserves base selectors and records actionable TODOs when relabel keep rules conflict with existing match labels.
• Safety Guardrail Label:
  • Injects gmp.googleapis.com/migration-review-required: "true" into spec.selector.matchLabels whenever [ERROR] TODO items are present, preventing unintentional scraping until user review.

[gemini-code-assist]

Code Review

This pull request introduces a mechanism to inject sequential TODO annotations and safety guardrails (gmp.googleapis.com/migration-review-required: "true") into migrated resources when configuration issues are encountered, and updates the migration report to track these action items. The review feedback highlights several violations of migration rules where the code falls back to default values or placeholders (such as for invalid proxy URLs, invalid scrape intervals, or unresolvable Service ports) instead of returning a fatal error. Malformed configurations that would lead to failed scrapes must result in errors rather than falling back to defaults or logging warnings.

[bwplotka]

Looks like a reasonable approach if you can make it a pattern for all errors/warnings (e.g. we don't forget about something).

Not sure about blocking scrape logic - it feels odd, but it is one of the least bad options if we assume people or agents will apply without looking

[dashpole]

When backing Services are missing, using sm.Namespace for dummySvc ignores targetNamespaces (e.g. from namespaceSelector.matchNames), causing draft PodMonitoring manifests to be generated in sm.Namespace instead of the targeted namespaces. Consider creating dummy Services for each namespace in targetNamespaces.

[dashpole]

When dropping a keep or drop rule on node metadata, scopeExpanded is returned as false. Consider returning scopeExpanded: true if action == relabel.Keep || action == relabel.Drop (like lines 926-929 for pod annotations) so a TODO and guardrail label are attached.

[dashpole]

Consider using the GuardrailLabelKey and GuardrailLabelValue constants here instead of hardcoded strings:

Suggested change

	fmt.Fprintln(m.Stderr, "Review the TODO annotations in the generated manifests and remove this label when ready to activate scraping.")
	fmt.Fprintf(m.Stderr, " '%s: \"%s\"'\n", GuardrailLabelKey, GuardrailLabelValue)

[dashpole]

Note that TestServiceMonitorConverter_Convert currently guards its output length assertion with if tc.expected != nil, which causes tests with expected: nil to skip verifying that outputs is actually empty.

[dashpole]

If the proxy URL contains a username without a password (such as http://user@host), hasPass is false so credentials aren't stripped and the resulting URL still contains @ which is rejected by GMP. Consider checking if parsed.User != nil directly without requiring a password:

Suggested change

	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()

[karthunni]

Looks like a reasonable approach if you can make it a pattern for all errors/warnings (e.g. we don't forget about something).

Not sure about blocking scrape logic - it feels odd, but it is one of the least bad options if we assume people or agents will apply without looking

Reconsidered potential options and decided to take the following approach:

On a default CLI call (i.e no flags), we output the manifests without TODO items, while noting in the stderr output summary: (such that users can safely blindly apply)

X 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.

When the --all flag is added, all manifests are written to stdout including the ones with TODOs (still in the annotations but no longer blocking scraping logic with the matchLabels), and the output summary states: (such that an explicit call is needed to output "dangerous" CRs)

X 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.