Adding hostname to groups discovery information #20626

nikhiljindal · 2016-02-04T06:01:33Z

Based on #20358 (comment)

Ref #20229

Adding a publicHostname field to groups discovery information.

To be used by dynamic client talking to a cluster with federated api servers.

cc @bgrant0607 @lavalamp @smarterclayton @kubernetes/sig-api-machinery

k8s-github-robot · 2016-02-04T06:02:05Z

Labelling this PR as size/XS

k8s-bot · 2016-02-04T06:32:47Z

GCE e2e test build/test passed for commit 8607da2200e16a68fb9004c8d56f4b4e793df2ec.

k8s-bot · 2016-02-04T07:25:46Z

GCE e2e test build/test passed for commit 64b6b8cf1ba86ecf83cbd1b473befba39a234589.

lavalamp · 2016-02-04T18:35:30Z

Can you also add the master change needed to populate this field? I think you want that in 1.2, also.

k8s-github-robot · 2016-02-04T19:40:36Z

Labelling this PR as size/M

bgrant0607 · 2016-02-04T19:51:36Z

pkg/api/unversioned/types.go

@@ -298,6 +298,8 @@ const (
 // +protobuf.options.(gogoproto.goproto_stringer)=false
 type APIVersions struct {
 	TypeMeta `json:",inline"`
+	// public hostname of the server that is serving this group.
+	PublicHostname string `json:"publicHostname"`


I'd like to be consistent with service terminology, which is "External" rather than "Public":

https://github.com/kubernetes/kubernetes/blob/master/pkg/api/v1/types.go#L1740

And are we also going to want the port?

And path prefix?

And would we want to allow an IP?

Will change to External.

Yes, it can have IP and port as well.

nikhiljindal · 2016-02-04T20:06:35Z

Have updated the PR to populate the field as well.

This is how it looks for me on local cluster:

localhost:8080/apis:

localhost:8080/api:

smarterclayton · 2016-02-04T20:40:33Z

Do we want to call it public? What does "public" mean? Do we use "public"
anywhere else in the API? Do we want to formalize "public" as meaning
"visible outside the cluster"?

On Thu, Feb 4, 2016 at 3:09 PM, Nikhil Jindal notifications@github.com
wrote:

Have updated the PR to populate the field as well.

This is how it looks for me on local cluster:

localhost:8080/apis:
[image: apis]
https://cloud.githubusercontent.com/assets/10199099/12828099/a2f7e568-cb37-11e5-8a58-d570c73d8a1f.png

localhost:8080/api:
[image: api]
https://cloud.githubusercontent.com/assets/10199099/12828101/a79136b0-cb37-11e5-9e17-568bb0837013.png

—
Reply to this email directly or view it on GitHub
#20626 (comment)
.

nikhiljindal · 2016-02-04T23:58:09Z

You are right @smarterclayton. @bgrant0607 also said the same thing. He suggested renaming public to external (which is what we use in ServiceSpec: https://github.com/kubernetes/kubernetes/blob/master/pkg/api/v1/types.go#L1740).

But on discussing further with @lavalamp and @bgrant0607, we decided to replace the external address field with a CIDRToAddressMap.
Clients can match their IP with the CIDR and figure out the address that they should use.
This assumes that the subnets have exclusive CIDRs. Clients can use the address with the longest CIDR that they match.
I will update the PR accordingly.

cc @thockin @bprashanth

nikhiljindal · 2016-02-05T00:06:57Z

Pushed a new commit as per the comment above.

I am using ServiceClusterIPRange to determine the internal CIDR (not sure if thats the right thing to use).
This is what I see now:

localhost:8080/api:

localhost:8080/apis:

k8s-bot · 2016-02-05T00:43:34Z

GCE e2e test build/test passed for commit 56557fbe7f70e30869d2e66aca7ef8dbe1ecac76.

smarterclayton · 2016-02-05T00:49:28Z

Wow.... that's... interesting. It uh... is certainly flexible.... :)

On Thu, Feb 4, 2016 at 7:43 PM, Kubernetes Bot notifications@github.com
wrote:

GCE e2e test build/test passed for commit 56557fb
56557fb
.

Build Log
https://storage.cloud.google.com/kubernetes-jenkins/pr-logs/pull/20626/kubernetes-pull-build-test-e2e-gce/26852/build-log.txt

Test Artifacts
https://console.developers.google.com/storage/browser/kubernetes-jenkins/pr-logs/pull/20626/kubernetes-pull-build-test-e2e-gce/26852/artifacts/

Internal Jenkins Results
http://goto.google.com/prkubekins/job/kubernetes-pull-build-test-e2e-gce//26852

—
Reply to this email directly or view it on GitHub
#20626 (comment)
.

smarterclayton · 2016-02-05T00:49:43Z

Will this map to IPv6 cleanly?

On Thu, Feb 4, 2016 at 7:49 PM, Clayton Coleman ccoleman@redhat.com wrote:

Wow.... that's... interesting. It uh... is certainly flexible.... :)

On Thu, Feb 4, 2016 at 7:43 PM, Kubernetes Bot notifications@github.com
wrote:

GCE e2e test build/test passed for commit 56557fb
56557fb
.

Build Log
https://storage.cloud.google.com/kubernetes-jenkins/pr-logs/pull/20626/kubernetes-pull-build-test-e2e-gce/26852/build-log.txt

Test Artifacts
https://console.developers.google.com/storage/browser/kubernetes-jenkins/pr-logs/pull/20626/kubernetes-pull-build-test-e2e-gce/26852/artifacts/

Internal Jenkins Results
http://goto.google.com/prkubekins/job/kubernetes-pull-build-test-e2e-gce//26852

—
Reply to this email directly or view it on GitHub
#20626 (comment)
.

lavalamp · 2016-02-05T00:57:24Z

It looks like you have the wrong port on the external address, I thought 6443 was the one we listen on, but we actually should be advertising 443? I forget how it's connected.

lavalamp · 2016-02-05T00:58:02Z

pkg/api/unversioned/types.go

@@ -321,6 +323,15 @@ type APIGroup struct {
 	// preferredVersion is the version preferred by the API server, which
 	// probably is the storage version.
 	PreferredVersion GroupVersionForDiscovery `json:"preferredVersion,omitempty"`
+	// a map of CIDR to address of the server that is serving this group. Clients can use the appropriate address as per the CIDR that they match.
+	CIDRToAddressMap []CIDRToAddress `json:"cidrToAddressMap"`


@thockin we need you to bless this type.

How about ServerAddressByClientCIDR?

lavalamp · 2016-02-05T01:03:13Z

I believe CIDRs are still things for IPv6, so it should. Also this would
let us provide IPv6 and IPv4 addresses, which could be handy, though that
wasn't what we were thinking.

On Thu, Feb 4, 2016 at 4:49 PM, Clayton Coleman notifications@github.com
wrote:

Will this map to IPv6 cleanly?

On Thu, Feb 4, 2016 at 7:49 PM, Clayton Coleman ccoleman@redhat.com
wrote:

Wow.... that's... interesting. It uh... is certainly flexible.... :)

On Thu, Feb 4, 2016 at 7:43 PM, Kubernetes Bot <notifications@github.com

wrote:

GCE e2e test build/test passed for commit 56557fb
<
56557fb

.

Build Log
<
https://storage.cloud.google.com/kubernetes-jenkins/pr-logs/pull/20626/kubernetes-pull-build-test-e2e-gce/26852/build-log.txt

Test Artifacts
<
https://console.developers.google.com/storage/browser/kubernetes-jenkins/pr-logs/pull/20626/kubernetes-pull-build-test-e2e-gce/26852/artifacts/

Internal Jenkins Results
<
http://goto.google.com/prkubekins/job/kubernetes-pull-build-test-e2e-gce//26852

—
Reply to this email directly or view it on GitHub
<
#20626 (comment)

.

—
Reply to this email directly or view it on GitHub
#20626 (comment)
.

smarterclayton · 2016-02-05T02:21:40Z

I'm ok with the map, I don't know how humans will understand it (it's awful
clever). But I'd rather have this than something not well thought through.

On Thu, Feb 4, 2016 at 8:03 PM, Daniel Smith notifications@github.com
wrote:

I believe CIDRs are still things for IPv6, so it should. Also this would
let us provide IPv6 and IPv4 addresses, which could be handy, though that
wasn't what we were thinking.

On Thu, Feb 4, 2016 at 4:49 PM, Clayton Coleman notifications@github.com
wrote:

Will this map to IPv6 cleanly?

On Thu, Feb 4, 2016 at 7:49 PM, Clayton Coleman ccoleman@redhat.com
wrote:

Wow.... that's... interesting. It uh... is certainly flexible.... :)

On Thu, Feb 4, 2016 at 7:43 PM, Kubernetes Bot <
notifications@github.com

wrote:

GCE e2e test build/test passed for commit 56557fb
<

56557fb

.

Build Log
<

https://storage.cloud.google.com/kubernetes-jenkins/pr-logs/pull/20626/kubernetes-pull-build-test-e2e-gce/26852/build-log.txt

Test Artifacts
<

https://console.developers.google.com/storage/browser/kubernetes-jenkins/pr-logs/pull/20626/kubernetes-pull-build-test-e2e-gce/26852/artifacts/

Internal Jenkins Results
<

http://goto.google.com/prkubekins/job/kubernetes-pull-build-test-e2e-gce//26852

—
Reply to this email directly or view it on GitHub
<

#20626 (comment)

.

—
Reply to this email directly or view it on GitHub
<
#20626 (comment)

.

—
Reply to this email directly or view it on GitHub
#20626 (comment)
.

nikhiljindal · 2016-02-05T06:15:58Z

Updated to use ServiceReadWritePort instead of ReadWritePort which switched the address to use 443 instead of 6443 port.
Also pushed a new version as per comments.

k8s-bot · 2016-02-05T06:56:41Z

GCE e2e build/test failed for commit f178c1631a9f26d55a7f98fcbbce3fb25b59a289.

lavalamp · 2016-02-16T18:01:41Z

pkg/util/net/http.go

+	hdrRealIp := hdr.Get("X-Real-Ip")
+	if hdrRealIp != "" {
+		ip := net.ParseIP(hdrRealIp)
+		return ip


If ip is nil here, I think you should keep going also.

nikhiljindal · 2016-02-18T07:49:04Z

Renamed ServerAddressByClientCIDRMap to ServerAddressByClientCIDRs and pushed updated code as per comments.
PTAL

k8s-bot · 2016-02-18T07:51:56Z

GCE e2e test build/test passed for commit 641d34ca3647ccbb6b753dda06af3270fbc99d30.

k8s-bot · 2016-02-18T07:52:40Z

GCE e2e build/test failed for commit c1c0dcc7bd815e064fd8d29c03b74c489c98e3cb.

k8s-bot · 2016-02-18T08:17:37Z

GCE e2e test build/test passed for commit 00b1fd9de3217a74c3c7a44e4dda168e13d2363c.

k8s-github-robot · 2016-02-18T14:54:23Z

PR needs rebase

lavalamp · 2016-02-18T18:20:21Z

LGTM

nikhiljindal · 2016-02-18T20:02:09Z

Rebased. Readding the LGTM tag

k8s-bot · 2016-02-18T20:34:30Z

GCE e2e test build/test passed for commit 20ce4ae.

k8s-github-robot · 2016-02-19T05:23:18Z

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

k8s-bot · 2016-02-19T05:53:15Z

GCE e2e test build/test passed for commit 20ce4ae.

k8s-github-robot · 2016-02-19T05:53:23Z

Automatic merge from submit-queue

Auto commit by PR queue bot

nikhiljindal · 2016-03-11T18:18:42Z

Added the release note label

@ncdc

Automatic merge from submit-queue Fixing logic to generate ExternalHost in genericapiserver @ncdc pointed it out (https://kubernetes.slack.com/archives/sig-api-machinery/p1464974528000139) that lines 305 and 306 dont match. We should be using ReadWritePort instead of ServiceReadWritePort in line 306. #20626 seems to be the culprit PR.

googlebot added the cla: yes label Feb 4, 2016

k8s-github-robot assigned bgrant0607 Feb 4, 2016

k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 4, 2016

nikhiljindal force-pushed the serverIPInDiscovery branch from 64b6b8c to 23093cc Compare February 4, 2016 19:31

k8s-github-robot removed the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Feb 4, 2016

k8s-github-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 4, 2016

bgrant0607 reviewed Feb 4, 2016
View reviewed changes

lavalamp reviewed Feb 5, 2016
View reviewed changes

nikhiljindal force-pushed the serverIPInDiscovery branch from 56557fb to f178c16 Compare February 5, 2016 06:12

lavalamp reviewed Feb 16, 2016
View reviewed changes

nikhiljindal force-pushed the serverIPInDiscovery branch 3 times, most recently from 641d34c to 00b1fd9 Compare February 18, 2016 07:48

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

lavalamp added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 18, 2016

Adding hostname to groups discovery information

20ce4ae

nikhiljindal force-pushed the serverIPInDiscovery branch from 00b1fd9 to 20ce4ae Compare February 18, 2016 19:58

nikhiljindal added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 18, 2016

k8s-github-robot pushed a commit that referenced this pull request Feb 19, 2016

Merge pull request #20626 from nikhiljindal/serverIPInDiscovery

5d7d0ee

Auto commit by PR queue bot

k8s-github-robot merged commit 5d7d0ee into kubernetes:master Feb 19, 2016

deads2k mentioned this pull request Feb 19, 2016

fix failing travis #21539

Merged

nikhiljindal mentioned this pull request Feb 19, 2016

Update swagger specs #21545

Merged

nikhiljindal added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Mar 11, 2016

nikhiljindal mentioned this pull request Jun 3, 2016

Fixing logic to generate ExternalHost in genericapiserver #26796

Merged

Adding hostname to groups discovery information #20626

Adding hostname to groups discovery information #20626

Conversation

nikhiljindal commented Feb 4, 2016

k8s-github-robot commented Feb 4, 2016

k8s-bot commented Feb 4, 2016

k8s-bot commented Feb 4, 2016

lavalamp commented Feb 4, 2016

k8s-github-robot commented Feb 4, 2016

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

Choose a reason for hiding this comment

nikhiljindal commented Feb 4, 2016

smarterclayton commented Feb 4, 2016

nikhiljindal commented Feb 4, 2016

nikhiljindal commented Feb 5, 2016

k8s-bot commented Feb 5, 2016

smarterclayton commented Feb 5, 2016

smarterclayton commented Feb 5, 2016

lavalamp commented Feb 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavalamp commented Feb 5, 2016

smarterclayton commented Feb 5, 2016

nikhiljindal commented Feb 5, 2016

k8s-bot commented Feb 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikhiljindal commented Feb 18, 2016

k8s-bot commented Feb 18, 2016

k8s-bot commented Feb 18, 2016

k8s-bot commented Feb 18, 2016

k8s-github-robot commented Feb 18, 2016

lavalamp commented Feb 18, 2016

nikhiljindal commented Feb 18, 2016

k8s-bot commented Feb 18, 2016

k8s-github-robot commented Feb 19, 2016

k8s-bot commented Feb 19, 2016

k8s-github-robot commented Feb 19, 2016

nikhiljindal commented Mar 11, 2016