Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add proxyVarsFromSecret: "" option #141

Closed

Conversation

jan-brychta
Copy link

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.

@pierluigilenoci
Copy link
Contributor

@jan-brychta
Copy link
Author

@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.

@pierluigilenoci
Copy link
Contributor

pierluigilenoci commented Apr 20, 2023

@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.

@jan-brychta
Copy link
Author

Alright, thanks for the heads up @pierluigilenoci

@jan-brychta
Copy link
Author

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

@pierluigilenoci
Copy link
Contributor

pierluigilenoci commented May 15, 2023

@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.

@pierluigilenoci
Copy link
Contributor

@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.

@pierluigilenoci
Copy link
Contributor

@jan-brychta, could you please revisit your PR?
Ref: #141 (comment)

@aslafy-z
Copy link
Contributor

aslafy-z commented Jan 23, 2024

@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.

@pierluigilenoci
Copy link
Contributor

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

@pierluigilenoci
Copy link
Contributor

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

@aslafy-z
Copy link
Contributor

Co-authored-by: Zadkiel Aharonian <zadkiel.aharonian@gmail.com>
@aslafy-z
Copy link
Contributor

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

@aslafy-z
Copy link
Contributor

aslafy-z commented Mar 6, 2024

My comment #141 (comment) is still to be addressed
image

@pierluigilenoci
Copy link
Contributor

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

@pierluigilenoci
Copy link
Contributor

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

@aslafy-z
Copy link
Contributor

aslafy-z commented Apr 9, 2024

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.

@pierluigilenoci
Copy link
Contributor

Closed in favor of #196

@jan-brychta
Copy link
Author

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

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

Successfully merging this pull request may close these issues.

None yet

3 participants