Skip to content
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

Endpoints controller demands ports for headless services #32796

Closed
thockin opened this issue Sep 15, 2016 · 17 comments · Fixed by #47250
Closed

Endpoints controller demands ports for headless services #32796

thockin opened this issue Sep 15, 2016 · 17 comments · Fixed by #47250
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. sig/network Categorizes an issue or PR as relevant to SIG Network.
Milestone

Comments

@thockin
Copy link
Member

thockin commented Sep 15, 2016

API validation explicitly allows headless services to not have ports, but we seem to have not updated endpoints controller to know this:

https://github.com/kubernetes/kubernetes/blob/master/pkg/api/validation/validation.go#L2390

vs

https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/endpoint/endpoints_controller.go#L389

Superficially this seems like an easy fix. May be a candidate for 1.4.x

@sebgoa

@thockin thockin added sig/network Categorizes an issue or PR as relevant to SIG Network. team/cluster help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. labels Sep 15, 2016
@fraenkel
Copy link
Contributor

@thockin What did you expect to happen? It will skip creating any EndpointSubsets. It handles having no ports.

@thockin
Copy link
Member Author

thockin commented Sep 15, 2016

I think it should create a Subset with a 0 port (or a dummy value). We
don't currently represent port ranges, but if we did it would be (1-655356)
here.

On Thu, Sep 15, 2016 at 12:23 PM, Michael Fraenkel <notifications@github.com

wrote:

@thockin https://github.com/thockin What did you expect to happen? It
will skip creating any EndpointSubsets. It handles having no ports.


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

@sebgoa
Copy link
Contributor

sebgoa commented Sep 16, 2016

correct, it should create a subset with the PodIP that match the service selector. 0 port would work (or even no port at all if endpoints could support that).

@fraenkel
Copy link
Contributor

Looking at RepackSubsets, it will drop out endpoints with no ports so going with the 0 port is what I did.

@thockin
Copy link
Member Author

thockin commented Sep 17, 2016

Yeah, without trawling code (thanks!) It is not clear if a zero port would
work. If I were clever, I would have written the validation to disallow 0,
because no-port-needed logic came later, and then would have expanded it to
make port=0 mean no-port-needed.

Simply trying it might be the best course, but we should audit kubectl, APi
validation, API defaulting, subset repacking, endpoints controller,
kube-proxy, and DNS server for correct behavior in the face of headless
without ports. Clearly some of that is wrong today, no surprise if more of
it is wrong still :)

On Sep 16, 2016 10:57 AM, "Michael Fraenkel" notifications@github.com
wrote:

Looking at RepackSubsets, it will drop out endpoints with no ports so going
with the 0 port is what I did.


You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#32796 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFVgVFJ6ubH_f_Tknymh-mJfhj3ketPtks5qqtiZgaJpZM4J9-qX
.

@F21
Copy link

F21 commented Nov 17, 2016

I just ran into this myself. A work around is to do something like this:

apiVersion: v1
kind: Service
metadata:
  name: my-svc
  labels:
    run: my-svc
spec:
  clusterIP: None
  selector:
    run: my-svc
  ports:

@je-al
Copy link

je-al commented Jan 4, 2017

Furthermore, kubectl needs some love:

➜  kubectl create service clusterip my-nginx-hl --clusterip='None' --tcp='80:80'
error: can not map ports with clusterip=None

➜  kubectl create service clusterip my-nginx-hl --clusterip='None'
service "my-nginx-hl" created

➜  kubectl describe svc/my-nginx-hl
Name:                   my-nginx-hl
Namespace:              default
Labels:                 app=my-nginx-hl
Selector:               app=my-nginx-hl
Type:                   ClusterIP
IP:                     None
Session Affinity:       None
No events.

➜  kubectl patch svc --type=json -p='[{"op":"replace","path":"/spec/selector","value":{"run":"my-nginx"}}]' my-nginx-hl                                                               <<<
"my-nginx-hl" patched

➜  kubectl describe svc/my-nginx-hl
Name:                   my-nginx-hl
Namespace:              default
Labels:                 app=my-nginx-hl
Selector:               run=my-nginx
Type:                   ClusterIP
IP:                     None
Session Affinity:       None
No events.

➜  kubectl patch svc --type=json -p='[{"op":"replace","path":"/spec/ports","value":[{"name":"80-80","port":80, "targetPort":80,"procotol":"TCP"}]}]' my-nginx-hl
"my-nginx-hl" patched

➜  kubectl describe svc/my-nginx-hl
Name:                   my-nginx-hl
Namespace:              default
Labels:                 app=my-nginx-hl
Selector:               run=my-nginx
Type:                   ClusterIP
IP:                     None
Port:                   80-80   80/TCP
Endpoints:              172.17.0.5:80,172.17.0.6:80,172.17.0.7:80
Session Affinity:       None
No events.

P.S.: disregard the selector patching, that's an unrelated issue.

@jianzi123
Copy link

I would like to work on this issue

@thockin thockin added this to the v1.7 milestone May 27, 2017
@thockin
Copy link
Member Author

thockin commented Jun 1, 2017

Any progress here?

@thockin
Copy link
Member Author

thockin commented Jun 9, 2017

Can we get an update?

@xiangpengzhao
Copy link
Contributor

Seems like @fraenkel PR #32875 had fixed this main issue. But the PR was closed due to inactive for 90 days. If @fraenkel doesn't have plan to reopen it again and @jianzi123 doesn't have new progress, I'd like to pick it up. What I can think the work might be polishing PR #32875 and fixing #32796 (comment). I will also try to find out if there is something more to do per #32796 (comment).

@thockin WDYT?

@xiangpengzhao
Copy link
Contributor

I send PR #47250. Hope we can fix this ASAP before 1.7.

@bowei
Copy link
Member

bowei commented Jun 12, 2017

@dchen1107 move to next milestone.

@dchen1107
Copy link
Member

There is a pending pr to address this issue, not update if that is the right fix.

@xiangpengzhao
Copy link
Contributor

yeah that PR is still under reviewing.

@bowei
Copy link
Member

bowei commented Jun 13, 2017

I took a look at the PR and it looks like there are still some outstanding review comments and its does not have an LGTM. It feel like this should be moved to the next milestone to be safe. Please comment if you disagree.

@marun
Copy link
Contributor

marun commented Jun 19, 2017

Moving to 1.8.

@marun marun modified the milestones: v1.8, v1.7 Jun 19, 2017
jcbsmpsn pushed a commit to jcbsmpsn/kubernetes that referenced this issue Jun 29, 2017
Automatic merge from submit-queue (batch tested with PRs 47850, 47835, 46197, 47250, 48284)

Populate endpoints for headless service with no ports

**What this PR does / why we need it**:
- populate endpoints with headless service (thanks @fraenkel for the original PR!)
- allow ports with headless service
- nits

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

**Special notes for your reviewer**:
/cc @thockin @fraenkel 
**Release note**:

```release-note
NONE
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. sig/network Categorizes an issue or PR as relevant to SIG Network.
Projects
None yet
10 participants