[Bugfix][Helm] Only pass --k8s-service-discovery-type when explicitly set to non-default - #1068
Conversation
There was a problem hiding this comment.
Code Review
This pull request modifies the Helm deployment template for the router to conditionally omit the --k8s-service-discovery-type argument when its value is "pod-ip". The reviewer points out that hardcoding "pod-ip" in the template condition can lead to configuration drift if the router's default changes in the future, and suggests using a standard Helm pattern of checking if the value is set, along with updating values.yaml.
| {{- if and .Values.routerSpec.k8sServiceDiscoveryType (ne .Values.routerSpec.k8sServiceDiscoveryType "pod-ip") }} | ||
| - "--k8s-service-discovery-type" | ||
| - "{{ default "pod-ip" .Values.routerSpec.k8sServiceDiscoveryType }}" | ||
| - "{{ .Values.routerSpec.k8sServiceDiscoveryType }}" | ||
| {{- end }} |
There was a problem hiding this comment.
Hardcoding "pod-ip" in the template condition to omit the argument can lead to configuration drift if the router's default service discovery type changes in a future version. If a user explicitly configures k8sServiceDiscoveryType: "pod-ip", the argument will not be passed, and the router would use whatever its default is at that time (which might no longer be "pod-ip").
A more robust and standard Helm pattern is to set the default value of k8sServiceDiscoveryType to empty ("") in values.yaml (representing "use container default"), and then simply check if the value is set in the template before passing it.
Please also update helm/values.yaml to set k8sServiceDiscoveryType: "" (and update the schema in helm/values.schema.json if necessary) to complete this change.
{{- if .Values.routerSpec.k8sServiceDiscoveryType }}
- "--k8s-service-discovery-type"
- "{{ .Values.routerSpec.k8sServiceDiscoveryType }}"
{{- end }}…n-default The Helm chart unconditionally passes --k8s-service-discovery-type pod-ip to the router container, but older router images may not support this argument, causing the router to crash with: error: unrecognized arguments: --k8s-service-discovery-type pod-ip Since the router already defaults to pod-ip behavior when the argument is not provided, only pass --k8s-service-discovery-type when the user explicitly configures it to a non-default value (e.g. service-name). Signed-off-by: Asthenia <asthenia0412@gmail.com>
5f65dd0 to
1842d4e
Compare
…eryType Per review feedback, set k8sServiceDiscoveryType default to "" in values.yaml and only pass --k8s-service-discovery-type when the value is explicitly set. This avoids hardcoding "pod-ip" in the template condition, preventing configuration drift if the router's default changes in a future version. Signed-off-by: Asthenia <asthenia0412@gmail.com>
Fixes #746
Description
The Helm chart unconditionally passes
--k8s-service-discovery-type pod-ipto the router container. However, older router images may not support this argument, causing the router to crash with:Fix
Since the router already defaults to
pod-ipbehavior when the argument is not provided (seeservice_discovery.pyline 1341), only pass--k8s-service-discovery-typewhen the user explicitly configures it to a non-default value (e.g.service-name).This provides backward compatibility with older router images that do not support this argument.
Testing
k8sServiceDiscoveryType: "pod-ip"):--k8s-service-discovery-typeis NOT passed, router uses its own default → works with both old and new imagesk8sServiceDiscoveryType: "service-name"):--k8s-service-discovery-type service-nameIS passed → works with new images that support this argument