NE-1091: Add proxy protocol support to IBMLoadBalancerParameters #1236

SzucsAti · 2022-07-25T12:25:41Z

On IBMCloud users can set a loadbalancer annotation to enable proxy protocol on the loadbalancer:
service.kubernetes.io/ibm-load-balancer-cloud-provider-enable-features: "proxy-protocol"
https://cloud.ibm.com/docs/containers?topic=containers-vpc-lbaas

With this PR users can configure proxy protocol on the ingresscontroller custom resource. Cluster-ingress-operator will read the value of this field and configure the loadbalancer accordingly.

  endpointPublishingStrategy:
    loadBalancer:
      providerParameters:
        type: IBM
        ibm:
          protocol: PROXY
      scope: External
    type: LoadBalancerService

This PR is based on an other PR which is not yet merged, it implements IBMLoadBalancerParameters:
#1208

openshift-ci · 2022-07-25T12:25:53Z

Hello @SzucsAti! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

For merging purposes, this repository follows the no-Feature-Freeze process which means that in addition to the standard lgtm and approved labels this repository requires either:

bugzilla/valid-bug - applied if your PR references a valid bugzilla bug

OR

qe-approved, docs-approved, and px-approved - these labels can be applied by anyone in the openshift org via the /label command.

Who should apply these qe/docs/px labels?

For a no-Feature-Freeze team who is merging a feature before code freeze, they need to get those labels applied to their api repo PR by the appropriate teams (i.e. qe, docs, px)
For a Feature Freeze (traditional) team who is merging a feature before FF, they can self-apply the labels (via /label commands), they are basically irrelevant for those teams
For a Feature Freeze team who is merging a feature after FF, the PR should be rejected barring an exception

SzucsAti · 2022-07-26T08:07:29Z

/test verify

SzucsAti · 2022-07-26T08:40:02Z

/assign

SzucsAti · 2022-07-26T08:42:01Z

/assign @deads2k @soltysh

JoelSpeed · 2022-10-24T12:07:14Z

operator/v1/types_ingress.go

 type ProviderLoadBalancerParameters struct {
 	// type is the underlying infrastructure provider for the load balancer.
-	// Allowed values are "AWS", "Azure", "BareMetal", "GCP", "Nutanix",
+	// Allowed values are "AWS", "Azure", "BareMetal", "GCP", "IBM", "Nutanix",


Where is the change to the discriminator enum to allow this? Can't see a change to LoadBalancerProviderType included here

It was already changed here: https://github.com/openshift/api/blob/master/operator/v1/types_ingress.go#L473-#L490. Comment was not up to date.

JoelSpeed · 2022-10-24T12:07:54Z

operator/v1/types_ingress.go

+	// protocol specifies whether the load balancer uses PROXY protocol to forward connections to
+	// the IngressController. See "service.kubernetes.io/ibm-load-balancer-cloud-provider-enable-features:
+	// "proxy-protocol"" at https://cloud.ibm.com/docs/containers?topic=containers-vpc-lbaas"
+


Is this blank line expected?

Not expected, fixed.

JoelSpeed · 2022-10-24T12:48:08Z

The changes to the API look good here but, given the proximity to branch cuts, I'd like to see a QE approval of the feature before we merge this

/hold

openshift-bot · 2023-01-23T01:00:41Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

SzucsAti · 2023-01-23T10:14:17Z

/remove-lifecycle stale

JoelSpeed · 2023-01-27T09:55:14Z

operator/v1/types_ingress.go

+	// The following values are valid for this field:
+	//
+	// * The empty string.
+	// * "TCP".
+	// * "PROXY".
+	//
+	// The empty string specifies the default, which is TCP without PROXY
+	// protocol.  Note that the default is subject to change.


Just want to update the wording to be consistent with our standard wording on other APIs

Suggested change

// The following values are valid for this field:

//

// * The empty string.

// * "TCP".

// * "PROXY".

//

// The empty string specifies the default, which is TCP without PROXY

// protocol. Note that the default is subject to change.

// Valid values for protocol and TCP, PROXY and omitted.

// When omitted, this means no opinion and the platform is left to choose a reasonable default, which is subject to change over time.

// The current default is TCP, without the proxy protocol enabled.

Thank you, applied your suggestion.

@JoelSpeed, should we update the godoc on the other protocol fields likewise? I can get that in a follow-up if you like.

Yeah a follow up would be great if you can, thanks

JoelSpeed · 2023-01-27T09:55:30Z

operator/v1/types_ingress.go

+	// +kubebuilder:validation:Optional
+	// +optional


You don't need both versions of this, just +optional will suffice

Removed // +kubebuilder:validation:Optional.

Co-authored-by: Vincent Burckhardt <vincent.burckhardt@ie.ibm.com>

JoelSpeed · 2023-01-30T13:26:57Z

API looks good, have you run through any pre-merge validation with the QE team? Could we look to get their ack on this feature?

candita · 2023-02-01T17:37:30Z

/assign @Miciah
/assign @gcs278

lihongan · 2023-02-02T10:15:03Z

For pre-merge validation, I think we have no chance to test the api directly.
Can anyone help to create PR to cluster-ingress-operator with the API bump? then QE can do pre-merge validation with the two PRs.
cc @SzucsAti @Miciah

candita · 2023-02-02T18:49:27Z

For pre-merge validation, I think we have no chance to test the api directly. Can anyone help to create PR to cluster-ingress-operator with the API bump? then QE can do pre-merge validation with the two PRs. cc @SzucsAti @Miciah

@SzucsAti can you confirm that openshift/cluster-ingress-operator#812 is a PR that can be used to do pre-merge validation?

attiss · 2023-02-02T19:01:11Z

Hi @lihongan @candita,

Yes, openshift/cluster-ingress-operator#812 is the correct PR for pre-merge validations.

lihongan · 2023-02-03T02:21:09Z

@attiss thanks for confirming. Looks openshift/cluster-ingress-operator#812 needs rebase and some checks were failed.
Anyway, I'll have a try to see if I can build payload with the two PRs.

lihongan · 2023-02-03T02:32:44Z

Oops, cannot build the payload with cluster-bot and it reports:

pull request openshift/cluster-ingress-operator#812 needs to be rebased to branch master

so the rebase is mandatory

SzucsAti · 2023-02-03T08:56:30Z

Rebased openshift/cluster-ingress-operator#812.

lihongan · 2023-02-07T05:35:09Z

tested with pre-merge validation and passed

$ oc get clusterversion
NAME      VERSION                                                   AVAILABLE   PROGRESSING   SINCE   STATUS
version   4.13.0-0.ci.test-2023-02-07-013419-ci-ln-k91zc62-latest   True        False         139m    Cluster version is 4.13.0-0.ci.test-2023-02-07-013419-ci-ln-k91zc62-latest

### after updating the ingresscontroller/default and got 
$ oc -n openshift-ingress-operator get ingresscontroller/default -oyaml
<---snip-->
spec:
  endpointPublishingStrategy:
    loadBalancer:
      dnsManagementPolicy: Managed
      providerParameters:
        ibm:
          protocol: PROXY
        type: IBM
      scope: External
    type: LoadBalancerService
<------>
status:
  domain: apps.hongli-ibm.ibmcloud.qe.devcluster.openshift.com
  endpointPublishingStrategy:
    loadBalancer:
      dnsManagementPolicy: Managed
      providerParameters:
        ibm:
          protocol: PROXY
        type: IBM
      scope: External
    type: LoadBalancerService
<------>

### ensure router pods use PROXY protocol
$ oc -n openshift-ingress get deployment/router-default -oyaml
<---snip--->
        - name: ROUTER_USE_PROXY_PROTOCOL
          value: "true"

### ensure the proxy-protocol annotation is added to the LB service 
$ oc -n openshift-ingress get svc/router-default -oyaml
apiVersion: v1
kind: Service
metadata:
  annotations:
    service.kubernetes.io/ibm-load-balancer-cloud-provider-enable-features: proxy-protocol
    service.kubernetes.io/ibm-load-balancer-cloud-provider-ip-type: public
<---snip--->

### ensure routes and ingress operator work well
$ curl https://canary-openshift-ingress-canary.apps.hongli-ibm.ibmcloud.qe.devcluster.openshift.com -k
Healthcheck requested
 
$ oc get co/ingress
NAME      VERSION                                                   AVAILABLE   PROGRESSING   DEGRADED   SINCE   MESSAGE
ingress   4.13.0-0.ci.test-2023-02-07-013419-ci-ln-k91zc62-latest   True        False         False      153m

lihongan · 2023-02-07T05:35:40Z

/label qe-approved

Miciah · 2023-02-07T14:24:36Z

/lgtm
/hold cancel
@JoelSpeed, are you ready to approve the API change?

ahardin-rh · 2023-02-07T14:39:09Z

/label docs-approved

CFields651 · 2023-02-07T14:41:05Z

/label px-approved

gcs278 · 2023-02-07T15:43:21Z

/lgtm

JoelSpeed · 2023-02-07T17:58:30Z

/lgtm

openshift-ci · 2023-02-07T17:59:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gcs278, JoelSpeed, Miciah, SzucsAti

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:

~~OWNERS~~ [JoelSpeed]

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

openshift-ci · 2023-02-07T18:18:36Z

@SzucsAti: all tests passed!

Full PR test history. Your PR dashboard.

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.

SzucsAti mentioned this pull request Jul 25, 2022

NE-1092: Add proxy protocol support for IBMCloud loadbalancers openshift/cluster-ingress-operator#812

Merged

openshift-ci bot requested review from deads2k and soltysh July 25, 2022 12:26

SzucsAti force-pushed the proxy-protocol branch from 077e19f to 29eb0df Compare July 26, 2022 07:41

openshift-ci bot assigned SzucsAti Jul 26, 2022

SzucsAti removed their assignment Jul 26, 2022

openshift-ci bot assigned deads2k and soltysh Jul 26, 2022

SzucsAti force-pushed the proxy-protocol branch from 29eb0df to 520b901 Compare August 15, 2022 07:49

SzucsAti force-pushed the proxy-protocol branch 3 times, most recently from 6d2e3a3 to 40d91c3 Compare October 5, 2022 13:13

SzucsAti changed the title ~~Add proxy protocol support to IBMLoadBalancerParameters~~ NE-1091: Add proxy protocol support to IBMLoadBalancerParameters Oct 21, 2022

SzucsAti force-pushed the proxy-protocol branch from 40d91c3 to 941c6c7 Compare October 21, 2022 16:27

JoelSpeed reviewed Oct 24, 2022

View reviewed changes

SzucsAti force-pushed the proxy-protocol branch from 941c6c7 to 727438c Compare October 24, 2022 12:39

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 24, 2022

openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 23, 2023

openshift-ci bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 23, 2023

SzucsAti force-pushed the proxy-protocol branch from 727438c to 639b2af Compare January 24, 2023 16:30

JoelSpeed reviewed Jan 27, 2023

View reviewed changes

SzucsAti force-pushed the proxy-protocol branch from 639b2af to e091593 Compare January 30, 2023 09:48

add proxy protocol support for IBMCloud

f4d09b5

Co-authored-by: Vincent Burckhardt <vincent.burckhardt@ie.ibm.com>

SzucsAti force-pushed the proxy-protocol branch from e091593 to f4d09b5 Compare January 30, 2023 10:47

openshift-ci bot assigned gcs278 and Miciah Feb 1, 2023

openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Feb 7, 2023

openshift-ci bot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Feb 7, 2023

openshift-ci bot added the docs-approved Signifies that Docs has signed off on this PR label Feb 7, 2023

openshift-ci bot added the px-approved Signifies that Product Support has signed off on this PR label Feb 7, 2023

openshift-ci bot assigned JoelSpeed Feb 7, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 7, 2023

openshift-merge-robot merged commit 3961c9d into openshift:master Feb 7, 2023

NE-1091: Add proxy protocol support to IBMLoadBalancerParameters #1236

NE-1091: Add proxy protocol support to IBMLoadBalancerParameters #1236

Uh oh!

Conversation

SzucsAti commented Jul 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Jul 25, 2022

Uh oh!

SzucsAti commented Jul 26, 2022

Uh oh!

SzucsAti commented Jul 26, 2022

Uh oh!

SzucsAti commented Jul 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoelSpeed commented Oct 24, 2022

Uh oh!

openshift-bot commented Jan 23, 2023

Uh oh!

SzucsAti commented Jan 23, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoelSpeed commented Jan 30, 2023

Uh oh!

candita commented Feb 1, 2023

Uh oh!

lihongan commented Feb 2, 2023

Uh oh!

candita commented Feb 2, 2023

Uh oh!

attiss commented Feb 2, 2023

Uh oh!

lihongan commented Feb 3, 2023

Uh oh!

lihongan commented Feb 3, 2023

Uh oh!

SzucsAti commented Feb 3, 2023

Uh oh!

lihongan commented Feb 7, 2023

Uh oh!

lihongan commented Feb 7, 2023

Uh oh!

Miciah commented Feb 7, 2023

Uh oh!

ahardin-rh commented Feb 7, 2023

Uh oh!

CFields651 commented Feb 7, 2023

Uh oh!

gcs278 commented Feb 7, 2023

Uh oh!

JoelSpeed commented Feb 7, 2023

Uh oh!

openshift-ci bot commented Feb 7, 2023

Uh oh!

openshift-ci bot commented Feb 7, 2023

Uh oh!

Uh oh!

SzucsAti commented Jul 25, 2022 •

edited

Loading