[jjo] support also using status.loadBalancer.ingress IPs via annotation #339

jjo · 2018-03-13T19:14:03Z

No description provided.

SEJeff · 2018-03-15T15:59:49Z

👍 nicely done! Tests would be nice though

jjo · 2018-03-16T13:44:42Z

nicely done! Tests would be nice though
@SEJeff: tnx! , added tests at f30795d ;)

murali-reddy · 2018-03-21T20:00:39Z

@jjo Can you please rebase. Apologies for delay in the review.

Wondering do we need an annotation? If the load balancer IP is set would it make sense to interpret it just as external IP.

jjo · 2018-03-21T20:49:19Z

@jjo Can you please rebase. Apologies for delay in the review.

nw, just done it.

Wondering do we need an annotation? If the load balancer IP is set would it make sense to interpret it just as external IP.

Afaics the (only?) use case for landing LB IPs locally at nodes would
be indeed baremetal loadbalancing, as cloud-y LBs will obviously
handle those public IPs themselves, that's why I chose to keep the
current behavior unless annotated.
One clear consequence of above would be that a kube-router deploy
on cloud would start advertising those LB IPs towards k8s nodes,
possibly interfering with cloud provider net infra.

* support also using status.loadBalancer.ingress IPs via kube-router.io/service.uselbips: "true" annotation * this annotation is set for a LoadBalancer service, the ingress IP(s) set by the LoadBalancer will: - be locally added to nodes (to `kube-dummy-if` network iface, to LVS) - be advertised to BGP peers

…Ps() to reuse essentially same pre-existing code

murali-reddy · 2018-03-22T12:25:49Z

Afaics the (only?) use case for landing LB IPs locally at nodes would
be indeed baremetal loadbalancing, as cloud-y LBs will obviously
handle those public IPs themselves, that's why I chose to keep the
current behavior unless annotated.
One clear consequence of above would be that a kube-router deploy
on cloud would start advertising those LB IPs towards k8s nodes,
possibly interfering with cloud provider net infra.

So i see two different aspects here. One aspect is setting up service proxy to the LoadBalancer IP as VIP and setting up its endpoints. Second is how the traffic really ends up at a node for load-balncer IP.

In baremetal deployments each node (through kube-router) can advertises the loadbalancer IP so we have ECMP load balancing. Kube-router already does this with --advertise-cluster-ip and --advertise-external-ip. IMO we could have similar flag for load balancer IP's.

I have to test it though, with your changes to network service controller, service of LoadBalancer type should work in cloud provider environments.

I am not sure what benefit we get enabling selectivley few services with annotation.

SEJeff · 2018-03-23T03:27:30Z

Concur. Perhaps flip it to have an annotation to turn it off (if a user is worried about it interfering with cloud provider networking), or as a kube-router flag. This has the benefit of being a massively better user experience for both on premise clusters (like mine) and cloud clusters running kube-router.

jjo · 2018-03-23T11:39:55Z

Thanks @murali-reddy @SEJeff for the feedback, I still think the user
should be able to selectively dis/en-able which LB services to treat "as"
externalIPs, concur on having for a more useful default behavior,
I like @SEJeff take on having sth like --advertise-loadbalancer-ip plus
per-service annotation to disable it, I'll implement it.

murali-reddy · 2018-03-23T11:55:49Z

Thanks @jjo for your patience :) i will test it out today this functionality with ELB to see what would be ideal default behaviour.

andrewsykim · 2018-03-23T16:46:28Z

app/controllers/network_routes_controller.go

+
+				if !nodeHasEndpoints {
+					for _, externalIP := range svc.Spec.ExternalIPs {
+						err := nrc.UnadvertiseClusterIp(externalIP)


I don't think it makes sense to call UnadvertiseClusterIp in getExternalIps(). WDTY?

andrewsykim · 2018-03-23T16:47:48Z

app/controllers/network_services_controller.go

+		if svc.uselbips {
+			extIPSet = extIPSet.Union(sets.NewString(svc.loadBalancerIPs...))
+		}
+		glog.V(2).Infof("Service \"%s\" using extIPSet: %v", svc.name, extIPSet.List())


nit: you can use %q if you want quotes on your strings

…sToAdvertise

SEJeff · 2018-03-24T18:34:36Z

Documentation/README.md

+If you want to also advertise loadbalancer set IPs
+(`status.loadBalancer.ingress` IPs), e.g. when using it with MetalLb,
+add the `--advertise-loadbalancer-ip` flag (`false` by default).
+


This is perfect from a user workflow. Thanks for humoring me!

jjo · 2018-03-25T00:41:36Z

Thanks folks for the reviews - I've messed with the branches while
rebasing against master, will reopen another PR from a cleanup tree
(and refer to this one for the discussion)

jjo force-pushed the jjo-add-loadbalancer-ips-support branch 2 times, most recently from c15f784 to 449a887 Compare March 15, 2018 11:49

SEJeff mentioned this pull request Mar 15, 2018

Document which parts of MetalLB are compatible with kube-router metallb/metallb#160

Open

jjo force-pushed the jjo-add-loadbalancer-ips-support branch from eb3ca2e to f30795d Compare March 16, 2018 13:43

murali-reddy mentioned this pull request Mar 21, 2018

kuber-router does not work with loadbalancer #310

Closed

jjo force-pushed the jjo-add-loadbalancer-ips-support branch from f30795d to a2daf09 Compare March 21, 2018 20:43

jjo mentioned this pull request Mar 21, 2018

[jjo] added network_services_controller ginkgo testing #348

Merged

jjo added 4 commits March 22, 2018 01:58

add some logging

f033a07

add bit more logic to getExternalIps, call it from advertiseExternalI…

a9b671b

…Ps() to reuse essentially same pre-existing code

[jjo] add service.uselbips testing

488fbf0

jjo force-pushed the jjo-add-loadbalancer-ips-support branch from e21b3df to 488fbf0 Compare March 22, 2018 04:59

andrewsykim reviewed Mar 23, 2018

View reviewed changes

jjo added 3 commits March 23, 2018 22:12

[jjo] full refactor for get{Cluster,External,LoadBalancer}IPs + getIp…

842c758

…sToAdvertise

update Documentation/README.md

ea43b36

update Documentation/README.md

e15a9ab

SEJeff reviewed Mar 24, 2018

View reviewed changes

jjo closed this Mar 25, 2018

jjo deleted the jjo-add-loadbalancer-ips-support branch March 25, 2018 00:38

jjo mentioned this pull request Mar 25, 2018

[jjo] support advertising status.loadBalancer.ingress IPs via flag #354

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[jjo] support also using status.loadBalancer.ingress IPs via annotation #339

[jjo] support also using status.loadBalancer.ingress IPs via annotation #339

jjo commented Mar 13, 2018

SEJeff commented Mar 15, 2018

jjo commented Mar 16, 2018

murali-reddy commented Mar 21, 2018

jjo commented Mar 21, 2018

murali-reddy commented Mar 22, 2018 •

edited

Loading

SEJeff commented Mar 23, 2018

jjo commented Mar 23, 2018

murali-reddy commented Mar 23, 2018 •

edited

Loading

andrewsykim Mar 23, 2018

andrewsykim Mar 23, 2018 •

edited

Loading

SEJeff Mar 24, 2018

jjo commented Mar 25, 2018

[jjo] support also using status.loadBalancer.ingress IPs via annotation #339

[jjo] support also using status.loadBalancer.ingress IPs via annotation #339

Conversation

jjo commented Mar 13, 2018

SEJeff commented Mar 15, 2018

jjo commented Mar 16, 2018

murali-reddy commented Mar 21, 2018

jjo commented Mar 21, 2018

murali-reddy commented Mar 22, 2018 • edited Loading

SEJeff commented Mar 23, 2018

jjo commented Mar 23, 2018

murali-reddy commented Mar 23, 2018 • edited Loading

andrewsykim Mar 23, 2018

Choose a reason for hiding this comment

andrewsykim Mar 23, 2018 • edited Loading

Choose a reason for hiding this comment

SEJeff Mar 24, 2018

Choose a reason for hiding this comment

jjo commented Mar 25, 2018

murali-reddy commented Mar 22, 2018 •

edited

Loading

murali-reddy commented Mar 23, 2018 •

edited

Loading

andrewsykim Mar 23, 2018 •

edited

Loading