Disallow headless Services with LB type #33274

nebril · 2016-09-22T14:12:29Z

What this PR does / why we need it: It adds new validation rule for Services, to ensure that creating LoadBalancer type service with cluster IP set to "None" fails.

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

Release note:

Creating LoadBalancer Service with "None" ClusterIP is no longer possible

This change is

smarterclayton · 2016-09-22T15:06:34Z

Would adding this validation break existing users? Strengthening validation is an API change so we have to ensure we cannot break an existing use case / automation setup that depends on it.

nebril · 2016-09-22T17:02:32Z

@smarterclayton I will try to figure this out. Are there any procedures for checking whether the API change is breaking, or should I just move around the GH and check whether the change breaks existing manifests?

smarterclayton · 2016-09-22T17:22:16Z

Easiest to justify is:

Was this a valid scenario before where someone accidentally could have
this set, and if we change it will break them? If not, then it's less
important (so if you can prove no one would be broken)
Can you find examples of this in the real world (can be hard, I don't
always expect this but trying it is good)

On Thu, Sep 22, 2016 at 1:02 PM, Maciej Kwiek notifications@github.com
wrote:

@smarterclayton https://github.com/smarterclayton I will try to figure
this out. Are there any procedures for checking whether the API change is
breaking, or should I just move around the GH and check whether the change
breaks existing manifests?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#33274 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p1P1kuULfHaVkNG7T-S5pos2eKvMks5qsrS1gaJpZM4KD8Ko
.

nebril · 2016-09-23T08:55:06Z

@smarterclayton according to docs: http://kubernetes.io/docs/user-guide/services/#headless-services it doesn't make sense in any case to create a headless load balancer. Excerpt from the doc: "For such Services a cluster IP is not allocated, the kube proxy does not handle these services, and there is no load balancing or proxying done by the platform for them.". I will try to ask around if anyone ever tried to use this case for anything, but it doesn't seem to make any sense.

smarterclayton · 2016-09-25T14:12:11Z

There's no lie balancing done by the platform - could an integrator off the platform use this correctly (or in a more powerful way) if they wanted?

smarterclayton · 2016-09-25T14:24:17Z

"Load"

nebril · 2016-09-26T07:18:48Z

According to the docs, ClusterIP is a virtual IP address used by Kubernetes to fetch traffic to backend pods. So headless LoadBalancer doesn't seem to make sense, because it doesn't have an address to balance the load to and therefore should be disallowed by API.

smarterclayton · 2016-09-26T21:51:16Z

LoadBalancer services don't actually have to load balance to the cluster
IP, though. That's just an artifact of implementation. A LoadBalancer
service is given an external load balancing address, and many of the
implementations just happen to use the clusterIP, but it does not require
that function.

On Mon, Sep 26, 2016 at 3:19 AM, Maciej Kwiek notifications@github.com
wrote:

According to the docs, ClusterIP is a virtual IP address used by
Kubernetes to fetch traffic to backend pods. So headless LoadBalancer
doesn't seem to make sense, because it doesn't have an address to balance
the load to and therefore should be disallowed by API.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#33274 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p2WJVaLBPEXGYjcx0QmDQpTtuBrMks5qt3HlgaJpZM4KD8Ko
.

nebril · 2016-09-27T09:03:27Z

@smarterclayton can you give me an example of such implementation? I was not aware of that.

@thockin does the issue this PR aims to fix actually make sense given what Clayton is saying?

thockin · 2016-09-27T20:50:46Z

I don't think there any combination of headless+LB that valid today. This is technically a breaking change, but every platform we support fails to handle this.

thockin · 2016-09-27T20:51:30Z

pkg/api/validation/validation.go

@@ -2402,6 +2402,9 @@ func ValidateService(service *api.Service) field.ErrorList {
 				allErrs = append(allErrs, field.Invalid(portPath, port.Port, "may not expose port 10250 externally since it is used by kubelet"))
 			}
 		}
+		if service.Spec.ClusterIP == "None" {
+			allErrs = append(allErrs, field.Invalid(specPath.Child("clusterIP"), service.Spec.ClusterIP, "may not be set to 'None' for LoadBalancer"))


nit: add the word "services" at the end

smarterclayton · 2016-09-27T21:32:10Z

Really, every cloud provider we have written an implementation for does not do this. But anyone who is on metal could have done this just by setting an ingress IP and adding the right scripting to populate their LB. I'm not terribly concerned because most people probably default to clusterIP, and an integration like that would still work even with a clusterIP. I guess someone could write a service load balancer integration that gives each endpoint one VIP and maintains a mapping, but given I haven't heard about anyone doing that... I guess I'm reasonably convinced no one would be impacted outside of the first party circle, and if they are impacted they'll let us know. Needs a release note.

nebril · 2016-09-28T08:56:56Z

Is release note in PR description enough?

k8s-ci-robot · 2016-09-28T12:46:36Z

Jenkins unit/integration failed for commit 95f4b6a. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-09-28T13:00:49Z

Jenkins GCI GCE e2e failed for commit 95f4b6a. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

smarterclayton · 2016-09-28T16:40:36Z

Yes

On Wed, Sep 28, 2016 at 9:07 AM, k8s-ci-robot notifications@github.com
wrote:

Jenkins GKE smoke e2e failed
https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/33274/kubernetes-pull-build-test-e2e-gke/14517/
for commit 95f4b6a
95f4b6a.
Full PR test history http://pr-test.k8s.io/33274.

The magic incantation to run this job again is @k8s-bot gke e2e test this.
Please help us cut down flakes by linking to an open flake issue
https://github.com/kubernetes/kubernetes/issues?q=is:issue+label:kind/flake+is:open
when you hit one in your PR.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#33274 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_pzktGnmnasI_Jn7unwfS_TC8HnBrks5qumZ5gaJpZM4KD8Ko
.

k8s-ci-robot · 2016-09-28T17:24:54Z

Jenkins GKE smoke e2e failed for commit 95f4b6a. Full PR test history.

The magic incantation to run this job again is @k8s-bot gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

thockin · 2016-09-28T21:33:19Z

pkg/api/validation/validation.go

@@ -2406,6 +2406,9 @@ func ValidateService(service *api.Service) field.ErrorList {
 				allErrs = append(allErrs, field.Invalid(portPath, port.Port, "may not expose port 10250 externally since it is used by kubelet"))
 			}
 		}
+		if service.Spec.ClusterIP == "None" {
+			allErrs = append(allErrs, field.Invalid(specPath.Child("clusterIP"), service.Spec.ClusterIP, "may not be set to 'None' for LoadBalancer service"))


nit, since CI is still flaking. "services" not "service"

Aw man, sorry about that. Will fix and rebase, flakes are still strong here.

thockin · 2016-09-28T21:41:13Z

@k8s-bot unit this

thockin · 2016-09-28T21:42:51Z

@k8s-bot gci gce e2e test this

thockin · 2016-09-28T21:44:24Z

I've LGTM'ed this, despite asking for a 1 character change. If this goes through, please followup. If it doesn't, please fix :)

If the Service is a Load Balancer, it should not have None Cluster IP. If it does, Service validation fails.

nebril · 2016-09-29T10:37:43Z

Fixed the typo, rebased, flakes are gone. Please re-review.

smarterclayton · 2016-09-29T23:27:12Z

Lgtm

k8s-github-robot · 2016-09-30T02:52:42Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2016-09-30T03:28:55Z

Automatic merge from submit-queue

googlebot added the cla: yes label Sep 22, 2016

k8s-github-robot assigned smarterclayton Sep 22, 2016

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Sep 22, 2016

smarterclayton added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Sep 22, 2016

nebril force-pushed the headless-lb branch from 1fca6ca to f5578f3 Compare September 23, 2016 09:36

k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 23, 2016

thockin reviewed Sep 27, 2016

View reviewed changes

nebril force-pushed the headless-lb branch from f5578f3 to 6c35646 Compare September 28, 2016 08:57

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

nebril force-pushed the headless-lb branch from 6c35646 to 95f4b6a Compare September 28, 2016 12:23

thockin changed the title ~~Validate empty Service ClusterIP against LB type~~ Disallow headless Services with LB type Sep 28, 2016

thockin reviewed Sep 28, 2016

View reviewed changes

thockin added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 28, 2016

Validate None Service ClusterIP against LB type

9e1c886

If the Service is a Load Balancer, it should not have None Cluster IP. If it does, Service validation fails.

nebril force-pushed the headless-lb branch from 95f4b6a to 9e1c886 Compare September 29, 2016 08:20

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 29, 2016

k8s-github-robot mentioned this pull request Sep 29, 2016

[k8s.io] EmptyDir wrapper volumes should not conflict {Kubernetes e2e suite} #32467

Closed

smarterclayton added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 29, 2016

k8s-github-robot merged commit 6a46bf1 into kubernetes:master Sep 30, 2016

k8s-github-robot mentioned this pull request Oct 6, 2016

[k8s.io] Downward API volume should update labels on modification [Conformance] {Kubernetes e2e suite} #34206

Closed

derekwaynecarr mentioned this pull request Oct 19, 2016

Service with type=LoadBalancer and clusterIP=None is allowed #30909

Closed

xiangpengzhao mentioned this pull request Jun 9, 2017

Validation Enhancement: type:LoadBalancer with firewall rules does not open node ports on minions. Service functions from within pods. #34625

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disallow headless Services with LB type #33274

Disallow headless Services with LB type #33274

nebril commented Sep 22, 2016 •

edited

Loading

smarterclayton commented Sep 22, 2016

nebril commented Sep 22, 2016

smarterclayton commented Sep 22, 2016

nebril commented Sep 23, 2016

smarterclayton commented Sep 25, 2016

smarterclayton commented Sep 25, 2016

nebril commented Sep 26, 2016

smarterclayton commented Sep 26, 2016

nebril commented Sep 27, 2016

thockin commented Sep 27, 2016

thockin Sep 27, 2016

smarterclayton commented Sep 27, 2016 via email

nebril commented Sep 28, 2016

k8s-ci-robot commented Sep 28, 2016

k8s-ci-robot commented Sep 28, 2016

smarterclayton commented Sep 28, 2016

k8s-ci-robot commented Sep 28, 2016

thockin Sep 28, 2016

nebril Sep 29, 2016

thockin commented Sep 28, 2016

thockin commented Sep 28, 2016

thockin commented Sep 28, 2016

nebril commented Sep 29, 2016

smarterclayton commented Sep 29, 2016

k8s-github-robot commented Sep 30, 2016

k8s-github-robot commented Sep 30, 2016

Disallow headless Services with LB type #33274

Disallow headless Services with LB type #33274

Conversation

nebril commented Sep 22, 2016 • edited Loading

smarterclayton commented Sep 22, 2016

nebril commented Sep 22, 2016

smarterclayton commented Sep 22, 2016

nebril commented Sep 23, 2016

smarterclayton commented Sep 25, 2016

smarterclayton commented Sep 25, 2016

nebril commented Sep 26, 2016

smarterclayton commented Sep 26, 2016

nebril commented Sep 27, 2016

thockin commented Sep 27, 2016

thockin Sep 27, 2016

Choose a reason for hiding this comment

smarterclayton commented Sep 27, 2016 via email

nebril commented Sep 28, 2016

k8s-ci-robot commented Sep 28, 2016

k8s-ci-robot commented Sep 28, 2016

smarterclayton commented Sep 28, 2016

k8s-ci-robot commented Sep 28, 2016

thockin Sep 28, 2016

Choose a reason for hiding this comment

nebril Sep 29, 2016

Choose a reason for hiding this comment

thockin commented Sep 28, 2016

thockin commented Sep 28, 2016

thockin commented Sep 28, 2016

nebril commented Sep 29, 2016

smarterclayton commented Sep 29, 2016

k8s-github-robot commented Sep 30, 2016

k8s-github-robot commented Sep 30, 2016

nebril commented Sep 22, 2016 •

edited

Loading