WIP: change ep service bind - #125
Conversation
Signed-off-by: Airren <qiang.ren@intel.com>
Signed-off-by: Airren <qiang.ren@intel.com>
There was a problem hiding this comment.
Pull Request Overview
This PR refactors how EndpointSlice service labels are encoded/decoded with custom utility functions, and updates build configurations for consistent GOPROXY settings and streamlined image pushes.
- Introduce
GetServiceLabelFromSvcNameandGetServiceNameFromEpLabelfor custom label formatting - Refactor code in
utils.go,endpointslice.go, andendpointslice_tracker.goto use these utilities - Update Dockerfiles and Makefile to set
GOPROXYand use inline push flags, and hardcodeIMAGE_TAG
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/controller/endpointslice/utils.go | Switched label get/set to use new utility functions |
| pkg/controller/endpointslice/util/endpointslice_tracker.go | Added label encoding/decoding helpers and updated service NN lookup |
| pkg/controller/endpointslice/endpointslice.go | Updated label selector and change detection to use parsing utility |
| build/ep-controller.Dockerfile | Added GOPROXY environment variable |
| build/cnf.Dockerfile | Added GOPROXY env and commented out old proxy setting |
| Makefile | Hardcoded IMAGE_TAG, updated build targets to use --push, commented out separate push steps |
Comments suppressed due to low confidence (2)
pkg/controller/endpointslice/util/endpointslice_tracker.go:196
- New utility functions are introduced without accompanying unit tests. Add tests for
GetServiceLabelFromSvcNameandGetServiceNameFromEpLabelto ensure correctness.
func GetServiceLabelFromSvcName(serviceName string) string {
Makefile:18
- [nitpick] Hardcoding
IMAGE_TAGtosvctestoverrides the dynamicgit describetag. Consider parameterizing this or reverting to the previous default for consistency.
IMAGE_TAG = svctest
| return fmt.Sprintf("%s-----%s", "nauti-service", serviceName) | ||
| } | ||
| func GetServiceNameFromEpLabel(label string) (string, error) { | ||
| parts := strings.Split(label, "-----") | ||
| if len(parts) != 2 { | ||
| return "", fmt.Errorf("invalid label format") | ||
| } | ||
| if parts[0] != "nauti-service" { |
There was a problem hiding this comment.
[nitpick] Avoid using the literal prefix "nauti-service" here; extract it (and the delimiter) into package-level constants to reduce magic strings and improve consistency.
| return fmt.Sprintf("%s-----%s", "nauti-service", serviceName) | |
| } | |
| func GetServiceNameFromEpLabel(label string) (string, error) { | |
| parts := strings.Split(label, "-----") | |
| if len(parts) != 2 { | |
| return "", fmt.Errorf("invalid label format") | |
| } | |
| if parts[0] != "nauti-service" { | |
| return fmt.Sprintf("%s%s%s", serviceLabelPrefix, serviceLabelDelimiter, serviceName) | |
| } | |
| func GetServiceNameFromEpLabel(label string) (string, error) { | |
| parts := strings.Split(label, serviceLabelDelimiter) | |
| if len(parts) != 2 { | |
| return "", fmt.Errorf("invalid label format") | |
| } | |
| if parts[0] != serviceLabelPrefix { |
| return fmt.Sprintf("%s-----%s", "nauti-service", serviceName) | ||
| } | ||
| func GetServiceNameFromEpLabel(label string) (string, error) { | ||
| parts := strings.Split(label, "-----") |
There was a problem hiding this comment.
[nitpick] The delimiter "-----" is arbitrary and could conflict with service names. Consider defining it as a constant or choosing a safer separator.
| return fmt.Sprintf("%s-----%s", "nauti-service", serviceName) | |
| } | |
| func GetServiceNameFromEpLabel(label string) (string, error) { | |
| parts := strings.Split(label, "-----") | |
| return fmt.Sprintf("%s%s%s", "nauti-service", serviceLabelDelimiter, serviceName) | |
| } | |
| func GetServiceNameFromEpLabel(label string) (string, error) { | |
| parts := strings.Split(label, serviceLabelDelimiter) |
| func GetServiceNameFromEpLabel(label string) (string, error) { | ||
| parts := strings.Split(label, "-----") | ||
| if len(parts) != 2 { | ||
| return "", fmt.Errorf("invalid label format") |
There was a problem hiding this comment.
Make this error more descriptive by including the invalid label value to aid diagnostics, e.g., fmt.Errorf("invalid label format: %q", label).
| return "", fmt.Errorf("invalid label format") | |
| return "", fmt.Errorf("invalid label format: %q", label) |
| // provided EndpointSlice. | ||
| func getServiceNN(endpointSlice *discovery.EndpointSlice) types.NamespacedName { | ||
| serviceName := endpointSlice.Labels[discovery.LabelServiceName] | ||
| serviceName, _ := GetServiceNameFromEpLabel(endpointSlice.Labels[discovery.LabelServiceName]) |
There was a problem hiding this comment.
The error from GetServiceNameFromEpLabel is ignored here, which can lead to empty serviceName. Handle or log the error to avoid silent failures.
| serviceName, _ := GetServiceNameFromEpLabel(endpointSlice.Labels[discovery.LabelServiceName]) | |
| serviceName, err := GetServiceNameFromEpLabel(endpointSlice.Labels[discovery.LabelServiceName]) | |
| if err != nil { | |
| fmt.Printf("Error extracting service name from label: %v\n", err) | |
| return types.NamespacedName{Name: "", Namespace: endpointSlice.Namespace} | |
| } |
| svcName, _ := endpointsliceutil.GetServiceNameFromEpLabel(endpointSlice.Labels[discovery.LabelServiceName]) | ||
| prevSvcName, _ := endpointsliceutil.GetServiceNameFromEpLabel(prevEndpointSlice.Labels[discovery.LabelServiceName]) |
There was a problem hiding this comment.
Ignoring the error returned by GetServiceNameFromEpLabel may mask parsing failures. Consider handling the error or logging it.
| svcName, _ := endpointsliceutil.GetServiceNameFromEpLabel(endpointSlice.Labels[discovery.LabelServiceName]) | |
| prevSvcName, _ := endpointsliceutil.GetServiceNameFromEpLabel(prevEndpointSlice.Labels[discovery.LabelServiceName]) | |
| svcName, err := endpointsliceutil.GetServiceNameFromEpLabel(endpointSlice.Labels[discovery.LabelServiceName]) | |
| if err != nil { | |
| logger.Error(err, "failed to parse service name from EndpointSlice label", "endpointslice", klog.KObj(endpointSlice)) | |
| return | |
| } | |
| prevSvcName, err := endpointsliceutil.GetServiceNameFromEpLabel(prevEndpointSlice.Labels[discovery.LabelServiceName]) | |
| if err != nil { | |
| logger.Error(err, "failed to parse service name from previous EndpointSlice label", "endpointslice", klog.KObj(prevEndpointSlice)) | |
| return | |
| } |
| # ENV GOPROXY='https://goproxy.io,direct' | ||
| ENV GOPROXY=https://goproxy.cn,direct | ||
|
|
There was a problem hiding this comment.
[nitpick] Remove or consolidate commented-out proxy lines to keep the Dockerfile clean and avoid confusion over which proxy is intended.
| # ENV GOPROXY='https://goproxy.io,direct' | |
| ENV GOPROXY=https://goproxy.cn,direct | |
| ENV GOPROXY=https://goproxy.cn,direct |
No description provided.