-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Conversation
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
Template the port name values and set a default value
5e38576
to
10ac225
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Out of curiosity, why would you want to change the names of these ports?
jenkins test this please |
jenkins test this please |
eb334d8
to
46d6091
Compare
The helm lint tests were failing because the httpPortName and transportPortName key value were missing from the values.yaml file. Also update both services to include the port name template.
46d6091
to
8dc44dd
Compare
@Crazybus Thanks for reviewing, I made some changes to get the build to pass. Also when I run
Also to answer your question and to expand on the PR description, we are using the all of the ELK helm charts (which are a work of art imo) behind an Isito service mesh which enables mTLS traffic between pods. After enabling the elastic security features including SSL istio could not interpret the traffic and the masters could never cluster. I needed to changes how istio handles the traffic between pods which is taken care of by updating the service port name as described here. Following this issue I updated the service port name to be |
Thank you so much, it really means a lot :)
Perfect! Thank you so much for explaining this. In the next major release we could consider using istio compatible naming for these ports. However I'm sure every service mesh has their own standards so they will always need to be configurable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! The | default
can be removed now that they are in the values.yaml but this change isn't blocking. Feel free to follow up in a future PR to remove it. I'm planning on releasing today (still catching up from my vacation) so I'd rather get it out as is.
@@ -18,13 +18,13 @@ spec: | |||
chart: "{{ .Chart.Name }}-{{ .Chart.Version }}" | |||
app: "{{ template "uname" . }}" | |||
ports: | |||
- name: http | |||
- name: {{ .Values.service.httpPortName | default "http" }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The defaults can be removed from here now that you have added the defaults into the values.yaml.
- name: {{ .Values.service.httpPortName | default "http" }} | |
- name: {{ .Values.service.httpPortName }} |
jenkins test this please |
Thanks again, I look forward to contributing again in the future. |
Template the port name values and set a default value
This is requested because running ES in an Istio Service Mesh with security enabled the port name needs to be different so Istio handles the traffic differently. Istio can not handle https traffic when it is expecting http traffic via the port name. https://istio.io/docs/setup/kubernetes/additional-setup/requirements/
${CHART}/tests/*.py
${CHART}/examples/*/test/goss.yaml