Skip to content

Comments

Add custom DNS settings to service definition#1681

Merged
aaronlehmann merged 1 commit intomoby:masterfrom
yongtang:24391-dns-setting
Oct 26, 2016
Merged

Add custom DNS settings to service definition#1681
aaronlehmann merged 1 commit intomoby:masterfrom
yongtang:24391-dns-setting

Conversation

@yongtang
Copy link
Member

@yongtang yongtang commented Oct 22, 2016

This fix tries to fix the issue raised in docker:
moby/moby#24391
about allowing custom DNS settings to service definition.

This fix adds Dns, DnsOptions, DnsSearch to service definition.

This fix is related to the pull request:
moby/moby#27567

Signed-off-by: Yong Tang [email protected]

@yongtang yongtang mentioned this pull request Oct 22, 2016
Merged
@codecov-io
Copy link

codecov-io commented Oct 22, 2016 •
edited
Loading

Current coverage is 56.07% (diff: 100%)

Merging #1681 into master will increase coverage by 0.04%

@@             master      #1681   diff @@
==========================================
  Files            90         90          
  Lines         14521      14521          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           8137       8143     +6   
+ Misses         5297       5292     -5   
+ Partials       1087       1086     -1

Sunburst

Powered by Codecov. Last update 0628b65...b3be8a0

@aluzzardi
Copy link
Member

aluzzardi commented Oct 24, 2016

/cc @stevvooe

@stevvooe
Copy link
Contributor

stevvooe commented Oct 25, 2016

Network options are part of the network, so I am slightly reticent here. (cc @mrjana)

Right now, we consider hostname a property of the ContainerSpec because it changes the hostname as seen inside the container. If we could see that same argument made here, then I can see this as working.

At least, I'd prefer the DNS options to be grouped into a DNS message. I think if we say this is a resolv.conf equivalent, then the argument above would be quite clear.

@stevvooe
Copy link
Contributor

stevvooe commented Oct 25, 2016

After some though, I think ContainerSpec is the right spot for these fields. Let's group them in a message:

message DNSConfig {
  repeated string nameservers;
  repeated string search;
  repeated string options; // document as available in http://man7.org/linux/man-pages/man5/resolv.conf.5.html
}

Also, please create a TODO indicated that domain is not supported yet.

@aluzzardi
Copy link
Member

aluzzardi commented Oct 25, 2016

👍 @stevvooe

@yongtang Could you please re-organize the fields as suggested?

Thank you!

@aluzzardi
Copy link
Member

aluzzardi commented Oct 25, 2016

/cc @dnephin @aanand

@yongtang
Copy link
Member Author

yongtang commented Oct 26, 2016

Thanks @aluzzardi @stevvooe. The PR has been updated. Please let me know if there are any issues.

aaronlehmann
aaronlehmann reviewed Oct 26, 2016
View reviewed changes
api/specs.proto Outdated
}

// DNSConfig allows one to specify DNS related configuration in resolv.conf
DNSConfig dns_config = 15;
Copy link
Collaborator

@aaronlehmann aaronlehmann Oct 26, 2016

Choose a reason for hiding this comment

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

Can you please give this a customname so that the field name follows Go coding standards?

[(gogoproto.customname) = "DNSConfig"]

Copy link
Member Author

@yongtang yongtang Oct 26, 2016

Choose a reason for hiding this comment

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

Thanks @aaronlehmann. The PR has been updated.

@yongtang
This fix tries to fix the issue raised in docker:
b3be8a0 
moby/moby#24391
about allowing custom DNS settings to service definition.

This fix adds `DNSConfig`  to service definition.

This fix is related to the pull request:
moby/moby#27567

Signed-off-by: Yong Tang <[email protected]>
@stevvooe
Copy link
Contributor

stevvooe commented Oct 26, 2016

LGTM

It would be good to follow this up with a swarmkit implementation.

@aaronlehmann
Copy link
Collaborator

aaronlehmann commented Oct 26, 2016

LGTM

@aaronlehmann aaronlehmann merged commit ff68c46 into moby:master Oct 26, 2016
@yongtang yongtang deleted the 24391-dns-setting branch October 26, 2016 03:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aaronlehmann aaronlehmann aaronlehmann left review comments

Assignees

No one assigned

Labels

status/1-design-review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@yongtang @codecov-io @aluzzardi @stevvooe @aaronlehmann @GordonTheTurtle