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

[Lucasgranet]

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)

[linux-foundation-easycla[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: Lucasgranet / name: Lucas Granet (ec09723f75b0d64d46c3cbae5c5d2536ae34e978)

[k8s-ci-robot]

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. :smiley:

[k8s-ci-robot]

[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]

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]

/ok-to-test

[kayrus]

@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]

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,

[dulek]

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]

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]

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]

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]

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

[k8s-ci-robot]

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]

@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	ec09723f75b0d64d46c3cbae5c5d2536ae34e978	link	true	/test openstack-cloud-csi-cinder-e2e-test
openstack-cloud-keystone-authentication-authorization-test	ec09723f75b0d64d46c3cbae5c5d2536ae34e978	link	true	/test openstack-cloud-keystone-authentication-authorization-test
openstack-cloud-csi-manila-e2e-test	ec09723f75b0d64d46c3cbae5c5d2536ae34e978	link	true	/test openstack-cloud-csi-manila-e2e-test
openstack-cloud-csi-cinder-sanity-test	ec09723f75b0d64d46c3cbae5c5d2536ae34e978	link	true	/test openstack-cloud-csi-cinder-sanity-test
openstack-cloud-controller-manager-e2e-test	ec09723f75b0d64d46c3cbae5c5d2536ae34e978	link	true	/test openstack-cloud-controller-manager-e2e-test
openstack-cloud-csi-manila-sanity-test	ec09723f75b0d64d46c3cbae5c5d2536ae34e978	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]

Hello @dulek

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

Lucas,

[dulek]

@Lucasgranet https://github.com/kubernetes/cloud-provider-openstack/pull/2383 is just merged, so great timing. ;)

[k8s-triage-robot]

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]

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]

I rebased this in #2549. Closing this one.