-
-
Notifications
You must be signed in to change notification settings - Fork 523
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
feat: use credential helper in docker config, even if auth is empty in .docker/config.json #1079
Conversation
@rokjoana this LGTM at first sight, but wonder if you could cover the changes with some tests if possible (I could understand that mocking GCR would be difficult, but in any case, let's think if a test could be added). In any case, thanks for the quick fix! |
Kudos, SonarCloud Quality Gate passed! |
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
I've described two test possibilities but both are tricky and dirty. I can try the dirtiest one, where we could define a mock credential helper executable that we build before the tests and add it to the path so that cpuguy83/dockercfg can pick it up when calling in the path using the name in the configurations. |
Dirty but working :) Thanks for taking the time to explain and develop the fix |
BTW there is a file that has not been gofmt-ed, as golangci-lint is complaining in the GH action. Could you take a look 🙏 ? |
@rokjoana I think it would be great to resolve this bug for the next release. Are you able to continue or would you need more time for it? I can take the lead and continue on top of your work if needed. |
I have a similar but different problem and was hoping this PR would fix it. Sadly, it did not. I tested this PR locally but my issue persisted. I am on a Mac (Ventura 13.4) and was getting an error:
I dug into it a little more. My current theory is in the docker_auth.go file, the DockerImageAuth function is looking for the registry for the image. The default registry on Mac is |
I do apologize for the delay. I had some personal stuff to deal with. I'll have a look at your comments and finish the PR soon. |
@moneymikeMD this log message is printed out in the case there is an error getting the DockerAuth, BUT the pull will take place: Please see Lines 962 to 977 in c0e1c32
Is Ryuk effectively pulled into your system? |
I totally get it, no worries at all 😊 |
Kudos, SonarCloud Quality Gate passed! |
@mdelapenya I had a look and the only way for us to specifically test this is to, during the tests, register an executable in the PATH that would output the credentials of a mocked private registry repository (just like the docker credential helpers do). |
No, it doesn't pull the image because its not using credentials. Then, I get the original |
@moneymikeMD I can confirm that the log occurs but on my machine the images are pulled. As far as the issue is about, I think it could be something unrelated. Could you share the format of your .docker/config.json? |
Kudos, SonarCloud Quality Gate passed! |
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.
LGTM! Thanks for your patience during the review, it took a while, having summer and parental leave in between, but here we go 🚀
@moneymikeMD I'd appreciate if you can double check what @rokjoana asked here, in order to try to debug the error you are experiencing:
It will need opening a different GH issue for tracking, I can do the linking if needed 🙏 |
* main: Simplify dependabot updates sorting (testcontainers#1460) feat: use credential helper in docker config, even if auth is empty in .docker/config.json (testcontainers#1079) chore(deps): bump github.com/aws/aws-sdk-go-v2/service/s3 (testcontainers#1457)
* main: (29 commits) Add support for MongoDB testing module (testcontainers#1447) Support groups in dependabot updates (testcontainers#1459) chore: run modulegen tests on Windows (testcontainers#1478) Add default labels when Ryuk is disabled (testcontainers#1339) feat: add clickhouse module (testcontainers#1372) chore: increase timeout for go test and GH action steps (testcontainers#1475) chore: triple max timeout for the workflow run, which takes +10m (testcontainers#1474) chore(deps): bump github.com/aws dependencies in /modules/localstack (testcontainers#1472) chore(deps): bump Google emulators dependencies in /examples (testcontainers#1471) all: fix goroutine leaks (testcontainers#1358) chore(deps): bump github.com/neo4j/neo4j-go-driver/v5 in /modules/neo4j (testcontainers#1427) chore(deps): bump github.com/tidwall/gjson from 1.14.4 to 1.15.0 in /modules/vault (testcontainers#1428) chore: add a GH action for release drafter (testcontainers#1470) chore(deps): bump mkdocs-material from 3.2.0 to 8.2.7 (testcontainers#1468) Add global testcontainers header to docs (testcontainers#1308) Simplify dependabot updates sorting (testcontainers#1460) feat: use credential helper in docker config, even if auth is empty in .docker/config.json (testcontainers#1079) chore(deps): bump github.com/aws/aws-sdk-go-v2/service/s3 (testcontainers#1457) Revert "chore: run Windows tests on a Linux container (testcontainers#1456)" chore: run Windows tests on a Linux container (testcontainers#1456) ...
What does this PR do?
If when retrieving the docker configuration, when there's no
auth
field on the user's docker config and if the user has configured credential helpers, the code will use the credential helpers to retrieve the auth information for the image repository instead of setting it to empty.Why is it important?
Credential helpers are often used by third party docker repository providers to perform access control on the img repository.
In my case I'm using GCP but this is the case also for aws and azure. This change will extend the possibility of usage of images that are located in those repositories.
Related issues
Closes #1078
How to test this PR
This is a tricky one to test because of the way the credentials are retrieved using these helpers.
cpuguy83/dockercfg
replicates the usage the docker CLI command: it calls an executable located in the PATH, which needs to output the credentials to stdout. This is tricky to emulate on go because one would need to use a script to link an executable that would mock the behaviour of a credential helper to the PATH when running the test.The other solution would involve having a private registry setup in a private registry provider such as GCP and use the credential helper of the provider. That would also imply some dependencies on the tests.
Fortunately I had access to a private registry, so I was able to verify it by just using my fork in the project where I was using an image of the priv repository in my tests.