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

Task DNS Options #7661

Merged
merged 18 commits into from
Apr 28, 2020
Merged

Task DNS Options #7661

merged 18 commits into from
Apr 28, 2020

Conversation

nickethier
Copy link
Member

@nickethier nickethier commented Apr 8, 2020

This PR adds a dns stanza to network {...} which allows setting the dns servers, searches and options. Example:

network {
  dns {
    servers = ["10.0.0.1", "10.0.0.1"]
    searches = ["internal.corp"]
    options = ["ndots:2"]
  }
}

It is implemented to pass on the configuration to task drivers. This means the task driver is responsible for configuring the DNS settings. For executor style drivers, a resolvconf utility package is included to build the resolv.conf file on Linux and generate the drivers.Mount struct to mount it in.

Caveats

  • raw_exec is not supported since it doesn't sandbox the filesystem
  • qemu only exposes options to set the DNS resolver, thus searches and options are currently ignored.

TODO

  • Support all built-in drivers
    • docker
    • exec
    • raw_exec Not supported
    • java
    • qemu
  • Update network stanza docs
  • Update API docs
  • Update example job

fixes #7283 #7393 #6727

@danlsgiga
Copy link
Contributor

Thank you for working on this... its a big blocker for us to use exec task isolation via bridged networks.

@nickethier nickethier marked this pull request as ready for review April 22, 2020 18:00
@nickethier nickethier requested review from tgross and shoenig April 22, 2020 18:03
Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

This looks great @nickethier. I left one question and some minor cleanup/docs items.

jobspec/test-fixtures/tg-network.hcl Outdated Show resolved Hide resolved
website/pages/docs/job-specification/network.mdx Outdated Show resolved Hide resolved
@@ -74,6 +74,10 @@ job "docs" {
- `host` - Each task will join the host network namespace and a shared network
namespace is not created. This matches the current behavior in Nomad 0.9.

- `dns` <code>([DNSConfig](#dns-parameters): nil)</code> - Sets the DNS configuration
Copy link
Member

Choose a reason for hiding this comment

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

I think it might be valuable to have some brief info here about how this interacts with network isolation. If you set particular IPs as the nameservers, the routing is going to be different if the task uses host networking vs other networking modes (particularly once CNI support is complete). So something like a reminder that this uses the task's view of the network address space.

"github.com/stretchr/testify/require"
)

// TestTaskDNSConfig asserts that a task is running with the given DNSConfig
Copy link
Member

Choose a reason for hiding this comment

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

I really like how clean this makes the task driver tests that need it. 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

I borrowed the idea from @notnoop!

path := filepath.Join(taskDir, "resolv.conf")
mount := &drivers.MountConfig{
TaskPath: "/etc/resolv.conf",
HostPath: path,
Copy link
Member

Choose a reason for hiding this comment

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

It looks like we're only using this function for exec and java task drivers. When we pass the TaskDir into this function to create the HostPath, is that path writable to the task? Mounting it from there read-only to /etc/resolv.conf makes it seem like it shouldn't be. It doesn't seem like a security issue because it's a copy of the host's /etc/resolv.conf and not a mount to it. Are we doing this bind-mount mostly for consistency with how we create mounts?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah the idea being that if Nomad is going to manage the dns configuration (resolv.conf) then the task shouldn't be able to write to it. I couldn't find a great place to write the resolv.conf as we don't really have a per alloc directory thats not actually accessible from the alloc. I figured mounting as ro would give the "Nomad manages this file" UX even if we change where its mounted from in the future.

I did try and keep the concept of just passing a bind mount consistent as that seems the most compatible way other drivers could consume this.

Copy link
Member

Choose a reason for hiding this comment

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

👍 That all sounds like solid reasoning.

nomad/structs/network.go Show resolved Hide resolved
@nickethier nickethier requested a review from shoenig April 27, 2020 19:54
@shoenig
Copy link
Member

shoenig commented Apr 27, 2020

@nickethier double checking - this is going into master rather than a 0.12 branch?

@nickethier
Copy link
Member Author

No I just haven’t made the 0.12 branch yet.

@nickethier nickethier changed the base branch from master to next April 28, 2020 02:07
@nickethier nickethier merged commit dae0027 into next Apr 28, 2020
@nickethier nickethier deleted the f-dns-config branch April 28, 2020 03:11
drewbailey pushed a commit that referenced this pull request May 8, 2020
Co-Authored-By: Tim Gross <[email protected]>
Co-Authored-By: Seth Hoenig <[email protected]>
drewbailey pushed a commit that referenced this pull request May 8, 2020
Co-Authored-By: Tim Gross <[email protected]>
Co-Authored-By: Seth Hoenig <[email protected]>
nickethier added a commit that referenced this pull request May 15, 2020
Co-Authored-By: Tim Gross <[email protected]>
Co-Authored-By: Seth Hoenig <[email protected]>
nickethier added a commit that referenced this pull request Jun 16, 2020
Co-Authored-By: Tim Gross <[email protected]>
Co-Authored-By: Seth Hoenig <[email protected]>
schmichael pushed a commit that referenced this pull request Jun 18, 2020
Co-Authored-By: Tim Gross <[email protected]>
Co-Authored-By: Seth Hoenig <[email protected]>
@github-actions
Copy link

github-actions bot commented Jan 8, 2023

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE]: Add custom DNS options for the exec driver
4 participants