From 8fedfb6f4262a202f51982020b92574754e27e19 Mon Sep 17 00:00:00 2001 From: Miheer Salunke Date: Tue, 5 Jul 2022 01:33:59 +1000 Subject: [PATCH] Use ingress config to set default LB type on AWS 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 Modified-by: Miciah Dashiel Butler Masters --- pkg/operator/controller/ingress/controller.go | 87 +++++++--- .../controller/ingress/controller_test.go | 155 +++++++++++++++++- pkg/operator/operator.go | 41 +++-- test/e2e/all_test.go | 1 + test/e2e/operator_test.go | 118 +++++++++++++ 5 files changed, 366 insertions(+), 36 deletions(-) diff --git a/pkg/operator/controller/ingress/controller.go b/pkg/operator/controller/ingress/controller.go index 950ec905b6..2aec7765be 100644 --- a/pkg/operator/controller/ingress/controller.go +++ b/pkg/operator/controller/ingress/controller.go @@ -265,8 +265,9 @@ func (r *reconciler) Reconcile(ctx context.Context, request reconcile.Request) ( // Admit if necessary. Don't process until admission succeeds. If admission is // successful, immediately re-queue to refresh state. - if !isAdmitted(ingress) || needsReadmission(ingress) { - if err := r.admit(ingress, ingressConfig, platformStatus, dnsConfig); err != nil { + alreadyAdmitted := isAdmitted(ingress) + if !alreadyAdmitted || needsReadmission(ingress) { + if err := r.admit(ingress, ingressConfig, platformStatus, dnsConfig, alreadyAdmitted); err != nil { switch err := err.(type) { case *admissionRejection: r.recorder.Event(ingress, "Warning", "Rejected", err.Reason) @@ -320,7 +321,7 @@ func (r *reconciler) Reconcile(ctx context.Context, request reconcile.Request) ( // fields. Returns an error value, which will have a non-nil value of type // admissionRejection if the ingresscontroller was rejected, or a non-nil // value of a different type if the ingresscontroller could not be processed. -func (r *reconciler) admit(current *operatorv1.IngressController, ingressConfig *configv1.Ingress, platformStatus *configv1.PlatformStatus, dnsConfig *configv1.DNS) error { +func (r *reconciler) admit(current *operatorv1.IngressController, ingressConfig *configv1.Ingress, platformStatus *configv1.PlatformStatus, dnsConfig *configv1.DNS, alreadyAdmitted bool) error { updated := current.DeepCopy() setDefaultDomain(updated, ingressConfig) @@ -329,7 +330,7 @@ func (r *reconciler) admit(current *operatorv1.IngressController, ingressConfig // so that we can set the appropriate dnsManagementPolicy. This can only be // done after status.domain has been updated in setDefaultDomain(). domainMatchesBaseDomain := manageDNSForDomain(updated.Status.Domain, platformStatus, dnsConfig) - setDefaultPublishingStrategy(updated, platformStatus, domainMatchesBaseDomain) + setDefaultPublishingStrategy(updated, platformStatus, domainMatchesBaseDomain, ingressConfig, alreadyAdmitted) // The TLS security profile need not be defaulted. If none is set, we // get the default from the APIServer config (which is assumed to be @@ -410,7 +411,7 @@ func setDefaultDomain(ic *operatorv1.IngressController, ingressConfig *configv1. return false } -func setDefaultPublishingStrategy(ic *operatorv1.IngressController, platformStatus *configv1.PlatformStatus, domainMatchesBaseDomain bool) bool { +func setDefaultPublishingStrategy(ic *operatorv1.IngressController, platformStatus *configv1.PlatformStatus, domainMatchesBaseDomain bool, ingressConfig *configv1.Ingress, alreadyAdmitted bool) bool { effectiveStrategy := ic.Spec.EndpointPublishingStrategy.DeepCopy() if effectiveStrategy == nil { var strategyType operatorv1.EndpointPublishingStrategyType @@ -441,6 +442,9 @@ func setDefaultPublishingStrategy(ic *operatorv1.IngressController, platformStat effectiveStrategy.LoadBalancer.DNSManagementPolicy = operatorv1.UnmanagedLoadBalancerDNS } + // Set provider parameters based on the cluster ingress config. + setDefaultProviderParameters(effectiveStrategy.LoadBalancer, ingressConfig, alreadyAdmitted) + case operatorv1.NodePortServiceStrategyType: if effectiveStrategy.NodePort == nil { effectiveStrategy.NodePort = &operatorv1.NodePortStrategy{} @@ -519,13 +523,8 @@ func setDefaultPublishingStrategy(ic *operatorv1.IngressController, platformStat if statusLB.ProviderParameters.AWS == nil { statusLB.ProviderParameters.AWS = &operatorv1.AWSLoadBalancerParameters{} } - // Assume the LB type is "Classic" if the field is empty. - specLBType := operatorv1.AWSClassicLoadBalancer - if specLB.ProviderParameters.AWS != nil && len(specLB.ProviderParameters.AWS.Type) != 0 { - specLBType = specLB.ProviderParameters.AWS.Type - } - if specLBType != statusLB.ProviderParameters.AWS.Type { - statusLB.ProviderParameters.AWS.Type = specLBType + if specLB.ProviderParameters.AWS.Type != statusLB.ProviderParameters.AWS.Type { + statusLB.ProviderParameters.AWS.Type = specLB.ProviderParameters.AWS.Type changed = true } if statusLB.ProviderParameters.AWS.Type == operatorv1.AWSClassicLoadBalancer { @@ -552,18 +551,17 @@ func setDefaultPublishingStrategy(ic *operatorv1.IngressController, platformStat case operatorv1.GCPLoadBalancerProvider: // The only provider parameter that is supported // for GCP is the ClientAccess parameter. - var specClientAccess, statusClientAccess operatorv1.GCPClientAccess - if specLB.ProviderParameters != nil && specLB.ProviderParameters.GCP != nil { - specClientAccess = specLB.ProviderParameters.GCP.ClientAccess - } + var statusClientAccess operatorv1.GCPClientAccess + specClientAccess := specLB.ProviderParameters.GCP.ClientAccess if statusLB.ProviderParameters != nil && statusLB.ProviderParameters.GCP != nil { statusClientAccess = statusLB.ProviderParameters.GCP.ClientAccess } if specClientAccess != statusClientAccess { if statusLB.ProviderParameters == nil { - statusLB.ProviderParameters = &operatorv1.ProviderLoadBalancerParameters{ - Type: operatorv1.GCPLoadBalancerProvider, - } + statusLB.ProviderParameters = &operatorv1.ProviderLoadBalancerParameters{} + } + if len(statusLB.ProviderParameters.Type) == 0 { + statusLB.ProviderParameters.Type = operatorv1.GCPLoadBalancerProvider } if statusLB.ProviderParameters.GCP == nil { statusLB.ProviderParameters.GCP = &operatorv1.GCPLoadBalancerParameters{} @@ -633,6 +631,57 @@ func setDefaultPublishingStrategy(ic *operatorv1.IngressController, platformStat return false } +// setDefaultProviderParameters mutates the given LoadBalancerStrategy by +// defaulting its ProviderParameters field based on the defaults in the provided +// ingress config object. +func setDefaultProviderParameters(lbs *operatorv1.LoadBalancerStrategy, ingressConfig *configv1.Ingress, alreadyAdmitted bool) { + var provider operatorv1.LoadBalancerProviderType + if lbs.ProviderParameters != nil { + provider = lbs.ProviderParameters.Type + } + if len(provider) == 0 && !alreadyAdmitted { + // Infer the LB type from the cluster ingress config, but only + // if the ingresscontroller isn't already admitted. + switch ingressConfig.Spec.LoadBalancer.Platform.Type { + case configv1.AWSPlatformType: + provider = operatorv1.AWSLoadBalancerProvider + } + } + switch provider { + case operatorv1.AWSLoadBalancerProvider: + if lbs.ProviderParameters == nil { + lbs.ProviderParameters = &operatorv1.ProviderLoadBalancerParameters{} + } + lbs.ProviderParameters.Type = provider + defaultLBType := operatorv1.AWSClassicLoadBalancer + if p := ingressConfig.Spec.LoadBalancer.Platform; !alreadyAdmitted && p.Type == configv1.AWSPlatformType && p.AWS != nil { + if p.AWS.Type == configv1.NLB { + defaultLBType = operatorv1.AWSNetworkLoadBalancer + } + } + if lbs.ProviderParameters.AWS == nil { + lbs.ProviderParameters.AWS = &operatorv1.AWSLoadBalancerParameters{} + } + if len(lbs.ProviderParameters.AWS.Type) == 0 { + lbs.ProviderParameters.AWS.Type = defaultLBType + } + switch lbs.ProviderParameters.AWS.Type { + case operatorv1.AWSClassicLoadBalancer: + if lbs.ProviderParameters.AWS.ClassicLoadBalancerParameters == nil { + lbs.ProviderParameters.AWS.ClassicLoadBalancerParameters = &operatorv1.AWSClassicLoadBalancerParameters{} + } + } + case operatorv1.GCPLoadBalancerProvider: + if lbs.ProviderParameters == nil { + lbs.ProviderParameters = &operatorv1.ProviderLoadBalancerParameters{} + } + lbs.ProviderParameters.Type = provider + if lbs.ProviderParameters.GCP == nil { + lbs.ProviderParameters.GCP = &operatorv1.GCPLoadBalancerParameters{} + } + } +} + // tlsProfileSpecForIngressController returns a TLS profile spec based on either // the profile specified by the given ingresscontroller, the profile specified // by the APIServer config if the ingresscontroller does not specify one, or the diff --git a/pkg/operator/controller/ingress/controller_test.go b/pkg/operator/controller/ingress/controller_test.go index bf3be209e2..f7b622587b 100644 --- a/pkg/operator/controller/ingress/controller_test.go +++ b/pkg/operator/controller/ingress/controller_test.go @@ -123,6 +123,41 @@ func TestSetDefaultPublishingStrategySetsPlatformDefaults(t *testing.T) { }, }, } + ingressControllerWithAWSClassicLB = &operatorv1.IngressController{ + Status: operatorv1.IngressControllerStatus{ + EndpointPublishingStrategy: &operatorv1.EndpointPublishingStrategy{ + Type: operatorv1.LoadBalancerServiceStrategyType, + LoadBalancer: &operatorv1.LoadBalancerStrategy{ + DNSManagementPolicy: operatorv1.ManagedLoadBalancerDNS, + Scope: operatorv1.ExternalLoadBalancer, + ProviderParameters: &operatorv1.ProviderLoadBalancerParameters{ + Type: operatorv1.AWSLoadBalancerProvider, + AWS: &operatorv1.AWSLoadBalancerParameters{ + Type: operatorv1.AWSClassicLoadBalancer, + ClassicLoadBalancerParameters: &operatorv1.AWSClassicLoadBalancerParameters{}, + }, + }, + }, + }, + }, + } + ingressControllerWithAWSNLB = &operatorv1.IngressController{ + Status: operatorv1.IngressControllerStatus{ + EndpointPublishingStrategy: &operatorv1.EndpointPublishingStrategy{ + Type: operatorv1.LoadBalancerServiceStrategyType, + LoadBalancer: &operatorv1.LoadBalancerStrategy{ + DNSManagementPolicy: operatorv1.ManagedLoadBalancerDNS, + Scope: operatorv1.ExternalLoadBalancer, + ProviderParameters: &operatorv1.ProviderLoadBalancerParameters{ + Type: operatorv1.AWSLoadBalancerProvider, + AWS: &operatorv1.AWSLoadBalancerParameters{ + Type: operatorv1.AWSNetworkLoadBalancer, + }, + }, + }, + }, + }, + } ingressControllerWithLoadBalancerUnmanagedDNS = &operatorv1.IngressController{ Status: operatorv1.IngressControllerStatus{ EndpointPublishingStrategy: &operatorv1.EndpointPublishingStrategy{ @@ -152,11 +187,37 @@ func TestSetDefaultPublishingStrategySetsPlatformDefaults(t *testing.T) { Type: platform, } } + + ingressConfigWithDefaultClassicLB = &configv1.Ingress{ + Spec: configv1.IngressSpec{ + LoadBalancer: configv1.LoadBalancer{ + Platform: configv1.IngressPlatformSpec{ + Type: configv1.AWSPlatformType, + AWS: &configv1.AWSIngressSpec{ + Type: configv1.Classic, + }, + }, + }, + }, + } + ingressConfigWithDefaultNLB = &configv1.Ingress{ + Spec: configv1.IngressSpec{ + LoadBalancer: configv1.LoadBalancer{ + Platform: configv1.IngressPlatformSpec{ + Type: configv1.AWSPlatformType, + AWS: &configv1.AWSIngressSpec{ + Type: configv1.NLB, + }, + }, + }, + }, + } ) testCases := []struct { name string platformStatus *configv1.PlatformStatus + ingressConfig *configv1.Ingress expectedIC *operatorv1.IngressController domainMatchesBaseDomain bool }{ @@ -172,6 +233,20 @@ func TestSetDefaultPublishingStrategySetsPlatformDefaults(t *testing.T) { expectedIC: ingressControllerWithLoadBalancer, domainMatchesBaseDomain: true, }, + { + name: "AWS with ingress config specifying Classic LB", + platformStatus: makePlatformStatus(configv1.AWSPlatformType), + ingressConfig: ingressConfigWithDefaultClassicLB, + expectedIC: ingressControllerWithAWSClassicLB, + domainMatchesBaseDomain: true, + }, + { + name: "AWS with ingress config specifying NLB", + platformStatus: makePlatformStatus(configv1.AWSPlatformType), + ingressConfig: ingressConfigWithDefaultNLB, + expectedIC: ingressControllerWithAWSNLB, + domainMatchesBaseDomain: true, + }, { name: "Azure", platformStatus: makePlatformStatus(configv1.AzurePlatformType), @@ -255,7 +330,12 @@ func TestSetDefaultPublishingStrategySetsPlatformDefaults(t *testing.T) { t.Run(tc.name, func(t *testing.T) { ic := &operatorv1.IngressController{} platformStatus := tc.platformStatus.DeepCopy() - if actualResult := setDefaultPublishingStrategy(ic, platformStatus, tc.domainMatchesBaseDomain); actualResult != true { + ingressConfig := tc.ingressConfig + if ingressConfig == nil { + ingressConfig = &configv1.Ingress{} + } + alreadyAdmitted := false + if actualResult := setDefaultPublishingStrategy(ic, platformStatus, tc.domainMatchesBaseDomain, ingressConfig, alreadyAdmitted); actualResult != true { t.Errorf("expected result %v, got %v", true, actualResult) } if diff := cmp.Diff(tc.expectedIC, ic); len(diff) != 0 { @@ -407,11 +487,37 @@ func TestSetDefaultPublishingStrategyHandlesUpdates(t *testing.T) { Type: operatorv1.PrivateStrategyType, } } + + ingressConfigWithDefaultNLB = &configv1.Ingress{ + Spec: configv1.IngressSpec{ + LoadBalancer: configv1.LoadBalancer{ + Platform: configv1.IngressPlatformSpec{ + Type: configv1.AWSPlatformType, + AWS: &configv1.AWSIngressSpec{ + Type: configv1.NLB, + }, + }, + }, + }, + } + ingressConfigWithDefaultClassicLB = &configv1.Ingress{ + Spec: configv1.IngressSpec{ + LoadBalancer: configv1.LoadBalancer{ + Platform: configv1.IngressPlatformSpec{ + Type: configv1.AWSPlatformType, + AWS: &configv1.AWSIngressSpec{ + Type: configv1.Classic, + }, + }, + }, + }, + } ) testCases := []struct { name string ic *operatorv1.IngressController + ingressConfig *configv1.Ingress expectedIC *operatorv1.IngressController expectedResult bool domainMatchesBaseDomain bool @@ -458,6 +564,30 @@ func TestSetDefaultPublishingStrategyHandlesUpdates(t *testing.T) { expectedIC: makeIC(spec(elb()), status(elbWithNullParameters())), domainMatchesBaseDomain: true, }, + { + name: "loadbalancer type changed from NLB to unset, with Classic LB as default", + ic: makeIC(spec(eps(lbs(operatorv1.ExternalLoadBalancer, &managedDNS))), status(nlb())), + ingressConfig: ingressConfigWithDefaultClassicLB, + expectedResult: false, + expectedIC: makeIC(spec(eps(lbs(operatorv1.ExternalLoadBalancer, &managedDNS))), status(nlb())), + domainMatchesBaseDomain: true, + }, + { + name: "loadbalancer type changed from Classic LB to unset, with NLB as default", + ic: makeIC(spec(eps(lbs(operatorv1.ExternalLoadBalancer, &managedDNS))), status(elb())), + ingressConfig: ingressConfigWithDefaultNLB, + expectedResult: false, + expectedIC: makeIC(spec(eps(lbs(operatorv1.ExternalLoadBalancer, &managedDNS))), status(elb())), + domainMatchesBaseDomain: true, + }, + { + name: "loadbalancer type changed from NLB to unset, with NLB as default", + ic: makeIC(spec(eps(lbs(operatorv1.ExternalLoadBalancer, &managedDNS))), status(nlb())), + ingressConfig: ingressConfigWithDefaultNLB, + expectedResult: false, + expectedIC: makeIC(spec(eps(lbs(operatorv1.ExternalLoadBalancer, &managedDNS))), status(nlb())), + domainMatchesBaseDomain: true, + }, { name: "loadbalancer ELB connection idle timeout changed from unset with null provider parameters to 2m", ic: makeIC(spec(elbWithIdleTimeout(metav1.Duration{Duration: 2 * time.Minute})), status(elbWithNullParameters())), @@ -649,6 +779,22 @@ func TestSetDefaultPublishingStrategyHandlesUpdates(t *testing.T) { expectedIC: makeIC(spec(nil), status(eps(lbs(operatorv1.ExternalLoadBalancer, &unmanagedDNS)))), domainMatchesBaseDomain: false, }, + { + name: "when endpointPublishingStrategy is nil and lbType in ingress config is set to Classic", + ic: makeIC(spec(nil), status(nil)), + ingressConfig: ingressConfigWithDefaultClassicLB, + expectedResult: true, + expectedIC: makeIC(spec(nil), status(eps(lbs(operatorv1.ExternalLoadBalancer, &managedDNS)))), + domainMatchesBaseDomain: true, + }, + { + name: "when endpointPublishingStrategy is nil and lbType in ingress config is set to NLB", + ic: makeIC(spec(nil), status(nil)), + ingressConfig: ingressConfigWithDefaultNLB, + expectedResult: true, + expectedIC: makeIC(spec(nil), status(eps(lbs(operatorv1.ExternalLoadBalancer, &managedDNS)))), + domainMatchesBaseDomain: true, + }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { @@ -656,7 +802,12 @@ func TestSetDefaultPublishingStrategyHandlesUpdates(t *testing.T) { platformStatus := &configv1.PlatformStatus{ Type: configv1.AWSPlatformType, } - if actualResult := setDefaultPublishingStrategy(ic, platformStatus, tc.domainMatchesBaseDomain); actualResult != tc.expectedResult { + ingressConfig := tc.ingressConfig + if ingressConfig == nil { + ingressConfig = &configv1.Ingress{} + } + alreadyAdmitted := true + if actualResult := setDefaultPublishingStrategy(ic, platformStatus, tc.domainMatchesBaseDomain, ingressConfig, alreadyAdmitted); actualResult != tc.expectedResult { t.Errorf("expected result %v, got %v", tc.expectedResult, actualResult) } if diff := cmp.Diff(tc.expectedIC, ic); len(diff) != 0 { diff --git a/pkg/operator/operator.go b/pkg/operator/operator.go index 8b95e34b5e..e14763526a 100644 --- a/pkg/operator/operator.go +++ b/pkg/operator/operator.go @@ -236,7 +236,13 @@ func (o *Operator) Start(ctx context.Context) error { log.Error(nil, "failed to sync cache before ensuring default ingresscontroller") return } - err := o.ensureDefaultIngressController(infraConfig) + ingressConfigName := operatorcontroller.IngressClusterConfigName() + ingressConfig := &configv1.Ingress{} + if err := o.client.Get(context.TODO(), ingressConfigName, ingressConfig); err != nil { + log.Error(err, "failed to fetch ingress config") + return + } + err := o.ensureDefaultIngressController(infraConfig, ingressConfig) if err != nil { log.Error(err, "failed to ensure default ingresscontroller") } @@ -262,15 +268,6 @@ func (o *Operator) Start(ctx context.Context) error { } } -func (o *Operator) determineReplicasForDefaultIngressController(infraConfig *configv1.Infrastructure) (int32, error) { - ingressConfig := &configv1.Ingress{} - if err := o.client.Get(context.TODO(), types.NamespacedName{Name: "cluster"}, ingressConfig); err != nil { - return 0, fmt.Errorf("failed fetching ingress config: %w", err) - } - - return ingress.DetermineReplicas(ingressConfig, infraConfig), nil -} - // handleSingleNode4Dot11Upgrade sets the defaultPlacement status in the // ingress config CR of none-platform single node clusters to "ControlPlane" if // it's not already set. The situations in which this value is not set are in @@ -343,7 +340,7 @@ func (o *Operator) handleSingleNode4Dot11Upgrade() error { // ensureDefaultIngressController creates the default ingresscontroller if it // doesn't already exist. -func (o *Operator) ensureDefaultIngressController(infraConfig *configv1.Infrastructure) error { +func (o *Operator) ensureDefaultIngressController(infraConfig *configv1.Infrastructure, ingressConfig *configv1.Ingress) error { name := types.NamespacedName{Namespace: o.namespace, Name: manifests.DefaultIngressControllerName} ic := &operatorv1.IngressController{} if err := o.client.Get(context.TODO(), name, ic); err == nil { @@ -356,10 +353,7 @@ func (o *Operator) ensureDefaultIngressController(infraConfig *configv1.Infrastr // persisted value will be nil, which causes GETs on the /scale // subresource to fail, which breaks the scaling client. See also: // https://github.com/kubernetes/kubernetes/pull/75210 - replicas, err := o.determineReplicasForDefaultIngressController(infraConfig) - if err != nil { - return fmt.Errorf("failed to determine number of replicas for default ingress controller: %w", err) - } + replicas := ingress.DetermineReplicas(ingressConfig, infraConfig) ic = &operatorv1.IngressController{ ObjectMeta: metav1.ObjectMeta{ @@ -370,6 +364,23 @@ func (o *Operator) ensureDefaultIngressController(infraConfig *configv1.Infrastr Replicas: &replicas, }, } + if ingressConfig.Spec.LoadBalancer.Platform.Type == configv1.AWSPlatformType { + if ingressConfig.Spec.LoadBalancer.Platform.AWS != nil && ingressConfig.Spec.LoadBalancer.Platform.AWS.Type == configv1.NLB { + ic.Spec.EndpointPublishingStrategy = &operatorv1.EndpointPublishingStrategy{ + Type: operatorv1.LoadBalancerServiceStrategyType, + LoadBalancer: &operatorv1.LoadBalancerStrategy{ + Scope: "External", + ProviderParameters: &operatorv1.ProviderLoadBalancerParameters{ + Type: operatorv1.AWSLoadBalancerProvider, + AWS: &operatorv1.AWSLoadBalancerParameters{ + Type: operatorv1.AWSNetworkLoadBalancer, + }, + }, + }, + } + } + } + if err := o.client.Create(context.TODO(), ic); err != nil { return err } diff --git a/test/e2e/all_test.go b/test/e2e/all_test.go index f9a3fd817a..67e63dab36 100644 --- a/test/e2e/all_test.go +++ b/test/e2e/all_test.go @@ -84,6 +84,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 4b926f380f..5a5554f2f5 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()