[occm] KEP-1860: Add support for LoadBalancer ipMode #2587

kbudde · 2024-04-28T07:55:32Z

What this PR does / why we need it:
This PR adds support for ipMode in LoadBalancer status field introduced in KEP-1860.
This enables the usage of Proxy Protocol without workaround (EnableIngressHostname + IngressHostnameSuffix).

The workarounds were introducing new issues, e.g. externalDNS was not functional with a loadbalancer with ingress Hostname set.

Which issue this PR fixes(if applicable):
There is no open issue as a workaround was introduced.

Special notes for reviewers:
Tthe ipMode field is available even if IpMode is still behind a featureGate, so this change could be backported to 1.29.

Release note:

[occm] KEP-1860: Add support for LoadBalancer ipMode

linux-foundation-easycla · 2024-04-28T07:55:38Z

The committers listed above are authorized under a signed CLA.

✅ login: kbudde / name: Kris Budde (63bd374, 015fcf1)

k8s-ci-robot · 2024-04-28T07:55:41Z

Welcome @kbudde!

It looks like this is your first PR to kubernetes/cloud-provider-openstack 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/cloud-provider-openstack has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2024-04-28T07:55:41Z

Hi @kbudde. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

dulek · 2024-04-28T08:06:33Z

@gryf, you might want to check this out.

jichenjc · 2024-04-30T09:52:40Z

/ok-to-test

zetaab

/approve

k8s-ci-robot · 2024-05-05T06:06:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zetaab

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/openstack/OWNERS~~ [zetaab]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

zetaab · 2024-05-05T06:08:41Z

we are soonish releasing 1.30 (#2584) but IMO this could be interesting feature to new release.

cc @dulek @jichenjc

jichenjc · 2024-05-06T02:53:49Z

/lgtm
/hold

@dulek maybe you can check this again before merge

dulek · 2024-05-06T17:16:56Z

pkg/openstack/loadbalancer.go

+		// Set the LoadBalancerIPMode to Proxy to prevent kube-proxy from injecting an iptables bypass.
+		// https://github.com/kubernetes/enhancements/tree/master/keps/sig-network/1860-kube-proxy-IP-node-binding
+		ipMode = corev1.LoadBalancerIPModeProxy


Okay, so reading [1] this seems pretty much correct, but reading [2] I would argue we should always set IPMode to Proxy, because following is true for both Amphora and OVN:

Setting this to "Proxy" indicates that traffic is delivered to the node or pod with the destination set to the node's IP and node port or the pod's IP and port. Service implementations may use this information to adjust traffic routing.

What am I missing?

Also - doesn't this apply to other Amphora functions, like TLS termination or XForwardedFor header injection?

[1] https://kubernetes.io/blog/2023/12/18/kubernetes-1-29-feature-loadbalancer-ip-mode-alpha/
[2] https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.30/#loadbalanceringress-v1-core

afaik 1 is how I understood this should work

Same here. 1 is how it is supposed to work.

The ipMode field should only be set, if the loadbalancer IP is used and not an Hostname.
LoadBalancer with IngressHostname are not optimized in terms of traffic flow as kube-proxy would need to do a dns query before writing the firewall rules (and update it every time dns changes).

dulek · 2024-05-06T17:17:00Z

pkg/openstack/loadbalancer.go

-	status.Ingress = []corev1.LoadBalancerIngress{{IP: addr}}
+	status.Ingress = []corev1.LoadBalancerIngress{{
+		IP:     addr,
+		IPMode: &ipMode,


What happens when the feature gate is off and this is set? Is it a no-op? Should we have this behind a feature gate?

It's save, the field is just ignored if the feature is not enabed.

https://github.com/kubernetes/enhancements/tree/master/keps/sig-network/1860-kube-proxy-IP-node-binding#rollout-upgrade-and-rollback-planning

Disable the feature flag ("downgrade")
Edit kube-apiserver manifest and disable the feature flag
Edit kube-proxy configmap, disable the feature and restart kube-proxy Pods
Confirmed that both iptables rules are present, even if the ipMode field was still set as Proxy, confirming the feature is disabled. Both accesses are working

dulek · 2024-05-06T17:17:13Z

I added a bunch of questions.

dulek · 2024-05-07T09:32:51Z

/hold cancel

KEP-1860: Add support for LoadBalancer ipMode

63bd374

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Apr 28, 2024

k8s-ci-robot requested review from anguslees and FengyunPan2 April 28, 2024 07:55

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Apr 28, 2024

cleaner: use assertEqual for test

015fcf1

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 30, 2024

zetaab approved these changes May 5, 2024

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 5, 2024

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 6, 2024

k8s-ci-robot assigned jichenjc May 6, 2024

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

dulek reviewed May 6, 2024

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 7, 2024

k8s-ci-robot merged commit 7a4290e into kubernetes:master May 7, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[occm] KEP-1860: Add support for LoadBalancer ipMode #2587

[occm] KEP-1860: Add support for LoadBalancer ipMode #2587

kbudde commented Apr 28, 2024 •

edited

linux-foundation-easycla bot commented Apr 28, 2024 •

edited

k8s-ci-robot commented Apr 28, 2024

k8s-ci-robot commented Apr 28, 2024

dulek commented Apr 28, 2024

jichenjc commented Apr 30, 2024

zetaab left a comment

k8s-ci-robot commented May 5, 2024

zetaab commented May 5, 2024

jichenjc commented May 6, 2024

dulek May 6, 2024

zetaab May 6, 2024

kbudde May 6, 2024

kbudde May 6, 2024

dulek May 6, 2024

kbudde May 6, 2024

dulek commented May 6, 2024

dulek commented May 7, 2024

[occm] KEP-1860: Add support for LoadBalancer ipMode #2587

[occm] KEP-1860: Add support for LoadBalancer ipMode #2587

Conversation

kbudde commented Apr 28, 2024 • edited

linux-foundation-easycla bot commented Apr 28, 2024 • edited

k8s-ci-robot commented Apr 28, 2024

k8s-ci-robot commented Apr 28, 2024

dulek commented Apr 28, 2024

jichenjc commented Apr 30, 2024

zetaab left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 5, 2024

zetaab commented May 5, 2024

jichenjc commented May 6, 2024

dulek May 6, 2024

Choose a reason for hiding this comment

zetaab May 6, 2024

Choose a reason for hiding this comment

kbudde May 6, 2024

Choose a reason for hiding this comment

kbudde May 6, 2024

Choose a reason for hiding this comment

dulek May 6, 2024

Choose a reason for hiding this comment

kbudde May 6, 2024

Choose a reason for hiding this comment

dulek commented May 6, 2024

dulek commented May 7, 2024

kbudde commented Apr 28, 2024 •

edited

linux-foundation-easycla bot commented Apr 28, 2024 •

edited