r/service: Add `external_traffic_policy` to spec #59

phamann · 2017-08-25T10:49:44Z

This adds the externalTrafficPolicy property to the spec schema on the Service resource.

By default k8s obscures the source IP from packets replacing it with the node IP. However certain applications require source IP preservation. This behaviour is managed via the externalTrafficPolicy. More info here: https://kubernetes.io/docs/tutorials/services/source-ip/

Until recently we got around the lack of this property by using an internal k8s annotation to apply the behaviour using service.metadata.annotations[0]. However, #50 (and the 1.0.0 release) broke this by treating internal annotations as invalid. Using the spec.externalTrafficPolicy is the preferred method anyway hence this PR.

sl1pm4t · 2017-08-27T21:17:05Z

kubernetes/structure_service_spec.go

@@ -136,6 +136,9 @@ func expandServiceSpec(l []interface{}) v1.ServiceSpec {
 	if v, ok := in["external_name"].(string); ok {
 		obj.ExternalName = v
 	}
+	if v, ok := in["external_traffic_policy"].(string); ok {
+		obj.ExternalName = v


Is this corrrect - obj.ExternalName ?
Shouldn't that be something like obj.ExternalTrafficPolicy ?

We'll also need to modify flattenServiceSpec similarly.

radeksimko

Hi @phamann
thanks for the contribution.

Except what @sl1pm4t and I mentioned we'll need to upgrade vendored K8S to 1.7 as this field was introduced in that version. I planned to do it anyway - just FYI that this is a blocker. I'll hopefully get to it this week.

After vendor upgrade is done you'd just rebase your PR and run acceptance tests to see if everything's working correctly.

radeksimko · 2017-08-29T14:15:17Z

kubernetes/structure_service_spec.go

@@ -136,6 +136,9 @@ func expandServiceSpec(l []interface{}) v1.ServiceSpec {
 	if v, ok := in["external_name"].(string); ok {
 		obj.ExternalName = v
 	}
+	if v, ok := in["external_traffic_policy"].(string); ok {
+		obj.ExternalName = v


We'll also need to modify flattenServiceSpec similarly.

phamann · 2017-08-30T10:46:22Z

Thanks for the review. I've now updated/added all of the correct references within flattenServiceSpec(), expandServiceSpec(), and patchServiceSpec() in 6bccc7c.

The build is now failing (correctly) due to the vendored K8S not having the the field. As @radeksimko suggested, I'll wait until the upgrade has been done and then re-run.

phamann · 2017-10-03T13:12:31Z

@radeksimko Any update on when you will upgrade the vendored K8S to 1.7? To resolve the issues with this PR?

sl1pm4t · 2017-10-29T19:59:03Z

@phamann I created a PR #80 that bumps the vendored k8s client code to v1.7.
Do you want to try rebase this PR on mine to see if the build is successful?

nicknovitski · 2018-02-22T22:55:23Z

Since #80 has been merged, it seems this is due to be rebased.

phamann · 2018-04-03T13:20:45Z

@radeksimko Apologies for neglecting this this, we had the provider version pinned as a temporary resolution to this problem internally. Needing some other features in the latest release reminded me of this PR.

I've now rebased with master and the tests are all passing 😄

phamann · 2018-06-21T10:38:12Z

Any update on this @radeksimko? It still has the waiting-response label even though I have rebased and fixed issues, which may be why you haven't reviewed again?

jhoblitt · 2018-06-26T21:17:16Z

I just smacked into this as well: Error: module.pkgroot.kubernetes_service.ssl_proxy: spec.0: invalid or unknown key: externalTrafficPolicy -- it would be great to see this feature merged.

codercr · 2019-01-14T12:31:16Z

This is good but should default to the Kubernetes default of Cluster. The user should set to Local when they choose not to use default behavior.

@radeksimko, what else is needed to get this in? This is a very important feature.

yacut · 2019-02-19T16:09:52Z

@phamann @radeksimko any update on this?

ulm0 · 2019-04-15T19:02:21Z

Hi, guys. any update on this? i need this field to deploy nginx ingress controller with terraform.

roidelapluie

LGTM

annaGo · 2019-04-30T08:01:47Z

I need this field as well, going to pass on terraform and use kubectl :(

markodkrstic · 2019-05-20T14:29:41Z

@radeksimko this is a crucial element to deploy nginx ingress controller with terraform as previously mentioned. is there any timeline on this?

zioproto · 2019-05-20T14:50:46Z

I rebased this PR on top of the origin master and I pushed the branch here:
https://github.com/zioproto/terraform-provider-kubernetes/tree/PR-59
@phamann could you check if the rebase is OK for you and push the changes to this PR ?
thanks

zioproto · 2019-05-20T14:55:29Z

@phamann if my rebase looks correct, to solve the git stuff, you could do like this:

git remote add zioproto https://github.com/zioproto/terraform-provider-kubernetes
git fetch --all
git checkout b-service-external-traffic-policy
git reset --hard zioproto/PR-59
git push -f origin b-service-external-traffic-policy

This will refresh the current PR59 with the rebased version I proposed.
Let me know if you are still interested in getting this merged, otherwise I will propose a new PR.
Thanks !! :)

alexsomesan · 2019-05-20T15:58:52Z

kubernetes/resource_kubernetes_service.go

+						"external_traffic_policy": {
+							Type:        schema.TypeString,
+							Description: "Denotes if this Service desires to route external traffic to node-local or cluster-wide endpoints. `Local` preserves the client source IP and avoids a second hop for LoadBalancer and Nodeport type services, but risks potentially imbalanced traffic spreading. `Cluster` obscures the client source IP and may cause a second hop to another node, but should have good overall load-spreading. More info: https://kubernetes.io/docs/tutorials/services/source-ip/",
+							Optional:    true,


There are two possible values for this field: "Local" and "Cluster".
It'd be nice to have validation in place to express this.

Suggested change

Optional: true,

Optional: true,

ValidateFunc: validation.StringInSlice([]string{"Local", "Cluster"}, false),

…icy'

alexsomesan

CI passes. LGTM.

regner · 2019-05-22T21:05:46Z

Thanks for your work on this everyone. Really looking forward to taking advantage of it.

sl1pm4t reviewed Aug 27, 2017

View reviewed changes

radeksimko changed the title ~~r/Service add external_traffic_policy to spec~~ r/service: Add external_traffic_policy to spec Aug 29, 2017

radeksimko added the enhancement label Aug 29, 2017

radeksimko suggested changes Aug 29, 2017

View reviewed changes

radeksimko added the waiting-response label Aug 29, 2017

radeksimko added upstream and removed waiting-response labels Oct 23, 2017

radeksimko added waiting-response and removed upstream labels Nov 13, 2017

radeksimko added the kubernetes/1.7 label Mar 23, 2018

phamann force-pushed the b-service-external-traffic-policy branch 3 times, most recently from d6aedbf to f169008 Compare April 3, 2018 13:16

codercr mentioned this pull request Jan 14, 2019

kubernetes_service externalTrafficPolicy support #273

Closed

ghost removed the waiting-response label Jan 14, 2019

regner mentioned this pull request Feb 25, 2019

Replace shellout to kubectl with kubernetes_stateful_set sethvargo/vault-on-gke#19

Closed

roidelapluie approved these changes Apr 30, 2019

View reviewed changes

alexsomesan reviewed May 20, 2019

View reviewed changes

ghost added size/XS documentation labels May 20, 2019

phamann and others added 3 commits May 20, 2019 18:36

r/Service add external_traffic_policy to spec

01ba478

Update/add references to ExternalTrafficPolicy in structure_service_spec

222a114

Fix formatting

df911a9

alexsomesan force-pushed the b-service-external-traffic-policy branch from 2ef8764 to df911a9 Compare May 20, 2019 16:40

alexsomesan added 5 commits May 20, 2019 18:59

remove duplicate code from merge conflict resolution

3d77518

Add validation to 'external_traffic_policy'

d230f87

Set default for 'external_traffic_policy'

8725043

Remove default and make 'external_traffic_policy' computed.

01d5bd1

Add 'external_traffic_policy' to the service datasource.

3b4d1f3

ghost added size/S and removed size/XS labels May 22, 2019

alexsomesan added 2 commits May 22, 2019 21:30

Add 'external_traffic_policy' to the kubernete_service datasource docs.

c7483d2

Adapt TestAccKubernetesService_loadBalancer for 'external_traffic_pol…

8238910

…icy'

alexsomesan approved these changes May 22, 2019

View reviewed changes

alexsomesan merged commit 5e11ec4 into hashicorp:master May 22, 2019

ghost locked and limited conversation to collaborators Apr 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

r/service: Add `external_traffic_policy` to spec #59

r/service: Add `external_traffic_policy` to spec #59

phamann commented Aug 25, 2017

sl1pm4t Aug 27, 2017

radeksimko Aug 29, 2017

radeksimko left a comment

radeksimko Aug 29, 2017

phamann commented Aug 30, 2017

phamann commented Oct 3, 2017

sl1pm4t commented Oct 29, 2017

nicknovitski commented Feb 22, 2018

phamann commented Apr 3, 2018

phamann commented Jun 21, 2018

jhoblitt commented Jun 26, 2018

codercr commented Jan 14, 2019 •

edited

Loading

yacut commented Feb 19, 2019

ulm0 commented Apr 15, 2019

roidelapluie left a comment

annaGo commented Apr 30, 2019

markodkrstic commented May 20, 2019

zioproto commented May 20, 2019

zioproto commented May 20, 2019

alexsomesan May 20, 2019

alexsomesan left a comment

regner commented May 22, 2019

	Optional: true,
	Optional: true,
	ValidateFunc: validation.StringInSlice([]string{"Local", "Cluster"}, false),

r/service: Add external_traffic_policy to spec #59

r/service: Add external_traffic_policy to spec #59

Conversation

phamann commented Aug 25, 2017

sl1pm4t Aug 27, 2017

Choose a reason for hiding this comment

radeksimko Aug 29, 2017

Choose a reason for hiding this comment

radeksimko left a comment

Choose a reason for hiding this comment

radeksimko Aug 29, 2017

Choose a reason for hiding this comment

phamann commented Aug 30, 2017

phamann commented Oct 3, 2017

sl1pm4t commented Oct 29, 2017

nicknovitski commented Feb 22, 2018

phamann commented Apr 3, 2018

phamann commented Jun 21, 2018

jhoblitt commented Jun 26, 2018

codercr commented Jan 14, 2019 • edited Loading

yacut commented Feb 19, 2019

ulm0 commented Apr 15, 2019

roidelapluie left a comment

Choose a reason for hiding this comment

annaGo commented Apr 30, 2019

markodkrstic commented May 20, 2019

zioproto commented May 20, 2019

zioproto commented May 20, 2019

alexsomesan May 20, 2019

Choose a reason for hiding this comment

alexsomesan left a comment

Choose a reason for hiding this comment

regner commented May 22, 2019

r/service: Add `external_traffic_policy` to spec #59

r/service: Add `external_traffic_policy` to spec #59

codercr commented Jan 14, 2019 •

edited

Loading