aggregator: add port support, unified for both modes #45082

sttts · 2017-04-28T08:58:03Z

Add a port field to the service reference of an APIService.

k8s-reviewable · 2017-04-28T08:58:09Z

This change is

k8s-github-robot · 2017-04-28T08:58:23Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sttts
We suggest the following additional approver: deads2k

Assign the PR to them by writing /assign @deads2k in a comment when ready.

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

staging/src/k8s.io/kube-aggregator/OWNERS

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

deads2k · 2017-04-28T12:59:48Z

@cheftako you want to take a look?

deads2k · 2017-04-28T13:01:09Z

staging/src/k8s.io/kube-aggregator/pkg/apiserver/apiservice_controller.go

@@ -96,7 +97,10 @@ func (c *APIServiceRegistrationController) sync(key string) error {
 		return err
 	}

-	c.apiHandlerManager.AddAPIService(apiService, c.getDestinationHost(apiService))
+	host := c.getDestinationHost(apiService)
+	if len(host) > 0 {


If an APIService is updated to remove its referenced service (becomes local), we need to update to reflect that, right?

cheftako · 2017-04-28T19:18:41Z

staging/src/k8s.io/kube-aggregator/pkg/apiserver/apiservice_controller.go

+
+	port := int32(443)
+	if len(service.Spec.Ports) > 0 {
+		port = service.Spec.Ports[0].Port


This seems fine if len() == 1. However if len() > 1 this seems problematic. I couldn't find any indication of ordering in our documentation so I'm not sure we can rely on that. In fact one of our examples has the https port second and that seems like the port we should be using. So if len() > 1 we could look for a name which indicates what we should be using, such as https. We could also look to fix the registration system to specify a port (?name) if there is more than 1 port. Picking the first port seems like what we should do if all other resolution mechanisms fail.

I assumed that there is an order. If not, this does not make sense, totally agree. Actually, I do not want too many heuristic. I would prefer adding an explicit port name to the apiservice spec and only allow to leave that empty if there is exactly one port in a service. Wdyt?

cheftako · 2017-04-28T19:58:10Z

staging/src/k8s.io/kube-aggregator/pkg/apiserver/apiservice_controller.go

+	}
+
+	port := int32(443)
+	if len(service.Spec.Ports) > 0 {


The validation code as far as I can tell prevents you getting in to the length equals 0 case. Not sure it would be safe to force the port to 443 if that wasn't true. Not sure how the networking code would function if no ports were specified but if it either left all ports open or forwarded based on which ports the relevant pod specified, then you could be forcing to the wrong port and something that had been working would now be broken.

luxas · 2017-05-30T12:55:53Z

First port is used if defined. Otherwise, 443. We could extend this with a portName field in the future.

@sttts Why not just support portName right away?

sttts · 2017-05-30T13:12:50Z

@deads2k @cheftako I rebased this onto the merged pod ip resolver code, reactivated our old resolver tests and unified both resolvers. I would very much like to get this into 1.7 before people start using the new api, especially the requirements around service ports and the service name syntax scheme:name:port.

deads2k · 2017-05-30T13:26:47Z

staging/src/k8s.io/apiserver/pkg/util/proxy/proxy.go

+	}
+	if len(portStr) == 0 {
+		switch svcScheme {
+		case "http":


You can't safely proxy over http. I think we should always force https.

My argument for http was that it's useful during development without certs. But if we can ignore self-signed certs anyway, there is not much need for http.

deads2k · 2017-05-30T13:36:29Z

especially the requirements around service ports and the service name syntax scheme:name:port.

We have to use SSL in order to identify the aggregator from the user API server. For ports, my first choice is always 443 on the service (the endpoint thing is going to have to do the translation), my second choice is to add a port field and the endpoint thing still has to do the translation from service to endpoint port.

sttts · 2017-05-30T14:21:12Z

I have extracted the uncontroversial part of the code without the scheme+port syntax into #46623. ptal.

sttts · 2017-05-30T14:22:04Z

my second choice is to add a port field and the endpoint thing still has to do the translation from service to endpoint port.

I favor this one. Let us add a Port *IntString field which defaults to 443.

sttts · 2017-05-30T16:47:24Z

@deads2k @cheftako added the port field to ServiceReference. ptal

sttts · 2017-05-31T13:26:08Z

@k8s-bot pull-kubernetes-unit test this

sttts · 2017-05-31T13:31:27Z

@k8s-bot pull-kubernetes-unit test this

sttts · 2017-05-31T15:13:01Z

@k8s-bot pull-kubernetes-verify test this

Automatic merge from submit-queue (batch tested with PRs 43505, 45168, 46439, 46677, 46623) aggregator: unify resolver implementation and tests This is #45082, but without the port support.

k8s-github-robot · 2017-06-02T01:11:06Z

@sttts PR needs rebase

k8s-github-robot · 2017-07-08T17:55:09Z

This PR hasn't been active in 38 days. It will be closed in 51 days (Aug 29, 2017).

cc @cheftako @deads2k @sttts

You can add 'keep-open' label to prevent this from happening, or add a comment to keep it open another 90 days

sttts · 2017-07-19T13:25:20Z

We decided not to implement custom port support. So closing it for now.

WIZARD-CXY · 2017-12-31T06:56:32Z

@sttts I wonder why " we decided not to implement custom port support"? I think it is nice to have.

sttts · 2018-01-02T06:54:10Z

@WIZARD-CXY because it is only "nice to have", i.e. you can easily use a default port.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 28, 2017

sttts assigned deads2k Apr 28, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Apr 28, 2017

deads2k assigned cheftako Apr 28, 2017

deads2k reviewed Apr 28, 2017

View reviewed changes

sttts force-pushed the sttts-aggregator-ports branch 2 times, most recently from ff58e0d to d79e123 Compare April 28, 2017 15:42

cheftako reviewed Apr 28, 2017

View reviewed changes

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

sttts force-pushed the sttts-aggregator-ports branch from d79e123 to 1588934 Compare May 30, 2017 13:09

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 30, 2017

sttts changed the title ~~kube-aggregator: add support for service port definitions~~ aggregator: unify resolver implementation and tests May 30, 2017

sttts added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels May 30, 2017

deads2k reviewed May 30, 2017

View reviewed changes

sttts force-pushed the sttts-aggregator-ports branch from cce231a to f3fa433 Compare May 30, 2017 14:17

sttts changed the title ~~aggregator: unify resolver implementation and tests~~ aggregator: add port support, unified for both modes May 30, 2017

sttts mentioned this pull request May 30, 2017

aggregator: unify resolver implementation and tests #46623

Merged

sttts force-pushed the sttts-aggregator-ports branch from f3fa433 to 5d2838e Compare May 30, 2017 16:45

sttts force-pushed the sttts-aggregator-ports branch 3 times, most recently from 0e094f7 to 5fdf001 Compare May 31, 2017 10:32

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 31, 2017

sttts force-pushed the sttts-aggregator-ports branch from 5fdf001 to 6fc2aae Compare May 31, 2017 11:38

sttts added 6 commits May 31, 2017 14:20

aggregation: restore cluster-ip routing tests

f6c470d

aggregation: unify mode implementations

1934053

aggregation: handle ClusterIPNone correctly

4c508ef

aggregation: add ExternalName service support

6be7ec0

Update bazel

112c878

aggregation: add port field to service ref

810422a

sttts force-pushed the sttts-aggregator-ports branch from 6fc2aae to 810422a Compare May 31, 2017 12:23

sttts modified the milestone: v1.7 May 31, 2017

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

sttts closed this Jul 19, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

aggregator: add port support, unified for both modes #45082

aggregator: add port support, unified for both modes #45082

sttts commented Apr 28, 2017 •

edited

Loading

k8s-reviewable commented Apr 28, 2017

k8s-github-robot commented Apr 28, 2017

deads2k commented Apr 28, 2017

deads2k Apr 28, 2017

cheftako Apr 28, 2017

sttts May 2, 2017

cheftako Apr 28, 2017

luxas commented May 30, 2017 •

edited

Loading

sttts commented May 30, 2017

deads2k May 30, 2017

sttts May 30, 2017

deads2k commented May 30, 2017

sttts commented May 30, 2017

sttts commented May 30, 2017

sttts commented May 30, 2017

sttts commented May 31, 2017

sttts commented May 31, 2017

sttts commented May 31, 2017

k8s-github-robot commented Jun 2, 2017

k8s-github-robot commented Jul 8, 2017

sttts commented Jul 19, 2017

WIZARD-CXY commented Dec 31, 2017

sttts commented Jan 2, 2018

aggregator: add port support, unified for both modes #45082

aggregator: add port support, unified for both modes #45082

Conversation

sttts commented Apr 28, 2017 • edited Loading

k8s-reviewable commented Apr 28, 2017

k8s-github-robot commented Apr 28, 2017

deads2k commented Apr 28, 2017

deads2k Apr 28, 2017

Choose a reason for hiding this comment

cheftako Apr 28, 2017

Choose a reason for hiding this comment

sttts May 2, 2017

Choose a reason for hiding this comment

cheftako Apr 28, 2017

Choose a reason for hiding this comment

luxas commented May 30, 2017 • edited Loading

sttts commented May 30, 2017

deads2k May 30, 2017

Choose a reason for hiding this comment

sttts May 30, 2017

Choose a reason for hiding this comment

deads2k commented May 30, 2017

sttts commented May 30, 2017

sttts commented May 30, 2017

sttts commented May 30, 2017

sttts commented May 31, 2017

sttts commented May 31, 2017

sttts commented May 31, 2017

k8s-github-robot commented Jun 2, 2017

k8s-github-robot commented Jul 8, 2017

sttts commented Jul 19, 2017

WIZARD-CXY commented Dec 31, 2017

sttts commented Jan 2, 2018

sttts commented Apr 28, 2017 •

edited

Loading

luxas commented May 30, 2017 •

edited

Loading