NE-518 GCP: Implement GCP Internal LB Global Access option #550

sgreene570 · 2021-02-12T19:41:10Z

GCP: Implement GCP Internal LB Global Access option

This commit is in support of https://issues.redhat.com/browse/NE-518.

pkg/operator/controller/ingress/load_balancer_service.go:

Apply the GCP "Global Access" annotation to an Ingress Controller's LoadBalancer service on GCP if and only if the Ingress Controller specifies an Internal load balancer as well as a GCP ClientAccess Provider Parameter. Modify updateLoadBalancerService to trigger updates to an Ingress Controller's LoadBalancer service if the GCP Global Access annotation is changed.

pkg/operator/controller/ingress/load_balancer_service_test.go:

Add two unit test cases to TestDesiredLoadBalancerService, one for an internal Load Balancer on GCP with ClientAccess set to Global, and one for an internal Load Balancer on GCP with ClientAccess set to Local.

pkg/operator/controller/ingress/controller.go:

Sync GCP provider parameters updates from Spec to Status in SetDefaultPublishingStrategy, since those changes are safe to roll out.

test/e2e: Add GCP Global Access e2e test

test/e2e/operator_test.go:

Add a new e2e test to exercise creating an internal load balancer on gcp with ClientAccess set to Global, and then mutating the ClientAccess field to Local.

bindata: Update local CRD

go.mod: Bump openshift/api to latest

Bump openshift/api to pull in openshift/api#845.

Prerequisites:

operator/ingress: Add GCP Global Access for LBs api#845
Ingress Operator: Add optional e2e-gcp-operator job release#15823
TODO: Add e2e tests to this PR

sgreene570 · 2021-02-15T16:33:41Z

Let's take our new CI job for a spin.
/test e2e-gcp-operator

sgreene570 · 2021-02-16T22:22:20Z

/test e2e-gcp-operator

sgreene570 · 2021-02-17T16:11:44Z

Let's make sure the first pass wasn't a fluke
/test e2e-gcp-operator

candita · 2021-02-18T20:12:47Z

pkg/operator/controller/ingress/controller.go

+	specLB := effectiveStrategy.LoadBalancer
+	if specLB != nil && statusLB != nil && specLB.ProviderParameters != nil && statusLB.ProviderParameters != nil &&
+		specLB.ProviderParameters.GCP != statusLB.ProviderParameters.GCP {
+		ic.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.GCP = effectiveStrategy.LoadBalancer.ProviderParameters.GCP


It might be a good time to remove the return value, since it isn't used by the caller. If we anticipate it to be used in the future, you might want to return true here.

In the future we might use the return value from setDefaultPublishingStrategy to detect if any changes were made to ic.Status, so for now I'll add a return value here (surprisingly this was also overlooked in the reverted mutable-scope PR #472, so nice catch!).

candita · 2021-02-18T20:23:24Z

pkg/operator/controller/ingress/load_balancer_service.go


+	// GCPGlobalAccessAnnotation is the annotation used on an internal load balancer service
+	// to enable the GCP Global Access feature.
+	GCPGlobalAccessAnnotation = "networking.gke.io/internal-load-balancer-allow-global-access"


I would suggest moving this to api/operator/v1/types_ingress.go, along with the other new GCP consts.

Typically we don't put implementation specific constants in the ingress controller API. I think it makes the most sense to define the annotation constant here alongside all of the other provider-specific load balancer service annotations.

candita · 2021-02-18T20:53:46Z

pkg/operator/controller/ingress/load_balancer_service.go

 	}

 	updated.Annotations[awsLBHealthCheckIntervalAnnotation] = expected.Annotations[awsLBHealthCheckIntervalAnnotation]
+	updated.Annotations[GCPGlobalAccessAnnotation] = expected.Annotations[GCPGlobalAccessAnnotation]


Could we check the platform type and only add these annotations if the platform matches?

We could, but we already check the platform type before setting the annotations anyways on expected, so I think that would be redundant.

test/e2e/operator_test.go

sgreene570 · 2021-02-18T21:23:07Z

/test e2e-gcp-operator

sgreene570 · 2021-02-19T14:35:35Z

myriad of flakes
/retest

sgreene570 · 2021-02-19T17:15:22Z

Failing tests:

[sig-arch][Feature:ClusterUpgrade] Cluster should remain functional during upgrade [Disruptive] [Serial]

/test e2e-upgrade

sgreene570 · 2021-02-23T14:28:04Z

/test e2e-upgrade

sgreene570 · 2021-02-24T15:54:19Z

/retest

sgreene570 · 2021-02-24T18:06:38Z

/retest

sgreene570 · 2021-02-25T19:44:27Z

/retest

Bump openshift/api to pull in openshift/api#845.

This commit is in support of https://issues.redhat.com/browse/NE-518. pkg/operator/controller/ingress/load_balancer_service.go: Apply the GCP "Global Access" annotation to an Ingress Controller's LoadBalancer service on GCP if and only if the Ingress Controller specifies an Internal load balancer as well as a GCP `ClientAccess` Provider Parameter. Modify `updateLoadBalancerService` to trigger updates to an Ingress Controller's LoadBalancer service if the GCP Global Access annotation is changed. pkg/operator/controller/ingress/load_balancer_service_test.go: Add two unit test cases to `TestDesiredLoadBalancerService`, one for an internal Load Balancer on GCP with `ClientAccess` set to `Global`, and one for an internal Load Balancer on GCP with `ClientAccess` set to `Local`. pkg/operator/controller/ingress/controller.go: Sync GCP provider parameters updates from Spec to Status in `SetDefaultPublishingStrategy`, since those changes are safe to roll out.

test/e2e/operator_test.go: Add a new e2e test to exercise creating an internal load balancer on gcp with `ClientAccess` set to `Global`, and then mutating the `ClientAccess` field to `Local`.

sgreene570 · 2021-03-03T14:55:59Z

/test e2e-gcp-operator

sgreene570 · 2021-03-03T16:45:39Z

must-gather failures
/test e2e-aws-operator

sgreene570 · 2021-03-03T17:23:53Z

/test e2e-upgrade

sgreene570 · 2021-03-04T16:42:44Z

FAIL: TestUniqueIdHeader
operator_test.go:1727: failed to observe the expected log message: timed out waiting for the condition
/retest

sgreene570 · 2021-03-04T19:47:40Z

/retest

Miciah · 2021-03-08T16:38:02Z

/lgtm
Thanks!

openshift-ci-robot · 2021-03-08T16:38:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Miciah, sgreene570

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [Miciah,sgreene570]

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

openshift-bot · 2021-03-08T17:02:23Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-03-08T17:28:22Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-03-08T22:27:05Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-03-08T22:40:04Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 12, 2021

openshift-ci-robot requested review from ironcladlou and knobunc February 12, 2021 19:41

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 12, 2021

sgreene570 force-pushed the gcp-ilb-global-access branch 2 times, most recently from 0e0afcf to d82e731 Compare February 12, 2021 21:13

sgreene570 force-pushed the gcp-ilb-global-access branch 3 times, most recently from 7c0a9ee to af797cd Compare February 16, 2021 22:21

sgreene570 changed the title ~~[WIP] GCP: Implement GCP Internal LB Global Access option~~ NE-518 GCP: Implement GCP Internal LB Global Access option Feb 17, 2021

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 17, 2021

candita reviewed Feb 18, 2021

View reviewed changes

test/e2e/operator_test.go Outdated Show resolved Hide resolved

sgreene570 force-pushed the gcp-ilb-global-access branch from af797cd to 8c62352 Compare February 18, 2021 21:19

sgreene570 added 2 commits March 3, 2021 09:50

go.mod: Bump openshift/api to latest.

0c744e7

Bump openshift/api to pull in openshift/api#845.

bindata: Update local CRD

f2a8361

sgreene570 force-pushed the gcp-ilb-global-access branch from 8c62352 to 04fdc9d Compare March 3, 2021 14:50

test/e2e: Add GCP Global Access e2e test

fb9cf27

test/e2e/operator_test.go: Add a new e2e test to exercise creating an internal load balancer on gcp with `ClientAccess` set to `Global`, and then mutating the `ClientAccess` field to `Local`.

sgreene570 force-pushed the gcp-ilb-global-access branch from 04fdc9d to fb9cf27 Compare March 3, 2021 14:55

openshift-ci-robot assigned Miciah Mar 8, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 8, 2021

openshift-merge-robot merged commit 630c1cf into openshift:master Mar 8, 2021

Miciah mentioned this pull request Feb 11, 2022

OSDOCS-1768: document mutating .spec.endpointPublishingStrategy (new) openshift/openshift-docs#39451

Merged

NE-518 GCP: Implement GCP Internal LB Global Access option #550

NE-518 GCP: Implement GCP Internal LB Global Access option #550

Uh oh!

Conversation

sgreene570 commented Feb 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sgreene570 commented Feb 15, 2021

Uh oh!

sgreene570 commented Feb 16, 2021

Uh oh!

sgreene570 commented Feb 17, 2021

Uh oh!

candita Feb 18, 2021

Choose a reason for hiding this comment

Uh oh!

sgreene570 Feb 18, 2021

Choose a reason for hiding this comment

Uh oh!

sgreene570 Feb 18, 2021

Choose a reason for hiding this comment

Uh oh!

candita Feb 18, 2021

Choose a reason for hiding this comment

Uh oh!

sgreene570 Feb 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

candita Feb 18, 2021

Choose a reason for hiding this comment

Uh oh!

sgreene570 Feb 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sgreene570 commented Feb 18, 2021

Uh oh!

sgreene570 commented Feb 19, 2021

Uh oh!

sgreene570 commented Feb 19, 2021

Uh oh!

sgreene570 commented Feb 23, 2021

Uh oh!

sgreene570 commented Feb 24, 2021

Uh oh!

sgreene570 commented Feb 24, 2021

Uh oh!

sgreene570 commented Feb 25, 2021

Uh oh!

sgreene570 commented Mar 3, 2021

Uh oh!

sgreene570 commented Mar 3, 2021

Uh oh!

sgreene570 commented Mar 3, 2021

Uh oh!

sgreene570 commented Mar 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sgreene570 commented Mar 4, 2021

Uh oh!

Miciah commented Mar 8, 2021

Uh oh!

openshift-ci-robot commented Mar 8, 2021

Uh oh!

openshift-bot commented Mar 8, 2021

Uh oh!

openshift-bot commented Mar 8, 2021

Uh oh!

openshift-bot commented Mar 8, 2021

Uh oh!

openshift-bot commented Mar 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

sgreene570 commented Feb 12, 2021 •

edited

Loading

sgreene570 Feb 18, 2021 •

edited

Loading

sgreene570 Feb 18, 2021 •

edited

Loading

sgreene570 commented Mar 4, 2021 •

edited

Loading