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.
| portVal, foundField, err := unstructured.NestedFieldNoCopy(portMap, "port") | ||
| if err != nil { | ||
| 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") | ||
| } | ||
| portNum, ok := asInt32(portVal) | ||
| if !ok { | ||
| return intstr.IntOrString{}, fmt.Errorf("invalid port number type in Service spec: %T", portVal) | ||
| } |
There was a problem hiding this comment.
In resolveServicePort, returning a fatal error immediately when encountering a malformed port entry (e.g., missing or invalid port field) will prevent the resolution of other valid ports in the same Service. Instead of returning a fatal error or silently skipping, we should log a warning and inject a placeholder string to prevent the entire migration process from failing.
| portVal, foundField, err := unstructured.NestedFieldNoCopy(portMap, "port") | |
| if err != nil { | |
| 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") | |
| } | |
| portNum, ok := asInt32(portVal) | |
| if !ok { | |
| return intstr.IntOrString{}, fmt.Errorf("invalid port number type in Service spec: %T", portVal) | |
| } | |
| portVal, foundField, err := unstructured.NestedFieldNoCopy(portMap, "port") | |
| if err != nil || !foundField { | |
| log.Warn("Missing port key, injecting placeholder") | |
| portNum = placeholderPort | |
| } else { | |
| var ok bool | |
| portNum, ok = asInt32(portVal) | |
| if !ok { | |
| log.Warn("Invalid port format, injecting placeholder") | |
| portNum = placeholderPort | |
| } | |
| } |
References
- For non-fatal configuration issues (such as missing keys, empty selector names, or missing referenced resources), log a warning and inject a placeholder string instead of returning a fatal error, to prevent the entire migration process from failing.
There was a problem hiding this comment.
Putting a placeholder would not be ideal as it would lead to a failed scrape. We can continue to fail the migration for a malformed port, or another option is to just drop the port
1 participant
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.