Skip to content
This repository has been archived by the owner on May 16, 2023. It is now read-only.

ES Variable Port Name #270

Merged
merged 3 commits into from
Sep 19, 2019
Merged

Conversation

GreenKnight15
Copy link
Contributor

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 version not bumped (the versions are all bumped and released at the same time)
  • README.md updated with any new values or changes
  • Updated template tests in ${CHART}/tests/*.py
  • Updated integration tests in ${CHART}/examples/*/test/goss.yaml

@elasticmachine
Copy link
Collaborator

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
Crazybus
Crazybus previously approved these changes Sep 18, 2019
Copy link
Contributor

@Crazybus Crazybus left a 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?

@Crazybus
Copy link
Contributor

jenkins test this please

@Crazybus
Copy link
Contributor

jenkins test this please

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.
@GreenKnight15
Copy link
Contributor Author

@Crazybus Thanks for reviewing, I made some changes to get the build to pass. Also when I run helm lint --strict I run into other errors unrelated to my changes.

elasticsearch/templates/service.yaml" at <.Values.service.nodePort>: map has no entry for key "nodePort"

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 tcp-http which changes how istio handles the traffic which fixed the issue, and because elastic has ssl enable traffic is still secure.

@Crazybus
Copy link
Contributor

we are using the all of the ELK helm charts (which are a work of art imo)

Thank you so much, it really means a lot :)

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 tcp-http which changes how istio handles the traffic which fixed the issue, and because elastic has ssl enable traffic is still secure.

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.

Copy link
Contributor

@Crazybus Crazybus left a 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" }}
Copy link
Contributor

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.

Suggested change
- name: {{ .Values.service.httpPortName | default "http" }}
- name: {{ .Values.service.httpPortName }}

@Crazybus
Copy link
Contributor

jenkins test this please

@Crazybus Crazybus merged commit 573f2f3 into elastic:master Sep 19, 2019
@GreenKnight15
Copy link
Contributor Author

Thanks again, I look forward to contributing again in the future.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants