NE-975: Use ingress config to set default LB type on AWS by gcs278 · Pull Request #837 · openshift/cluster-ingress-operator

gcs278 · 2022-10-11T21:42:19Z

Note: Cloned from #798 so that I could modify.

The cluster ingress config object can now specify the AWS ELB type that was specified when installing the cluster. The ingress operator will refer to this object whenever someone deletes the default ingresscontroller and the
operator has to recreate it. Before this commit, the operator would always set the default ELB type
(Classic ELB) when creating the default ingresscontroller. Fixes https://issues.redhat.com/browse/NE-975

pkg/operator/operator.go: Check if infrastructure object was set with the LB type "NLB" and create the ingresscontroller accordingly.
pkg/operator/ingress/controller.go: setDefaultPublishingStrategy refactoring. Introduce a new setDefaultPublishingStrategy function to simplify setDefaultPublishingStrategy, correct the defaulting behavior when status.endpointPublishingStrategy is nil.
pkg/operator/controller/ingress/controller_test.go: Add some test cases to TestSetDefaultPublishingStrategySetsPlatformDefaults and
changes to TestSetDefaultPublishingStrategyHandlesUpdates.

gcs278 · 2022-10-11T21:43:46Z

/assign @Miciah
/assign @rfredette

Miciah · 2022-10-11T22:21:24Z

/retitle NE-975: Use ingress config to set default LB type on AWS

miheer · 2022-10-12T10:45:26Z

/approved

miheer · 2022-10-12T12:16:36Z

/retest

CFields651 · 2022-10-12T13:32:05Z

/label px-approved

miheer · 2022-10-12T14:10:07Z

/lgtm

gcs278 · 2022-10-12T14:22:52Z

All failures are cluster installation failures
/retest

candita · 2022-10-12T16:07:26Z

/assign @gcs278

ShaunaDiaz · 2022-10-12T17:30:19Z

/label docs-approved

Miciah · 2022-10-12T19:05:17Z

pkg/operator/controller/ingress/controller.go

-			var lbType operatorv1.LoadBalancerProviderType
-			if specLB.ProviderParameters != nil {
-				lbType = specLB.ProviderParameters.Type
-			}
-			switch lbType {
+			switch specLB.ProviderParameters.Type {


Now that setDefaultProviderParameters can leave specLB.ProviderParameters with nil, we should revert the change that I am highlighting.

Miciah · 2022-10-12T19:05:40Z

pkg/operator/controller/ingress/controller_test.go

-				Scope: scope,
+				Scope:              scope,
+				ProviderParameters: &operatorv1.ProviderLoadBalancerParameters{},
 			}
 			if policy != nil {
 				lbs.DNSManagementPolicy = *policy
 			}
 			return lbs
 		}
+		lbsWithNoProviderParameters = func(scope operatorv1.LoadBalancerScope, policy *operatorv1.LoadBalancerDNSManagementPolicy) *operatorv1.LoadBalancerStrategy {
+			lbs_no_provider_params := &operatorv1.LoadBalancerStrategy{
+				Scope:              scope,
+				ProviderParameters: nil,
+			}
+			if policy != nil {
+				lbs_no_provider_params.DNSManagementPolicy = *policy
+			}
+			return lbs_no_provider_params
+		}


Instead of adding lbsWithNoProviderParameters, we can revert the change to lbs.

Miciah · 2022-10-13T00:13:08Z

This PR needs to pull in openshift/api@24ee13c. I also have a suggested fixup to avoid defaulting ProviderParameters in status if the ingresscontroller is already admitted. See gcs278#1.

miheer · 2022-10-14T02:46:25Z

/lgtm

miheer · 2022-10-14T02:58:23Z

/approve

openshift-ci · 2022-10-14T02:58:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: miheer

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

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

miheer · 2022-10-14T03:05:05Z

/hold

miheer · 2022-10-14T07:18:16Z

/retest

Miciah · 2022-10-14T15:26:08Z

Here's an E2E test we could use (lightly tested).

Details

diff --git a/test/e2e/all_test.go b/test/e2e/all_test.go
index 7c0d5e6d..db162cdf 100644
--- a/test/e2e/all_test.go
+++ b/test/e2e/all_test.go
@@ -83,6 +83,7 @@ func TestAll(t *testing.T) {
 		t.Run("TestDefaultIngressCertificate", TestDefaultIngressCertificate)
 		t.Run("TestDefaultIngressClass", TestDefaultIngressClass)
 		t.Run("TestOperatorRecreatesItsClusterOperator", TestOperatorRecreatesItsClusterOperator)
+		t.Run("TestAWSLBTypeDefaulting", TestAWSLBTypeDefaulting)
 		t.Run("TestHstsPolicyWorks", TestHstsPolicyWorks)
 		t.Run("TestIngressControllerCustomEndpoints", TestIngressControllerCustomEndpoints)
 		t.Run("TestIngressStatus", TestIngressStatus)
diff --git a/test/e2e/operator_test.go b/test/e2e/operator_test.go
index 4b926f38..9cc6010b 100644
--- a/test/e2e/operator_test.go
+++ b/test/e2e/operator_test.go
@@ -1186,6 +1186,103 @@ func TestAWSLBTypeChange(t *testing.T) {
 	}
 }
 
+// TestAWSLBTypeDefaulting verifies that the ingress operator correctly sets a
+// default load balancer type on AWS when the cluster ingress config specifies
+// one.
+//
+// This test is a serial test because it modifies the cluster ingress config and
+// therefore cannot run in parallel with other tests.
+func TestAWSLBTypeDefaulting(t *testing.T) {
+	if infraConfig.Status.Platform != configv1.AWSPlatformType {
+		t.Skipf("test skipped on platform %q", infraConfig.Status.Platform)
+	}
+
+	clbName := types.NamespacedName{Namespace: operatorNamespace, Name: "aws-lb-type-defaulting-to-clb"}
+	clbIC := newLoadBalancerController(clbName, clbName.Name+"."+dnsConfig.Spec.BaseDomain)
+	t.Logf("creating ingresscontroller %s with default LB type, which is Classic", clbName)
+	if err := kclient.Create(context.TODO(), clbIC); err != nil {
+		t.Fatalf("failed to create ingresscontroller %s: %v", clbName, err)
+	}
+	defer assertIngressControllerDeleted(t, kclient, clbIC)
+
+	if err := waitForIngressControllerCondition(t, kclient, 5*time.Minute, clbName, availableConditionsForIngressControllerWithLoadBalancer...); err != nil {
+		t.Fatalf("failed to observe expected conditions: %v", err)
+	}
+
+	t.Logf("verifying that ingresscontroller %s's LoadBalancer service doesn't specify NLB", clbName)
+	assertServiceAnnotation(t, controller.LoadBalancerServiceName(clbIC), ingresscontroller.AWSLBTypeAnnotation, "", false)
+
+	t.Log("changing the default LB type to NLB")
+	if err := updateIngressConfigSpecWithRetryOnConflict(t, clusterConfigName, timeout, func(spec *configv1.IngressSpec) {
+		spec.LoadBalancer.Platform.Type = configv1.AWSPlatformType
+		spec.LoadBalancer.Platform.AWS = &configv1.AWSIngressSpec{
+			Type: configv1.NLB,
+		}
+	}); err != nil {
+		t.Fatalf("failed to update ingress config: %v", err)
+	}
+	defer func() {
+		t.Log("resetting the default LB type to Classic")
+		if err := updateIngressConfigSpecWithRetryOnConflict(t, clusterConfigName, timeout, func(spec *configv1.IngressSpec) {
+			spec.LoadBalancer.Platform.AWS.Type = configv1.Classic
+		}); err != nil {
+			t.Errorf("failed to restore ingress config: %v", err)
+		}
+	}()
+
+	t.Logf("updating ingresscontroller %s to trigger re-admission", clbName)
+	if err := kclient.Get(context.TODO(), clbName, clbIC); err != nil {
+		t.Fatalf("failed to get ingresscontroller: %v", err)
+	}
+	clbIC.Spec.UnsupportedConfigOverrides = runtime.RawExtension{
+		Raw: []byte(`{"foo":"bar"}`),
+	}
+	// This unsupported config override does not cause any changes to the
+	// operands but does force the reconciler to re-admit the
+	// ingresscontroller.  The purpose of this update is to verify that
+	// re-admission does not cause the operator to update the existing
+	// ingresscontroller with the new default LB type.
+	if err := kclient.Update(context.TODO(), clbIC); err != nil {
+		t.Fatalf("failed to update ingresscontroller %s: %v", clbName, err)
+	}
+	
+	nlbName := types.NamespacedName{Namespace: operatorNamespace, Name: "aws-lb-type-defaulting-to-nlb"}
+	nlbIC := newLoadBalancerController(nlbName, nlbName.Name+"."+dnsConfig.Spec.BaseDomain)
+	t.Logf("creating ingresscontroller %s with default LB type, which is now NLB", nlbName)
+	if err := kclient.Create(context.TODO(), nlbIC); err != nil {
+		t.Fatalf("failed to create ingresscontroller %s: %v", nlbName, err)
+	}
+	defer assertIngressControllerDeleted(t, kclient, nlbIC)
+
+	if err := waitForIngressControllerCondition(t, kclient, 5*time.Minute, nlbName, availableConditionsForIngressControllerWithLoadBalancer...); err != nil {
+		t.Fatalf("failed to observe expected conditions: %v", err)
+	}
+
+	t.Logf("verifying that ingresscontroller %s's LoadBalancer service specifies NLB", nlbName)
+	assertServiceAnnotation(t, controller.LoadBalancerServiceName(nlbIC), ingresscontroller.AWSLBTypeAnnotation, ingresscontroller.AWSNLBAnnotation, true)
+
+	t.Logf("verifying that ingresscontroller %s's LoadBalancer service still doesn't specify NLB", clbName)
+	// This is step is to verify that updating the cluster ingress config
+	// doesn't impact ingresscontrollers that have previously taken the
+	// default setting.
+	assertServiceAnnotation(t, controller.LoadBalancerServiceName(clbIC), ingresscontroller.AWSLBTypeAnnotation, "", false)
+
+	t.Log("resetting the default LB type to Classic")
+	if err := updateIngressConfigSpecWithRetryOnConflict(t, clusterConfigName, timeout, func(spec *configv1.IngressSpec) {
+		spec.LoadBalancer.Platform.Type = configv1.AWSPlatformType
+		spec.LoadBalancer.Platform.AWS = &configv1.AWSIngressSpec{
+			Type: configv1.NLB,
+		}
+	}); err != nil {
+		t.Fatalf("failed to update ingress config: %v", err)
+	}
+
+	t.Logf("verifying that ingresscontroller %s's LoadBalancer service still specifies NLB", nlbName)
+	assertServiceAnnotation(t, controller.LoadBalancerServiceName(clbIC), ingresscontroller.AWSLBTypeAnnotation, "", false)
+	t.Logf("verifying that ingresscontroller %s's LoadBalancer service still doesn't specify NLB", clbName)
+	assertServiceAnnotation(t, controller.LoadBalancerServiceName(nlbIC), ingresscontroller.AWSLBTypeAnnotation, ingresscontroller.AWSNLBAnnotation, true)
+}
+
 // TestScopeChange creates an ingresscontroller with the "LoadBalancerService"
 // endpoint publishing strategy type and verifies that the operator behaves
 // correctly when the ingresscontroller's scope is mutated.  The correct
@@ -3490,6 +3587,27 @@ func deleteIngressController(t *testing.T, cl client.Client, ic *operatorv1.Ingr
 	return nil
 }
 
+// assertServiceAnnotation asserts that the given service has the specified
+// annotation set (or not) as specified by the expectedValue and expected
+// arguments.
+func assertServiceAnnotation(t *testing.T, serviceName types.NamespacedName, annotationKey, expectedValue string, expected bool) {
+	t.Helper()
+
+	service := &corev1.Service{}
+	if err := kclient.Get(context.TODO(), serviceName, service); err != nil {
+		t.Fatalf("expected service %s to be present: %v", serviceName, err)
+	}
+	actualValue, ok := service.Annotations[annotationKey]
+	switch {
+	case !expected && ok:
+		t.Errorf("service %s has unexpected %s=%s annotation", serviceName, annotationKey, actualValue)
+	case expected && !ok:
+		t.Errorf("service %s is missing expected %s=%s annotation: %v", serviceName, annotationKey, expectedValue, service.Annotations)
+	case expected && expectedValue != actualValue:
+		t.Errorf("expected service %[1]s to have annotation %[2]s=%[3]s, found %[2]s=%[4]s", serviceName, annotationKey, expectedValue, actualValue)
+	}
+}
+
 // assertServiceNotDeleted asserts that a provide service wasn't deleted.
 func assertServiceNotDeleted(t *testing.T, serviceName types.NamespacedName, oldUid types.UID) {
 	t.Helper()

pkg/operator/controller/ingress/controller.go

pkg/operator/operator.go

The cluster ingress config object can now specify the AWS ELB type that was specified when installing the cluster. The ingress operator will refer to this object whenever someone deletes the default ingresscontroller and the operator has to recreate it. Before this commit, the operator would always set the default ELB type (Classic ELB) when creating the default ingresscontroller. This commit implements NE-975. https://issues.redhat.com/browse/NE-975 * pkg/operator/ingress/controller.go (Reconcile): Pass the result of isAdmitted to admit. (admit): Add an "alreadyAdmitted" parameter to indicate whether the ingresscontroller has previously been admitted and is being re-admitted. Pass alreadyAdmitted and ingressConfig to setDefaultPublishingStrategy. (setDefaultPublishingStrategy): Add parameters for the cluster ingress config and the ingresscontroller's admitted status. Refactor some defaulting logic into the new setDefaultProviderParameters function. (setDefaultProviderParameters): New function. Initialize the given EndpointPublishingStrategy's ProviderParameters field and subfields as appropriate, using the cluster ingress config to set a default LB type on AWS if the ingresscontroller is being admitted for the first time. * pkg/operator/controller/ingress/controller_test.go (TestSetDefaultPublishingStrategySetsPlatformDefaults): Add test cases to verify the default LB type that is specified in the cluster ingress config is used for AWS. (TestSetDefaultPublishingStrategyHandlesUpdates): Add test cases to verify that changing the default LB type in the cluster ingress config does not change the LB type on an already admitted ingresscontroller. * pkg/operator/operator.go (Start): Get the cluster ingress config and pass it to ensureDefaultIngressController. (determineReplicasForDefaultIngressController): Delete function. This is no longer needed after some refactoring in ensureDefaultIngressController. (ensureDefaultIngressController): Add a parameter for the cluster ingress config. Use it to determine desired replicas and to determine the default ELB type on AWS. * test/e2e/operator_test.go (TestAWSLBTypeDefaulting): New test for LB type defaulting. * test/e2e/all_test.go (TestAll): Add new E2E test. Modified-by: Grant Spence <[email protected]> Modified-by: Miciah Dashiel Butler Masters <[email protected]>

gcs278 · 2022-10-14T18:27:38Z

/lgtm
🥇

openshift-ci · 2022-10-14T18:27:39Z

@gcs278: you cannot LGTM your own PR.

Details

In response to this:

/lgtm
🥇

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.

miheer · 2022-10-14T18:29:47Z

/lgtm

miheer · 2022-10-14T18:34:02Z

/hold cancel

openshift-ci-robot · 2022-10-14T20:16:22Z

/retest-required

Remaining retests: 0 against base HEAD 24e1863 and 2 for PR HEAD 8fedfb6 in total

openshift-ci-robot · 2022-10-15T02:16:23Z

/retest-required

Remaining retests: 0 against base HEAD 6f63bd3 and 1 for PR HEAD 8fedfb6 in total

Miciah · 2022-10-15T07:23:12Z

#839 and #842 have merged.
/test all

openshift-ci-robot · 2022-10-15T09:16:19Z

/retest-required

Remaining retests: 0 against base HEAD 68121ce and 0 for PR HEAD 8fedfb6 in total

Miciah · 2022-10-15T09:51:43Z

/test e2e-azure-operator
/test e2e-gcp-operator

openshift-ci · 2022-10-15T10:36:38Z

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

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-aws-ovn-single-node	`8fedfb6`	link	false	`/test e2e-aws-ovn-single-node`

Full PR test history. Your PR dashboard.

Details

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.

openshift-ci bot requested review from alebedev87 and knobunc October 11, 2022 21:43

openshift-ci bot assigned Miciah and rfredette Oct 11, 2022

gcs278 force-pushed the NE-942 branch 2 times, most recently from cbcc588 to f158d2e Compare October 11, 2022 22:11

openshift-ci bot changed the title ~~In the infra config cluster object cluster object we need to store AwsLBType info for persistence. #798~~ NE-975: Use ingress config to set default LB type on AWS Oct 11, 2022

openshift-ci bot added the px-approved Signifies that Product Support has signed off on this PR label Oct 12, 2022

openshift-ci bot assigned miheer Oct 12, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 12, 2022

openshift-ci bot assigned gcs278 Oct 12, 2022

openshift-ci bot added the docs-approved Signifies that Docs has signed off on this PR label Oct 12, 2022

miheer force-pushed the NE-942 branch from f158d2e to 4d294f1 Compare October 12, 2022 18:03

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 12, 2022

miheer force-pushed the NE-942 branch 2 times, most recently from b45db80 to b21bd96 Compare October 12, 2022 19:02

gcs278 mentioned this pull request Oct 12, 2022

In the infra config cluster object cluster object we need to store AwsLBType info for persistence. #798

Closed

Miciah reviewed Oct 12, 2022

View reviewed changes

miheer force-pushed the NE-942 branch from b21bd96 to 3bb32ad Compare October 12, 2022 19:29

gcs278 mentioned this pull request Oct 12, 2022

NE-975: Update openshift/api for updated ingress config loadBalancer fields openshift/cluster-config-operator#268

Merged

gcs278 force-pushed the NE-942 branch from 88a86ca to 11e6edb Compare October 14, 2022 00:37

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 14, 2022

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 14, 2022

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 14, 2022

gcs278 force-pushed the NE-942 branch from 11e6edb to cf3f25c Compare October 14, 2022 17:02

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 14, 2022

gcs278 commented Oct 14, 2022

View reviewed changes

pkg/operator/controller/ingress/controller.go Show resolved Hide resolved

pkg/operator/controller/ingress/controller.go Show resolved Hide resolved

pkg/operator/operator.go Show resolved Hide resolved

pkg/operator/operator.go Show resolved Hide resolved

Miciah force-pushed the NE-942 branch from cf3f25c to 8fedfb6 Compare October 14, 2022 18:17

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 14, 2022

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

openshift-merge-robot merged commit 6d63b3c into openshift:master Oct 15, 2022

This was referenced Oct 15, 2022

NE-1071: Default HAProxy maxconn value to 50000 for OCP 4.12) #836

Merged

OCPBUGS-2435: TestRouterCompressionOperation: Nil-pointer fix #843

Merged

Conversation

gcs278 commented Oct 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gcs278 commented Oct 11, 2022

Uh oh!

Miciah commented Oct 11, 2022 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miheer commented Oct 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miheer commented Oct 12, 2022

Uh oh!

CFields651 commented Oct 12, 2022

Uh oh!

miheer commented Oct 12, 2022

Uh oh!

gcs278 commented Oct 12, 2022

Uh oh!

candita commented Oct 12, 2022

Uh oh!

ShaunaDiaz commented Oct 12, 2022

Uh oh!

Miciah Oct 12, 2022

Choose a reason for hiding this comment

Uh oh!

Miciah Oct 12, 2022

Choose a reason for hiding this comment

Uh oh!

Miciah commented Oct 13, 2022

Uh oh!

miheer commented Oct 14, 2022

Uh oh!

miheer commented Oct 14, 2022

Uh oh!

openshift-ci bot commented Oct 14, 2022

Uh oh!

miheer commented Oct 14, 2022

Uh oh!

miheer commented Oct 14, 2022

Uh oh!

Miciah commented Oct 14, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gcs278 commented Oct 14, 2022

Uh oh!

openshift-ci bot commented Oct 14, 2022

Uh oh!

miheer commented Oct 14, 2022

Uh oh!

miheer commented Oct 14, 2022

Uh oh!

openshift-ci-robot commented Oct 14, 2022

Uh oh!

openshift-ci-robot commented Oct 15, 2022

Uh oh!

Miciah commented Oct 15, 2022

Uh oh!

openshift-ci-robot commented Oct 15, 2022

Uh oh!

Miciah commented Oct 15, 2022

Uh oh!

openshift-ci bot commented Oct 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

gcs278 commented Oct 11, 2022 •

edited

Loading

Miciah commented Oct 11, 2022 •

edited by openshift-ci bot

Loading

miheer commented Oct 12, 2022 •

edited

Loading