Skip to content
This repository has been archived by the owner on Dec 18, 2020. It is now read-only.

Enable CoreDNS in nodeup/protokube #6

Merged
merged 2 commits into from
Mar 23, 2017

Conversation

luomiao
Copy link

@luomiao luomiao commented Mar 22, 2017

Added a few branches since ResourceRecordSets.List() of CoreDNS is not available.

Copy link

@prashima prashima left a comment

Choose a reason for hiding this comment

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

If you know, can you please point out the code where the master's dns configuration is being updated in nodeup or protokube code?

@@ -95,7 +96,7 @@ func main() {
os.Exit(1)
}

dnsController, err := dns.NewDNSController(dnsProvider, zoneRules)
dnsController, err := dns.NewDNSController(dnsProvider, zoneRules, dnsProviderId)

Choose a reason for hiding this comment

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

I am not able to put a comment on line#49, can you please add coredns in the string message.

Also, can you please share what value will this dnsProviderId will carry?

Copy link
Author

Choose a reason for hiding this comment

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

This is decided by the --dns flags inside DAEMON_ARGS. So it's setting to coredns at here: https://github.com/vmware/kops/pull/6/files/6326f161619435794c37157b7ef7c79db636ea1b#diff-b25ed23c3990586a8ed6e64c73bb178cR276

}
} else {

Choose a reason for hiding this comment

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

Nit: Adding a comment that aws route53 and gce's clouddns will get executed by else-block would be helpful here and at other similar places in code.

@@ -423,6 +428,30 @@ func (o *dnsOp) deleteRecords(k recordKey) error {
return fmt.Errorf("no suitable zone found for %q", fqdn)
}

if dnsProviderId == k8scoredns.ProviderName {
// TODO: work-around before ResourceRecordSets.List() is implemented for CoreDNS

Choose a reason for hiding this comment

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

Nit: moving this comment on top of if-block will make it clearer that the whole if-block is part of the work-around.

lines = append(lines, "etcd-endpoints = "+dnsServer)
lines = append(lines, "zones = "+zones[0])
config := "[global]\n" + strings.Join(lines, "\n") + "\n"
file = bytes.NewReader([]byte(config))

Choose a reason for hiding this comment

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

Can you please explain the role of this config file?

Copy link
Author

Choose a reason for hiding this comment

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

AWS or GCE's DNS server doesn't need any specific information because they have public server address to access. But to use CoreDNS, we need to pass information about the IP address of the CoreDNS's etcd and zones to query. These are required to start a CoreDNS client. And this config file (it's not a real file though) is used to pass in these information. Without this config file, the DNS provider won't know which CoreDNS server to talk to.

@luomiao
Copy link
Author

luomiao commented Mar 23, 2017

@prashima I think it's done inside this dnscontroller.go too. Need to verify it.

Copy link

@prashima prashima left a comment

Choose a reason for hiding this comment

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

LGTM.

Please run make push-sphere to make sure protokube and nodeup are getting built successfully.

@luomiao luomiao merged commit 93c4c71 into vmware-archive:vsphere-develop Mar 23, 2017
@luomiao luomiao deleted the vsphere-develop branch March 24, 2017 00:19
luomiao pushed a commit that referenced this pull request Mar 30, 2017
* Enable CoreDNS in nodeup/protokube.

* Address comments.
luomiao pushed a commit that referenced this pull request Apr 7, 2017
* Enable CoreDNS in nodeup/protokube.

* Address comments.
luomiao pushed a commit that referenced this pull request Apr 10, 2017
* Enable CoreDNS in nodeup/protokube.

* Address comments.
msterin pushed a commit that referenced this pull request Apr 11, 2017
* Enable CoreDNS in nodeup/protokube.

* Address comments.
luomiao pushed a commit that referenced this pull request Apr 21, 2017
* Enable CoreDNS in nodeup/protokube.

* Address comments.
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.

2 participants