-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
[Federation] Setting up CoreDNS as DNS provider for Cluster Federation #2810
Conversation
Reviewed 2 of 2 files at r1. docs/tutorials/federation/set-up-coredns-provider-federation.md, line 28 at r1 (raw file):
Same comments for all other occurrences. docs/tutorials/federation/set-up-coredns-provider-federation.md, line 33 at r1 (raw file):
Can we use a better, more descriptive namespace? docs/tutorials/federation/set-up-coredns-provider-federation.md, line 41 at r1 (raw file):
Remove the quotes and just linkify the URL. docs/tutorials/federation/set-up-coredns-provider-federation.md, line 44 at r1 (raw file):
Link to the default docs/tutorials/federation/set-up-coredns-provider-federation.md, line 64 at r1 (raw file):
What's the difference between the two types? I am not sure I know the distinction. docs/tutorials/federation/set-up-coredns-provider-federation.md, line 70 at r1 (raw file):
Do docs/tutorials/federation/set-up-coredns-provider-federation.md, line 72 at r1 (raw file):
Same as above. docs/tutorials/federation/set-up-coredns-provider-federation.md, line 89 at r1 (raw file):
It is probably worth mentioning that docs/tutorials/federation/set-up-coredns-provider-federation.md, line 98 at r1 (raw file):
I would make the host name part consistent. I understand why you are doing this. But I think it is worth making it consistent everywhere. Using docs/tutorials/federation/set-up-coredns-provider-federation.md, line 99 at r1 (raw file):
Trailing dot for consistency as we did here - #2899 (review) docs/tutorials/federation/set-up-coredns-provider-federation.md, line 103 at r1 (raw file):
and say "which is same as docs/tutorials/federation/set-up-coredns-provider-federation.md, line 105 at r1 (raw file):
resolv.conf chain? docs/tutorials/federation/set-up-coredns-provider-federation.md, line 109 at r1 (raw file):
resolv.conf chain? docs/tutorials/federation/set-up-coredns-provider-federation.md, line 115 at r1 (raw file):
Is it not possible to use the Comments from Reviewable |
@madhusudancs, Handled review comments. PTAL Review status: 1 of 2 files reviewed at latest revision, 14 unresolved discussions. docs/tutorials/federation/set-up-coredns-provider-federation.md, line 28 at r1 (raw file): Previously, madhusudancs (Madhusudan.C.S) wrote…
ok. Done docs/tutorials/federation/set-up-coredns-provider-federation.md, line 33 at r1 (raw file): Previously, madhusudancs (Madhusudan.C.S) wrote…
used my-namespace instead of ns docs/tutorials/federation/set-up-coredns-provider-federation.md, line 41 at r1 (raw file): Previously, madhusudancs (Madhusudan.C.S) wrote…
removed the quotes docs/tutorials/federation/set-up-coredns-provider-federation.md, line 44 at r1 (raw file): Previously, madhusudancs (Madhusudan.C.S) wrote…
CoreDNS chart is not merged yet, so the link would not be stable if i link it to PR. On the other hand Values.yaml in helm charts is well known, so i think user should not have any difficulty finding out the default configuration given the chart. docs/tutorials/federation/set-up-coredns-provider-federation.md, line 64 at r1 (raw file): Previously, madhusudancs (Madhusudan.C.S) wrote…
CoreDNS chart can be deployed basically as 2 variants.
they basically differ in labels and annotations. docs/tutorials/federation/set-up-coredns-provider-federation.md, line 70 at r1 (raw file): Previously, madhusudancs (Madhusudan.C.S) wrote…
CoreDNS users do understand about the middleware. I had given the reference Values.yaml above which can be used to deploy CoreDNS for federation dns server. This explanation is just to let know the user of the configuration details and i believe whoever wants to tweak the configuration would know how to fit it to their needs. docs/tutorials/federation/set-up-coredns-provider-federation.md, line 72 at r1 (raw file): Previously, madhusudancs (Madhusudan.C.S) wrote…
Same as previous reply docs/tutorials/federation/set-up-coredns-provider-federation.md, line 89 at r1 (raw file): Previously, madhusudancs (Madhusudan.C.S) wrote…
Added a Note docs/tutorials/federation/set-up-coredns-provider-federation.md, line 98 at r1 (raw file): Previously, madhusudancs (Madhusudan.C.S) wrote…
Agree, used etcd-cluster.my-namespace everywhere docs/tutorials/federation/set-up-coredns-provider-federation.md, line 99 at r1 (raw file): Previously, madhusudancs (Madhusudan.C.S) wrote…
Done docs/tutorials/federation/set-up-coredns-provider-federation.md, line 103 at r1 (raw file): Previously, madhusudancs (Madhusudan.C.S) wrote…
Added. docs/tutorials/federation/set-up-coredns-provider-federation.md, line 105 at r1 (raw file): Previously, madhusudancs (Madhusudan.C.S) wrote…
Changed. Done docs/tutorials/federation/set-up-coredns-provider-federation.md, line 109 at r1 (raw file): Previously, madhusudancs (Madhusudan.C.S) wrote…
Changed. Done docs/tutorials/federation/set-up-coredns-provider-federation.md, line 115 at r1 (raw file): Previously, madhusudancs (Madhusudan.C.S) wrote…
kube-dns ConfigMap in the current release cannot handle nameserver listening on any port other than 53. I did a PR to specify port for nameserver, but that would be included in subsequent releases. Comments from Reviewable |
Madhu -- can you lgtm the changes if they look good to you? |
@madhusudancs, the CoreDNS helm chart PR is merged now. request a LGTM from you. is the fixes ok? |
docs/tutorials/federation/set-up-coredns-provider-federation.md, line 99 at r2 (raw file):
Does the user need to own this domain name? (In the light of kubernetes/kubernetes#43599) Comments from Reviewable |
@shashidharatd a few minor comments. Reviewed 1 of 1 files at r2. docs/tutorials/federation/set-up-coredns-provider-federation.md, line 33 at r1 (raw file): Previously, shashidharatd (Shashidhara T D) wrote…
I would have preferred a descriptive one. Something like docs/tutorials/federation/set-up-coredns-provider-federation.md, line 41 at r1 (raw file): Previously, shashidharatd (Shashidhara T D) wrote…
Can you please linkify this so that people following the doc can just click the link? docs/tutorials/federation/set-up-coredns-provider-federation.md, line 44 at r1 (raw file): Previously, shashidharatd (Shashidhara T D) wrote…
Ok, fair enough. docs/tutorials/federation/set-up-coredns-provider-federation.md, line 64 at r1 (raw file): Previously, shashidharatd (Shashidhara T D) wrote…
Oh I see what you mean. This defines whether the deployment is a cluster component. Is that right? Could you please clarify this in the doc. It is not clear right now. If that's the case, should users care about it? What are the pros and cons? docs/tutorials/federation/set-up-coredns-provider-federation.md, line 70 at r1 (raw file): Previously, shashidharatd (Shashidhara T D) wrote…
Ok, cool. docs/tutorials/federation/set-up-coredns-provider-federation.md, line 72 at r1 (raw file): Previously, shashidharatd (Shashidhara T D) wrote…
Ack. docs/tutorials/federation/set-up-coredns-provider-federation.md, line 115 at r1 (raw file): Previously, shashidharatd (Shashidhara T D) wrote…
I see. Do you think it is worth documenting both these techniques along with your explanation above? docs/tutorials/federation/set-up-coredns-provider-federation.md, line 99 at r2 (raw file): Previously, clenimar (Clenimar Filemon) wrote…
Not necessary if you are using CoreDNS as the DNS provider. However, it is the user's (read: cluster operator's) responsibility to
Comments from Reviewable |
@madhusudancs, have tried adressing your comments. PTAL Review status: 0 of 2 files reviewed at latest revision, 6 unresolved discussions. docs/tutorials/federation/set-up-coredns-provider-federation.md, line 33 at r1 (raw file): Previously, madhusudancs (Madhusudan.C.S) wrote…
Using something like docs/tutorials/federation/set-up-coredns-provider-federation.md, line 41 at r1 (raw file): Previously, madhusudancs (Madhusudan.C.S) wrote…
oh, i misunderstood the comment. Fixed Now. docs/tutorials/federation/set-up-coredns-provider-federation.md, line 64 at r1 (raw file): Previously, madhusudancs (Madhusudan.C.S) wrote…
For federation use case. CoreDNS has to be deployed as normal k8s app and this is not default while installing helm chart. So i have given the overriding configuration to deploy CoreDNS as a normal k8s app with etcd as middleware. docs/tutorials/federation/set-up-coredns-provider-federation.md, line 115 at r1 (raw file): Previously, madhusudancs (Madhusudan.C.S) wrote…
Once the kube-dns configuration related PR is merged and available to users. users need not do this manual configuration at all. we should remove this section in future. So i feel we should leave out those details which we do take care of. Comments from Reviewable |
Reviewed 2 of 2 files at r3. docs/tutorials/federation/set-up-coredns-provider-federation.md, line 33 at r1 (raw file): Previously, shashidharatd (Shashidhara T D) wrote…
Fair enough. docs/tutorials/federation/set-up-coredns-provider-federation.md, line 64 at r1 (raw file): Previously, shashidharatd (Shashidhara T D) wrote…
Ok makes sense. docs/tutorials/federation/set-up-coredns-provider-federation.md, line 115 at r1 (raw file): Previously, shashidharatd (Shashidhara T D) wrote…
Ok. Comments from Reviewable |
@devin-donnelly @chenopis @jaredbhatti could you please take a look? |
@shashidharatd this is also needs a rebase. |
@madhusudancs, sorry for the delay. rebased now |
Hello @shashidharatd, I tried
|
Hello @arthur0, |
Another possibility: federation doesn't create a default namespace. The failing "gets" suggest that might be the case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor grammar nits.
{:toc} | ||
|
||
This guide explains how to configure and deploy CoreDNS to be used as | ||
DNS provider for Cluster Federation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add "the" before "DNS provider..."
- shashidharatd | ||
title: Setting up CoreDNS as DNS provider for Cluster Federation | ||
--- | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you could please use the Tutorial Template, that would really help us out. Thanks!
This guide explains how to configure and deploy CoreDNS to be used as | ||
DNS provider for Cluster Federation. | ||
|
||
CoreDNS can be deployed in various configuration suiting the cluster |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"configuration" -> "configurations"
DNS provider for Cluster Federation. | ||
|
||
CoreDNS can be deployed in various configuration suiting the cluster | ||
federation deployment. In this guide we are explaining CoreDNS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sentence "In this guide..." is redundant and can be removed because the points it makes are already mentioned above.
|
||
## Deploying CoreDNS and etcd charts | ||
|
||
To deploy CoreDNS we shall make use of helm charts. CoreDNS will be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add comma: "To deploy CoreDNS,
we shall..."
|
||
## Deploying Federation with CoreDNS as DNS provider | ||
|
||
Federation control plane can be deployed using `kubefed init`. CoreDNS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add "the": "The
Federation control plane..."
## Deploying Federation with CoreDNS as DNS provider | ||
|
||
Federation control plane can be deployed using `kubefed init`. CoreDNS | ||
can be chosen as DNS provider by specifying two additional parameters. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add "the": "...can be chosen as the
DNS provider..."
|
||
Once the federation control plane is deployed and federated clusters | ||
are joined to the federation, you need to add the CoreDNS server to | ||
pod's nameserver resolv.conf chain in all the federated clusters as this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add "the": "...the
pod's nameserver..."
|
||
Replace `example.com` above with federation domain. | ||
|
||
> Note: Adding CoreDNS server to pod's nameserver resolv.conf chain will be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add "the": "...server to the
pod's nameserver..."
automated in subsequent releases. | ||
|
||
|
||
Now the federated cluster is ready for cross-cluster service dicovery ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo and remove extra space: "...cross-cluster service discovery!
"
@chenopis, i have addressed your comments. PTAL Review status: 1 of 3 files reviewed at latest revision, 21 unresolved discussions. docs/tutorials/federation/set-up-coredns-provider-federation.md, line 6 at r3 (raw file): Previously, chenopis (Andrew Chen) wrote…
I have used the template now. PTAL docs/tutorials/federation/set-up-coredns-provider-federation.md, line 11 at r3 (raw file): Previously, chenopis (Andrew Chen) wrote…
Done. docs/tutorials/federation/set-up-coredns-provider-federation.md, line 13 at r3 (raw file): Previously, chenopis (Andrew Chen) wrote…
Done. docs/tutorials/federation/set-up-coredns-provider-federation.md, line 14 at r3 (raw file): Previously, chenopis (Andrew Chen) wrote…
Done. docs/tutorials/federation/set-up-coredns-provider-federation.md, line 27 at r3 (raw file): Previously, chenopis (Andrew Chen) wrote…
Done. docs/tutorials/federation/set-up-coredns-provider-federation.md, line 29 at r3 (raw file): Previously, chenopis (Andrew Chen) wrote…
Done. docs/tutorials/federation/set-up-coredns-provider-federation.md, line 40 at r3 (raw file): Previously, chenopis (Andrew Chen) wrote…
Done. docs/tutorials/federation/set-up-coredns-provider-federation.md, line 43 at r3 (raw file): Previously, chenopis (Andrew Chen) wrote…
Done. docs/tutorials/federation/set-up-coredns-provider-federation.md, line 44 at r3 (raw file): Previously, chenopis (Andrew Chen) wrote…
Done. docs/tutorials/federation/set-up-coredns-provider-federation.md, line 45 at r3 (raw file): Previously, chenopis (Andrew Chen) wrote…
Done. docs/tutorials/federation/set-up-coredns-provider-federation.md, line 63 at r3 (raw file): Previously, chenopis (Andrew Chen) wrote…
Done. docs/tutorials/federation/set-up-coredns-provider-federation.md, line 64 at r3 (raw file): Previously, chenopis (Andrew Chen) wrote…
Done. docs/tutorials/federation/set-up-coredns-provider-federation.md, line 65 at r3 (raw file): Previously, chenopis (Andrew Chen) wrote…
Done. docs/tutorials/federation/set-up-coredns-provider-federation.md, line 66 at r3 (raw file): Previously, chenopis (Andrew Chen) wrote…
Done. docs/tutorials/federation/set-up-coredns-provider-federation.md, line 88 at r3 (raw file): Previously, chenopis (Andrew Chen) wrote…
Done. docs/tutorials/federation/set-up-coredns-provider-federation.md, line 89 at r3 (raw file): Previously, chenopis (Andrew Chen) wrote…
Done. docs/tutorials/federation/set-up-coredns-provider-federation.md, line 111 at r3 (raw file): Previously, chenopis (Andrew Chen) wrote…
Done. docs/tutorials/federation/set-up-coredns-provider-federation.md, line 122 at r3 (raw file): Previously, chenopis (Andrew Chen) wrote…
Done. docs/tutorials/federation/set-up-coredns-provider-federation.md, line 126 at r3 (raw file): Previously, chenopis (Andrew Chen) wrote…
Done. Comments from Reviewable |
/lgtm @shashidharatd @madhusudancs Anything else before I merge this? |
loogs good to me. @madhusudancs PTAL one final time. |
@chenopis @shashidharatd looks good to me too. Go go go! |
@chenopis, @madhusudancs, This document should be available for 1.6, should i raise to release-1.6 branch also? |
Note that using ServiceType = "LoadBalancer" gives this result: Using ServiceType = "NodePort" seems to be necessary if you want to support both TCP and UDP. |
@chrisjones262, on what cloud provider are you seeing the above issue? |
@shashidharatd Since 1.6 was already released, I can merge this directly into |
@chrisjones262, interesting to know that loadbalancers in 'Azure Container Service' also has similar characteristics as that GCE loadbalancers. We can just start the service to listen either on TCP or UDP in that case and think about implications for DNS server. I would request you to file this issue in https://github.com/kubernetes/charts as the fix need to go in that repo for CoreDNS helm chart. |
@chenopis, just curious, can this documentation be part of 1.6.2 patch release? Does the hosted https://kubernetes.io reflect the changes done to patch release? Also the issue raised by @chrisjones262, affect subset of environments. IMHO, we should not hold back this documentation pr for that issue as there is no change required here. that issue is being addressed in helm/charts#928 and would take its own time to merge and is independent of kubernetes release cycles. @chrisjones262 do you agree? |
@shashidharatd As long as that issue is being addressed, I'm fine w/ merging this now. Thx |
Addresses and Fixes: #2197
cc @madhusudancs @kubernetes/sig-federation-misc
This change is