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

MrHohn · 2017-02-08T22:15:05Z

Fixes #33625. Feature issue: kubernetes/enhancements#27.

Bullet points:

Declare 2 fields (ExternalTraffic and HealthCheckNodePort) that mirror the ESIPP annotations.
ESIPP alpha annotations will be ignored.
Existing ESIPP beta annotations will still be fully supported.
Allow promoting beta annotations to first class fields or reversely.
Disallow setting invalid ExternalTraffic and HealthCheckNodePort on services. Default ExternalTraffic field for nodePort or loadBalancer type service to "Global" if not set.

Release note:

Promotes Source IP preservation for Virtual IPs to GA.

Two api fields are defined correspondingly:
- Service.Spec.ExternalTrafficPolicy <- 'service.beta.kubernetes.io/external-traffic' annotation.
- Service.Spec.HealthCheckNodePort <- 'service.beta.kubernetes.io/healthcheck-nodeport' annotation.

k8s-reviewable · 2017-02-08T22:15:14Z

This change is

MrHohn · 2017-02-09T04:02:38Z

@bprashanth @thockin

thockin · 2017-02-21T22:37:38Z

pkg/api/service/annotations.go

-	AnnotationValueExternalTrafficGlobal = "Global"
-
-	// TODO: The alpha annotations have been deprecated, remove them when we move this feature to GA.
+	// TODO: The beta annotations have been deprecated, remove them when we release k8s 1.7.


You'll have to send a notice to kubernetes-announce, too

Sure, made a note for this.

thockin · 2017-02-21T22:42:57Z

pkg/api/service/util.go

+	// If they transition to the first class field, there's no way to go back to beta without
+	// rolling back the cluster.
+	if l, ok := service.Annotations[BetaAnnotationExternalTraffic]; ok {
+		if l == string(api.ServiceExternalTrafficTypeOnlyLocal) {


This would read better as a switch

thockin · 2017-02-21T22:52:42Z

pkg/api/types.go

+	// local endpoints only. This preserves Source IP and avoids a second hop for
+	// LoadBalancer and Nodeport type services.
+	// +optional
+	ExternalTraffic ServiceExternalTrafficType


I'm not in love with the naming around this feature (never was). Can we think of anything more expressive?

Maybe externalTrafficPolicy: Local or something a little less awkward?

Sounds good, so the candidates would be:

externalTrafficPolicy: Local externalTrafficPolicy: Global

And do we want to use externalTrafficPolicy: None for the other service types (not NodePort or LoadBalancer)? This field is now being defaulted to empty string ("").

I guess "" is appropriate for those cases. Defaulting (and update) should detect LoadBalancer and NodePort and choose "Global", I think. Thoughts? If we do that then we really should change it back to "" if the type is updated to anything else.

Yeah I have the same thought.
function defaultExternalTrafficIfNeeded() exists for defaulting this field to Global for LoadBalancer and NodePort service if this field is not set.
And function externalTrafficUpdate() exists for changing this field back to "" when the type is updated to others (not LoadBalancer nor NodePort).

Changed to use externalTrafficPolicy: Local or Global.

thockin · 2017-02-21T22:56:21Z

pkg/api/validation/validation.go

+	// Validate ExternalTraffic and HealthCheckNodePort have legal value.
+	if service.Spec.HealthCheckNodePort < 0 {
+		allErrs = append(allErrs, field.Invalid(specPath.Child("healthCheckNodePort"), service.Spec.HealthCheckNodePort,
+			"can not set HealthCheckNodePort to negative value"))


error messages should be positive: "must be greater than 0"

thockin · 2017-02-21T22:56:54Z

pkg/api/validation/validation.go

+		allErrs = append(allErrs, field.Invalid(specPath.Child("healthCheckNodePort"), service.Spec.HealthCheckNodePort,
+			"can not set HealthCheckNodePort to negative value"))
+	}
+	if service.Spec.ExternalTraffic != "" &&


why is "" allowed? won't defaulting set it to a sane value?

Yeah I also think this is inappropriate. I'm changing the default value to None.

Updates...Per discussion above, "" might be allowed for service that does not have LoadBalancer or NodePort type.

thockin · 2017-02-21T22:58:06Z

pkg/api/validation/validation.go

+		service.Spec.Type != api.ServiceTypeNodePort &&
+		service.Spec.ExternalTraffic != "" {
+		allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "externalTraffic"), service.Spec.ExternalTraffic,
+			"can only be set on nodePort / loadBalancer service"))


NodePort and LoadBalancer

thockin · 2017-02-21T22:58:16Z

pkg/api/validation/validation.go

+		(service.Spec.Type != api.ServiceTypeLoadBalancer ||
+			service.Spec.ExternalTraffic != api.ServiceExternalTrafficTypeOnlyLocal) {
+		allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "healthCheckNodePort"), service.Spec.ExternalTraffic,
+			"can only be set on loadBalancer service with ExternalTraffic=OnlyLocal"))


LoadBalancer

thockin · 2017-02-22T01:21:43Z

pkg/api/validation/validation.go


-	_, healthCheckBetaOk := service.Annotations[apiservice.BetaAnnotationHealthCheckNodePort]
-	_, onlyLocalBetaOk := service.Annotations[apiservice.BetaAnnotationExternalTraffic]
+type serviceExternalTrafficAnnotationsUpdateStatus struct {


thockin · 2017-02-22T01:24:07Z

pkg/api/validation/validation.go

+	// to first class fields before making any modifications, even though the
+	// system continues to respect the beta version.
+
+	// On upgrading to a 1.6 cluster, the user is locked in at the current


I'm not sure I get this. Why do we need to stop users from creating new instances with the beta annotation - this is a breaking change, and I don't think we can allow it. Beta has to keep working for at least 1 release, and that means fully working.

I wanted to say up-conversion, but saw your old comment about it. Supporting beta annotations for one more release seems to be the right thing to do. I will re-think this part.

Yeah, we should not up-convert

thockin · 2017-02-22T05:09:53Z

@bprashanth - when this went Beta I raised an issue wrt healthchecks and NodePort. I can't find the comment - do you recall what the issue I raised was?

MrHohn · 2017-02-24T18:28:07Z

Per discussion above, updated to fully support both ESIPP Beta annotations and first class fields:

Allow legally create Beta annotations or first class fields.
Allow legally modify Beta annotations or first class fields.
Allow legally delete Beta annotations or reset first class fields.
Allow transition from Beta annotations to first class fields or reverse.
Disallow mixing Beta annotations with first class fields.

MrHohn · 2017-02-24T18:29:44Z

cc @freehan who might also be familiar with ESIPP validation :)

MrHohn · 2017-05-07T19:03:10Z

Rebased.

Note that most of the worth-looking-at changes are in the first two commits :)

MrHohn · 2017-05-09T22:24:54Z

/assign @freehan

Many thanks for helping out!

MrHohn · 2017-05-11T16:39:14Z

Rebased :)

@freehan Any changes to take a look?

MrHohn · 2017-05-11T17:21:09Z

@k8s-bot kops aws e2e test this, issue: #43236

freehan

Looks good overall. Some nits.

freehan · 2017-05-12T00:24:42Z

pkg/api/service/util.go

-				continue
-			}
-			return int32(p)
+	// First check the beta annotation and then the first class field. This is so


nit: s/This is so/so that

freehan · 2017-05-12T00:24:51Z

pkg/api/service/util.go

+
+// ClearExternalTrafficPolicy resets the ExternalTrafficPolicy field.
+func ClearExternalTrafficPolicy(service *api.Service) {
+	// First check the beta annotation and then the first class field. This is so existing


freehan · 2017-05-12T00:25:01Z

pkg/api/service/util.go

+// SetServiceHealthCheckNodePort sets the given health check node port on service.
+// It does not check whether this service needs healthCheckNodePort.
+func SetServiceHealthCheckNodePort(service *api.Service, hcNodePort int32) {
+	// First check the beta annotation and then the first class field. This is so existing


freehan · 2017-05-12T00:30:09Z

pkg/api/service/util.go

+// DefaultExternalTrafficPolicyIfNeeded defaults the ExternalTrafficPolicy field
+// for NodePort / LoadBalancer service to Global for consistency.
+// TODO: Move this default logic to default.go once beta annotation is deprecated.
+func DefaultExternalTrafficPolicyIfNeeded(service *api.Service) {


nit: s/DefaultExternalTrafficPolicyIfNeeded/SetDefaultExternalTrafficPolicyIfNeeded

freehan · 2017-05-12T00:33:01Z

pkg/api/v1/service/util.go

+// DefaultExternalTrafficPolicyIfNeeded defaults the ExternalTrafficPolicy field
+// for NodePort / LoadBalancer service to Global for consistency.
+// TODO: Move this default logic to default.go once beta annotation is deprecated.
+func DefaultExternalTrafficPolicyIfNeeded(service *v1.Service) {


nit: s/DefaultExternalTrafficPolicyIfNeeded/SetDefaultExternalTrafficPolicyIfNeeded

freehan · 2017-05-12T00:47:52Z

pkg/api/validation/validation.go

 // have legal value.
-func validateServiceExternalTrafficFields(service *api.Service) field.ErrorList {
+func validateServiceExternalTrafficFieldsValue(service *api.Service) field.ErrorList {


Not sure if it is relevant.
Is it allowed to use healthcheck node port with Global?
Is it allowed to mixa usage of beta annotation and fields? (I think annotaion is preempting, so we do not care? )

Is it allowed to use healthcheck node port with Global?

Nope, we disallow this case in ValidateServiceExternalTrafficFieldsCombination().

Is it allowed to mixa usage of beta annotation and fields? (I think annotaion is preempting, so we do not care? )

Nope, we disallow this case in validateServiceExternalTrafficAPIVersion().

Sorry for the mess, but I have to separate the validation into multiple pieces because they are called in different phases --- especially ValidateServiceExternalTrafficFieldsCombination() needs to be called after defaulting and resetting ESIPP fields :(

MrHohn

Thanks! WIll address the comments soon.

MrHohn · 2017-05-12T01:15:41Z

pkg/api/validation/validation.go

 // have legal value.
-func validateServiceExternalTrafficFields(service *api.Service) field.ErrorList {
+func validateServiceExternalTrafficFieldsValue(service *api.Service) field.ErrorList {


Is it allowed to use healthcheck node port with Global?

Nope, we disallow this case in ValidateServiceExternalTrafficFieldsCombination().

Is it allowed to mixa usage of beta annotation and fields? (I think annotaion is preempting, so we do not care? )

Nope, we disallow this case in validateServiceExternalTrafficAPIVersion().

Sorry for the mess, but I have to separate the validation into multiple pieces because they are called in different phases --- especially ValidateServiceExternalTrafficFieldsCombination() needs to be called after defaulting and resetting ESIPP fields :(

freehan · 2017-05-12T17:48:47Z

/lgtm

k8s-github-robot · 2017-05-12T17:48:56Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: MrHohn, freehan
We suggest the following additional approver: thockin

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

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

api/OWNERS
docs/OWNERS
federation/OWNERS
pkg/OWNERS
staging/src/k8s.io/client-go/pkg/api/OWNERS
test/OWNERS

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

MrHohn · 2017-05-12T17:52:08Z

Thanks! Squashing commits.

k8s-github-robot · 2017-05-12T22:00:45Z

Automatic merge from submit-queue (batch tested with PRs 45623, 45241, 45460, 41162)

@madhusudancs

Automatic merge from submit-queue [Federation] Fix federated service reconcilation issue due to addition of External… …TrafficPolicy field to v1.Service **What this PR does / why we need it**: New fields (ExternalTrafficPolicy) are introduced to v1.Service by this PR #41162. If this field is not specified in service spec, the service controller will assign default and updates the service spec. In federation, the service spec is not updated and we continuously try to reconcile as the federated service and the service in federated cluster do not match. **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes #45795 **Special notes for your reviewer**: **Release note**: ``` NONE ``` cc @kubernetes/sig-federation-bugs @madhusudancs

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

k8s-github-robot assigned rootfs Feb 8, 2017

k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels Feb 8, 2017

MrHohn force-pushed the esipp-ga branch 2 times, most recently from da4410b to bf27c47 Compare February 9, 2017 02:09

MrHohn force-pushed the esipp-ga branch from bf27c47 to f763b7f Compare February 9, 2017 17:36

bprashanth assigned thockin and bprashanth and unassigned rootfs Feb 9, 2017

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 Feb 9, 2017

bprashanth added this to the 1.6 milestone Feb 9, 2017

calebamiles modified the milestones: v1.6, 1.6 Feb 13, 2017

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

thockin reviewed Feb 22, 2017

View reviewed changes

MrHohn force-pushed the esipp-ga branch 2 times, most recently from 5d037ff to 7825dcf Compare February 24, 2017 18:02

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

MrHohn force-pushed the esipp-ga branch from 7825dcf to 538d845 Compare February 24, 2017 19:39

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

MrHohn force-pushed the esipp-ga branch from 538d845 to b9e27ac Compare February 27, 2017 18:37

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

k8s-ci-robot assigned freehan May 9, 2017

MrHohn force-pushed the esipp-ga branch from 6ca13a1 to 12a2a2a Compare May 11, 2017 16:38

freehan reviewed May 12, 2017

View reviewed changes

MrHohn commented May 12, 2017

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 12, 2017

MrHohn force-pushed the esipp-ga branch from d8b0931 to 686d1f6 Compare May 12, 2017 17:58

MrHohn added 5 commits May 12, 2017 10:58

Define ESIPP GA fields and deprecate alpha annotations

73fa1dd

Adjust ESIPP validation and service registry codes

79ca717

Update ESIPP validation and service registry unit tests

12277a0

Change to use ESIPP first class fields and update comments

7ed716a

Autogenerated files

12b6c2b

MrHohn force-pushed the esipp-ga branch from 686d1f6 to 12b6c2b Compare May 12, 2017 17:59

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 12, 2017

freehan added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 12, 2017

k8s-github-robot merged commit 35eba22 into kubernetes:master May 12, 2017

shashidharatd mentioned this pull request May 15, 2017

[Federation] Fix federated service reconcilation issue due to addition of External… #45798

Merged

MrHohn mentioned this pull request May 15, 2017

Remove ExternalTrafficLocalOnly from kube feature gate #45857

Closed

MrHohn deleted the esipp-ga branch May 16, 2017 23:56

MrHohn mentioned this pull request May 27, 2017

AWS: support node port health check #43585

Merged

MrHohn mentioned this pull request May 21, 2018

Re-add support for the deprecated ESIPP beta annotations for v1.8 #64124

Closed

aojea mentioned this pull request Jan 21, 2022

KEP-3015: PreferLocal traffic policy kubernetes/enhancements#3016

Closed

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

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

Conversation

MrHohn commented Feb 8, 2017 • edited Loading

k8s-reviewable commented Feb 8, 2017

MrHohn commented Feb 9, 2017

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented Feb 22, 2017

MrHohn commented Feb 24, 2017 • edited Loading

MrHohn commented Feb 24, 2017

MrHohn commented May 7, 2017

MrHohn commented May 9, 2017

MrHohn commented May 11, 2017

MrHohn commented May 11, 2017

freehan left a 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

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

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

MrHohn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freehan commented May 12, 2017

k8s-github-robot commented May 12, 2017

MrHohn commented May 12, 2017

k8s-github-robot commented May 12, 2017

MrHohn commented Feb 8, 2017 •

edited

Loading

MrHohn commented Feb 24, 2017 •

edited

Loading