-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Feature/756 #868
Feature/756 #868
Conversation
…for docker-credential-helper. Instead of exception return NULL to allow fallback in lookupAuthConfig logic https://github.com/docker/docker-credential-helpers/blob/master/credentials/credentials.go#L125-L128
… NULL instead of exception to allow fallback in lookupAuthConfig logic
log.debug("RegistryAuthLocator is not supported on Windows. Please help test or improve it and update " + | ||
"https://github.com/testcontainers/testcontainers-java/issues/756"); | ||
return defaultAuthConfig; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😍 I already love this PR!
Will give this a proper review and try later today, but thank you for taking this on!
log.debug("Credentials not found in native keychain, credential helper ({}), return null to allow fallback", credentialHelperName); | ||
return null; | ||
} | ||
log.debug("Failure running docker credential helper ({}) with output '{}'", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's cool that we're now detecting the 'credentials not found' case and dealing with it in a better way!
Now, I wonder if the logging on line 222 and 226 should be at a higher level. There's probably no 'good' reason why these lines should be hit, so perhaps we should log at ~warn level to aid users in diagnosing a fault.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure, cause we are throwing exception from there, so maybe we should log it on higher level, like here:
testcontainers-java/core/src/main/java/org/testcontainers/utility/RegistryAuthLocator.java
Lines 118 to 123 in 665e08b
} catch (Exception e) { | |
log.debug("Failure when attempting to lookup auth config (dockerImageName: {}, configFile: {}. Falling back to docker-java default behaviour. Exception message: {}", | |
dockerImageName, | |
configFile, | |
e.getMessage()); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true; please could we then:
- Make the top level failure logging (line 119) log at warn level
- Still, also, make line 219 be at info or warn level. The reason I think we should do this is the credentials-not-found message is probably something that is helpful in understanding why things aren't working. I'd assume that most people are filtering Testcontainers' logs at INFO+ level, so debug is too low a level to hide such information.
Either way this is all just icing on the cake - we can always finesse logging levels later.
I changed logging. |
Cool, thanks. Please could you just use the logger for the class as in a conventional project?
We tend to only use DockerLoggerFactory for high level narrative of container lifecycle events.
Thank you!
|
You're welcome. Then its done by be for now. |
// ErrCredentialsNotFound standardizes the not found error, so every helper returns the same message | ||
// We can handle it properly with fallback to other resources. | ||
// https://github.com/docker/docker-credential-helpers/blob/19b711cc92fbaa47533646fa8adb457d199c99e1/credentials/error.go#L4-L6 | ||
if ("credentials not found in native keychain".equals(e.getResult().outputString().trim())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest checking only "credentials not found", "native keychain" says something about the implementation and might change in future
Also, what about this one?
https://github.com/docker/docker-credential-helpers/blob/d499cf5cb90029fdd50c550243b16363857694d7/osxkeychain/osxkeychain_darwin.go#L21
To be honest, I feel uneasy about these string checks. That's just a text without an error code or something, and (knowing Docker guys) it might be changed anytime.
Is it possible to perform a forward check instead of relying on the message only?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, that just text isn't good.
I could change to check the content of "credentials not found".
In osxkeychain, they translate it to general message as used here:
https://github.com/docker/docker-credential-helpers/blob/d499cf5cb90029fdd50c550243b16363857694d7/osxkeychain/osxkeychain_darwin.go#L90-L91
Central/common credentials error messages
https://github.com/docker/docker-credential-helpers/blob/19b711cc92fbaa47533646fa8adb457d199c99e1/credentials/error.go#L24-L28
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or, I have even crazier idea :D
Before checking, we run code with 100% "credentials not found" case, and then use the message we receive to compare with user's credentials, so that we never hardcode the message :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:-) will see how it comes out
…tials not found' error message for each credentials helper
…tials not found' error message for each credentials helper. unix fake credential helper fix
Implemented @bsideup proposed idea to first find out with what message exactly specific credentials helper response. Its not 100% confident solution, but i think enough. To be 100% sure, then probably should ask first the list of host names from credentials to be sure not to ask credentials for existing one. In unit tests I changed fake credentials helpers to response with error message, when there is try with "registry2.example.com" or with hostname used by new implemented logic to do fake call. And added simple test to confirm that we did fake call and find out how helper response for unknown host. |
Thanks @aulea! I had a few tiny naming/style tweaks I wanted to make, so rather than give you a list I've just pushed these to your branch - I hope these all look OK to you. For the 'test' registry hostname I've updated it to something that would be a bit less mysterious for people who see it. I've retested with my company's ECR setup and it continues to work well for me - I'm happy with this. |
Changes are ok by me - better naming. |
Merged! Thank you for the contribution @aulea! |
Released for preview in 1.9.0-rc2, to be published on Bintray. |
#756 tested on Windows.
Fixed RegistryAuthLocatorTest on Windows and also allowed better fallbacks from running credential provider (to allow lookup alternative AuthConfigs), when:
Main reason for failing for me on Windows machine was #710 changes. When i used Netty or OkHttp together with npipe, then it worked fine. Yesterday evening i found out the reason and today morning i found also fix in master for that :-) - #865, breaking docker response by line breaks.