fix: look for vault cached token before login

[DaThumpingRabbit]

Description

This commit adds the check of the cached vault token before doing the authentication flow over again

Fixes: #536

Checklist

Please make sure that your PR fulfills the following requirements:

• Reviewed the guidelines for contributing to this repository
• The commit message follows the Conventional Commits Guidelines.
• Tests for the changes have been updated
• Are you adding dependencies? If so, please run go mod tidy -compat=1.17 to ensure only the minimum is pulled in.
• Docs have been added / updated
• Optional. My organization is added to USERS.md.

Type of Change

• Bugfix
• Feature
• Code style update (formatting, local variables)
• Refactoring (no functional changes, no api changes)
• New tests
• Build/CI related changes
• Documentation content changes
• Other (please describe)

Other information

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 85.29412% with 5 lines in your changes missing coverage. Please review.

Project coverage is 71.75%. Comparing base (61a4ef6) to head (9b76995).
Report is 19 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/utils/util.go	50.00%	4 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #544      +/-   ##
==========================================
+ Coverage   71.32%   71.75%   +0.43%     
==========================================
  Files          26       26              
  Lines        1953     1983      +30     
==========================================
+ Hits         1393     1423      +30     
  Misses        460      460              
  Partials      100      100              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[DaThumpingRabbit]

@werne2j @jkayani
Can you please run the workflow again ?
I have added tests on the use of cached tokens
Thanks !

[DaThumpingRabbit]

Hello @werne2j @jkayani
Any news on this ? Can you please run the workflow ?
Thank you 😄

[werne2j]

@DaThumpingRabbit can you rebase your branch with main? Will run once up to date

[DaThumpingRabbit]

Ah yes sorry I didn't notice the updates on the main branch, I just rebased it

[DaThumpingRabbit]

Will you have a chance to review that ? I am waiting for this to reduce my vault lease consumption

[werne2j]

@DaThumpingRabbit thanks for the PR. Looking it over, I think the approach I would like to see taken is to move all the token stuff (checking for and setting token) from the individual vault auth files to the vault backend https://github.com/argoproj-labs/argocd-vault-plugin/blob/main/pkg/backends/vault.go#L32.

We then call the v.AuthType.Authenticate func if can’t find a token, we can change that func to return the token and then call the set token func with that token.

[DaThumpingRabbit]

I see, I started to do those changes, however, I think that the token storing part was delegated to the individual auth files because of the specific token use-case (where the vault client detects the variable itself)
Do you think I should still extract this part to the vault backend file and make an extra case for the token to be handled differently ?

[DaThumpingRabbit]

Or if this seems cleaner to you, we could add a method to the AuthType type to indicate whether the storing / retrieving of the token should be considered (maybe a ShouldStoreToken function that returns a boolean and the vault backend conditions its behavior according to that)

[DaThumpingRabbit]

@werne2j I just committed a proposition to remove duplicated code in the vault auth types as you mentioned by following the idea I posted above
Let me know if this isn't ok for you and I can revert this commit

[werne2j]

@DaThumpingRabbit Looking back over it, i think going with the first strategy is fine. No need to over think it. If you want to roll back to that commit we can kick off the CI and try to get this into the next release

[DaThumpingRabbit]

@werne2j Thanks for checking it, I just rolled back to the first solution as you said and I am ready on my side if you want to trigger the CI again and merge it !

[DaThumpingRabbit]

@werne2j I am not sure I understand what went wrong with the pipeline on the mac tests
It doesn't look like something linked to what I added, what do you think ?

[werne2j]

Thanks @DaThumpingRabbit