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 jobs.api_key so jobs can use api_key instead of username/password #256

Closed
wants to merge 4 commits into from

Conversation

cognifloyd
Copy link
Member

@cognifloyd cognifloyd commented Oct 13, 2021

I always insist that automated jobs use API keys instead of username/password. This allows using an st2 API key with the helm hook jobs during helm upgrade. Just set jobs.api_key to a key that you created for helm to use.

Closes #233

@pull-request-size pull-request-size bot added the size/M PR that changes 30-99 lines. Good size to review. label Oct 13, 2021
@cognifloyd cognifloyd changed the title add jobs_api_key so jobs can use api_key instead of username/password Add jobs_api_key so jobs can use api_key instead of username/password Oct 13, 2021
@cognifloyd cognifloyd self-assigned this Oct 13, 2021
@cognifloyd cognifloyd changed the title Add jobs_api_key so jobs can use api_key instead of username/password Add jobs.api_key so jobs can use api_key instead of username/password Oct 13, 2021
Copy link
Member

@arm4b arm4b left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, but I'm afraid the edge case as I remember caused by inability to create the required LDAP user and seen in a single environment shouldn't be a reason to apply it as a workaround to everyone in the community. I don't believe this PR should be part of the official Helm chart.

I'd recommend using kustomize for very specific cases, it's a de-facto standard perfectly suitable for this example that can help.
See https://austindewey.com/2020/07/27/patch-any-helm-chart-template-using-a-kustomize-post-renderer/

values.yaml Outdated
Comment on lines 721 to 724
# For jobs that need to use the ST2 APIs, if you set jobs.api_key
# then the jobs will use jobs.api_key instead of st2.username/st2.password
# This is only useful when running `helm upgrade` as the api_key must be present
# before the apikey-load job runs (it will use this api_key if defined).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Relying on API key which will only be available on upgrade, - the scenario here looks like a problem in something else.

@cognifloyd
Copy link
Member Author

OK. I would insist on using an API key even without the issues with LDAP. That said, I think there are a couple of changes that might facilitate using a helm post-processor instead.

I'll mark this as draft for now, and probably close it later.

@cognifloyd cognifloyd marked this pull request as draft October 18, 2021 17:40
@cognifloyd cognifloyd removed the RFR label Oct 18, 2021
@rush-skills
Copy link
Member

I am not sure if this fits, but I also have certain use cases (install packs and running other jobs using puppet) where using API-keys or auth tokens to authenticate is better than using username/password (since in my case the puppet install is not aware of the LDAP credentials of the service account, it is just injected in the config at a later stage). I have been playing around with the puppet module to add the ability to install packs with API-key instead of doing user auth (currently it requires authenticating as a Stanley system user to get the tokens, my environment has no Stanley user). So I won't say it's a single environment use case and might be something that is helpful for the community in general.

Also again, it is okayish/safer to put an auth-token/API-key in the repo/conf/hiera than LDAP credentials of a user, at least in our case.

For the chicken and egg problem - maybe we can make the scripts use username/password is api-key is missing, and use them to generate a "system" api-key that can be used for further transactions.

@pull-request-size pull-request-size bot added size/S PR that changes 10-29 lines. Very easy to review. and removed size/M PR that changes 30-99 lines. Good size to review. labels Oct 26, 2021
@cognifloyd cognifloyd marked this pull request as ready for review October 26, 2021 17:49
@cognifloyd
Copy link
Member Author

@armab I think I clarified the issues with helm install vs helm upgrade.
What do you think of this PR now? Is this closer to something you're willing to include in this chart?

@cognifloyd
Copy link
Member Author

Closing in favor of #262

@cognifloyd cognifloyd closed this Nov 9, 2021
@cognifloyd cognifloyd deleted the jobs_api_key branch November 11, 2021 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Helm security size/S PR that changes 10-29 lines. Very easy to review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Allow hook/jobs to use an st2 api_key instead of user/pass
3 participants