pachyderm / helmchart

Helm Chart for Pachyderm
5 stars 9 forks source link

Allow TLS to be disabled by default #81

Closed chainlink closed 3 years ago

chainlink commented 3 years ago

Instead of explicitly setting two values to correctly disable SSL, it should be disabled by default. Unfortunately, with json schema, we can't have our cake and eat it too. If we want full object validation, we can't set null in values.yaml, but if we don't set it to null we can't have a no TLS default. The solution is to have a create flag explicitly create the manifest.

Other improvements proposed in this PR:

Closes: https://github.com/pachyderm/helmchart/issues/77 Closes: https://github.com/pachyderm/helmchart/issues/64

robert-uhl commented 3 years ago

I think that it’s great to split out the dash and pachd TLS, and love seeing an improvement to the wonky way the chart currently handles TLS and creation.

This would be an example invocation at the command line; it would be similar when calling Helm as a library:

helm install pachd pachyderm/pachyderm \
  --set dash.tls.secretName pach-tls \
  --set dash.tls.enabled true \ # implied by the fact that dash.tls.secretName is non-empty
  --set pachd.tls.secretName pach-tls \ # same as dash.tls.secretName
  --set-file pachd.tls.newSecret.crt my.crt \
  --set-file pachd.tls.newSecret.key my.key \
  --set pachd.tls.newSecret.create true \ # implied by pachd.tls.newSecret.crt & pachd.tls.newSecret.key above
  --set pachd.tls.enabled true \ # implied by the fact that pachd.tls.secretName is non-empty
  ⋮

I think there’s a risk of users getting tripped up by having to set values and set a value saying to use the set values. But it keeps the template logic clean, which is good. In the longer term there may be some better ways to handle this, but in the short term this advances the system.

Let’s circle back to it in the future.