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()