[elasticsearch-exporter] Support ssl-skip-verify option (and make securitycontext configurable)

[ghost]

What this PR does / why we need it:

• support ssl-skip-verify option (for version >= 1.0.4rc1)
• make securityContext configurable (so we can remove runAsUser:1000 that prevent the chart from running on OpenShift)

(Redo of #12627 that I accidentally closed)

[k8s-ci-robot]

Hi @bquartier. Thanks for your PR.

I'm waiting for a helm member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[ghost]

@svenmueller , @desaintmartin ,
This is an updated version of #1262 that I accidentally closed.
Sorry for the delay between the initial version and the corrected one.

[desaintmartin]

/assign
/ok-to-test
Thanks!

[desaintmartin]

More generally, could you add a test file in the ci directory (see https://github.com/helm/charts/tree/master/stable/elasticsearch/ci for inspiration)?

[desaintmartin]

Suggested change

	version: 1.4.1
	version: 1.4.0

[ghost]

I updated the version.

Regarding the test, I will need to have a look at the documentation.
What do you think should be tested in this case?

[svenmueller]

if you want to write tests, you might want to look at some examples, e.g. https://github.com/helm/charts/blob/master/stable/grafana/templates/tests/test-podsecuritypolicy.yaml

[ghost]

I don't really want to add test for such a small PR, but I was respondint to @desaintmartin coment regarding https://github.com/helm/charts/tree/master/stable/elasticsearch/ci

[desaintmartin]

Oh, it is very simple, just adding a file or two in ci that acts as values.yaml testing the different configurations you are introducing (along with an empty file to also test default values).
In my experience those things tend to break after some "unrelated" PRs if it is not tested.

[desaintmartin]

Maybe state that you should comment in order to disable.

Usually this is done by a enabled flag but it does not work this way here unless we explicitely get runAsNonRoot runAsUser in deployment.yaml. What do you think?

[ghost]

What is needed in the securityContext depends on the version of the image you use and the flavor of Kubernetes. I think it is good to have it completely configurable.
I added a comment in the value file.

[svenmueller]

i double-checked how other charts are doing it. It seems to be standard to use a flag for enabling/disabling configuration snippets (as @desaintmartin mentioned already), e.g.

{{- if .Values.securityContext.enabled }}
    securityContext:
        runAsNonRoot: {{ .Values.securityContext.runAsNonRoot }}
        runAsUser: {{ .Values.securityContext.runAsUser }}
       ...
{{- end }}

would this be an option?

[ghost]

@svenmueller this is an option.
However, with your example it would not be possible to set runAsNonRoot without setting runAsUser.
I could do:

{{- if .Values.securityContext.enabled }}
    securityContext:
        {{- if .Values.securityContext.runAsNonRoot }}
        runAsNonRoot: {{ .Values.securityContext.runAsNonRoot }}
        {{- end}}
        {{- if .Values.securityContext.runAsUser }}
        runAsUser: {{ .Values.securityContext.runAsUser }}
         {{- end }}
       ...
{{- end }}

but it seems quite heavy, not sure it is really worth it.

@svenmueller , @desaintmartin what do you think?

[svenmueller]

in that case, i would prefer you current solution.

[ghost]

Ok, thanks @svenmueller .

The lgtm label is still needed to merge this PR.

[ghost]

Who can put the lgtm label (my understanding of the process is that it comes before the approve label)?

[desaintmartin]

I've just realized something.
Actually, if you comment the whole section in your own custom values.yaml, it will take values from the default values.yaml (this file!) and it will be still used. Same if you comment runAsNonRoot for example.
This is why the enabled flags are preferred.
But maybe we can workaround this by setting securityContext: {} or something like this.

[ghost]

@desaintmartin You're right, it is not good. :(.

Initially, I wanted to let the securityContext configuration 100% configurable. However, it is not what other stable charts in this repository do.
I searched this repository for securityContext.enabled and I propose to use the same pattern as they use in the mongodb chart. It is not as configurable as I initially wanted it to be but it covers the two cases I am aware of, Kubernetes and OpenShift.

[desaintmartin]

I agree, I don't like this pattern (any new parameter requires a PR) but this might be the simplest way for now.

[svenmueller]

/approve

[desaintmartin]

1/ It also requires an empty file as well to test with the default values : https://github.com/helm/charts/blob/30b5c13ae1cad13655c0e3f43c7f4cf525e76944/stable/redis/ci/default-values.yaml, so that the default values are tested

2/ In this file, you can change securityContext to be disabled.

And then let's merge this!

[ghost]

@desaintmartin like this?

By the way, I am not familiar at all with those files for the CI. Do you have a link to the documentation?

[desaintmartin]

Sure! ;)
https://github.com/helm/charts/tree/master/test#providing-custom-test-values

[ghost]

Do you think we can merge this PR now?

[desaintmartin]

Thanks for this work!
/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bquartier, desaintmartin, svenmueller

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• stable/elasticsearch-exporter/OWNERS [desaintmartin,svenmueller]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment