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

feat: allow set global dns servers from client plugin config for contains #304

Merged
merged 2 commits into from
Dec 14, 2023

Conversation

ttys3
Copy link
Contributor

@ttys3 ttys3 commented Dec 6, 2023

so we can config dns for containers like:

plugin "nomad-driver-podman" {
  config {
     dns_servers = ["192.168.8.100"]
  }
}

@jdoss
Copy link
Contributor

jdoss commented Dec 6, 2023

Why would you want to set this in the plugin section of the nomad config when you can just set DNS via https://developer.hashicorp.com/nomad/docs/job-specification/network#dns-1 in the networking block of the job?

@ttys3
Copy link
Contributor Author

ttys3 commented Dec 8, 2023

Why would you want to set this in the plugin section of the nomad config when you can just set DNS via https://developer.hashicorp.com/nomad/docs/job-specification/network#dns-1 in the networking block of the job?

Do you think I am unaware of this configuration option? No, I am not.
When I submitted this PR, I had already been using this solution for almost two years.
The issue is that this DNS configuration must be global; I do not need to set different DNS for different services. Additionally, I do not want containers to directly use the upstream gateway's DNS because I want to use Consul DNS for service resolution. It is clearly not practical to configure DNS for each task within each job.

the arch is:

                                 +-----------------+
                                 |  Nomad Cluster  |
                                 +-----------------+
                                        |
                                        |
                                        |
                                        v
                           +------------------------+
                           |                        |
                           |  app deployed via      |
                           |  nomad cluster         |
                           |                        |
                           +------------------------+
                                        |
                                        |
                                        |
   +------------------------+           v                  
   |                        |   +-----------------+        
   |   Container App        |<--+ Consul Catalog  |        
   |                        |   +-----------------+        
   | Query DNS via Consul   |                              
   +------------------------+                              
                                        |                  
                                        |                  
                                        |                  
                                        v                  
                                +---------------+          
                                |               |          
                                |   Consul DNS  |          
                                |               |          
                                +---------------+          
                                        |                  
                                        |                  
                                        |                  
                                        v                  
                                +---------------+          
                                |               |          
                                |  DNS Response |          
                                |               |          
                                +---------------+          

yes, I also have CNI network and uses flannel network fabric, so different nomad node machine can commuicate with each other via the cluster network.

config.go Outdated
Comment on lines 151 to 152
ExtraLabels []string `codec:"extra_labels"`
DNSServers []string `codec:"dns_servers"`
Copy link
Contributor

Choose a reason for hiding this comment

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

needs to be go-fmt'd

@shoenig
Copy link
Contributor

shoenig commented Dec 12, 2023

The docker task driver also already has a dns_servers config option; it would make sense to make it available for podman too.

@shoenig
Copy link
Contributor

shoenig commented Dec 12, 2023

Also @ttys3 if you don't mind rebasing on main; there's some CI fixes in there to help getting tests working again.

@jdoss
Copy link
Contributor

jdoss commented Dec 12, 2023

Do you think I am unaware of this configuration option? No, I am not. When I submitted this PR, I had already been using this solution for almost two years.

No need to get defensive. Your original PR had zero details on why this was needed and I was simply asking to better understand your use-case and the reason for the PR. Thank you for taking the time to explain why. I appreciate it.

@ttys3
Copy link
Contributor Author

ttys3 commented Dec 13, 2023

Do you think I am unaware of this configuration option? No, I am not. When I submitted this PR, I had already been using this solution for almost two years.

No need to get defensive. Your original PR had zero details on why this was needed and I was simply asking to better understand your use-case and the reason for the PR. Thank you for taking the time to explain why. I appreciate it.

Sorry my previous reply was a bit unkind. I take back what I said before.

```hcl
plugin "nomad-driver-podman" {
  config {
     dns_servers = ["192.168.8.100"]
  }
}
```
@ttys3 ttys3 force-pushed the feature/allow-set-global-dns-servers branch from e9d37f1 to ec1dcdd Compare December 13, 2023 13:05
@ttys3
Copy link
Contributor Author

ttys3 commented Dec 13, 2023

Also @ttys3 if you don't mind rebasing on main; there's some CI fixes in there to help getting tests working again.

@shoenig rebased and formated, PTAL

@ttys3 ttys3 requested a review from shoenig December 13, 2023 13:08
Copy link
Contributor

@shoenig shoenig left a comment

Choose a reason for hiding this comment

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

LGTM; thanks @ttys3!

@shoenig shoenig merged commit 89d6a0b into hashicorp:main Dec 14, 2023
2 checks passed
@ttys3 ttys3 deleted the feature/allow-set-global-dns-servers branch March 5, 2024 15:45
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.

3 participants