fix(occm/loadbalancer): enable the proxy-protocol only for supported listeners #2368

Lucasgranet · 2023-09-25T15:38:14Z

What this PR does / why we need it:
This PR fix an issue when trying to activate the ProxyProtocol on a K8S LoadBalancer with, at least, one UDP or SCTP port.

The CCM try, when the proxy-protocol's annotation is set, to enable the proxy-protocols on all pools, whatever the protocol used by the listener.

Octavia doesn't support the proxy protocol with a UDP or SCTP listener: https://docs.openstack.org/api-ref/load-balancer/v2/index.html?expanded=create-pool-detail#protocol-combinations-listener-pool

Which issue this PR fixes(if applicable):
N/A

Special notes for reviewers:
ATM, this service cannot be deployed. This kind of service will be more common due to "new" protocol HTTP/3/QUIC (TCP and UDP usage on the same service).

apiVersion: v1
kind: Service
metadata:
  name: octavia-udp-tcp-with-proxyprotocol
  annotations:
    loadbalancer.openstack.org/proxy-protocol: "true"
spec:
  ports:
  - name: client
    port: 443
    protocol: TCP
    targetPort: 443
  - name: client-udp
    port: 443
    protocol: UDP
    targetPort: 443
  selector:
    app: nginx
  type: LoadBalancer

Without this patch, this will result to a 409 from the Octavia's API.

Release note:

the proxy-protocol is ignored when the listener protocol is not supported (UDP/SCTP)

…eners

linux-foundation-easycla · 2023-09-25T15:38:19Z

The committers listed above are authorized under a signed CLA.

✅ login: Lucasgranet / name: Lucas Granet (ec09723)

k8s-ci-robot · 2023-09-25T15:38:24Z

Welcome @Lucasgranet!

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 · 2023-09-25T15:38:24Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign jichenjc for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

k8s-ci-robot · 2023-09-25T15:38:25Z

Hi @Lucasgranet. 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.

jichenjc · 2023-09-26T10:03:38Z

/ok-to-test

kayrus · 2023-09-26T10:19:18Z

@Lucasgranet for me this fix looks like a dirty hack. How does QUIC server behave when TCP listener does contain a proxy protocol header, but UDP listener doesn't?

Lucasgranet · 2023-09-27T12:49:30Z

Hello @kayrus,

With the current version of Octavia, the QUIC protocol cannot be enable with the ProxyProtocol indeed. (because he relies on UDP).

Whatever is the protocol, Octavia, atm, doesn't support the ProxyProtocol with the UDP (and SCTP) listeners.
You can find here a compatibility matrix for ProxyProtocol support.

This PR lead to fix LoadBalancer Service when:

the ProxyProtocol annotation is set
An unsupported protocol is used on a port (UDP or SCTP)

With the current upstream code, the CCM will get this error from the Octavia API:

{
    "faultcode": "Client", 
    "faultstring": "Validation failure: The pool protocol 'PROXY' is invalid while the listener protocol is 'UDP'.", 
    "debuginfo": null
}

Lucas,

kayrus · 2023-09-27T13:19:45Z

pkg/openstack/loadbalancer.go

+var (
+	// List based on Octavia documentation https://docs.openstack.org/api-ref/load-balancer/v2/index.html?expanded=create-pool-detail#protocol-combinations-listener-pool
+	proxyProtocolSupportedListeners                            = []listeners.Protocol{listeners.ProtocolHTTP, listeners.ProtocolHTTPS, listeners.ProtocolTCP, listeners.ProtocolTerminatedHTTPS}
+	_                               cloudprovider.LoadBalancer = &LbaasV2{}


what do you want to achieve with this string?

kayrus · 2023-09-27T13:21:25Z

pkg/openstack/loadbalancer.go

-var _ cloudprovider.LoadBalancer = &LbaasV2{}
+var (
+	// List based on Octavia documentation https://docs.openstack.org/api-ref/load-balancer/v2/index.html?expanded=create-pool-detail#protocol-combinations-listener-pool
+	proxyProtocolSupportedListeners                            = []listeners.Protocol{listeners.ProtocolHTTP, listeners.ProtocolHTTPS, listeners.ProtocolTCP, listeners.ProtocolTerminatedHTTPS}


it's better to define each supported protocol on a separate line. further list updates will look more readable.

kayrus · 2023-09-27T13:24:27Z

pkg/openstack/loadbalancer.go

+// for the pool configuration. In the case of UDP listener, Octavia doesn't support the ProxyProtocol
+// and should not be enabled to avoid 403 from Octavia API.
+// https://docs.openstack.org/api-ref/load-balancer/v2/index.html?expanded=create-pool-detail#protocol-combinations-listener-pool
+func isProtocolProxyProtocolSupported(protocol string) bool {


we already have

cloud-provider-openstack/pkg/util/util.go

Lines 65 to 73 in 5b36a53

// Contains searches if a string list contains the given string or not.

func Contains(list []string, strToSearch string) bool {

for _, item := range list {

if item == strToSearch {

return true

}

}

return false

}

which can be converted into:

func Contains[T comparable](list []T, strToSearch T) bool { for _, item := range list { if item == strToSearch { return true } } return false }

dulek · 2023-09-27T14:05:55Z

I think I understand the idea here. However instead of relying on a static list of supported protocols I think I'd prefer to catch the creation error and ignore it if message indicates that the problem is with the combination. Moreover we should only mask the error if the Service has other ports. If not, we should fail creation of the LB for that service and inform the user about this.

kayrus · 2023-09-27T14:11:48Z

catch the creation error and ignore it if message indicates that the problem is with the combination

is there a way to view these warnings using kubectl describe svc? I'm asking because such events must be visible for end users, who don't always have an access to OCCM logs.

Lucasgranet · 2023-10-03T14:46:15Z

I think I understand the idea here. However instead of relying on a static list of supported protocols I think I'd prefer to catch the creation error and ignore it if message indicates that the problem is with the combination.

Indeed, this behavior seems better, especially if Octavia will support this combination one day.

Moreover we should only mask the error if the Service has other ports. If not, we should fail creation of the LB for that service and inform the user about this.

I'm agree with this too. We should not allow the ProxyProtocol's annotation set to true when all used protocols are not compatible.

is there a way to view these warnings using kubectl describe svc?

An event is returned to the user.

  Warning  SyncLoadBalancerFailed  18s (x5 over 18m)   service-controller  Error syncing load balancer: failed to ensure load balancer: Bad request with: [POST https://load-balancer.gra9.cloud.ovh.net/v2.0/lbaas/pools], error message: {"faultcode": "Client", "faultstring": "Validation failure: The pool protocol 'PROXY' is invalid while the listener protocol is 'UDP'.", "debuginfo": null}

dulek · 2023-10-03T15:02:06Z

catch the creation error and ignore it if message indicates that the problem is with the combination

is there a way to view these warnings using kubectl describe svc? I'm asking because such events must be visible for end users, who don't always have an access to OCCM logs.

Fair point, only errors are logged as Events and I was proposing to not error. I'll try to draft some interface that would allow us to create events, based on how GCP provider does this.

dulek · 2023-10-03T15:59:29Z

Please see #2383. It's yet to be tested, but I don't see why it wouldn't work.

k8s-ci-robot · 2023-10-20T22:48:30Z

PR needs rebase.

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.

k8s-ci-robot · 2023-11-01T03:50:45Z

@Lucasgranet: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
openstack-cloud-csi-cinder-e2e-test	`ec09723`	link	true	`/test openstack-cloud-csi-cinder-e2e-test`
openstack-cloud-keystone-authentication-authorization-test	`ec09723`	link	true	`/test openstack-cloud-keystone-authentication-authorization-test`
openstack-cloud-csi-manila-e2e-test	`ec09723`	link	true	`/test openstack-cloud-csi-manila-e2e-test`
openstack-cloud-csi-cinder-sanity-test	`ec09723`	link	true	`/test openstack-cloud-csi-cinder-sanity-test`
openstack-cloud-controller-manager-e2e-test	`ec09723`	link	true	`/test openstack-cloud-controller-manager-e2e-test`
openstack-cloud-csi-manila-sanity-test	`ec09723`	link	true	`/test openstack-cloud-csi-manila-sanity-test`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

Lucasgranet · 2023-11-03T13:35:09Z

Hello @dulek

Sorry for the late, I was busy on other subjects.
I will check this soon.

Lucas,

dulek · 2023-11-03T16:16:00Z

@Lucasgranet #2383 is just merged, so great timing. ;)

k8s-triage-robot · 2024-02-01T16:52:22Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot · 2024-03-02T17:04:20Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle rotten
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

dulek · 2024-03-05T18:03:22Z

I rebased this in #2549. Closing this one.

fix(occm/loadbalancer): enable proxy-protocol only for supported list…

ec09723

…eners

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Sep 25, 2023

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Sep 25, 2023

k8s-ci-robot requested review from FengyunPan2 and kayrus September 25, 2023 15:38

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 25, 2023

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 25, 2023

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 Sep 26, 2023

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Sep 26, 2023

kayrus reviewed Sep 27, 2023

View reviewed changes

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

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 1, 2024

dulek mentioned this pull request Feb 9, 2024

fix(occm/loadbalancer): enable proxy-protocol only for supported list… #2549

Open

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Mar 2, 2024

dulek closed this Mar 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(occm/loadbalancer): enable the proxy-protocol only for supported listeners #2368

fix(occm/loadbalancer): enable the proxy-protocol only for supported listeners #2368

Lucasgranet commented Sep 25, 2023

linux-foundation-easycla bot commented Sep 25, 2023 •

edited

Loading

k8s-ci-robot commented Sep 25, 2023

k8s-ci-robot commented Sep 25, 2023

k8s-ci-robot commented Sep 25, 2023

jichenjc commented Sep 26, 2023

kayrus commented Sep 26, 2023 •

edited

Loading

Lucasgranet commented Sep 27, 2023

kayrus Sep 27, 2023

kayrus Sep 27, 2023 •

edited

Loading

kayrus Sep 27, 2023

dulek commented Sep 27, 2023

kayrus commented Sep 27, 2023

Lucasgranet commented Oct 3, 2023 •

edited

Loading

dulek commented Oct 3, 2023

dulek commented Oct 3, 2023

k8s-ci-robot commented Oct 20, 2023

k8s-ci-robot commented Nov 1, 2023

Lucasgranet commented Nov 3, 2023

dulek commented Nov 3, 2023

k8s-triage-robot commented Feb 1, 2024

k8s-triage-robot commented Mar 2, 2024

dulek commented Mar 5, 2024 •

edited

Loading

	// Contains searches if a string list contains the given string or not.
	func Contains(list []string, strToSearch string) bool {
	for _, item := range list {
	if item == strToSearch {
	return true
	}
	}
	return false
	}

fix(occm/loadbalancer): enable the proxy-protocol only for supported listeners #2368

fix(occm/loadbalancer): enable the proxy-protocol only for supported listeners #2368

Conversation

Lucasgranet commented Sep 25, 2023

linux-foundation-easycla bot commented Sep 25, 2023 • edited Loading

k8s-ci-robot commented Sep 25, 2023

k8s-ci-robot commented Sep 25, 2023

k8s-ci-robot commented Sep 25, 2023

jichenjc commented Sep 26, 2023

kayrus commented Sep 26, 2023 • edited Loading

Lucasgranet commented Sep 27, 2023

kayrus Sep 27, 2023

Choose a reason for hiding this comment

kayrus Sep 27, 2023 • edited Loading

Choose a reason for hiding this comment

kayrus Sep 27, 2023

Choose a reason for hiding this comment

dulek commented Sep 27, 2023

kayrus commented Sep 27, 2023

Lucasgranet commented Oct 3, 2023 • edited Loading

dulek commented Oct 3, 2023

dulek commented Oct 3, 2023

k8s-ci-robot commented Oct 20, 2023

k8s-ci-robot commented Nov 1, 2023

Lucasgranet commented Nov 3, 2023

dulek commented Nov 3, 2023

k8s-triage-robot commented Feb 1, 2024

k8s-triage-robot commented Mar 2, 2024

dulek commented Mar 5, 2024 • edited Loading

linux-foundation-easycla bot commented Sep 25, 2023 •

edited

Loading

kayrus commented Sep 26, 2023 •

edited

Loading

kayrus Sep 27, 2023 •

edited

Loading

Lucasgranet commented Oct 3, 2023 •

edited

Loading

dulek commented Mar 5, 2024 •

edited

Loading