Skip to content

support per-Run DNS options#3062

Closed
vito wants to merge 6 commits intomoby:masterfrom
vito:run-dns
Closed

support per-Run DNS options#3062
vito wants to merge 6 commits intomoby:masterfrom
vito:run-dns

Conversation

@vito
Copy link
Copy Markdown
Contributor

@vito vito commented Aug 24, 2022

Adds support for configuring DNS on a per-Run basis rather than relying on a single global DNS config.

The existing global DNS config is still supported. When both are present the per-Run DNS config takes precedence.

My use case is related to #3044. I want to support running things either in the bridge network (+ custom DNS) or in the host network when needed.1 For this to work I need to be able to set the bridge DNS server only for things that use the bridge network and let things that run in the host network inherit the host-level DNS config.

I considered a few other options before landing on this. Writing this partly for my own memory:

  • If I just set the bridge DNS server statically regardless it will only work as long as at least one container is running in the bridge network, since dnsname only runs dnsmasq when needed.
  • If I set both the bridge network and a "real" DNS server (i.e. 1.1.1.1) everything works fine, but I would rather not require the user to configure DNS; I would prefer to just have Buildkit inherit the host's /etc/resolv.conf. Relying on DNS failover also feels spooky.
  • If I avoid supporting host networking at all I'll probably need to support port forwarding in the future, which I'm OK with but I'm not sure if y'all are. Also, an especially tricky thing I'm trying to support is running a registry in Buildkit and using it for a later llb.Image, and this is much easier to support by running the registry in the host network since that's where the image puller runs. (Also using an address other than localhost/127.0.0.1 will require TLS.)

Footnotes

  1. Example: running a registry in Buildkit on 127.0.0.1:1234 and using it for other Buildkit runs. I can't use a bridge network for this because Buildkit's image fetching runs in the host network and doesn't respect a custom DNS config, plus having a non-localhsot/127.0.0.1 address requires TLS.

Comment thread solver/pb/ops.proto Outdated
Copy link
Copy Markdown
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

Add a capability for this and make sure it is set in client side https://github.com/moby/buildkit/blob/master/solver/pb/caps.go

@vito vito force-pushed the run-dns branch 2 times, most recently from ffecd71 to 2e3ed0c Compare August 24, 2022 23:48
vito added 2 commits August 24, 2022 20:10
Signed-off-by: Alex Suraci <suraci.alex@gmail.com>
Signed-off-by: Alex Suraci <suraci.alex@gmail.com>
@vito
Copy link
Copy Markdown
Contributor Author

vito commented Aug 25, 2022

Capability added.

}
}

resolvConf, err := oci.GetResolvConf(ctx, w.root, w.idmap, dns)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It doesn't look like GetResolvConf is safe to be called like this. Looks like it does bunch of internal caching atm and can return a cached result for incorrect dns value.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh, good catch, that makes things a bit trickier but I took a swing at it. Now it'll use a hashed filename derived from the DNS config.

Signed-off-by: Alex Suraci <suraci.alex@gmail.com>
@vito vito force-pushed the run-dns branch 2 times, most recently from 086fceb to 968affe Compare August 25, 2022 04:03
vito added 2 commits August 25, 2022 00:13
the cleanup is broken here since it'll be removed for any concurrent
dynamic config. hmm.

Signed-off-by: Alex Suraci <suraci.alex@gmail.com>
cleanup isn't safe because many containers may require the same config,
and we don't want it to be removed until they're _all_ done with it.

so just let them stick around, since there probably won't be that many
over time anyway (...right?)

alternatively we could do what we do for /etc/hosts: make a new distinct
file if alternative config is specified and clean it up after. however
the range of resolv.conf values should be much more limited so I would
guess there's more value in sharing a single file across all of them
rather than creating and deleting them all the time.

keeping this a separate commit so it's easy to revert if we want to do
that anyway.

Signed-off-by: Alex Suraci <suraci.alex@gmail.com>
@vito vito force-pushed the run-dns branch 2 times, most recently from 6a6b096 to 81a1d8e Compare August 25, 2022 04:14
Signed-off-by: Alex Suraci <suraci.alex@gmail.com>
@vito
Copy link
Copy Markdown
Contributor Author

vito commented Aug 26, 2022

I don't need this anymore, but I'm happy to keep going if it seems worthwhile. Otherwise feel free to close. 🙂

I didn't really like the tight coupling this introduced between the client and the server, and I ended up finding a way to avoid host networking altogether. Now I just add the CNI dnsmasq server to the Buildkit host's /etc/resolv.conf which also allows Buildkit to resolve container addresses. I had to implement support for generating TLS certs since Buildkit's image fetching requires it for any non-localhost/127.0.0.1 registry, but that's a pretty handy feature anyway. Much better than the reduced security and increased complexity of supporting host networks.

@tonistiigi
Copy link
Copy Markdown
Member

Let's close this then until there is a case that actually needs it.

Compared to the daemon config this also has different behavior concerning the build cache.

@tonistiigi tonistiigi closed this Aug 26, 2022
emmanuelguerin pushed a commit to emmanuelguerin/buildkit that referenced this pull request Mar 31, 2025
builder: return error if a node fails to boot
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants