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
| 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.
There was a problem hiding this comment.
This isn't the same mistake as #1988 (comment). It is saying that we need to sanitize before we validate
| 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.
There was a problem hiding this comment.
Hmmm... This seems good, but we definitely need to warn about dropped ports
| } | ||
|
|
||
| // 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.
|
|
||
| var matched []*unstructured.Unstructured | ||
|
|
||
| if c == nil || c.resources == nil { |
There was a problem hiding this comment.
Move if c == nil || c.resources == nil check to the top of findServicesBySelector to fail fast before parsing the label selector.
| { | ||
| name: "Match multiple services", | ||
| setupCache: func(cache *ResourceCache) error { | ||
| if err := addServiceToCache(cache, "default", "svc-a", map[string]string{"app": "foo"}); err != nil { |
There was a problem hiding this comment.
In TestFindServicesBySelector ("Match multiple services"), svc-a is inserted before svc-b, matching alphabetical order. Inserting svc-b first ensures the explicit key sorting logic is verified.
| } | ||
| } | ||
|
|
||
| func TestConvertServiceTargetLabels(t *testing.T) { |
There was a problem hiding this comment.
podmonitor_test.go validates generated resources by running them through ScrapeConfigs(). Feeding convertServiceTargetLabels output into a minimal PodMonitoring and invoking ScrapeConfigs() would catch invalid target label errors end-to-end.
| targetLabels []string | ||
| expected []monitoringv1.RelabelingRule | ||
| }{ | ||
| { |
There was a problem hiding this comment.
TestConvertServiceTargetLabels currently only tests simple labels (app, env, job). Add test cases covering labels with slashes and dots (e.g. app.kubernetes.io/name).
| 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.
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.