Add proxyVarsFromSecret: "" option #141

Closed jan-brychta closed 6 months ago

added option to load OAUTH2_PROXY variables directly from a secret

Reasoning ExternalSecrets with GitLab provider makes this chart really hard to use, since you cannot name a GitLab CI/CD variable using "-" such as cookie-secret.

@jan-brychta the proxyVarsAsSecrets is not already enough? Ref: https://github.com/oauth2-proxy/manifests/blob/main/helm/oauth2-proxy/templates/deployment.yaml#L122-L138

@pierluigilenoci Not really, that's why I have provided a reasoning beforehand.

proxyVarsAsSecrets forces you to have the keys in the form of client-id, client-secret and cookie-secret which is then passed to a variable OAUTH2_PROXY_whatever anyway. In my opinion, it's unnecessarily complicated.

In our case, we store this secret in GitLab CI/CD variables from where it is synced directly into the cluster. And you cannot name a variable with - there, as I mentioned before.

@jan-brychta is OK; I got it.

You could document it better in the PR so that the difference with the other option is apparent to those using the chart. And maybe a test. In addition, the chart version also needs a bump.

Alright, thanks for the heads up @pierluigilenoci

@pierluigilenoci @desaintmartin Hey, would you mind sparing a moment to give me a hand with this? I'd really appreciate it!

@jan-brychta , I'm not sure what you want from us. Let me explain it again.

What I asked you to integrate into your PR are two points:

You are introducing a new way to inject secrets besides what already exists into the pod. Your PR is missing any documentation of the new way. I want to ask you to document better the differences between the two methods in the README.md so that whoever has to choose which method to use has the cognitive tools to make a choice.
Currently, the chart does not have a test that allows you to verify that the current injection method works. It would be a plus to have a test that confirms that both methods work.

Concerning the PR probably also missing a change to the file that generates the secret.

@jan-brychta, I tagged you in the last comment wrong, so I wonder if you received a notification. I do it again in another comment just to be safe.

@jan-brychta, could you please revisit your PR? Ref: https://github.com/oauth2-proxy/manifests/pull/141#issuecomment-1547423378

@pierluigilenoci Can you please fix the deployment by moving the envFrom outside of env? I think the envFrom needs to be gated too. If you prefer, I can open a new PR with the required changes. Thank you.

@aslafy-z please feel free to open a PR to address that.

@jan-brychta, could you please revisit your PR?

@pierluigilenoci I meant https://github.com/oauth2-proxy/manifests/pull/141/files#r1506261867

Please note that it should be moved out of the env: block (before line 145 for eg.)

My comment https://github.com/oauth2-proxy/manifests/pull/141#issuecomment-1969520087 is still to be addressed

@aslafy-z, you must ask @jan-brychta to address your comments.

@aslafy-z or @jan-brychta, could you please take a look and try to push forward the PR?

This PR has little value to me. However, I opened #196 with the envFrom placement fixed. I closed it, feel free to re-open it if you want to proceed with it and add tests & mention in the readme. If no answer is received from @jan-brychta, I think you can close this PR too.

Closed in favor of #196

Appologies @pierluigilenoci, got busy with other project... Thanks for taking over @aslafy-z !

oauth2-proxy / manifests

Add proxyVarsFromSecret: "" option #141