Skip to content

Conversation

padamstx
Copy link
Contributor

@padamstx padamstx commented May 2, 2023

This commit adds support for a second default CR token filename. If the user doesn't specify a CR token filename, the authenticator will first try /var/run/secrets/tokens/vault-token, and then /var/run/secrets/tokens/sa-token in that order.

@padamstx padamstx requested review from dpopp07 and pyrooka May 2, 2023 20:15
@padamstx padamstx self-assigned this May 2, 2023
Copy link
Member

@pyrooka pyrooka left a comment

Choose a reason for hiding this comment

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

LGTM!

} else {
// If the user didn't specify a filename, try our two defaults.
crToken, err = authenticator.readFile(defaultCRTokenFilename1)
if err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

I think this is perfectly fine here, but a question came to my mind and can't let it go. We would like to try to read the 2nd file, even if the error is not a FileNotFound but - let's say - a permission issue, right? Really just asking... :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question... yes, I thought about the various errors that might occur and whether we should treat a permissions issue differently than a "file doesn't exist at all" issue.
I concluded that we should try to keep this simple and treat any error while trying to read the file the same... i.e. we couldn't read it (whatever the reason) so move on to the next one to try. I think it's extremely unlikely that we'd encounter a scenario where both default files (vault-token and sa-token) exist and we end up using "sa-token" by mistake because "vault-token" is not readable by the application.

Copy link
Contributor

@dpopp07 dpopp07 left a comment

Choose a reason for hiding this comment

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

Looks good and tested on internal cluster 👍

This commit adds support for a second default CR token filename.
If the user doesn't specify a CR token filename, the authenticator will
first try '/var/run/secrets/tokens/vault-token', and then
'/var/run/secrets/tokens/sa-token' in that order.

Signed-off-by: Phil Adams <phil_adams@us.ibm.com>
@padamstx padamstx force-pushed the add-default-filename branch from 00d9171 to 1362e18 Compare May 22, 2023 20:15
@padamstx padamstx merged commit 25472f3 into main May 22, 2023
@padamstx padamstx deleted the add-default-filename branch May 22, 2023 20:34
ibm-devx-sdk pushed a commit that referenced this pull request May 22, 2023
## [5.13.4](v5.13.3...v5.13.4) (2023-05-22)

### Bug Fixes

* **ContainerAuthenticator:** add sa-token as default CR token filename ([#183](#183)) ([25472f3](25472f3))
@ibm-devx-sdk
Copy link

🎉 This PR is included in version 5.13.4 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants