Skip to content
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

Fix AuthenticatedImagePullTest #4560

Merged
merged 1 commit into from
Oct 8, 2021
Merged

Conversation

bsideup
Copy link
Member

@bsideup bsideup commented Oct 7, 2021

Previous implementation was assuming that Docker can pull from the gateway, which may not always be the case (eg Docker Desktop).
This change makes the temporary registry run with host network, so that Docker can always pull from it.

Previous implementation was assuming that Docker can pull from the gateway, which may not always be the case.
This change makes the temporary registry run with host network, so that Docker can always pull from it.
@kiview
Copy link
Member

kiview commented Oct 8, 2021

Oh, this is interesting. There were failing tests about this feature on Docker for Windows, let me check if this fixes them as well.

@rnorth
Copy link
Member

rnorth commented Oct 8, 2021

As mentioned on Slack, this has been working fine on Docker Desktop for Mac for absolutely ages, so if it's started to fail now I worry that we've encountered another regression/change in Docker Desktop.

I'll see if I can reproduce the bug locally - I wonder if this might also be fixed by (TBD ticket ref for -p0: workaround)

@rnorth
Copy link
Member

rnorth commented Oct 8, 2021

Ooh this is interesting:

  • This bug is reproduceable all the way back to Docker Desktop for Mac 3.4.0, on both x86 and M1. I can't test with older versions as they're no longer available to download (403 errors) I think our tardiness in upgrading Docker Desktop on our dev machines has meant that we've missed this regression when it first started.
  • Use host port 0 when setting up PortBindings #4396 seems to fix it (which makes sense, as the underlying problem was docker not binding on the gateway IP when a random port was requested)

Given that #4396 fixes other use cases that are currently broken, can we just advance with that?

@kiview
Copy link
Member

kiview commented Oct 8, 2021

I can confirm that this PR fixes AuthenticatedImagePullTest on Docker Desktop for Windows with WSL backend, while ImagePullPolicyTest is still failing.

I can also confirm, that #4396 fixes AuthenticatedImagePullTest on Docker Desktop for Windows with WSL backend, while also fixing ImagePullPolicyTest.

Therefore, I am also in favor of merging #4396 instead and rely on this behavior, especially given that Docker wants to notify us in case of changes (as outlined in docker/for-mac#5588 (comment)):

I can't guarantee anything but I think there's no imminent danger of it stopping working: I'll add a test case which checks that it continues to work and linking to this issue so can avoid breaking it if possible / give you advance warning

@bsideup bsideup added this to the next milestone Oct 8, 2021
Copy link
Member

@rnorth rnorth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the basis that this helps with remote docker/docker-wormhole models, I'm happy with this.

I wonder if this scenario, of requiring docker daemon to be able to talk to a specific container, is something that we might repeat in a couple of other places. Regardless though, if it is, we could extract whatever common glue code we need in order to share it.

@bsideup bsideup merged commit 3740f65 into master Oct 8, 2021
@delete-merged-branch delete-merged-branch bot deleted the fix_AuthenticatedImagePullTest branch October 8, 2021 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants