PO to GMP Migration Tool: Service Resolution Functions - #2002
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces helper functions in pkg/migrate/helpers.go along with comprehensive unit tests in pkg/migrate/helpers_test.go to find Services by selector, resolve Service ports to target ports, and convert Service target labels to static metric relabeling rules. The reviewer provided valuable feedback, suggesting a nil check for the ResourceCache receiver to prevent panics, an explicit check for empty port strings to avoid accidental matches with unnamed ports, and handling additional numeric types (float64 and int) in the targetPort type switch to ensure robust parsing of unstructured objects.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces helper functions in pkg/migrate/helpers.go—including findServicesBySelector, resolveServicePort, and convertServiceTargetLabels—along with comprehensive unit tests to support Kubernetes Service resolution and label mapping during migration. The review feedback highlights that resolveServicePort uses a fragile type assertion for port numbers in unstructured objects and incorrectly propagates fatal errors instead of logging warnings and falling back to the port string. Consequently, the reviewer suggests refactoring resolveServicePort to handle type coercion safely and return fallbacks, and updating the unit tests accordingly.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds helper functions to find services by selector, resolve service ports to target ports, and convert service target labels to relabeling rules, along with comprehensive unit tests. The feedback suggests improving the robustness of the service port resolution by logging a warning and using a placeholder instead of returning a fatal error when encountering a malformed port, which prevents the entire migration process from failing.
6d133c9 to
648b3e1
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces helper functions to find services by selector, resolve service ports to target ports, and map service labels to metric relabeling rules, along with their corresponding unit tests. Feedback suggests modifying resolveServicePort to log a warning and inject a placeholder port instead of returning a fatal error when encountering a malformed port entry, preventing the entire migration process from failing.
648b3e1 to
8c87394
Compare
60e7cdc to
436c631
Compare
436c631 to
4bdfc9f
Compare
| } | ||
|
|
||
| // findServicesBySelector finds Services matching the selector in target namespaces (all if empty). | ||
| func (c *ResourceCache) findServicesBySelector(selector metav1.LabelSelector, namespaces []string) ([]*unstructured.Unstructured, error) { |
There was a problem hiding this comment.
PR Scope Note: This PR diffs against main at 2,371 lines / 41 commits because its base is the unmerged karthunni/po-migrate-refactor branch. Retargeting the PR base to karthunni/po-migrate-refactor would isolate the 5 commits (~485 lines) specific to service resolution.
| continue | ||
| } | ||
|
|
||
| target := l |
There was a problem hiding this comment.
convertServiceTargetLabels uses raw Kubernetes Service label keys (such as app.kubernetes.io/name) as TargetLabel. Kubernetes label keys often contain dots and slashes, which are invalid in Prometheus label names. Sanitize non-alphanumeric characters to underscores first, then check protectedLabels against the sanitized name.
| return intstr.IntOrString{}, fmt.Errorf("failed to read port field: %w", err) | ||
| } | ||
| if !foundField { | ||
| return intstr.IntOrString{}, errors.New("service port spec is missing the port number") |
There was a problem hiding this comment.
In resolveServicePort, encountering a port entry missing port or with an invalid type returns a fatal error immediately. If a Service has multiple ports and an earlier entry is malformed, resolving a valid target port later in the slice fails. Consider skipping malformed entries and returning an error only if no matching port is resolved.
| } | ||
|
|
||
| // asInt32 coerces various Go numeric types into an int32. | ||
| func asInt32(val any) (int32, bool) { |
There was a problem hiding this comment.
asInt32 converts int64 and float64 to int32 without bounds or range checks. Out-of-range values like 4294967297 truncate to 1 and can match the wrong port. Consider adding range validation for [1, 65535].
| } | ||
|
|
||
| // targetPort can be int, float64, or string. | ||
| if valStr, ok := targetPort.(string); ok { |
There was a problem hiding this comment.
targetPort: "" returns intstr.FromString("") without validation, while numeric zero falls back to portNum. Empty string targetPorts should be treated as invalid or fall back to portNum.
| Labels: labels, | ||
| }, | ||
| } | ||
| u, _ := runtime.DefaultUnstructuredConverter.ToUnstructured(svc) |
There was a problem hiding this comment.
addServiceToCache, makeTestService, and makeTestServiceWithLabels can be consolidated into a single builder taking labels and ports. Also, u, _ := swallows ToUnstructured conversion errors instead of using t.Fatal or returning the error.
| if len(got) != len(tc.expected) { | ||
| t.Fatalf("expected %d rules, got %d", len(tc.expected), len(got)) | ||
| } | ||
| for i, r := range got { |
There was a problem hiding this comment.
TestConvertServiceTargetLabels manually compares fields one-by-one. Using cmp.Diff (as done elsewhere in helpers_test.go) improves assertion readability and diff diagnostic quality.
| } | ||
|
|
||
| // findServicesBySelector finds Services matching the selector in target namespaces (all if empty). | ||
| func (c *ResourceCache) findServicesBySelector(selector metav1.LabelSelector, namespaces []string) ([]*unstructured.Unstructured, error) { |
There was a problem hiding this comment.
Parent Branch Bug Note (in convertRelabelingToMetricRelabeling): targetLabels.fromPod appends LabelMapping{From: p} without renaming protected labels to exported_<label>, unlike the sibling path. Pod labels matching protected labels (like job) cause operator validation failure.
| } | ||
|
|
||
| // findServicesBySelector finds Services matching the selector in target namespaces (all if empty). | ||
| func (c *ResourceCache) findServicesBySelector(selector metav1.LabelSelector, namespaces []string) ([]*unstructured.Unstructured, error) { |
There was a problem hiding this comment.
Parent Branch Bug Note (in shouldSkipRelabelConfig): shouldSkipRelabelConfig lists labelkeep as supported, promoting it to metricRelabeling where it drops protected labels like __address__, cluster, namespace. Pre-scrape labelkeep/labeldrop act on __meta_* labels and should be dropped with a warning rather than promoted.
| } | ||
|
|
||
| // findServicesBySelector finds Services matching the selector in target namespaces (all if empty). | ||
| func (c *ResourceCache) findServicesBySelector(selector metav1.LabelSelector, namespaces []string) ([]*unstructured.Unstructured, error) { |
There was a problem hiding this comment.
Parent Branch Bug Note (in convertSecretSelector): convertSecretSelector sets Namespace: c.namespace on secret references, but PodMonitoring CEL rules strictly forbid Namespace on secret references ("Namespace not allowed on PodMonitoring secret references.").
2 participants
This PR implements the core service and port resolution helper functions in
pkg/migrate/helpers.gorequired for the upcoming ServiceMonitor migration work.Key additions:
findServicesBySelector: Traverses the ResourceCache to return Service CRs matching a specified LabelSelector within target namespaces.resolveServicePort: Inspects a Service'sspec.portslist to map a target port reference (either a string name or a port number) to the Pod's actual container targetPort value.convertServiceTargetLabels: Maps Service-level labels (as defined inspec.targetLabelsof ServiceMonitor) to static metric relabeling rules (action: replace), appending the "exported_" prefix if it clashes with protected Prometheus labels.helpers_test.goverifying all helper behaviors, error paths, and edge cases.