cli/config/credentials: refactor DetectDefaultStore and add tests #5568
Conversation
thaJeztah
added
status/2-code-review
kind/refactor
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5568 +/- ##
==========================================
+ Coverage 59.60% 59.67% +0.07%
==========================================
Files 345 343 -2
Lines 29103 29113 +10
==========================================
+ Hits 17346 17373 +27
+ Misses 10788 10770 -18
- Partials 969 970 +1 |
There was a problem hiding this comment.
Left some nits/comments. I'm not super convinced about having the whole pass logic (check for both the pass binary and docker-credential-pass) outside of _linux.go.
Maybe we could have a separate checkForPreferredHelper func that we can override instead, or some other solution that allows to keep this code separate while still making it possible to test cross-platform?
| func DetectDefaultStore(customStore string) string { | ||
| if customStore != "" { | ||
| // use user-defined | ||
| return store | ||
| return customStore | ||
| } |
There was a problem hiding this comment.
Sidenote, but
func bork(foo string) string {
if foo != "" {
return foo
}
...
}is definitely something 😅 we should see if we could change this for the next major.
There was a problem hiding this comment.
Right, so I the logic here was that if the config-file has a credentials-store configured, we accept it whatever it is, and consider it an option set by the user; when used, it may error, but that's intentional (you configured a helper, we tried using it, and it failed! intervention is needed).
There was a problem hiding this comment.
But now the fun bit;
Lines 162 to 172 in 32ff200
See what's wrong in the logic there? Hint:
Line 168 in 32ff200
That nice porcelain ContainsAuth() function checks if either a credentials-store is configured or per-registry helpers, or contains credentials;
cli/cli/config/configfile/file.go
Lines 90 to 94 in 32ff200
I guess we could consider it an extra line of defence to never overwrite what's there, but definitely a bit iffy, and looks like it was there from the start moby/moby@cf721c2
| if preferred := findPreferredHelper(); preferred != "" { | ||
| return preferred | ||
| } | ||
| if defaultHelper == "" { | ||
| return "" | ||
| } | ||
| if _, err := exec.LookPath(remoteCredentialsPrefix + defaultHelper); err != nil { | ||
| return "" | ||
| } | ||
| return defaultHelper | ||
| } |
There was a problem hiding this comment.
Why are we testing for defaultHelper's existence, but not preferred's?
There was a problem hiding this comment.
Nevermind, I see that the test for the preferred helper is inside findPreferredHelper. I find this a bit hard to read, maybe because it mixes levels of abstraction. Maybe instead we could do something like
| if preferred := findPreferredHelper(); preferred != "" { | |
| return preferred | |
| } | |
| if defaultHelper == "" { | |
| return "" | |
| } | |
| if _, err := exec.LookPath(remoteCredentialsPrefix + defaultHelper); err != nil { | |
| return "" | |
| } | |
| return defaultHelper | |
| } | |
| if preferred := findPreferredHelper(); preferred != "" { | |
| return preferred | |
| } | |
| return findDefaultHelper() | |
| } | |
| func findDefaultHelper() string { | |
| if defaultHelper == "" { | |
| return "" | |
| } | |
| if _, err := exec.LookPath(remoteCredentialsPrefix + defaultHelper); err != nil { | |
| return "" | |
| } | |
| return defaultHelper | |
| } |
| // Note that the logic below is specific to detection needed for the | ||
| // "pass" credentials-helper on Linux (which is the only platform with | ||
| // a "preferred" and "default" helper. This logic may change if a similar | ||
| // order of preference is needed on other platforms. |
There was a problem hiding this comment.
The "detection" logic below is generic, right? It's only used for linux (because linux is the only platform with a preferred helper), but would work for any other platform all the same?
There was a problem hiding this comment.
// a "preferred" and "default" helper. This logic may change if a similar
// order of preference is needed on other platforms.If this comment as a whole is about the preferred vs default helper order of preference, then I'd suggest placing it above, inside DetectDefaultStore near the operations regarding both the preferred and default helpers.
There was a problem hiding this comment.
The "detection" logic below is generic, right? It's only used for linux (because linux is the only platform with a preferred helper), but would work for any other platform all the same?
AAAHH, this is about checking for both pass and docker-credential-pass. I wonder if we can make this clearer. Maybe separate this comment from the comment about the order (preferred vs default)? It was somewhat hard to figure out what this comment was about.
There was a problem hiding this comment.
I realize you moved this here in order to make this easier to test cross-platform, but this is exactly the type of thing that would make sense to put in a _linux.go file and not leak to the rest of the code – otherwise, we need to have it here with a number of comments explaining why, while it's unnecessary for anyone worrying about any other platform to look at).
Refactor the DetectDefaultStore to allow testing it cross-platform, and
without the actual helpers installed.
This also makes a small change in the logic for detecting the preferred
helper. Previously, it only detected the "helper" binary ("pass"), but
would fall back to using plain-text if the pass credentials-helper was
not installed.
With this patch, it falls back to the platform default (secretservice),
before falling back to using no credentials helper (and storing unencrypted).
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah
force-pushed
the
refactor_DetectDefaultStore
branch
from
e8e05fd to
555aba2
Compare
|
Thanks for reviewing! I fixed the typo; will have a look at the other bits tomorrow. I guess we could generalize the for a credentials helper to have a "prerequisites check" (for Perhaps ultimately, the credentials-helpers themselves could have some check ( |
Honestly that sounds like the most sane idea to me – the CLI can't be expected to account for all the different credential helpers and their requirements, they should do it themselves. Maybe even something like the metadata we have for plugins, credential-helpers could have a similar thing that would include if they are "ready" to be used or not, and some error message. Or the API could just be |
Refactor the DetectDefaultStore to allow testing it cross-platform, and without the actual helpers installed.
This also makes a small change in the logic for detecting the preferred helper. Previously, it only detected the "helper" binary ("pass"), but would fall back to using plain-text if the pass credentials-helper was not installed.
With this patch, it falls back to the platform default (secretservice), before falling back to using no credentials helper (and storing unencrypted).
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)