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

Remove the base URI for docker.io images #37913

Closed
wants to merge 1 commit into from

Conversation

ggrebert
Copy link
Contributor

This feature allow users to use a mirror registry
to bypass the docker hub rate limit.

fix #37912

@quarkus-bot quarkus-bot bot added area/dependencies Pull requests that update a dependency file area/documentation area/hibernate-search Hibernate Search area/kafka area/redis labels Dec 22, 2023
Copy link

quarkus-bot bot commented Dec 22, 2023

/cc @gsmet (hibernate-search), @yrodiere (hibernate-search)

Copy link

quarkus-bot bot commented Dec 22, 2023

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

  • title should preferably start with an uppercase character (if it makes sense!)

This message is automatically generated by a bot.

@ggrebert ggrebert changed the title remove the base URI for docker.io images Remove the base URI for docker.io images Dec 22, 2023
Comment on lines 51 to 52
STRIMZI("quay.io/strimzi-test-container/test-container:latest-kafka-3.2.1"),
KAFKA_NATIVE("quay.io/ogunalp/kafka-native:latest");
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the same apply to other registries as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For quay.io it is more complicated: the --registry-mirror option works only with relative image.

But we can had a property to replace this value. But in an other PR,

@gsmet
Copy link
Member

gsmet commented Jan 3, 2024

@cescoffier @holly-cummins @Sanne could you have a look at this one? I remember we added the docker.io registry specifically at some point. Don't remember exactly why but I wonder if it was related to Podman.

@holly-cummins
Copy link
Contributor

holly-cummins commented Jan 3, 2024

I'd forgotten all about this, but 57332c3 confirms the decision to be explicit about the name was related to Podman, and short names. It was an issue that @Sanne found.

#31253 (comment) has more details:

Short names of containers. Testcontainers and Quarkus Dev Services also expect the container service they make requests against to be non-interactive. If someone has multiple registries configured, and when using short image names, Podman responds with a prompt asking which registry should be used to pull images. Testcontainers defaults to using short names internally, so users need to change this default this prompt by setting the short-name-mode="disabled" configuration property of Podman in /etc/containers/registries.conf. containers/podman#16400 may be tracking this, or it may be a related testcontainers + podman issue. I can't quite tell.

@n1hility was going to check if it was fixed (he thinks it might be), but I can't find the response. A possible workaround is "to pull them by full name before running any tests."

I agree that not hardcoding the registry everywhere seems desirable ... this might be something where the podman CI is useful for validating the impact of this PR. (Although it looks like the Podman CI is suffering from another issue and needs some attention anyway.)

@cescoffier
Copy link
Member

Yes, the registry name was required by podman. I'm finding references about something also required on fedora 35+.

Also, aren't all these images customizable by the user? Using the prefix also guarantees the origin of the image (and somewhat avoids having someone publishing a tainted image on a mirror).

@ggrebert
Copy link
Contributor Author

ggrebert commented Jan 3, 2024

Yes, the registry name was required by podman. I'm finding references about something also required on fedora 35+.

Also, aren't all these images customizable by the user? Using the prefix also guarantees the origin of the image (and somewhat avoids having someone publishing a tainted image on a mirror).

Image are customizable by the user (actual workaround).
But (to my known and searches) there is not a prefix property to change the DNS of devservices images.

@cescoffier
Copy link
Member

I'm not sure a prefix would help:

  • should it be added to all images without prefix?
  • should all images be prefix-less, but what do we do for images from quay or google?

@ggrebert
Copy link
Contributor Author

ggrebert commented Jan 7, 2024

  • should it be added to all images without prefix?

For me no. Because images without prefix are hosted on docker.io and can be mirrored directly by docker or podman.

  • should all images be prefix-less, but what do we do for images from quay or google?

Images hosted on quay.io and gcr.io have no rate limit. And some are not replicated on docker.io, so we cannot use prefix-less for those.
But it could be interesting to have a property to change easily the DNS (for internet less networks).

quarkus.devservices.registry.quay.dns=quay.io
quarkus.devservices.registry.gcr.dns=gcr.io

@cescoffier
Copy link
Member

To do that, we need to parse the image names and replace the DNS (by the way, I'm not sure of the wording here; I would prefer something like registry-hostname (and even hostname is not perfect as you can pass IPs).

The next question is about the scope. Should it be global (like in your proposal) or per image (which is literally what we have today, as you can replace the image)? I like the global approach as it's more in phase with the idea of a mirror. However, as I'm not an expert on image registry mirroring, don't we have the risk of having a mirror containing some images but not others (like if some images are private, the mirror may not have them)?

Finally, when the user overrides the image name, should we still apply the hostname? I would say no, but let's make sure we all agree.

@ggrebert
Copy link
Contributor Author

ggrebert commented Jan 8, 2024

new proposal:

quarkus.devservices.registry.quay.host=localhost:5000
quarkus.devservices.registry.gcr.host=gcr.mirror.internal

For private images, we already have to override the image property. So I am in tune with you:

When we override the image name
And the value use a full path
Then pull the image form this location
When we override the image name
And the value not use a full path
Then use the docker/podman configuration to pull the image
   (`docker.io` if no mirror is set)

So:

  • if a user wants to pull a custom image from the quay.io registry, he must use a full path.
  • if a user wants to pull a custom image from the docker.io registry, he can use full path ou relative path.

The good: this solution don't change the current behaviour)

@cescoffier
Copy link
Member

Completely agree with this new proposal.

@ggrebert
Copy link
Contributor Author

ggrebert commented Jan 9, 2024

cool,

What do you prefer ? :

  1. I rebase this PR (and check if no new image have been haded) and merge it to implement the docker.io/mirror part and I do a new one for the other registries ?
  2. edit this PR which implement all the features

@ggrebert
Copy link
Contributor Author

ggrebert commented Jan 9, 2024

to me, I prefer the fisrt one

@cescoffier
Copy link
Member

I would have done everything in this PR, but we can start with docker.io only and then do another PR with the other registries (less critical as it does not have rate limits)

This feature allow users to use a mirror registry
to bypass the docker hub rate limit.
@ggrebert
Copy link
Contributor Author

ready to merge for me.

Issue created for the other registries: #38146

@cescoffier
Copy link
Member

Why didn't you implement what we discussed:

  • a new property to change the host name of the docker.io registry
  • keep the docker.io name and apply the new host

@cescoffier
Copy link
Member

@ggrebert ping?

@ggrebert
Copy link
Contributor Author

problem of understanding on my part.

I thought that for docker.io we remove the host to allow the usage of the native --registry-mirror option.

When we override the image name
And the value not use a full path
Then use the docker/podman configuration to pull the image
(docker.io if no mirror is set)

If I understand correctly what you want, we never use the native configuration for a mirror. We always use a quarkus property to overide a host, even for docker.io which is a default regardless of the engine used

@cescoffier
Copy link
Member

Yes, I think it's better, as it makes things more homogenous with the other registries.

@ggrebert
Copy link
Contributor Author

OK, thx @cescoffier for clarifying the subject.

It can be take time to me due to all the different way the docker images are set in all the extensions and i work on it only on my free time.
But I am on it 🤘

@gsmet
Copy link
Member

gsmet commented Jun 7, 2024

@ggrebert any progress on this one?

@ggrebert
Copy link
Contributor Author

ggrebert commented Jun 8, 2024

Hello @cescoffier and @gsmet,

Sorry, I really haven't had time to work on this as much as I would have hoped
(a complicated divorce that takes up all my free time...).

However, I still studied @cescoffier solution.
But it is not so easy to implement:

I was going to create a helper in ConfigureUtil.java which replaces the host.

But we then have to modify all the DevServices in each extension.
The real problem with this solution is for external extensions such as Quarkiverse.
And has you know, the Quarkiverse extentions don't use the latest Quarkus version,
So all extensions must update Quarkus before implementing the solution.

Note:
Tthere already exist docker.io images without host (solution based on registry-mirror) as proposed in the original PM.
example for MySQL

@Sanne
Copy link
Member

Sanne commented Jun 10, 2024

I agree that not hardcoding the registry everywhere seems desirable

I'm actually not sure of this being a good idea. The registry identifies which image one intends to use, and leaving it undefined opens up to various forms of supply chain attacks.

Podman was insisting in doing the right thing by refusing images whose source is unspecified; this is clearly the better behaviour, unfortunately since it breaks stuff intended for Docker in which this level of precision isn't necessary this created friction in migration and the podman team ultimately gave up and changed the default behaviour on Fedora.

But the option for the more secure behaviour is available still and I'd recommend to use it, and so do security guides on production systems. We should still support it.

So I'd prefer it we could keep the prefixes. Allowing an override would be welcome of course.

Regarding rate control limits, shouldn't people use proxies like they are intended to? Proxys can be quite flexible AND secured.

@gastaldi gastaldi added the triage/needs-rebase This PR needs to be rebased first because it has merge conflicts label Dec 9, 2024
@gastaldi
Copy link
Contributor

gastaldi commented Dec 9, 2024

@Sanne has a point here. Let's keep the prefix and allow overrides in a separate PR. Thanks!

@gastaldi gastaldi closed this Dec 9, 2024
@quarkus-bot quarkus-bot bot added the triage/invalid This doesn't seem right label Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dependencies Pull requests that update a dependency file area/documentation area/hibernate-search Hibernate Search area/kafka area/redis triage/invalid This doesn't seem right triage/needs-rebase This PR needs to be rebased first because it has merge conflicts
Projects
Development

Successfully merging this pull request may close these issues.

allow usage of mirror for docker.io images
6 participants