-
Notifications
You must be signed in to change notification settings - Fork 143
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
Standardize the fetching of creds #26
Comments
@majormoses I'd be happy to help with this issue and propose one (or several) pull request(s). As far as I understand (cf. https://github.com/sensu-plugins/sensu-plugins-aws#authentication) that would require:
Obviously, that would be a major breaking change, but that would make the configuration more uniform and would be a nice simplification/cleanup of all these checks. |
Thanks for taking a look and volunteering to help, let me try to address inline. @eheydrick correct me if I am missing anything.
Yup, any work on this front is greatly appreciated
Yup, I'd do it in the same PR as updating to sdk v2. Example: #248
Yes, I'd remove it in the same PR as updating to sdk v2.
Hmm I was under the impression only the checks that have been migrated to v2 had included
Well it basically updates those options if defined otherwise relies on the base aws config class which automatically searches for credentials from the ENV and the
It is very relevant and 3/4 of the checks need a region.
If you include |
Just a question. Why remove the ability to specify via command line? The code you documented, allows for CLI, Env, and .aws/config. The only thing it doesn't allow (but can with a very simple change) is specifying role by cli. |
The issue with credentials on the command line is that it's far too easy to do it insecurely. If you don't redact the keys you'll end up exposing them in logs, the API, source control, the dashboard, etc. By removing that option we avoid the exposure potential. |
Some of the plugins can pull from the env and others require CLI. In the long run we want to do this 100% by config, but for now we should support CLI/ENV for all plugins. Copy paste code here. Pretty simple change.
The text was updated successfully, but these errors were encountered: