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

aardvark-dns: add support for container's custom dns_servers #240

Merged

Conversation

flouthoc
Copy link
Collaborator

Aardvark-dns's config now allows containers to define a list of one of more custom DNS servers. If custom DNS servers are specified for a container aardvark-dns will use these custom DNS servers are resolver instead of host's default revolvers.

  • Newly added field to aardvark-dns's config is optional in nature.

How to verify

  • See newly added integration and unit tests.
  • Makes podman run -it --rm --network test --dns 8.8.8.8 ctr dig google.com to use 8.8.8.8 as a resolver instead of host's configured resolver.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 17, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@flouthoc flouthoc marked this pull request as draft October 17, 2022 08:38
flouthoc added a commit to flouthoc/netavark that referenced this pull request Oct 17, 2022
…ners

Netavark now accepts `dns_servers` as part of `NetworkOptions` which
contains list of comma seperated custom DNS servers for containers.
Aardvark-dns will use these as resolver for a specific container instead
of host's default resolver.

Implements feature here: containers/aardvark-dns#240

Signed-off-by: Aditya R <[email protected]>
flouthoc added a commit to flouthoc/common that referenced this pull request Oct 17, 2022
Netavark now accets `dns_servers` for each container which allows
containers to use custom DNS servers as resolvers instead of falling
back to host's resolver.

Following field allows callers to libnetwork to pass newly added field
to `netavark` and `aarvark-dns`

Actual feature implemented
* containers/aardvark-dns#240
* containers/netavark#452

[NO NEW TESTS NEEDED]
[NO TESTS NEEDED]

Signed-off-by: Aditya R <[email protected]>
@flouthoc flouthoc force-pushed the container_custom_dns_server branch from 2cfd736 to 89571c7 Compare October 17, 2022 09:35
flouthoc added a commit to flouthoc/common that referenced this pull request Oct 17, 2022
Netavark now accets `dns_servers` for each container which allows
containers to use custom DNS servers as resolvers instead of falling
back to host's resolver.

Following field allows callers to libnetwork to pass newly added field
to `netavark` and `aarvark-dns`

Actual feature implemented
* containers/aardvark-dns#240
* containers/netavark#452

[NO NEW TESTS NEEDED]
[NO TESTS NEEDED]

Signed-off-by: Aditya R <[email protected]>
flouthoc added a commit to flouthoc/podman that referenced this pull request Oct 17, 2022
Aardvark-dns and netavark now accepts custom DNS servers for containers
via new config field `dns_servers`. New field allows containers to use
custom resolvers instead of host's default resolvers.

Following commit instruments libpod to pass these custom DNS servers set
via `--dns` or central config to the network stack.

Depends-on:
* Common: containers/common#1189
* Netavark: containers/netavark#452
* Aardvark-dns: containers/aardvark-dns#240

[NO NEW TESTS ADDED]
[NO TESTS ADDED]

Signed-off-by: Aditya R <[email protected]>
@flouthoc
Copy link
Collaborator Author

Looks like I'm facing chicken-egg problem with integration tests, netavark needs new aardvark and aardvark needs new netavark.

flouthoc added a commit to flouthoc/common that referenced this pull request Oct 17, 2022
Netavark now accets `dns_servers` for each container which allows
containers to use custom DNS servers as resolvers instead of falling
back to host's resolver.

Following field allows callers to libnetwork to pass newly added field
to `netavark` and `aarvark-dns`

Actual feature implemented
* containers/aardvark-dns#240
* containers/netavark#452

[NO NEW TESTS NEEDED]
[NO TESTS NEEDED]

Signed-off-by: Aditya R <[email protected]>
flouthoc added a commit to flouthoc/netavark that referenced this pull request Oct 17, 2022
…ners

Netavark now accepts `dns_servers` as part of `NetworkOptions` which
contains list of comma seperated custom DNS servers for containers.
Aardvark-dns will use these as resolver for a specific container instead
of host's default resolver.

Implements feature here: containers/aardvark-dns#240

Signed-off-by: Aditya R <[email protected]>
flouthoc added a commit to flouthoc/netavark that referenced this pull request Oct 17, 2022
…ners

Netavark now accepts `dns_servers` as part of `NetworkOptions` which
contains list of comma seperated custom DNS servers for containers.
Aardvark-dns will use these as resolver for a specific container instead
of host's default resolver.

Implements feature here: containers/aardvark-dns#240

Signed-off-by: Aditya R <[email protected]>
@mheon
Copy link
Member

mheon commented Oct 17, 2022

We've done this at least once before. How did we handle it then?

@flouthoc
Copy link
Collaborator Author

We've done this at least once before. How did we handle it then?

@mheon That was only aardvark-change and netavark was blocked on it but no issue at all here as well, this is sorted here containers/netavark#452 (comment) , we will get netavark PR in first then we can merge this and after that i'll unskip the netavark test in a followup PR.

flouthoc added a commit to flouthoc/netavark that referenced this pull request Oct 17, 2022
…ners

Netavark now accepts `dns_servers` as part of `NetworkOptions` which
contains list of comma seperated custom DNS servers for containers.
Aardvark-dns will use these as resolver for a specific container instead
of host's default resolver.

Implements feature here: containers/aardvark-dns#240

Signed-off-by: Aditya R <[email protected]>
flouthoc added a commit to flouthoc/netavark that referenced this pull request Oct 17, 2022
…ners

Netavark now accepts `dns_servers` as part of `NetworkOptions` which
contains list of comma seperated custom DNS servers for containers.
Aardvark-dns will use these as resolver for a specific container instead
of host's default resolver.

Implements feature here: containers/aardvark-dns#240

Signed-off-by: Aditya R <[email protected]>
flouthoc added a commit to flouthoc/netavark that referenced this pull request Oct 17, 2022
…ners

Netavark now accepts `dns_servers` as part of `NetworkOptions` which
contains list of comma seperated custom DNS servers for containers.
Aardvark-dns will use these as resolver for a specific container instead
of host's default resolver.

Implements feature here: containers/aardvark-dns#240

Signed-off-by: Aditya R <[email protected]>
@flouthoc flouthoc force-pushed the container_custom_dns_server branch from 89571c7 to c35cffb Compare October 18, 2022 15:33
@flouthoc flouthoc marked this pull request as ready for review October 19, 2022 05:49
@flouthoc
Copy link
Collaborator Author

@Luap99 @mheon @baude PTAL this is ready for review and newly added integration tests pass with netavark from main.

@flouthoc flouthoc requested review from baude, Luap99 and mheon October 19, 2022 05:51
config.md Outdated Show resolved Hide resolved
test/helpers.bash Show resolved Hide resolved
@flouthoc flouthoc force-pushed the container_custom_dns_server branch from 641a5b1 to 1c6d50a Compare October 19, 2022 10:20
@flouthoc flouthoc force-pushed the container_custom_dns_server branch from 1c6d50a to f6df229 Compare October 20, 2022 11:42
@flouthoc flouthoc requested a review from Luap99 October 20, 2022 12:07
@flouthoc
Copy link
Collaborator Author

This is ready for review @Luap99 @baude @mheon PTAL

Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

code LGTM, just some comments for the tests

test/100-basic-name-resolution.bats Outdated Show resolved Hide resolved
test/100-basic-name-resolution.bats Outdated Show resolved Hide resolved
test/100-basic-name-resolution.bats Outdated Show resolved Hide resolved
src/test/test.rs Outdated Show resolved Hide resolved
@mheon
Copy link
Member

mheon commented Oct 20, 2022

Code LGTM minus comments from @Luap99

@flouthoc flouthoc force-pushed the container_custom_dns_server branch from f6df229 to 355293d Compare October 20, 2022 17:11
Aardvark-dns's config now allows containers to define a list of one of
more custom DNS servers. If custom DNS servers are specified for a
container aardvark-dns will use these custom DNS servers are resolvers
instead of host's default resolvers.

Signed-off-by: Aditya R <[email protected]>
@flouthoc flouthoc force-pushed the container_custom_dns_server branch from 355293d to d3cc7e0 Compare October 20, 2022 17:14
@flouthoc flouthoc requested a review from Luap99 October 20, 2022 17:15
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

LGTM

@Luap99
Copy link
Member

Luap99 commented Oct 20, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Oct 20, 2022
@openshift-merge-robot openshift-merge-robot merged commit 9e2aea5 into containers:main Oct 20, 2022
flouthoc added a commit to flouthoc/podman that referenced this pull request Jan 22, 2023
Aardvark-dns and netavark now accepts custom DNS servers for containers
via new config field `dns_servers`. New field allows containers to use
custom resolvers instead of host's default resolvers.

Following commit instruments libpod to pass these custom DNS servers set
via `--dns` or central config to the network stack.

Depends-on:
* Common: containers/common#1189
* Netavark: containers/netavark#452
* Aardvark-dns: containers/aardvark-dns#240

Signed-off-by: Aditya R <[email protected]>
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.

5 participants