search

concourse / concourse-chart

Helm chart to install Concourse
Apache License 2.0
143 stars 176 forks source link

fix: provide CONCOURSE_VAULT_AUTH_PARAM for ldap backend #223

Closed jmccann closed 3 years ago

jmccann commented 3 years ago

Existing Issue

Fixes #222

Why do we need this PR?

Allow setting CONCOURSE_VAULT_AUTH_PARAM when using CONCOURSE_VAULT_AUTH_BACKEND=ldap

Contributor Checklist

Reviewer Checklist

This section is intended for the core maintainers only, to track review progress. Please do not fill out this section.

  • [ ] Code reviewed
  • [ ] Topgun tests run
  • [ ] Back-port if needed
  • [ ] Is the correct branch targeted? (master or dev)
xtremerui commented 3 years ago

Hi could you please fix the DCO error?

jmccann commented 3 years ago

@xtremerui This is done ✅ ! Thanks!

xtremerui commented 3 years ago

I have limited knowledge of vault auth backend. However from the https://www.vaultproject.io/api-docs/auth list I can see there are multiple auth backends available. approle and ldap are only two of them.

So I wonder is there any other vault auth backend that also needs params when login through Concourse? In this case we might want to update the condition of setting CONCOURSE_VAULT_AUTH_PARAM in chart. WDYT?

edit: maybe we could just remove the condition to make the param available for all backends

jmccann commented 3 years ago

@xtremerui I'm not sure if/what all could require it.

Should I have some sort of "enable" flag for this? Maybe concourse.web.vault.useAuthParam=true/false? It'd be a breaking change then too, right?

xtremerui commented 3 years ago

I think removing the if condition is fine. So user could decide if they want to include auth params without worring about what auth backend it is. Ultimately freedom :)

jmccann commented 3 years ago

I think the issue is then it'd expect the secret to be provided. So if they don't need it there would be a secret lookup on a secret that does not exist which would result in an error.

That's why I'm wondering if we need to have some toggle to control trying to load that secret or not.

xtremerui commented 3 years ago

why would it be error? we have a default value in https://github.com/concourse/concourse-chart/blob/41a46463a2f3b165a80fbc9fb9137e3f735c224d/templates/web-secrets.yaml#L97

so if users don't need it they can just leave it unset in values.yml. Do you mean the error in helm or concourse?

maybe we could exclude that env var by 

     {{- if not (eq .Values.concourse.web.vault.authBackend "")}}

since we'd only assume auth param is not needed when auth backend is not set.

jmccann commented 3 years ago

why would it be error? we have a default value in

We are managing the secrets outside of the chart (secrets.create=false). If someone is manually managing their secrets they may not have secrets defined that they don't actually need/use.

I worry about a use-case where for example someone is using vault with the cert backend. They wouldn't use vault-client-auth-param. They don't have (and I don't think they should need) a blank vault-client-auth-param in their secrets they are manually managing. So wrapping the CONCOURSE_VAULT_AUTH_PARAM in the deployment template with something like concourse.web.vault.useAuthParam allows them to not include a secret they don't need ... but if they need the secret they can provide it and enable the use of it.

This is similar to CA certs. Not everyone needs to specify a CA cert. They can control with concourse.web.vault.useCaCert if they are providing it or not. https://github.com/concourse/concourse-chart/blob/36ddb7589d7e2a7ba908a1bdd6336fa85fcb48d6/templates/web-deployment.yaml#L1513-L1516

Then they don't need to include the secret vault-ca-cert in their secrets if they don't use/need it.

https://github.com/concourse/concourse-chart/blob/41a46463a2f3b165a80fbc9fb9137e3f735c224d/templates/web-secrets.yaml#L93

xtremerui commented 3 years ago

@jmccann thx for the explaination. I think it makes sense.

We will make sure this change goes with a minor release then.

jmccann commented 3 years ago

Thanks @xtremerui , I made the updates. Let me know if I missed anything!