-
Notifications
You must be signed in to change notification settings - Fork 0
Helm chart for stac-auth-proxy #44
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
Conversation
Dont forget serviceaccountname placeholder int deployment as we need it to access secrets in general and on azure specifically |
@batpad I will give a deeper look on the whole chart tomorrow. |
@emmanuelmathot what sort of secrets would stac-auth-proxy need access to? Afaik, it should just need the |
When utilizing RBAC resources, it's important to have the option to set the service account. For example, this is useful for pulling images from a secure hub when the keys are delegated to a specific service account. Best practices recommend always using a service account for security reasons. This approach does not necessarily complicate the chart. |
@emmanuelmathot made a push adding a service account definition and some configurability in |
that's perfect. Thank you @batpad |
I've used the helm chart here to deploy a test instance to our Labs cluster: https://stac-auth-proxy-test.k8s.labs.ds.io/ I used the following
The deploy seems to be working and the startup logs look good. @alukach I'll need your help to properly test this (as well as some questions around working with the JWT tokens we intend to use). @emmanuelmathot - this seems to successfully deploy - is there anything else you feel we should do before we can merge here? Else I'll feel okay merging once I've confirmed with @alukach that things work as expected. |
LGTM |
@@ -0,0 +1,71 @@ | |||
Thank you for installing {{ .Chart.Name }}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So is this rendered in your terminal at time of deployment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is what renders when you do helm install
/ helm upgrade
@alukach I have changed the structure of the I do much prefer this, so thank you. So now,
I've still left documentation for the possible env vars in the @emmanuelmathot let know if this change to just using Am really not sure why I went with the nested |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, should we merge?
Hmm .. tried rebasing and then testing running this on the labs cluster, and am getting errors in the logs that I'm not sure I can figure out @alukach -
I think this may be me not adding new required variables, but some of these are a bit confusing / unexpected to me. I've verified that OIDC_DISCOVERY_URL and UPSTREAM_URL env vars are correctly set in the container:
@alukach I think this will be a lot quicker to debug if we can pair 👀 on it quickly. Let me know if you have a bit of time once you're up. Thanks! We are definitely almost there. |
@batpad from the error message, looks like some of those urls have an extra pair of quotes around them? |
Where do you get your eyes from. Nice catch. Can try a couple things. But that's strange. |
@sunu that was it :-) - so that error is fixed. |
Maybe as a follow-up to ensure that helm chats is tested properly, a CI/CD test deploy like for titiler is useful |
@emmanuelmathot haa I was just looking into adding tests, but will do that as a separate PR. Thanks for the pointer! I'll open a separate ticket about that and would be good to get your thoughts on the multiple possible approaches to testing that I've been looking into a bit. |
Sure. eoapi has similar but slightly more complex tests: https://github.com/developmentseed/eoapi-k8s/blob/main/.github/workflows/helm-tests.yml |
… required values over-ridden
First draft for a Helm Chart for stac-auth-proxy.
@emmanuelmathot I liberally used AI to help me write this code, and I think some of the stuff might be a bit much in terms of the configuration options.
Would be great to get just a bit of initial feedback here and then I can work on testing this and then adding the CI to publish the chart, etc.
cc @sunu for visibility