Loadbalanced client src ip preservation enters beta #33957

bprashanth · 2016-10-03T18:57:34Z

Sounds like we're going to try out the proposal (#30819 (comment)) for annotations -> fields on just one feature in 1.5 (scheduler). Or do we want to just convert to fields right now?

This change is

thockin · 2016-10-03T20:13:44Z

Please re-title. "ESIPP" has almost no meaning outside our little crew in the office :)

thockin · 2016-10-03T20:14:42Z

hack/lib/util.sh

@@ -445,7 +445,7 @@ kube::util::fetch-openapi-spec() {
 # repo, e.g. "upstream" or "origin".
 kube::util::git_upstream_remote_name() {
  git remote -v | grep fetch |\
-    grep -E 'github.com[/:]kubernetes/kubernetes|k8s.io/kubernetes' |\
+    grep -E 'github.com[/:]kubernetes/kubernetes|k8s.io/kubernetes|GoogleCloudPlatform/kubernetes' |\


really? Isn't it time to change your remote? I don't think we should do this

thockin

LG overall - just concerned about breaking on existing objects

thockin · 2016-10-03T20:19:41Z

pkg/api/v1/conversion.go

+		in.Annotations[apiservice.AlphaAnnotationExternalTraffic] = valueBeta
+	}
+
+	return autoConvert_api_Service_To_v1_Service(in, out, s)


so if a user writes alpha, they will get both alpha and beta back out. I guess that is OK - just for simplicity?

in case they rollback

I rescind my OK. Consider this:

User writes alpha

You upconvert to beta

User reads it back in newer client and changes the beta config, writes back

Which do we use? alpha or beta? They should be mutually exclusive.

Clarify?

Going from v1 -> internal type, alpha clobbers beta. This will happen through kubectl etc.

Going from internal type -> v1, beta clobbers, because our internal system components do this (so assignment of healthcheck-nodeport shouldn't get clobbered by emptystring, for example).

We store in etcd as v1. On bootup apiserver reads and converts.

User reads it back in newer client and changes the beta config, writes back

So this is going from v1->internal? that will just overwrite beta with alpha.
If we end up with 2 versions, alpha wins, because presumably alpha is holding some resources from before the upgrade.

thockin · 2016-10-03T20:31:12Z

pkg/registry/core/service/rest.go

@@ -161,10 +161,10 @@ func (rs *REST) Create(ctx api.Context, obj runtime.Object) (runtime.Object, err
 	if shouldCheckOrAssignHealthCheckNodePort(service) {
 		var healthCheckNodePort int
 		var err error
-		if l, ok := service.Annotations[apiservice.AnnotationHealthCheckNodePort]; ok {
+		if l, ok := service.Annotations[apiservice.BetaAnnotationHealthCheckNodePort]; ok {


what about existing objects which have alpha annotation and not beta?

I'm copying it over during conversion, so when the apiserver/controller is restarted and gets objects down watch/list whatever they'll get substituted in when they translate to the internal type

bprashanth · 2016-10-03T23:33:01Z

@k8s-bot unit test this github issue: #33880

bprashanth · 2016-10-03T23:45:03Z

+ export PROJECT=k8s-jkns-pr-gci-gce
+ PROJECT=k8s-jkns-pr-gci-gce
�[0;33mAttempt 5 failed to create firewall rule e2e-gce-agent-pr-32-0-minion-all. Retrying.�[0m
ERROR: (gcloud.compute.firewall-rules.create) Some requests did not succeed:
 - Quota 'FIREWALLS' exceeded. Limit: 200.0

firewalls quota seems to now be available though, so kicking this

bprashanth · 2016-10-03T23:45:12Z

@k8s-bot gce test this

bprashanth · 2016-10-03T23:46:16Z

@k8s-bot gci gce e2e test this

bprashanth · 2016-10-04T00:19:49Z

Still firewall quota (#33957 (comment)), @kubernetes/goog-testing delete it?

bprashanth · 2016-10-04T00:20:58Z

@k8s-bot unit test this

ixdy · 2016-10-04T01:24:56Z

@bprashanth project is cleaned up now.

@k8s-bot gci gce e2e test this

k8s-ci-robot · 2016-10-05T17:54:11Z

Jenkins verification failed for commit 2726c2c46cce135d7bce368e7b9fa57d98eee1f0. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify 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-10-05T18:20:14Z

Jenkins Kubemark GCE e2e failed for commit 2726c2c46cce135d7bce368e7b9fa57d98eee1f0. Full PR test history.

The magic incantation to run this job again is @k8s-bot kubemark 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-10-19T07:23:25Z

ping me when ready

k8s-ci-robot · 2016-10-19T18:57:29Z

Jenkins unit/integration failed for commit c0492294ec7826c12ece0191ea8627bf9fc826b7. 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-10-19T19:24:30Z

Jenkins GCE e2e failed for commit c0492294ec7826c12ece0191ea8627bf9fc826b7. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gce e2e 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-10-19T19:29:31Z

Jenkins GCE etcd3 e2e failed for commit c0492294ec7826c12ece0191ea8627bf9fc826b7. Full PR test history.

The magic incantation to run this job again is @k8s-bot gce etcd3 e2e 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-10-19T19:30:01Z

Jenkins GCI GCE e2e failed for commit c0492294ec7826c12ece0191ea8627bf9fc826b7. 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.

k8s-ci-robot · 2016-10-19T22:07:26Z

Jenkins GKE smoke e2e failed for commit 5029bb0. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gke e2e 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-10-19T22:07:26Z

Jenkins GCI GKE smoke e2e failed for commit 5029bb0. Full PR test history.

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

bprashanth · 2016-10-20T00:51:14Z

Oh yeah, the validation was kind of a pain because I wanted to lock down the pair of alpha annotations but this should be good to go. Failures are flakes @thockin

thockin · 2016-10-20T05:03:22Z

Reviewed 1 of 9 files at r6, 2 of 8 files at r11, 4 of 5 files at r12, 1 of 1 files at r14, 4 of 4 files at r15.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

pkg/api/validation/validation_test.go, line 5211 at r15 (raw file):

      },
      {
          name: "LoadBalancer disallows onlyLocal alpha annotations",

test that it allows beta?

pkg/api/validation/validation_test.go, line 6485 at r15 (raw file):

      },
      {
          name: "Service disallows removing one onlyLocal alpha annotation",

Is it important to disallow the disabling of the alpha feature? I think I get the rest, but why this?

Comments from Reviewable

bprashanth · 2016-10-20T05:46:50Z

pkg/api/validation/validation_test.go, line 6485 at r15 (raw file):

Previously, thockin (Tim Hockin) wrote…

Is it important to disallow the disabling of the alpha feature? I think I get the rest, but why this?

in general I want to lock down the alpha, no updates, creates or deletes because one can't tell how behavior changed in beta.

In this specific case, we disallow deletion of healthCheckNodePort even in beta. The only way to release that is by deleting/flipping the onlyLocal annotation, at which point the system releases the port on your behalf. If we allowed deleting onlyLocal, the user can delete alpha.onlyLocal and add beta.onlyLocal and the apiserver code either needs to copy the healthCheckNodePort automatically from alpha to beta, or release the old one and allocate the new one. Messy. So overall just felt easier to force an update to beta before anything.

Comments from Reviewable

bprashanth · 2016-10-20T05:47:47Z

pkg/api/validation/validation_test.go, line 5211 at r15 (raw file):

Previously, thockin (Tim Hockin) wrote…

test that it allows beta?

figured e2es would break anyway, but it's easy enough to add another unittest

Comments from Reviewable

thockin · 2016-10-20T06:11:22Z

LGTM

On Wed, Oct 19, 2016 at 10:48 PM, Prashanth B notifications@github.com
wrote:

pkg/api/validation/validation_test.go, line 5211 at r15
https://reviewable.kubernetes.io:443/reviews/kubernetes/kubernetes/33957#-KUVRBFILtkuJUYzccUK:-KUVcG-tDllh7uMV2v8F:bariowu
(raw file

kubernetes/pkg/api/validation/validation_test.go

Line 5211 in 5029bb0

name: "LoadBalancer disallows onlyLocal alpha annotations",

):
Previously, thockin (Tim Hockin) wrote…

test that it allows beta?

figured e2es would break anyway, but it's easy enough to add another
unittest

Comments from Reviewable
https://reviewable.kubernetes.io:443/reviews/kubernetes/kubernetes/33957

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

k8s-github-robot · 2016-10-20T13:10:46Z

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

k8s-github-robot · 2016-10-20T13:53:07Z

Automatic merge from submit-queue

bprashanth assigned freehan and thockin Oct 3, 2016

googlebot added the cla: yes label Oct 3, 2016

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

thockin reviewed Oct 3, 2016

View reviewed changes

bprashanth changed the title ~~Esipp to beta~~ Annotations preserving lb source ip graduate to beta Oct 3, 2016

bprashanth force-pushed the esipp-beta branch from 551e920 to 9a0fc1f Compare October 3, 2016 23:06

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

bprashanth force-pushed the esipp-beta branch from 9a0fc1f to 2726c2c Compare October 5, 2016 17:46

k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 5, 2016

bprashanth mentioned this pull request Oct 5, 2016

[k8s.io] Services should be able to create services of type LoadBalancer and externalTraffic=localOnly [Slow][Feature:ExternalTrafficLocalOnly] {Kubernetes e2e suite} #34124

Closed

bprashanth force-pushed the esipp-beta branch from 2726c2c to fcad818 Compare October 6, 2016 21:11

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 6, 2016

bprashanth added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Oct 6, 2016

bprashanth changed the title ~~Annotations preserving lb source ip graduate to beta~~ Loadbalanced client src ip preservation enters beta Oct 6, 2016

bprashanth force-pushed the esipp-beta branch from f32868f to e861be7 Compare October 11, 2016 23:51

bprashanth force-pushed the esipp-beta branch from e861be7 to 25c00b6 Compare October 19, 2016 05:45

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

bprashanth force-pushed the esipp-beta branch from 25c00b6 to c049229 Compare October 19, 2016 18:42

bprashanth added 5 commits October 19, 2016 13:39

Fix health check node port leak

5cb8e8e

Promote source ip annotations to beta

a46a849

E2e tests

eb235a8

Flip the beta flag

243bd57

Validation

5029bb0

bprashanth force-pushed the esipp-beta branch from c049229 to 5029bb0 Compare October 19, 2016 21:52

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

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

k8s-github-robot merged commit e6b5b07 into kubernetes:master Oct 20, 2016

spxtr mentioned this pull request Oct 20, 2016

[k8s.io] ESIPP [Slow][Feature:ExternalTrafficLocalOnly] should handle updates to source ip annotation [Slow][Feature:ExternalTrafficLocalOnly] {Kubernetes e2e suite} #35225

Closed

MrHohn mentioned this pull request Feb 22, 2017

Promotes Source IP preservation for Virtual IPs from Beta to GA #41162

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Loadbalanced client src ip preservation enters beta #33957

Loadbalanced client src ip preservation enters beta #33957

bprashanth commented Oct 3, 2016 •

edited by k8s-oncall

Loading

thockin commented Oct 3, 2016

thockin Oct 3, 2016

thockin left a comment

thockin Oct 3, 2016

bprashanth Oct 3, 2016

thockin Oct 11, 2016

bprashanth Oct 12, 2016

thockin Oct 3, 2016

bprashanth Oct 3, 2016

bprashanth commented Oct 3, 2016

bprashanth commented Oct 3, 2016

bprashanth commented Oct 3, 2016

bprashanth commented Oct 3, 2016

bprashanth commented Oct 4, 2016

bprashanth commented Oct 4, 2016

ixdy commented Oct 4, 2016

k8s-ci-robot commented Oct 5, 2016

k8s-ci-robot commented Oct 5, 2016

thockin commented Oct 19, 2016

k8s-ci-robot commented Oct 19, 2016

k8s-ci-robot commented Oct 19, 2016

k8s-ci-robot commented Oct 19, 2016

k8s-ci-robot commented Oct 19, 2016

k8s-ci-robot commented Oct 19, 2016

k8s-ci-robot commented Oct 19, 2016

bprashanth commented Oct 20, 2016

thockin commented Oct 20, 2016

bprashanth commented Oct 20, 2016

bprashanth commented Oct 20, 2016

thockin commented Oct 20, 2016

k8s-github-robot commented Oct 20, 2016

k8s-github-robot commented Oct 20, 2016

Loadbalanced client src ip preservation enters beta #33957

Loadbalanced client src ip preservation enters beta #33957

Conversation

bprashanth commented Oct 3, 2016 • edited by k8s-oncall Loading

thockin commented Oct 3, 2016

thockin Oct 3, 2016

Choose a reason for hiding this comment

thockin left a comment

Choose a reason for hiding this comment

thockin Oct 3, 2016

Choose a reason for hiding this comment

bprashanth Oct 3, 2016

Choose a reason for hiding this comment

thockin Oct 11, 2016

Choose a reason for hiding this comment

bprashanth Oct 12, 2016

Choose a reason for hiding this comment

thockin Oct 3, 2016

Choose a reason for hiding this comment

bprashanth Oct 3, 2016

Choose a reason for hiding this comment

bprashanth commented Oct 3, 2016

bprashanth commented Oct 3, 2016

bprashanth commented Oct 3, 2016

bprashanth commented Oct 3, 2016

bprashanth commented Oct 4, 2016

bprashanth commented Oct 4, 2016

ixdy commented Oct 4, 2016

k8s-ci-robot commented Oct 5, 2016

k8s-ci-robot commented Oct 5, 2016

thockin commented Oct 19, 2016

k8s-ci-robot commented Oct 19, 2016

k8s-ci-robot commented Oct 19, 2016

k8s-ci-robot commented Oct 19, 2016

k8s-ci-robot commented Oct 19, 2016

k8s-ci-robot commented Oct 19, 2016

k8s-ci-robot commented Oct 19, 2016

bprashanth commented Oct 20, 2016

thockin commented Oct 20, 2016

bprashanth commented Oct 20, 2016

bprashanth commented Oct 20, 2016

thockin commented Oct 20, 2016

k8s-github-robot commented Oct 20, 2016

k8s-github-robot commented Oct 20, 2016

bprashanth commented Oct 3, 2016 •

edited by k8s-oncall

Loading