kubeadm: Create dnsIP by selecting the tenth IP from k8s svc CIDR #51990

madhukar32 · 2017-09-06T00:25:44Z

What this PR does / why we need it:

Creates dnsIP by selecting the ninth IP from k8s svc cluster IP, instead of appending 0 to the k8s svcIP string.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #51997

Special notes for your reviewer:
This is helpful when we have service cluster range CIDR as 10.87.116.64/26 (for example), previously this would have failed while parsing the dnsIP, as we used to append a 0 to the k8s svc clusterIP string. This will get the same dnsIP 10.96.0.10 for very widely used service cluster range CIDR 10.96.0.0/12

Release note:

None

madhukar32 · 2017-09-06T00:54:00Z

/assign @krousey

dixudx · 2017-09-06T02:56:46Z

cmd/kubeadm/app/phases/addons/dns/dns.go

 	}
+	dnsIP := svcIP.To4()
+	dnsIP[3] += 9


Why adding 9?

Just to stick with dnsIP which is being assigned today.

Assuming that svcIP is 10.96.0.1 with the above logic the user will get dnsIP as 10.96.0.10. This is the same IP which a k8s user is getting today. Open for any other suggestion

@madhukar32 Well, I think this will result in a conflict. We cannot guarantee 10.96.0.10 has been reserved only for dnsIP. It could be a svcIP, right?

@dixudx: Ok I am new to k8s so I might be completely wrong, please correct me.

We create this dnsIP in the above code(which is the svcIP for kube-dns svc) and then go and create the actual service here. After kube-dns svc is created it will be actually reserved for kube-dns svc only. Until and unless it is forcefully deleted. So we are pretty much reserving an IP today also isn't it?

@madhukar32 Also this value will exceed 255, right? What's more, dnsIP consumes ip resources from service ip pool, which means we can only create half of services as before. This may cause big trouble for large cluster upgrading.

I think we'd better avoid such hard-coded way.

@dixudx: On the value, I have cross verified that it will not exceed 255 https://play.golang.org/p/KlUAsNrzFJhttps://play.golang.org/p/KlUAsNrzFJ
Just to clarify here, when I say dnsIP its the service clusterIP of kube-dns svc and it is suppose to be from service ip pool.

The 9th ip will only have the same behaviour when the service ip is .1 (but is most consistent this solution than the one we have now)

please use "k8s.io/kubernetes/pkg/registry/core/service/ipallocator".GetIndexedIP(svcSubnet, 10) or similar for this

@luxas: Thanks! Thats a good idea, will update the commit

luxas · 2017-09-06T17:27:48Z

/ok-to-test

kevin-wangzefeng · 2017-09-07T12:30:46Z

/retest

madhukar32 · 2017-09-07T21:32:42Z

/retest

madhukar32 · 2017-09-08T01:11:29Z

@kubernetes/sig-cluster-lifecycle-pr-reviews /assign @luxas

k8s-ci-robot · 2017-09-08T01:11:37Z

@madhukar32: Reiterating the mentions to trigger a notification:
@kubernetes/sig-cluster-lifecycle-pr-reviews.

In response to this:

@kubernetes/sig-cluster-lifecycle-pr-reviews /assign @luxas

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

luxas

Please add --service-subnet to the kubeadm phase addons kube-dns flag with this change...

madhukar32 · 2017-09-08T17:21:50Z

@luxas: 👍 Have added --service-cidr flag for kubeadm phase addons kube-dns. Similar to the one we have for kubeadm init

madhukar32 · 2017-09-11T00:54:00Z

/retest

madhukar32 · 2017-09-13T03:56:21Z

/retest

madhukar32 · 2017-09-13T04:51:48Z

@luxas: Could you please review this once again. Have added --service-cidr flag for kubeadm phase addons kube-dns

mattymo · 2017-09-14T06:46:01Z

@madhukar32 Thanks for this PR! It will make the mandatory kubedns useful when using a custom service subnet.

madhukar32 · 2017-09-25T20:30:18Z

@luxas : Could you please review this

madhukar32 · 2017-10-04T05:11:26Z

CC @kubernetes/sig-cluster-lifecycle-pr-reviews

k8s-ci-robot · 2017-10-04T05:11:34Z

@madhukar32: Reiterating the mentions to trigger a notification:
@kubernetes/sig-cluster-lifecycle-pr-reviews

In response to this:

CC @kubernetes/sig-cluster-lifecycle-pr-reviews

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

luxas · 2017-11-08T21:54:02Z

@madhukar32 If you rebase this PR I can look at it and approve it

madhukar32 · 2017-11-14T19:14:43Z

@luxas have rebased this PR. Could you please review now

luxas

Thanks @madhukar32!
This basically looks good to me, but please fix the small nit and add an unit test while you do improve the code here :)

luxas · 2017-11-14T21:04:12Z

cmd/kubeadm/app/phases/addons/dns/dns.go

-func getDNSIP(client clientset.Interface) (net.IP, error) {
-	k8ssvc, err := client.CoreV1().Services(metav1.NamespaceDefault).Get("kubernetes", metav1.GetOptions{})
+// getDNSIP returns a dnsIP, which is 10th IP in svcSubnet CIDR range
+func getDNSIP(cfg *kubeadmapi.MasterConfiguration) (net.IP, error) {


while you're here, please add an unit test for this function

and a nit: I'd prefer if you only passed serviceSubnet string here, not the full cfg object

madhukar32 · 2017-11-15T11:01:59Z

@luxas thanks for reviewing, have taken care of your comments. Could you please check now?

luxas

/lgtm
/approve no-issue

k8s-github-robot · 2017-11-15T18:00:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: luxas, madhukar32

Associated issue: 51997

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~cmd/kubeadm/OWNERS~~ [luxas]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-11-16T11:09:22Z

Automatic merge from submit-queue (batch tested with PRs 55798, 49579, 54862, 55188, 51990). If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 6, 2017

k8s-github-robot assigned krousey and luxas Sep 6, 2017

k8s-github-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Sep 6, 2017

dixudx reviewed Sep 6, 2017

View reviewed changes

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 6, 2017

madhukar32 force-pushed the get_dns_ip branch from a6dd21b to f1f3416 Compare September 7, 2017 17:37

madhukar32 changed the title ~~kubeadm: Create dnsIP by selecting the ninth IP from k8s svc clusterIP~~ kubeadm: Create dnsIP by selecting the tenth IP from k8s svc CIDR Sep 7, 2017

madhukar32 force-pushed the get_dns_ip branch from f1f3416 to 4089eb0 Compare September 7, 2017 19:45

madhukar32 force-pushed the get_dns_ip branch from 4089eb0 to 99e1697 Compare September 7, 2017 23:41

k8s-ci-robot added the sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. label Sep 8, 2017

luxas reviewed Sep 8, 2017

View reviewed changes

madhukar32 force-pushed the get_dns_ip branch from 99e1697 to 4e0424f Compare September 8, 2017 16:03

madhukar32 force-pushed the get_dns_ip branch 3 times, most recently from 0f5714e to 54c882f Compare September 13, 2017 03:00

madhukar32 force-pushed the get_dns_ip branch from 54c882f to dd529b2 Compare September 25, 2017 19:10

madhukar32 force-pushed the get_dns_ip branch from dd529b2 to 63655b4 Compare September 25, 2017 19:21

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 18, 2017

madhukar32 force-pushed the get_dns_ip branch from 63655b4 to 2c8a42a Compare November 14, 2017 18:31

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 14, 2017

luxas reviewed Nov 14, 2017

View reviewed changes

kubeadm: change in logic of getDNSIP

359d814

madhukar32 force-pushed the get_dns_ip branch from 2c8a42a to 359d814 Compare November 15, 2017 10:12

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 15, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 15, 2017

luxas approved these changes Nov 15, 2017

View reviewed changes

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 15, 2017

k8s-github-robot merged commit 4060d23 into kubernetes:master Nov 16, 2017

madhukar32 deleted the get_dns_ip branch November 21, 2017 03:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubeadm: Create dnsIP by selecting the tenth IP from k8s svc CIDR #51990

kubeadm: Create dnsIP by selecting the tenth IP from k8s svc CIDR #51990

madhukar32 commented Sep 6, 2017 •

edited

madhukar32 commented Sep 6, 2017

dixudx Sep 6, 2017

madhukar32 Sep 6, 2017 •

edited

dixudx Sep 6, 2017

madhukar32 Sep 6, 2017

dixudx Sep 6, 2017

madhukar32 Sep 6, 2017 •

edited

GheRivero Sep 6, 2017

luxas Sep 6, 2017

madhukar32 Sep 6, 2017

luxas commented Sep 6, 2017

kevin-wangzefeng commented Sep 7, 2017

madhukar32 commented Sep 7, 2017

madhukar32 commented Sep 8, 2017

k8s-ci-robot commented Sep 8, 2017

luxas left a comment

madhukar32 commented Sep 8, 2017

madhukar32 commented Sep 11, 2017

madhukar32 commented Sep 13, 2017

madhukar32 commented Sep 13, 2017

mattymo commented Sep 14, 2017

madhukar32 commented Sep 25, 2017

madhukar32 commented Oct 4, 2017

k8s-ci-robot commented Oct 4, 2017

luxas commented Nov 8, 2017

madhukar32 commented Nov 14, 2017

luxas left a comment

luxas Nov 14, 2017

luxas Nov 14, 2017

madhukar32 commented Nov 15, 2017

luxas left a comment

k8s-github-robot commented Nov 15, 2017

k8s-github-robot commented Nov 16, 2017

kubeadm: Create dnsIP by selecting the tenth IP from k8s svc CIDR #51990

kubeadm: Create dnsIP by selecting the tenth IP from k8s svc CIDR #51990

Conversation

madhukar32 commented Sep 6, 2017 • edited

madhukar32 commented Sep 6, 2017

Choose a reason for hiding this comment

madhukar32 Sep 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madhukar32 Sep 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luxas commented Sep 6, 2017

kevin-wangzefeng commented Sep 7, 2017

madhukar32 commented Sep 7, 2017

madhukar32 commented Sep 8, 2017

k8s-ci-robot commented Sep 8, 2017

luxas left a comment

Choose a reason for hiding this comment

madhukar32 commented Sep 8, 2017

madhukar32 commented Sep 11, 2017

madhukar32 commented Sep 13, 2017

madhukar32 commented Sep 13, 2017

mattymo commented Sep 14, 2017

madhukar32 commented Sep 25, 2017

madhukar32 commented Oct 4, 2017

k8s-ci-robot commented Oct 4, 2017

luxas commented Nov 8, 2017

madhukar32 commented Nov 14, 2017

luxas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madhukar32 commented Nov 15, 2017

luxas left a comment

Choose a reason for hiding this comment

k8s-github-robot commented Nov 15, 2017

k8s-github-robot commented Nov 16, 2017

madhukar32 commented Sep 6, 2017 •

edited

madhukar32 Sep 6, 2017 •

edited

madhukar32 Sep 6, 2017 •

edited