Feature/756

[aulea]

#756 tested on Windows.
Fixed RegistryAuthLocatorTest on Windows and also allowed better fallbacks from running credential provider (to allow lookup alternative AuthConfigs), when:

1. there is no hostName, then there is no point to ask credentials
2. when credential helper response with "credentials not found in native keychain" to try other resources

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.

[rnorth]

😍 I already love this PR!

Will give this a proper review and try later today, but thank you for taking this on!

[rnorth]

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.

[aulea]

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());
	}

)

[rnorth]

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.

[rnorth]

I did some manual testing against our ECR registry today on OS X, mainly to check for regressions. It's working fine (better than before!) with a couple of different credential stores, so I'm happy with this PR and very keen to merge.

Thank you @aulea 🙇

@kiview, @bsideup anything you want to add?

[aulea]

I changed logging.
Should i use there logger from DockerLoggerFactory or what is policy of that?

[rnorth]

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!

[aulea]

You're welcome. Then its done by be for now.
I guess that the one failed travis test couldn't be caused by my changes.

[bsideup]

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?

[aulea]

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

[bsideup]

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 :)

[aulea]

:-) will see how it comes out

[aulea]

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.
Hardcoded the hostname to validate for.
Retrieved message is cached for each credentials helper on RegistryAuthLocator class level (its singleton).

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.

[rnorth]

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.

[aulea]

Changes are ok by me - better naming.
I updated fake url in unit tests aswell.

[rnorth]

Merged! Thank you for the contribution @aulea!

[rnorth]

Released for preview in 1.9.0-rc2, to be published on Bintray.