Skip to content

Conversation

crazy-max
Copy link
Member

@crazy-max crazy-max commented Nov 25, 2022

fixes #44525

- What I did

If the IP Address is a string called host-gateway, replace this value with the IP address stored in the daemon level HostGatewayIP config variable.

This is the current behavior with the classic builder.

Keeping in draft for now as it's still subject to discussion.

Need changes in Buildx client: https://github.com/docker/buildx/blob/23cabd67fb1c5e7ba2e3765e91bd5cc0f0311337/build/utils.go#L41-L56 cc @tonistiigi @jedevc

- How I did it

- How to verify it

- Description for the changelog

handle host-gateway with extra hosts.

- A picture of a cute animal (not mandatory but encouraged)

Signed-off-by: CrazyMax crazy-max@users.noreply.github.com

@thaJeztah
Copy link
Member

Might also need changes in Buildx client: https://github.com/docker/buildx/blob/23cabd67fb1c5e7ba2e3765e91bd5cc0f0311337/build/utils.go#L41-L56

Ah, yes, see docker/cli#2337

Although, I'm wondering if we should perform validation at all on the client side (basically; perhaps it should always be delegated to the API server to consider if options are valid or not)

@crazy-max
Copy link
Member Author

Although, I'm wondering if we should perform validation at all on the client side (basically; perhaps it should always be delegated to the API server to consider if options are valid or not)

Yes I think it would be better, we already validate extra hosts on BuildKit: https://github.com/moby/buildkit/blob/7804e2c7fc09b4a536934f5607698bc3255cf0ed/frontend/dockerfile/builder/build.go#L751-L775

@thaJeztah thaJeztah added this to the v-next milestone Nov 30, 2022
@thaJeztah thaJeztah marked this pull request as ready for review December 1, 2022 20:43
@thaJeztah thaJeztah requested a review from tonistiigi as a code owner December 1, 2022 20:43
Copy link
Contributor

@tiborvass tiborvass left a comment

Choose a reason for hiding this comment

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

lgtm, but didn't test and kind of unfortunate to have two copies of toBuildkitExtraHosts (here and in buildx). Not sure if they can factored since there is currently no way for buildx to retrieve the host-gateway-ip setting from the daemon (not even exposed in /info).

@thaJeztah
Copy link
Member

Not sure if they can factored since there is currently no way for buildx to retrieve the host-gateway-ip setting from the daemon (not even exposed in /info).

I think this feature would currently only apply to the docker builder, unless BuildKit itself (running as container-builder) would implement a configuration for this, in which case it would (should?) also do the conversion on the daemon side.

As mentioned in an earlier comment, I think we should remove validation from the client side altogether (other than splitting key/value where needed), and leave it to the daemon to return an error if the value is valid or not.

@crazy-max
Copy link
Member Author

crazy-max commented Dec 2, 2022

As mentioned in an earlier comment, I think we should remove validation from the client side altogether (other than splitting key/value where needed), and leave it to the daemon to return an error if the value is valid or not.

I discussed about this with @tonistiigi yesterday and the validation client-side avoid sending extra build request.

Edit: opened follow-up on buildx repo: docker/buildx#1446

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
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.

docker buildx fails to validate "--add-host=host.docker.internal:host-gateway"
3 participants