From 8b768d2d18d8f889f47e99c72c9a10de6ebcb15a Mon Sep 17 00:00:00 2001 From: miheer Date: Sat, 27 Jul 2024 18:38:48 +1000 Subject: [PATCH] NE-1674: Add LB EIP Allocation for AWS Adds implementation for the new IngressController AWS eipAllocation API. It takes the eipAllocations specified on the IngressController and propagates them to the service.beta.kubernetes.io/aws-load-balancer-eip-allocations annotation on the LB-type Service. This design requires a cluster admin to manually delete the LB-type Service in order to effectuate the eipAllocation update. Once the eipAllocations are updated in the IngressController spec, a LoadBalancerProgressing=True condition will be added to notify the cluster admin to delete the Service to effectuate the eipAllocation change. This change is being introduced under the Tech Preview SetEIPForNLBIngressController feature gate and will be later promoted to GA. --- pkg/operator/controller/ingress/controller.go | 9 +- .../ingress/load_balancer_service.go | 155 +++++- .../ingress/load_balancer_service_test.go | 129 ++++- pkg/operator/controller/ingress/status.go | 33 +- .../controller/ingress/status_test.go | 262 ++++++++- pkg/operator/operator.go | 10 +- test/e2e/all_test.go | 2 + test/e2e/aws_util_test.go | 164 ++++++ test/e2e/lb_eip_test.go | 508 ++++++++++++++++++ test/e2e/operator_test.go | 17 + test/e2e/util_test.go | 56 ++ 11 files changed, 1286 insertions(+), 59 deletions(-) create mode 100644 test/e2e/aws_util_test.go create mode 100644 test/e2e/lb_eip_test.go diff --git a/pkg/operator/controller/ingress/controller.go b/pkg/operator/controller/ingress/controller.go index d711fc630..97010f7c1 100644 --- a/pkg/operator/controller/ingress/controller.go +++ b/pkg/operator/controller/ingress/controller.go @@ -189,10 +189,11 @@ func enqueueRequestForOwningIngressController(namespace string) handler.EventHan // Config holds all the things necessary for the controller to run. type Config struct { - Namespace string - IngressControllerImage string - RouteExternalCertificateEnabled bool - IngressControllerLBSubnetsAWSEnabled bool + Namespace string + IngressControllerImage string + RouteExternalCertificateEnabled bool + IngressControllerLBSubnetsAWSEnabled bool + IngressControllerEIPAllocationsAWSEnabled bool } // reconciler handles the actual ingress reconciliation logic in response to diff --git a/pkg/operator/controller/ingress/load_balancer_service.go b/pkg/operator/controller/ingress/load_balancer_service.go index 90a13f074..d45af7fe9 100644 --- a/pkg/operator/controller/ingress/load_balancer_service.go +++ b/pkg/operator/controller/ingress/load_balancer_service.go @@ -1,6 +1,7 @@ package ingress import ( + "bytes" "context" "encoding/json" "fmt" @@ -88,6 +89,9 @@ const ( // awsLBSubnetsAnnotation specifies a list of subnets for both NLBs and CLBs. awsLBSubnetsAnnotation = "service.beta.kubernetes.io/aws-load-balancer-subnets" + // awsEIPAllocationsAnnotation specifies a list of eips for NLBs. + awsEIPAllocationsAnnotation = "service.beta.kubernetes.io/aws-load-balancer-eip-allocations" + // iksLBScopeAnnotation is the annotation used on a service to specify an IBM // load balancer IP type. iksLBScopeAnnotation = "service.kubernetes.io/ibm-load-balancer-cloud-provider-ip-type" @@ -276,7 +280,7 @@ var ( // Always returns the current LB service if one exists (whether it already // existed or was created during the course of the function). func (r *reconciler) ensureLoadBalancerService(ci *operatorv1.IngressController, deploymentRef metav1.OwnerReference, platformStatus *configv1.PlatformStatus) (bool, *corev1.Service, error) { - wantLBS, desiredLBService, err := desiredLoadBalancerService(ci, deploymentRef, platformStatus, r.config.IngressControllerLBSubnetsAWSEnabled) + wantLBS, desiredLBService, err := desiredLoadBalancerService(ci, deploymentRef, platformStatus, r.config.IngressControllerLBSubnetsAWSEnabled, r.config.IngressControllerEIPAllocationsAWSEnabled) if err != nil { return false, nil, err } @@ -342,7 +346,7 @@ func isServiceOwnedByIngressController(service *corev1.Service, ic *operatorv1.I // ingresscontroller, or nil if an LB service isn't desired. An LB service is // desired if the high availability type is Cloud. An LB service will declare an // owner reference to the given deployment. -func desiredLoadBalancerService(ci *operatorv1.IngressController, deploymentRef metav1.OwnerReference, platform *configv1.PlatformStatus, subnetsAWSEnabled bool) (bool, *corev1.Service, error) { +func desiredLoadBalancerService(ci *operatorv1.IngressController, deploymentRef metav1.OwnerReference, platform *configv1.PlatformStatus, subnetsAWSEnabled bool, eipAllocationsAWSEnabled bool) (bool, *corev1.Service, error) { if ci.Status.EndpointPublishingStrategy.Type != operatorv1.LoadBalancerServiceStrategyType { return false, nil, nil } @@ -416,6 +420,14 @@ func desiredLoadBalancerService(ci *operatorv1.IngressController, deploymentRef service.Annotations[awsLBSubnetsAnnotation] = JoinAWSSubnets(nlbParams.Subnets, ",") } } + + if eipAllocationsAWSEnabled { + nlbParams := getAWSNetworkLoadBalancerParametersInSpec(ci) + if nlbParams != nil && awsEIPAllocationsExist(nlbParams.EIPAllocations) { + service.Annotations[awsEIPAllocationsAnnotation] = JoinAWSEIPAllocations(nlbParams.EIPAllocations, ",") + } + } + case operatorv1.AWSClassicLoadBalancer: if aws.ClassicLoadBalancerParameters != nil { if v := aws.ClassicLoadBalancerParameters.ConnectionIdleTimeout; v.Duration > 0 { @@ -646,6 +658,9 @@ func shouldRecreateLoadBalancer(current, desired *corev1.Service, platform *conf if platform.Type == configv1.AWSPlatformType && !serviceSubnetsEqual(current, desired) { return true, "its subnets changed" } + if platform.Type == configv1.AWSPlatformType && !serviceEIPAllocationsEqual(current, desired) { + return true, "its eipAllocations changed" + } return false, "" } @@ -748,8 +763,8 @@ func loadBalancerServiceTagsModified(current, expected *corev1.Service) (bool, * // return value is nil. Otherwise, if something or someone else has modified // the service, then the return value is a non-nil error indicating that the // modification must be reverted before upgrading is allowed. -func loadBalancerServiceIsUpgradeable(ic *operatorv1.IngressController, deploymentRef metav1.OwnerReference, current *corev1.Service, platform *configv1.PlatformStatus, subnetsAWSEnabled bool) error { - want, desired, err := desiredLoadBalancerService(ic, deploymentRef, platform, subnetsAWSEnabled) +func loadBalancerServiceIsUpgradeable(ic *operatorv1.IngressController, deploymentRef metav1.OwnerReference, current *corev1.Service, platform *configv1.PlatformStatus, subnetsAWSEnabled bool, eipAllocationsAWSEnabled bool) error { + want, desired, err := desiredLoadBalancerService(ic, deploymentRef, platform, subnetsAWSEnabled, eipAllocationsAWSEnabled) if err != nil { return err } @@ -769,7 +784,7 @@ func loadBalancerServiceIsUpgradeable(ic *operatorv1.IngressController, deployme // loadBalancerServiceIsProgressing returns an error value indicating if the // load balancer service is in progressing status. -func loadBalancerServiceIsProgressing(ic *operatorv1.IngressController, service *corev1.Service, platform *configv1.PlatformStatus, subnetsAWSEnabled bool) error { +func loadBalancerServiceIsProgressing(ic *operatorv1.IngressController, service *corev1.Service, platform *configv1.PlatformStatus, subnetsAWSEnabled bool, eipAllocationsAWSEnabled bool) error { var errs []error wantScope := ic.Status.EndpointPublishingStrategy.LoadBalancer.Scope haveScope := operatorv1.ExternalLoadBalancer @@ -821,12 +836,52 @@ func loadBalancerServiceIsProgressing(ic *operatorv1.IngressController, service } } + if platform.Type == configv1.AWSPlatformType && eipAllocationsAWSEnabled && getAWSLoadBalancerTypeInStatus(ic) == operatorv1.AWSNetworkLoadBalancer { + var ( + wantEIPAllocations, haveEIPAllocations []operatorv1.EIPAllocation + ) + if nlbParams := getAWSNetworkLoadBalancerParametersInSpec(ic); nlbParams != nil { + wantEIPAllocations = nlbParams.EIPAllocations + } + if nlbParams := getAWSNetworkLoadBalancerParametersInStatus(ic); nlbParams != nil { + haveEIPAllocations = nlbParams.EIPAllocations + } + if !awsEIPAllocationsEqual(wantEIPAllocations, haveEIPAllocations) { + // Generate JSON for the oc patch command as well as "pretty" json + // that will be used for a more human-readable error message. + haveEIPAllocationsPatchJson := convertAWSEIPAllocationsListToPatchJson(haveEIPAllocations, "null") + haveEIPAllocationsPrettyJson := convertAWSEIPAllocationsListToPatchJson(haveEIPAllocations, "[]") + wantEIPAllocationsPrettyJson := convertAWSEIPAllocationsListToPatchJson(wantEIPAllocations, "[]") + changedMsg := fmt.Sprintf("The IngressController eipAllocations were changed from %q to %q.", haveEIPAllocationsPrettyJson, wantEIPAllocationsPrettyJson) + ocPatchRevertCmd := fmt.Sprintf("oc -n openshift-ingress-operator patch ingresscontrollers/%[1]s --type=merge --patch='{\"spec\":{\"endpointPublishingStrategy\":{\"type\":\"LoadBalancerService\",\"loadBalancer\":{\"providerParameters\":{\"type\":\"AWS\",\"aws\":{\"type\":\"%[2]s\",\"%[3]s\":{\"eipAllocations\":%[4]s}}}}}}}'", ic.Name, getAWSLoadBalancerTypeInStatus(ic), "networkLoadBalancer", haveEIPAllocationsPatchJson) + err := fmt.Errorf("%[1]s To effectuate this change, you must delete the service: `oc -n %[2]s delete svc/%[3]s`; the service load-balancer will then be deprovisioned and a new one created. This will most likely cause the new load-balancer to have a different host name and IP address and cause disruption. To return to the previous state, you can revert the change to the IngressController: `%[4]s`", changedMsg, service.Namespace, service.Name, ocPatchRevertCmd) + errs = append(errs, err) + } + } + errs = append(errs, loadBalancerSourceRangesAnnotationSet(service)) errs = append(errs, loadBalancerSourceRangesMatch(ic, service)) return kerrors.NewAggregate(errs) } +// convertAWSEIPAllocationsListToPatchJson converts an AWSEIPAllocations object to a JSON formatted string +// to build an oc patch command. It defaults nil or no eipAllocation values i.e an empty slice to emptyEIPAllocationValue +func convertAWSEIPAllocationsListToPatchJson(eipAllocations []operatorv1.EIPAllocation, emptyEIPAllocationValue string) string { + // If eipAllocations are nil, or an empty list, return the emptyEIPAllocationValue. + if len(eipAllocations) == 0 { + return emptyEIPAllocationValue + } + + // Marshal eipAllocations. + eipAllocationsJSONBytes, err := json.Marshal(eipAllocations) + if err != nil { + log.Error(err, "error marshaling eipAllocations") + return "" + } + return string(eipAllocationsJSONBytes) +} + // convertAWSSubnetListToPatchJson converts an AWSSubnets object to a JSON formatted string // to build an oc patch command. It defaults nil or no subnet values to emptySubnetValue // and empty ids or names slices to emptySubnetSliceValue. @@ -946,12 +1001,81 @@ func getSubnetsFromServiceAnnotation(service *corev1.Service) *operatorv1.AWSSub return awsSubnets } +// getEIPAllocationsFromServiceAnnotation gets the effective eipAllocations by looking at the +// service.beta.kubernetes.io/aws-load-balancer-eip-allocations annotation of the LoadBalancer-type Service. +// If no eipAllocations are specified in the annotation, this function returns nil. +func getEIPAllocationsFromServiceAnnotation(service *corev1.Service) []operatorv1.EIPAllocation { + if service == nil { + return nil + } + + var awsEIPAllocations []operatorv1.EIPAllocation + if a, ok := service.Annotations[awsEIPAllocationsAnnotation]; ok { + var eipAllocations []string + a = strings.TrimSpace(a) + if len(a) > 0 { + eipAllocations = strings.Split(a, ",") + } + + // Cast the slice of strings to EIPAllocations object. + for _, eipAllocation := range eipAllocations { + awsEIPAllocations = append(awsEIPAllocations, operatorv1.EIPAllocation(eipAllocation)) + } + } + + return awsEIPAllocations +} + // serviceSubnetsEqual compares the subnet annotations on two services to determine if they are equivalent, // ignoring the order of the subnets. func serviceSubnetsEqual(a, b *corev1.Service) bool { return awsSubnetsEqual(getSubnetsFromServiceAnnotation(a), getSubnetsFromServiceAnnotation(b)) } +func serviceEIPAllocationsEqual(a, b *corev1.Service) bool { + return awsEIPAllocationsEqual(getEIPAllocationsFromServiceAnnotation(a), getEIPAllocationsFromServiceAnnotation(b)) +} + +// awsEIPAllocationsEqual compares two AWSEIPAllocation slices and returns a boolean +// whether they are equal are not. The order of the EIP Allocations are ignored. +func awsEIPAllocationsEqual(eipAllocations1, eipAllocations2 []operatorv1.EIPAllocation) bool { + // If they are both nil, they are equal. + if eipAllocations1 == nil && eipAllocations2 == nil { + return true + } + + // If one is nil and the other is not, they are equal only if the non-nil one is empty. + if eipAllocations1 == nil { + return len(eipAllocations2) == 0 + } + if eipAllocations2 == nil { + return len(eipAllocations1) == 0 + } + + // If they both are non-nil, compare the length first, then do a more detailed comparison if needed. + if len(eipAllocations1) != len(eipAllocations2) { + return false + } + + // Create maps to track the IDs from each eipAllocation object for comparison. + eipAllocationMap1 := make(map[operatorv1.EIPAllocation]struct{}) + eipAllocationMap2 := make(map[operatorv1.EIPAllocation]struct{}) + for _, eipAllocation := range eipAllocations1 { + eipAllocationMap1[eipAllocation] = struct{}{} + } + for _, eipAllocation := range eipAllocations2 { + eipAllocationMap2[eipAllocation] = struct{}{} + } + // Check if maps contain the same eipAllocations. + for eipAllocation := range eipAllocationMap1 { + if _, found := eipAllocationMap2[eipAllocation]; !found { + return false + } + } + + return true +} + // awsSubnetsEqual compares two AWSSubnets objects and returns a boolean // whether they are equal are not. The order of the subnets is ignored. func awsSubnetsEqual(subnets1, subnets2 *operatorv1.AWSSubnets) bool { @@ -1012,6 +1136,10 @@ func awsSubnetsExist(subnets *operatorv1.AWSSubnets) bool { return subnets != nil && (len(subnets.Names) > 0 || len(subnets.IDs) > 0) } +func awsEIPAllocationsExist(eipAllocations []operatorv1.EIPAllocation) bool { + return len(eipAllocations) > 0 +} + // JoinAWSSubnets joins an AWS Subnets object into a string seperated by sep. func JoinAWSSubnets(subnets *operatorv1.AWSSubnets, sep string) string { if subnets == nil { @@ -1036,6 +1164,23 @@ func JoinAWSSubnets(subnets *operatorv1.AWSSubnets, sep string) string { return joinedSubnets } +// JoinAWSEIPAllocations joins an AWS EIPAllocations object into a string seperated by sep. +func JoinAWSEIPAllocations(eipAllocations []operatorv1.EIPAllocation, sep string) string { + var buffer bytes.Buffer + first := true + for _, eipAllocation := range eipAllocations { + if len(string(eipAllocation)) != 0 { + if !first { + buffer.WriteString(sep) + } else { + first = false + } + buffer.WriteString(string(eipAllocation)) + } + } + return buffer.String() +} + // getAWSLoadBalancerTypeInStatus gets the AWS Load Balancer Type reported in the status. func getAWSLoadBalancerTypeInStatus(ic *operatorv1.IngressController) operatorv1.AWSLoadBalancerType { if ic.Status.EndpointPublishingStrategy != nil && diff --git a/pkg/operator/controller/ingress/load_balancer_service_test.go b/pkg/operator/controller/ingress/load_balancer_service_test.go index a48e5b30c..cb0f61ea4 100644 --- a/pkg/operator/controller/ingress/load_balancer_service_test.go +++ b/pkg/operator/controller/ingress/load_balancer_service_test.go @@ -81,6 +81,22 @@ func Test_desiredLoadBalancerService(t *testing.T) { } return eps } + + // nlbWithEIPAllocations returns an AWS NLB with the specified EIP allocations. + nlbWithEIPAllocations = func(scope operatorv1.LoadBalancerScope, eipAllocations []operatorv1.EIPAllocation) *operatorv1.EndpointPublishingStrategy { + eps := lbs(scope) + eps.LoadBalancer.ProviderParameters = &operatorv1.ProviderLoadBalancerParameters{ + Type: operatorv1.AWSLoadBalancerProvider, + AWS: &operatorv1.AWSLoadBalancerParameters{ + Type: operatorv1.AWSNetworkLoadBalancer, + NetworkLoadBalancerParameters: &operatorv1.AWSNetworkLoadBalancerParameters{ + EIPAllocations: eipAllocations, + }, + }, + } + return eps + } + // gcpLB returns an EndpointPublishingStrategy with type // "LoadBalancerService" and the specified scope and with // providerParameters set with the specified GCP ClientAccess @@ -102,15 +118,16 @@ func Test_desiredLoadBalancerService(t *testing.T) { value string } testCases := []struct { - description string - strategySpec *operatorv1.EndpointPublishingStrategy - strategyStatus *operatorv1.EndpointPublishingStrategy - proxyNeeded bool - expectService bool - expectedServiceAnnotations map[string]annotationExpectation - expectedExternalTrafficPolicy corev1.ServiceExternalTrafficPolicy - platformStatus *configv1.PlatformStatus - subnetsAWSFeatureEnabled bool + description string + strategySpec *operatorv1.EndpointPublishingStrategy + strategyStatus *operatorv1.EndpointPublishingStrategy + proxyNeeded bool + expectService bool + expectedServiceAnnotations map[string]annotationExpectation + expectedExternalTrafficPolicy corev1.ServiceExternalTrafficPolicy + platformStatus *configv1.PlatformStatus + subnetsAWSFeatureEnabled bool + eipAllocationsAWSFeatureEnabled bool }{ { description: "external classic load balancer with scope for aws platform", @@ -437,6 +454,87 @@ func Test_desiredLoadBalancerService(t *testing.T) { awsLBSubnetsAnnotation: {true, "subnet-00000000000000001,subnet-00000000000000002,subnetA,subnetB"}, }, }, + { + description: "network load balancer with eipAllocations for aws platform when feature gate is enabled", + platformStatus: platformStatus(configv1.AWSPlatformType), + strategySpec: nlbWithEIPAllocations(operatorv1.ExternalLoadBalancer, + []operatorv1.EIPAllocation{"eipalloc-xxxxxxxxxxxxxxxxx", "eipalloc-yyyyyyyyyyyyyyyyy"}, + ), + strategyStatus: nlbWithEIPAllocations(operatorv1.ExternalLoadBalancer, + []operatorv1.EIPAllocation{"eipalloc-xxxxxxxxxxxxxxxxx", "eipalloc-yyyyyyyyyyyyyyyyy"}, + ), + proxyNeeded: false, + expectService: true, + eipAllocationsAWSFeatureEnabled: true, + expectedExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyLocal, + expectedServiceAnnotations: map[string]annotationExpectation{ + awsInternalLBAnnotation: {false, ""}, + awsLBAdditionalResourceTags: {false, ""}, + awsLBHealthCheckHealthyThresholdAnnotation: {true, awsLBHealthCheckHealthyThresholdDefault}, + awsLBHealthCheckIntervalAnnotation: {true, awsLBHealthCheckIntervalNLB}, + awsLBHealthCheckTimeoutAnnotation: {true, awsLBHealthCheckTimeoutDefault}, + awsLBHealthCheckUnhealthyThresholdAnnotation: {true, awsLBHealthCheckUnhealthyThresholdDefault}, + awsLBProxyProtocolAnnotation: {false, ""}, + AWSLBTypeAnnotation: {true, AWSNLBAnnotation}, + localWithFallbackAnnotation: {true, ""}, + awsLBSubnetsAnnotation: {false, ""}, + awsEIPAllocationsAnnotation: {true, "eipalloc-xxxxxxxxxxxxxxxxx,eipalloc-yyyyyyyyyyyyyyyyy"}, + }, + }, + { + description: "network load balancer with eipAllocations for aws platform when feature gate is disabled", + platformStatus: platformStatus(configv1.AWSPlatformType), + strategySpec: nlbWithEIPAllocations(operatorv1.ExternalLoadBalancer, + []operatorv1.EIPAllocation{"eipalloc-xxxxxxxxxxxxxxxxx", "eipalloc-yyyyyyyyyyyyyyyyy"}, + ), + strategyStatus: nlbWithEIPAllocations(operatorv1.ExternalLoadBalancer, + []operatorv1.EIPAllocation{"eipalloc-xxxxxxxxxxxxxxxxx", "eipalloc-yyyyyyyyyyyyyyyyy"}, + ), + proxyNeeded: false, + expectService: true, + eipAllocationsAWSFeatureEnabled: false, + expectedExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyLocal, + expectedServiceAnnotations: map[string]annotationExpectation{ + awsInternalLBAnnotation: {false, ""}, + awsLBAdditionalResourceTags: {false, ""}, + awsLBHealthCheckHealthyThresholdAnnotation: {true, awsLBHealthCheckHealthyThresholdDefault}, + awsLBHealthCheckIntervalAnnotation: {true, awsLBHealthCheckIntervalNLB}, + awsLBHealthCheckTimeoutAnnotation: {true, awsLBHealthCheckTimeoutDefault}, + awsLBHealthCheckUnhealthyThresholdAnnotation: {true, awsLBHealthCheckUnhealthyThresholdDefault}, + awsLBProxyProtocolAnnotation: {false, ""}, + AWSLBTypeAnnotation: {true, AWSNLBAnnotation}, + localWithFallbackAnnotation: {true, ""}, + awsLBSubnetsAnnotation: {false, ""}, + awsEIPAllocationsAnnotation: {false, ""}, + }, + }, + { + description: "network load balancer with nil eipAllocations for aws platform when feature gate is enabled", + platformStatus: platformStatus(configv1.AWSPlatformType), + strategySpec: nlbWithEIPAllocations(operatorv1.ExternalLoadBalancer, + nil, + ), + strategyStatus: nlbWithEIPAllocations(operatorv1.ExternalLoadBalancer, + nil, + ), + proxyNeeded: false, + expectService: true, + eipAllocationsAWSFeatureEnabled: true, + expectedExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyLocal, + expectedServiceAnnotations: map[string]annotationExpectation{ + awsInternalLBAnnotation: {false, ""}, + awsLBAdditionalResourceTags: {false, ""}, + awsLBHealthCheckHealthyThresholdAnnotation: {true, awsLBHealthCheckHealthyThresholdDefault}, + awsLBHealthCheckIntervalAnnotation: {true, awsLBHealthCheckIntervalNLB}, + awsLBHealthCheckTimeoutAnnotation: {true, awsLBHealthCheckTimeoutDefault}, + awsLBHealthCheckUnhealthyThresholdAnnotation: {true, awsLBHealthCheckUnhealthyThresholdDefault}, + awsLBProxyProtocolAnnotation: {false, ""}, + AWSLBTypeAnnotation: {true, AWSNLBAnnotation}, + localWithFallbackAnnotation: {true, ""}, + awsLBSubnetsAnnotation: {false, ""}, + awsEIPAllocationsAnnotation: {false, ""}, + }, + }, { description: "nodePort service for aws platform", platformStatus: platformStatus(configv1.AWSPlatformType), @@ -635,7 +733,7 @@ func Test_desiredLoadBalancerService(t *testing.T) { t.Errorf("expected IsProxyProtocolNeeded to return %v, got %v", tc.proxyNeeded, proxyNeeded) } - haveSvc, svc, err := desiredLoadBalancerService(ic, deploymentRef, infraConfig.Status.PlatformStatus, tc.subnetsAWSFeatureEnabled) + haveSvc, svc, err := desiredLoadBalancerService(ic, deploymentRef, infraConfig.Status.PlatformStatus, tc.subnetsAWSFeatureEnabled, tc.eipAllocationsAWSFeatureEnabled) switch { case err != nil: t.Error(err) @@ -819,7 +917,7 @@ func TestDesiredLoadBalancerServiceAWSIdleTimeout(t *testing.T) { }, }, } - haveSvc, svc, err := desiredLoadBalancerService(ic, deploymentRef, infraConfig.Status.PlatformStatus, true) + haveSvc, svc, err := desiredLoadBalancerService(ic, deploymentRef, infraConfig.Status.PlatformStatus, true, true) if err != nil { t.Fatal(err) } @@ -1074,6 +1172,13 @@ func Test_loadBalancerServiceChanged(t *testing.T) { }, expect: true, }, + { + description: "if the service.beta.kubernetes.io/aws-load-balancer-eip-allocations annotation added", + mutate: func(svc *corev1.Service) { + svc.Annotations["service.beta.kubernetes.io/aws-load-balancer-eip-allocations"] = "eipalloc-xxxxxxxxxxxxxxxxx,eipalloc-yyyyyyyyyyyyyyyyy" + }, + expect: false, + }, } for _, tc := range testCases { @@ -1419,7 +1524,7 @@ func TestUpdateLoadBalancerServiceSourceRanges(t *testing.T) { }, }, } - wantSvc, desired, err := desiredLoadBalancerService(ic, deploymentRef, infraConfig.Status.PlatformStatus, true) + wantSvc, desired, err := desiredLoadBalancerService(ic, deploymentRef, infraConfig.Status.PlatformStatus, true, true) if err != nil { t.Fatal(err) } diff --git a/pkg/operator/controller/ingress/status.go b/pkg/operator/controller/ingress/status.go index 73313976c..99d41d3ee 100644 --- a/pkg/operator/controller/ingress/status.go +++ b/pkg/operator/controller/ingress/status.go @@ -77,20 +77,23 @@ func (r *reconciler) syncIngressControllerStatus(ic *operatorv1.IngressControlle if platformStatus.Type == configv1.AWSPlatformType && r.config.IngressControllerLBSubnetsAWSEnabled { updateIngressControllerAWSSubnetStatus(updated, service) } + if platformStatus.Type == configv1.AWSPlatformType && r.config.IngressControllerEIPAllocationsAWSEnabled { + updateIngressControllerAWSEIPAllocationStatus(updated, service) + } updated.Status.Conditions = MergeConditions(updated.Status.Conditions, computeDeploymentAvailableCondition(deployment)) updated.Status.Conditions = MergeConditions(updated.Status.Conditions, computeDeploymentReplicasMinAvailableCondition(deployment, pods)) updated.Status.Conditions = MergeConditions(updated.Status.Conditions, computeDeploymentReplicasAllAvailableCondition(deployment)) updated.Status.Conditions = MergeConditions(updated.Status.Conditions, computeDeploymentRollingOutCondition(deployment)) updated.Status.Conditions = MergeConditions(updated.Status.Conditions, computeLoadBalancerStatus(ic, service, operandEvents)...) - updated.Status.Conditions = MergeConditions(updated.Status.Conditions, computeLoadBalancerProgressingStatus(updated, service, platformStatus, r.config.IngressControllerLBSubnetsAWSEnabled)) + updated.Status.Conditions = MergeConditions(updated.Status.Conditions, computeLoadBalancerProgressingStatus(updated, service, platformStatus, r.config.IngressControllerLBSubnetsAWSEnabled, r.config.IngressControllerEIPAllocationsAWSEnabled)) updated.Status.Conditions = MergeConditions(updated.Status.Conditions, computeDNSStatus(ic, wildcardRecord, platformStatus, dnsConfig)...) updated.Status.Conditions = MergeConditions(updated.Status.Conditions, computeIngressAvailableCondition(updated.Status.Conditions)) degradedCondition, err := computeIngressDegradedCondition(updated.Status.Conditions, updated.Name) errs = append(errs, err) updated.Status.Conditions = MergeConditions(updated.Status.Conditions, computeIngressProgressingCondition(updated.Status.Conditions)) updated.Status.Conditions = MergeConditions(updated.Status.Conditions, degradedCondition) - updated.Status.Conditions = MergeConditions(updated.Status.Conditions, computeIngressUpgradeableCondition(ic, deploymentRef, service, platformStatus, secret, r.config.IngressControllerLBSubnetsAWSEnabled)) + updated.Status.Conditions = MergeConditions(updated.Status.Conditions, computeIngressUpgradeableCondition(ic, deploymentRef, service, platformStatus, secret, r.config.IngressControllerLBSubnetsAWSEnabled, r.config.IngressControllerEIPAllocationsAWSEnabled)) updated.Status.Conditions = MergeConditions(updated.Status.Conditions, computeIngressEvaluationConditionsDetectedCondition(ic, service)) updated.Status.Conditions = PruneConditions(updated.Status.Conditions) @@ -640,13 +643,13 @@ func computeIngressDegradedCondition(conditions []operatorv1.OperatorCondition, } // computeIngressUpgradeableCondition computes the IngressController's "Upgradeable" status condition. -func computeIngressUpgradeableCondition(ic *operatorv1.IngressController, deploymentRef metav1.OwnerReference, service *corev1.Service, platform *configv1.PlatformStatus, secret *corev1.Secret, subnetsAWSEnabled bool) operatorv1.OperatorCondition { +func computeIngressUpgradeableCondition(ic *operatorv1.IngressController, deploymentRef metav1.OwnerReference, service *corev1.Service, platform *configv1.PlatformStatus, secret *corev1.Secret, subnetsAWSEnabled bool, eipAllocationsAWSEnabled bool) operatorv1.OperatorCondition { var errs []error errs = append(errs, checkDefaultCertificate(secret, "*."+ic.Status.Domain)) if service != nil { - errs = append(errs, loadBalancerServiceIsUpgradeable(ic, deploymentRef, service, platform, subnetsAWSEnabled)) + errs = append(errs, loadBalancerServiceIsUpgradeable(ic, deploymentRef, service, platform, subnetsAWSEnabled, eipAllocationsAWSEnabled)) } if err := kerrors.NewAggregate(errs); err != nil { @@ -808,6 +811,9 @@ func IngressStatusesEqual(a, b operatorv1.IngressControllerStatus) bool { if !awsSubnetsEqual(a.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.NetworkLoadBalancerParameters.Subnets, b.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.NetworkLoadBalancerParameters.Subnets) { return false } + if !awsEIPAllocationsEqual(a.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.NetworkLoadBalancerParameters.EIPAllocations, b.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.NetworkLoadBalancerParameters.EIPAllocations) { + return false + } } if a.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.ClassicLoadBalancerParameters != nil && b.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.ClassicLoadBalancerParameters != nil { if !awsSubnetsEqual(a.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.ClassicLoadBalancerParameters.Subnets, b.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.ClassicLoadBalancerParameters.Subnets) { @@ -904,7 +910,7 @@ func computeLoadBalancerStatus(ic *operatorv1.IngressController, service *corev1 // computeLoadBalancerProgressingStatus returns the LoadBalancerProgressing // conditions for the given ingress controller. These conditions subsequently determine // the ingress controller's Progressing status. -func computeLoadBalancerProgressingStatus(ic *operatorv1.IngressController, service *corev1.Service, platform *configv1.PlatformStatus, subnetsAWSEnabled bool) operatorv1.OperatorCondition { +func computeLoadBalancerProgressingStatus(ic *operatorv1.IngressController, service *corev1.Service, platform *configv1.PlatformStatus, subnetsAWSEnabled bool, eipAllocationsAWSEnabled bool) operatorv1.OperatorCondition { // Compute the IngressControllerLoadBalancerProgressingConditionType condition for the LoadBalancer if ic.Status.EndpointPublishingStrategy.Type == operatorv1.LoadBalancerServiceStrategyType { switch { @@ -923,7 +929,7 @@ func computeLoadBalancerProgressingStatus(ic *operatorv1.IngressController, serv Message: "LoadBalancer Service not created.", } default: - if err := loadBalancerServiceIsProgressing(ic, service, platform, subnetsAWSEnabled); err != nil { + if err := loadBalancerServiceIsProgressing(ic, service, platform, subnetsAWSEnabled, eipAllocationsAWSEnabled); err != nil { return operatorv1.OperatorCondition{ Type: IngressControllerLoadBalancerProgressingConditionType, Status: operatorv1.ConditionTrue, @@ -972,6 +978,21 @@ func updateIngressControllerAWSSubnetStatus(ic *operatorv1.IngressController, se } } +// updateIngressControllerAWSEIPAllocationStatus mutates the provided IngressController object to +// sync its status to the effective eipAllocations on the LoadBalancer-type service. +func updateIngressControllerAWSEIPAllocationStatus(ic *operatorv1.IngressController, service *corev1.Service) { + // Set the eipAllocations status based on the actual service annotation and on the load balancer type `NLB`. + switch getAWSLoadBalancerTypeInStatus(ic) { + case operatorv1.AWSNetworkLoadBalancer: + // NetworkLoadBalancerParameters should be initialized by setDefaultPublishingStrategy + // when an IngressController is admitted, so we don't need to initialize here. + nlbParams := getAWSNetworkLoadBalancerParametersInStatus(ic) + if nlbParams != nil { + nlbParams.EIPAllocations = getEIPAllocationsFromServiceAnnotation(service) + } + } +} + func isProvisioned(service *corev1.Service) bool { ingresses := service.Status.LoadBalancer.Ingress return len(ingresses) > 0 && (len(ingresses[0].Hostname) > 0 || len(ingresses[0].IP) > 0) diff --git a/pkg/operator/controller/ingress/status_test.go b/pkg/operator/controller/ingress/status_test.go index 2a3fbb0eb..7502430ee 100644 --- a/pkg/operator/controller/ingress/status_test.go +++ b/pkg/operator/controller/ingress/status_test.go @@ -695,6 +695,38 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { return ic } + loadBalancerIngressControllerWithAWSEIPAllocations := func(eipAllocationSpec []operatorv1.EIPAllocation, eipAllocationStatus []operatorv1.EIPAllocation) *operatorv1.IngressController { + eps := &operatorv1.EndpointPublishingStrategy{ + Type: operatorv1.LoadBalancerServiceStrategyType, + LoadBalancer: &operatorv1.LoadBalancerStrategy{ + Scope: operatorv1.ExternalLoadBalancer, + ProviderParameters: &operatorv1.ProviderLoadBalancerParameters{ + Type: operatorv1.AWSLoadBalancerProvider, + AWS: &operatorv1.AWSLoadBalancerParameters{ + Type: operatorv1.AWSNetworkLoadBalancer, + }, + }, + }, + } + ic := &operatorv1.IngressController{ + Spec: operatorv1.IngressControllerSpec{ + EndpointPublishingStrategy: eps.DeepCopy(), + }, + Status: operatorv1.IngressControllerStatus{ + EndpointPublishingStrategy: eps.DeepCopy(), + }, + } + + ic.Spec.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.NetworkLoadBalancerParameters = &operatorv1.AWSNetworkLoadBalancerParameters{ + EIPAllocations: eipAllocationSpec, + } + ic.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.NetworkLoadBalancerParameters = &operatorv1.AWSNetworkLoadBalancerParameters{ + EIPAllocations: eipAllocationStatus, + } + + return ic + } + loadBalancerIngressControllerWithInternalScope := operatorv1.IngressController{ Status: operatorv1.IngressControllerStatus{ EndpointPublishingStrategy: &operatorv1.EndpointPublishingStrategy{ @@ -756,12 +788,14 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { Type: configv1.AzurePlatformType, } tests := []struct { - name string - conditions []operatorv1.OperatorCondition - ic *operatorv1.IngressController - service *corev1.Service - platformStatus *configv1.PlatformStatus - awsSubnetsEnabled bool + name string + conditions []operatorv1.OperatorCondition + ic *operatorv1.IngressController + service *corev1.Service + platformStatus *configv1.PlatformStatus + awsSubnetsEnabled bool + awsEIPAllocationsEnabled bool + expectStatus operatorv1.ConditionStatus expectMessageContains string expectMessageDoesNotContain string @@ -1104,10 +1138,109 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { platformStatus: awsPlatformStatus, expectStatus: operatorv1.ConditionTrue, }, + { + name: "NLB LoadBalancerService, AWS EIPAllocations nil spec and nil status", + ic: loadBalancerIngressControllerWithAWSEIPAllocations( + nil, + nil, + ), + service: &corev1.Service{}, + awsEIPAllocationsEnabled: true, + platformStatus: awsPlatformStatus, + expectStatus: operatorv1.ConditionFalse, + }, + { + name: "NLB LoadBalancerService, AWS EIPAllocations nil spec and empty status", + ic: loadBalancerIngressControllerWithAWSEIPAllocations( + nil, + []operatorv1.EIPAllocation{}, + ), + service: &corev1.Service{}, + awsEIPAllocationsEnabled: true, + platformStatus: awsPlatformStatus, + expectStatus: operatorv1.ConditionFalse, + }, + { + name: "NLB LoadBalancerService, AWS EIPAllocations spec with eipAllocations and nil status, but feature gate disabled", + ic: loadBalancerIngressControllerWithAWSEIPAllocations( + []operatorv1.EIPAllocation{"eipalloc-xxxxxxxxxxxxxxxxx", "eipalloc-yyyyyyyyyyyyyyyyy"}, + nil, + ), + service: &corev1.Service{}, + awsEIPAllocationsEnabled: false, + platformStatus: awsPlatformStatus, + expectStatus: operatorv1.ConditionFalse, + }, + { + name: "NLB LoadBalancerService, AWS EIPAllocations spec with eipAllocations and nil status", + ic: loadBalancerIngressControllerWithAWSEIPAllocations( + []operatorv1.EIPAllocation{"eipalloc-xxxxxxxxxxxxxxxxx", "eipalloc-yyyyyyyyyyyyyyyyy"}, + nil, + ), + service: &corev1.Service{}, + awsEIPAllocationsEnabled: true, + platformStatus: awsPlatformStatus, + expectStatus: operatorv1.ConditionTrue, + }, + { + name: "NLB LoadBalancerService, AWS EIPAllocation nil spec and status with eipAllocations", + ic: loadBalancerIngressControllerWithAWSEIPAllocations( + nil, + []operatorv1.EIPAllocation{"eipalloc-xxxxxxxxxxxxxxxxx", "eipalloc-yyyyyyyyyyyyyyyyy"}, + ), + service: &corev1.Service{}, + awsEIPAllocationsEnabled: true, + platformStatus: awsPlatformStatus, + expectStatus: operatorv1.ConditionTrue, + }, + { + name: "NLB LoadBalancerService, AWS EIPAllocations spec and status are equal", + ic: loadBalancerIngressControllerWithAWSEIPAllocations( + []operatorv1.EIPAllocation{"eipalloc-xxxxxxxxxxxxxxxxx", "eipalloc-yyyyyyyyyyyyyyyyy"}, + []operatorv1.EIPAllocation{"eipalloc-xxxxxxxxxxxxxxxxx", "eipalloc-yyyyyyyyyyyyyyyyy"}, + ), + service: &corev1.Service{}, + awsEIPAllocationsEnabled: true, + platformStatus: awsPlatformStatus, + expectStatus: operatorv1.ConditionFalse, + }, + { + name: "NLB LoadBalancerService, AWS EIPAllocations spec and status are NOT equal", + ic: loadBalancerIngressControllerWithAWSEIPAllocations( + []operatorv1.EIPAllocation{"eipalloc-xxxxxxxxxxxxxxxxx", "eipalloc-yyyyyyyyyyyyyyyyy"}, + []operatorv1.EIPAllocation{"eipalloc-aaaaaaaaaaaaaaaaa", "eipalloc-bbbbbbbbbbbbbbbbb"}, + ), + service: &corev1.Service{}, + awsEIPAllocationsEnabled: true, + platformStatus: awsPlatformStatus, + expectStatus: operatorv1.ConditionTrue, + }, + { + name: "NLB LoadBalancerService, AWS EIPAllocations spec and status are equal with different order", + ic: loadBalancerIngressControllerWithAWSEIPAllocations( + []operatorv1.EIPAllocation{"eipalloc-xxxxxxxxxxxxxxxxx", "eipalloc-yyyyyyyyyyyyyyyyy"}, + []operatorv1.EIPAllocation{"eipalloc-yyyyyyyyyyyyyyyyy", "eipalloc-xxxxxxxxxxxxxxxxx"}, + ), + service: &corev1.Service{}, + awsEIPAllocationsEnabled: true, + platformStatus: awsPlatformStatus, + expectStatus: operatorv1.ConditionFalse, + }, + { + name: "NLB LoadBalancerService, AWS EIPAllocations spec and status have extra items", + ic: loadBalancerIngressControllerWithAWSEIPAllocations( + []operatorv1.EIPAllocation{"eipalloc-xxxxxxxxxxxxxxxxx", "eipalloc-yyyyyyyyyyyyyyyyy", "eipalloc-zzzzzzzzzzzzz"}, + []operatorv1.EIPAllocation{"eipalloc-yyyyyyyyyyyyyyyyy", "eipalloc-xxxxxxxxxxxxxxxxx"}, + ), + service: &corev1.Service{}, + awsEIPAllocationsEnabled: true, + platformStatus: awsPlatformStatus, + expectStatus: operatorv1.ConditionTrue, + }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - actual := computeLoadBalancerProgressingStatus(test.ic, test.service, test.platformStatus, test.awsSubnetsEnabled) + actual := computeLoadBalancerProgressingStatus(test.ic, test.service, test.platformStatus, test.awsSubnetsEnabled, test.awsEIPAllocationsEnabled) if actual.Status != test.expectStatus { t.Errorf("expected status to be %s, got %s", test.expectStatus, actual.Status) } @@ -1732,7 +1865,7 @@ func Test_computeIngressAvailableCondition(t *testing.T) { } func Test_IngressStatusesEqual(t *testing.T) { - icStatusWithSubnets := func(lbType operatorv1.AWSLoadBalancerType, subnets *operatorv1.AWSSubnets) operatorv1.IngressControllerStatus { + icStatusWithSubnetsOrEIPAllocations := func(lbType operatorv1.AWSLoadBalancerType, subnets *operatorv1.AWSSubnets, eipAllocations []operatorv1.EIPAllocation) operatorv1.IngressControllerStatus { icStatus := operatorv1.IngressControllerStatus{ EndpointPublishingStrategy: &operatorv1.EndpointPublishingStrategy{ Type: operatorv1.LoadBalancerServiceStrategyType, @@ -1748,7 +1881,8 @@ func Test_IngressStatusesEqual(t *testing.T) { switch lbType { case operatorv1.AWSNetworkLoadBalancer: icStatus.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.NetworkLoadBalancerParameters = &operatorv1.AWSNetworkLoadBalancerParameters{ - Subnets: subnets, + Subnets: subnets, + EIPAllocations: eipAllocations, } case operatorv1.AWSClassicLoadBalancer: icStatus.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.ClassicLoadBalancerParameters = &operatorv1.AWSClassicLoadBalancerParameters{ @@ -1870,129 +2004,201 @@ func Test_IngressStatusesEqual(t *testing.T) { { description: "NLB Subnets names changed", expected: false, - a: icStatusWithSubnets( + a: icStatusWithSubnetsOrEIPAllocations( operatorv1.AWSNetworkLoadBalancer, &operatorv1.AWSSubnets{ Names: []operatorv1.AWSSubnetName{"name-567890"}, }, + nil, ), - b: icStatusWithSubnets( + b: icStatusWithSubnetsOrEIPAllocations( operatorv1.AWSNetworkLoadBalancer, &operatorv1.AWSSubnets{ Names: []operatorv1.AWSSubnetName{"name-123456"}, }, + nil, ), }, { description: "NLB Subnets names changed with multiple", expected: false, - a: icStatusWithSubnets( + a: icStatusWithSubnetsOrEIPAllocations( operatorv1.AWSNetworkLoadBalancer, &operatorv1.AWSSubnets{ Names: []operatorv1.AWSSubnetName{"name-123456", "name-890123"}, }, + nil, ), - b: icStatusWithSubnets( + b: icStatusWithSubnetsOrEIPAllocations( operatorv1.AWSNetworkLoadBalancer, &operatorv1.AWSSubnets{ Names: []operatorv1.AWSSubnetName{"name-123456"}, }, + nil, ), }, { description: "NLB Subnets names equal", expected: true, - a: icStatusWithSubnets( + a: icStatusWithSubnetsOrEIPAllocations( operatorv1.AWSNetworkLoadBalancer, &operatorv1.AWSSubnets{ Names: []operatorv1.AWSSubnetName{"name-123456"}, }, + nil, ), - b: icStatusWithSubnets( + b: icStatusWithSubnetsOrEIPAllocations( operatorv1.AWSNetworkLoadBalancer, &operatorv1.AWSSubnets{ Names: []operatorv1.AWSSubnetName{"name-123456"}, }, + nil, ), }, { description: "NLB Subnets different order names equal", expected: true, - a: icStatusWithSubnets( + a: icStatusWithSubnetsOrEIPAllocations( operatorv1.AWSNetworkLoadBalancer, &operatorv1.AWSSubnets{ Names: []operatorv1.AWSSubnetName{"name-123456", "name-890123"}, }, + nil, ), - b: icStatusWithSubnets( + b: icStatusWithSubnetsOrEIPAllocations( operatorv1.AWSNetworkLoadBalancer, &operatorv1.AWSSubnets{ Names: []operatorv1.AWSSubnetName{"name-890123", "name-123456"}, }, + nil, ), }, { description: "CLB Subnets IDs changed", expected: false, - a: icStatusWithSubnets( + a: icStatusWithSubnetsOrEIPAllocations( operatorv1.AWSClassicLoadBalancer, &operatorv1.AWSSubnets{ IDs: []operatorv1.AWSSubnetID{"subnet-890123"}, }, + nil, ), - b: icStatusWithSubnets( + b: icStatusWithSubnetsOrEIPAllocations( operatorv1.AWSClassicLoadBalancer, &operatorv1.AWSSubnets{ IDs: []operatorv1.AWSSubnetID{"subnet-123456"}, }, + nil, ), }, { description: "CLB Subnets IDs changed with multiple", expected: false, - a: icStatusWithSubnets( + a: icStatusWithSubnetsOrEIPAllocations( operatorv1.AWSClassicLoadBalancer, &operatorv1.AWSSubnets{ IDs: []operatorv1.AWSSubnetID{"subnet-123456", "subnet-890123"}, }, + nil, ), - b: icStatusWithSubnets( + b: icStatusWithSubnetsOrEIPAllocations( operatorv1.AWSClassicLoadBalancer, &operatorv1.AWSSubnets{ IDs: []operatorv1.AWSSubnetID{"subnet-123456"}, }, + nil, ), }, { description: "CLB Subnets IDs equal", expected: true, - a: icStatusWithSubnets( + a: icStatusWithSubnetsOrEIPAllocations( operatorv1.AWSClassicLoadBalancer, &operatorv1.AWSSubnets{ IDs: []operatorv1.AWSSubnetID{"name-123456"}, }, + nil, ), - b: icStatusWithSubnets( + b: icStatusWithSubnetsOrEIPAllocations( operatorv1.AWSClassicLoadBalancer, &operatorv1.AWSSubnets{ IDs: []operatorv1.AWSSubnetID{"name-123456"}, }, + nil, ), }, { description: "CLB Subnets different order IDs equal", expected: true, - a: icStatusWithSubnets( + a: icStatusWithSubnetsOrEIPAllocations( operatorv1.AWSClassicLoadBalancer, &operatorv1.AWSSubnets{ IDs: []operatorv1.AWSSubnetID{"subnet-123456", "subnet-890123"}, }, + nil, ), - b: icStatusWithSubnets( + b: icStatusWithSubnetsOrEIPAllocations( operatorv1.AWSClassicLoadBalancer, &operatorv1.AWSSubnets{ IDs: []operatorv1.AWSSubnetID{"subnet-890123", "subnet-123456"}, }, + nil, + ), + }, + { + description: "NLB EIPAllocations changed", + expected: false, + a: icStatusWithSubnetsOrEIPAllocations( + operatorv1.AWSNetworkLoadBalancer, + nil, + []operatorv1.EIPAllocation{"eipalloc-xxxxxxxxxxxxxxxxx"}, + ), + b: icStatusWithSubnetsOrEIPAllocations( + operatorv1.AWSNetworkLoadBalancer, + nil, + []operatorv1.EIPAllocation{"eipalloc-yyyyyyyyyyyyyyyyy"}, + ), + }, + { + description: "NLB EIPAllocations changed with multiple", + expected: false, + a: icStatusWithSubnetsOrEIPAllocations( + operatorv1.AWSNetworkLoadBalancer, + nil, + []operatorv1.EIPAllocation{"eipalloc-xxxxxxxxxxxxxxxxx", "eipalloc-yyyyyyyyyyyyyyyyy"}, + ), + b: icStatusWithSubnetsOrEIPAllocations( + operatorv1.AWSNetworkLoadBalancer, + nil, + []operatorv1.EIPAllocation{"eipalloc-xxxxxxxxxxxxxxxxx"}, + ), + }, + { + description: "NLB EIPAllocations equal", + expected: true, + a: icStatusWithSubnetsOrEIPAllocations( + operatorv1.AWSNetworkLoadBalancer, + nil, + []operatorv1.EIPAllocation{"eipalloc-xxxxxxxxxxxxxxxxx"}, + ), + b: icStatusWithSubnetsOrEIPAllocations( + operatorv1.AWSNetworkLoadBalancer, + nil, + []operatorv1.EIPAllocation{"eipalloc-xxxxxxxxxxxxxxxxx"}, + ), + }, + { + description: "NLB EIPAllocations different order but equal", + expected: true, + a: icStatusWithSubnetsOrEIPAllocations( + operatorv1.AWSNetworkLoadBalancer, + nil, + []operatorv1.EIPAllocation{"eipalloc-xxxxxxxxxxxxxxxxx", "eipalloc-yyyyyyyyyyyyyyyyy"}, + ), + b: icStatusWithSubnetsOrEIPAllocations( + operatorv1.AWSNetworkLoadBalancer, + nil, + []operatorv1.EIPAllocation{"eipalloc-yyyyyyyyyyyyyyyyy", "eipalloc-xxxxxxxxxxxxxxxxx"}, ), }, } @@ -2776,7 +2982,7 @@ func Test_computeIngressUpgradeableCondition(t *testing.T) { }, }, } - wantSvc, service, err := desiredLoadBalancerService(ic, deploymentRef, platformStatus, true) + wantSvc, service, err := desiredLoadBalancerService(ic, deploymentRef, platformStatus, true, true) if err != nil { t.Errorf("unexpected error from desiredLoadBalancerService: %v", err) return @@ -2798,7 +3004,7 @@ func Test_computeIngressUpgradeableCondition(t *testing.T) { expectedStatus = operatorv1.ConditionTrue } - actual := computeIngressUpgradeableCondition(ic, deploymentRef, service, platformStatus, secret, true) + actual := computeIngressUpgradeableCondition(ic, deploymentRef, service, platformStatus, secret, true, true) if actual.Status != expectedStatus { t.Errorf("expected Upgradeable to be %q, got %q", expectedStatus, actual.Status) } @@ -2886,7 +3092,7 @@ func Test_computeIngressEvaluationConditionsDetectedCondition(t *testing.T) { }, } - wantSvc, service, err := desiredLoadBalancerService(ic, deploymentRef, platformStatus, true) + wantSvc, service, err := desiredLoadBalancerService(ic, deploymentRef, platformStatus, true, true) if err != nil { t.Fatalf("unexpected error from desiredLoadBalancerService: %v", err) } diff --git a/pkg/operator/operator.go b/pkg/operator/operator.go index 7e287bc48..5c36e2db5 100644 --- a/pkg/operator/operator.go +++ b/pkg/operator/operator.go @@ -134,6 +134,7 @@ func New(config operatorconfig.Config, kubeConfig *rest.Config) (*Operator, erro gatewayAPIEnabled := featureGates.Enabled(features.FeatureGateGatewayAPI) routeExternalCertificateEnabled := featureGates.Enabled(features.FeatureGateRouteExternalCertificate) ingressControllerLBSubnetsAWSEnabled := featureGates.Enabled(features.FeatureGateIngressControllerLBSubnetsAWS) + ingressControllerEIPAllocationsAWSEnabled := featureGates.Enabled(features.FeatureGateSetEIPForNLBIngressController) // Set up an operator manager for the operator namespace. mgr, err := manager.New(kubeConfig, manager.Options{ @@ -166,10 +167,11 @@ func New(config operatorconfig.Config, kubeConfig *rest.Config) (*Operator, erro // Create and register the ingress controller with the operator manager. if _, err := ingresscontroller.New(mgr, ingresscontroller.Config{ - Namespace: config.Namespace, - IngressControllerImage: config.IngressControllerImage, - RouteExternalCertificateEnabled: routeExternalCertificateEnabled, - IngressControllerLBSubnetsAWSEnabled: ingressControllerLBSubnetsAWSEnabled, + Namespace: config.Namespace, + IngressControllerImage: config.IngressControllerImage, + RouteExternalCertificateEnabled: routeExternalCertificateEnabled, + IngressControllerLBSubnetsAWSEnabled: ingressControllerLBSubnetsAWSEnabled, + IngressControllerEIPAllocationsAWSEnabled: ingressControllerEIPAllocationsAWSEnabled, }); err != nil { return nil, fmt.Errorf("failed to create ingress controller: %v", err) } diff --git a/test/e2e/all_test.go b/test/e2e/all_test.go index 85b944a9a..f4ed961f5 100644 --- a/test/e2e/all_test.go +++ b/test/e2e/all_test.go @@ -86,6 +86,8 @@ func TestAll(t *testing.T) { t.Run("TestGatewayAPI", TestGatewayAPI) t.Run("TestAWSLBSubnets", TestAWSLBSubnets) t.Run("TestUnmanagedAWSLBSubnets", TestUnmanagedAWSLBSubnets) + t.Run("TestAWSEIPAllocationsForNLB", TestAWSEIPAllocationsForNLB) + t.Run("TestUnmanagedAWSEIPAllocations", TestUnmanagedAWSEIPAllocations) }) t.Run("serial", func(t *testing.T) { diff --git a/test/e2e/aws_util_test.go b/test/e2e/aws_util_test.go new file mode 100644 index 000000000..13d226885 --- /dev/null +++ b/test/e2e/aws_util_test.go @@ -0,0 +1,164 @@ +//go:build e2e +// +build e2e + +package e2e + +import ( + "context" + "fmt" + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/aws/credentials" + "github.com/aws/aws-sdk-go/aws/session" + "github.com/aws/aws-sdk-go/service/ec2" + + configv1 "github.com/openshift/api/config/v1" + + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/types" +) + +// createEC2ServiceClient creates an ec2 client using aws credentials and region. +func createEC2ServiceClient(t *testing.T, infraConfig configv1.Infrastructure) *ec2.EC2 { + decodedAccessKeyID, decodedSecretAccessKey, err := getIDAndKey() + if err != nil { + t.Fatalf("failed to get access key id and secret access key due to error: %v", err) + } + region, err := getRegion(infraConfig) + if err != nil { + t.Fatalf("failed to get aws region due to error: %v", err) + } + // Set up an AWS session + sess, err := createSession(region, decodedAccessKeyID, decodedSecretAccessKey) + if err != nil { + t.Fatalf("failed to create session due to error: %v", err) + } + // Create an EC2 service client + return ec2.New(sess) +} + +// getClusterName fetches cluster name from the infrastructures.config.openshift.io cluster object. +func getClusterName(infraConfig configv1.Infrastructure) (string, error) { + if len(infraConfig.Status.InfrastructureName) != 0 { + return infraConfig.Status.InfrastructureName, nil + } + return "", fmt.Errorf("cluster name not found") +} + +// getRegion fetches region from the infrastructures.config.openshift.io cluster object. +func getRegion(infraConfig configv1.Infrastructure) (string, error) { + if infraConfig.Status.PlatformStatus.AWS != nil && len(infraConfig.Status.PlatformStatus.AWS.Region) != 0 { + return infraConfig.Status.PlatformStatus.AWS.Region, nil + } + return "", fmt.Errorf("region not found") +} + +// AWSCloudCredSecretName returns the name of the secret containing root aws credentials. +func AWSCloudCredSecretName() types.NamespacedName { + return types.NamespacedName{Namespace: "kube-system", Name: "aws-creds"} +} + +// getIDAndKey fetches the aws credentials from the secret containing root aws credentials in kube-system namespace. +func getIDAndKey() (string, string, error) { + awsSecret := corev1.Secret{} + var accessKeyID, secretAccessKey string + if err := kclient.Get(context.TODO(), AWSCloudCredSecretName(), &awsSecret); err != nil { + return "", "", fmt.Errorf("failed to get secret: %w", err) + } + accessKeyID = string(awsSecret.Data["aws_access_key_id"]) + secretAccessKey = string(awsSecret.Data["aws_secret_access_key"]) + return accessKeyID, secretAccessKey, nil +} + +// getPublicSubnets fetches public subnets for a AWS VPC ID. +func getPublicSubnets(vpcID string, svc *ec2.EC2) (map[string]struct{}, error) { + // Get the Internet Gateway associated with the VPC + input := &ec2.DescribeInternetGatewaysInput{ + Filters: []*ec2.Filter{ + { + Name: aws.String("attachment.vpc-id"), + Values: []*string{aws.String(vpcID)}, + }, + }, + } + + resultIG, err := svc.DescribeInternetGateways(input) + if err != nil { + return nil, fmt.Errorf("Error describing Internet Gateways: %v", err) + } + + // Extract Internet Gateway IDs + var igws []*ec2.InternetGateway + for _, igw := range resultIG.InternetGateways { + igws = append(igws, igw) + } + + // Find public subnets associated with the Internet Gateways + publicSubnets := make(map[string]struct{}) + for _, igw := range igws { + input := &ec2.DescribeRouteTablesInput{ + Filters: []*ec2.Filter{ + { + Name: aws.String("route.gateway-id"), + Values: []*string{igw.InternetGatewayId}, + }, + }, + } + + result, err := svc.DescribeRouteTables(input) + if err != nil { + return nil, fmt.Errorf("Error describing Route Tables: %v", err) + } + + for _, rt := range result.RouteTables { + for _, assoc := range rt.Associations { + if assoc != nil && assoc.SubnetId != nil { + publicSubnets[aws.StringValue(assoc.SubnetId)] = struct{}{} + } + } + } + } + return publicSubnets, nil +} + +// createSession creates a session using decoded AWS credentials. +func createSession(region, decodedAccessKeyID, decodedSecretAccessKey string) (*session.Session, error) { + return session.NewSession(&aws.Config{ + Region: aws.String(region), + Credentials: credentials.NewStaticCredentials(decodedAccessKeyID, decodedSecretAccessKey, ""), + }) +} + +// getVPCId return the VPC ID of the cluster +func getVPCId(ec2Client *ec2.EC2, clusterName string) (string, error) { + tagKey := "kubernetes.io/cluster/" + clusterName + tagValue := "owned" + vpcs, err := ec2Client.DescribeVpcs(&ec2.DescribeVpcsInput{ + Filters: []*ec2.Filter{ + { + Name: aws.String("tag:" + tagKey), + Values: []*string{aws.String(tagValue)}, + }, + }, + }) + if err != nil { + return "", fmt.Errorf("failed to list VPC with tag %s:%s : %w", tagKey, tagValue, err) + } + if len(vpcs.Vpcs) == 0 { + return "", fmt.Errorf("no VPC with tag %s:%s found : %w", tagKey, tagValue, err) + } + if len(vpcs.Vpcs) > 1 { + return "", fmt.Errorf("multiple VPCs with tag %s:%s found", tagKey, tagValue) + } + return aws.StringValue(vpcs.Vpcs[0].VpcId), nil +} + +func getTagKeyAndValue(t *testing.T) (string, string) { + tagKeyEIP := fmt.Sprintf("NE1674-%s", t.Name()) + tagValueEIP, err := getClusterName(infraConfig) + if err != nil { + t.Fatal(err) + } + return tagKeyEIP, tagValueEIP +} diff --git a/test/e2e/lb_eip_test.go b/test/e2e/lb_eip_test.go new file mode 100644 index 000000000..9697ce30f --- /dev/null +++ b/test/e2e/lb_eip_test.go @@ -0,0 +1,508 @@ +//go:build e2e +// +build e2e + +package e2e + +import ( + "context" + random "crypto/rand" + "encoding/hex" + "fmt" + "reflect" + "testing" + "time" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" + + configv1 "github.com/openshift/api/config/v1" + "github.com/openshift/api/features" + operatorv1 "github.com/openshift/api/operator/v1" + "github.com/openshift/cluster-ingress-operator/pkg/operator/controller" + "github.com/openshift/cluster-ingress-operator/pkg/operator/controller/ingress" + + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/rand" + "k8s.io/apimachinery/pkg/util/wait" +) + +const ( + awsLBEIPAllocationAnnotation = "service.beta.kubernetes.io/aws-load-balancer-eip-allocations" +) + +// TestAWSEIPAllocationsForNLB creates an IngressController with various eipAllocations in AWS. +// The test verifies the provisioning of the LB-type Service, confirms ingress connectivity, and +// confirms that the service.beta.kubernetes.io/aws-load-balancer-eip-allocations as well as the IngressController status is +// correctly configured. +func TestAWSEIPAllocationsForNLB(t *testing.T) { + t.Parallel() + if infraConfig.Status.PlatformStatus == nil { + t.Skip("test skipped on nil platform") + } + if infraConfig.Status.PlatformStatus.Type != configv1.AWSPlatformType { + t.Skipf("test skipped on platform %q", infraConfig.Status.PlatformStatus.Type) + } + if enabled, err := isFeatureGateEnabled(features.FeatureGateSetEIPForNLBIngressController); err != nil { + t.Fatalf("failed to get feature gate: %v", err) + } else if !enabled { + t.Skipf("test skipped because %q feature gate is not enabled", features.FeatureGateSetEIPForNLBIngressController) + } + + // Create an ingress controller with EIPs mentioned in the Ingress Controller CR. + var eipAllocations []operatorv1.EIPAllocation + + ec2ServiceClient := createEC2ServiceClient(t, infraConfig) + clusterName, err := getClusterName(infraConfig) + if err != nil { + t.Fatal(err) + } + vpcID, err := getVPCId(ec2ServiceClient, clusterName) + if err != nil { + t.Fatalf("failed to get VPC ID due to error: %v", err) + } + validEIPAllocations, err := createAWSEIPs(t, ec2ServiceClient, clusterName, vpcID) + if err != nil { + t.Fatalf("failed to create EIPs due to error: %v", err) + } + t.Cleanup(func() { assertEIPAllocationDeleted(t, ec2ServiceClient, 5*time.Minute, clusterName) }) + + for _, validEIPAllocation := range validEIPAllocations { + eipAllocations = append(eipAllocations, operatorv1.EIPAllocation(validEIPAllocation)) + } + + t.Logf("creating ingresscontroller with valid EIPs: %s", validEIPAllocations) + name := types.NamespacedName{Namespace: operatorNamespace, Name: "eiptest"} + domain := name.Name + "." + dnsConfig.Spec.BaseDomain + ic := newLoadBalancerController(name, domain) + ic.Spec.EndpointPublishingStrategy.LoadBalancer = &operatorv1.LoadBalancerStrategy{ + Scope: operatorv1.ExternalLoadBalancer, + DNSManagementPolicy: operatorv1.ManagedLoadBalancerDNS, + ProviderParameters: &operatorv1.ProviderLoadBalancerParameters{ + Type: operatorv1.AWSLoadBalancerProvider, + AWS: &operatorv1.AWSLoadBalancerParameters{ + Type: operatorv1.AWSNetworkLoadBalancer, + NetworkLoadBalancerParameters: &operatorv1.AWSNetworkLoadBalancerParameters{ + EIPAllocations: eipAllocations, + }, + }, + }, + } + + if err := kclient.Create(context.TODO(), ic); err != nil { + t.Fatalf("failed to create ingresscontroller: %v", err) + } + t.Cleanup(func() { + assertIngressControllerDeleted(t, kclient, ic) + serviceName := controller.LoadBalancerServiceName(ic) + // Waits for the service to clean up so EIPs can be released in the next t.Cleanup. + if err := waitForIngressControllerServiceDeleted(t, ic, 4*time.Minute); err != nil { + t.Errorf("failed to delete IngressController service %s/%s due to error: %v", serviceName.Namespace, serviceName.Name, err) + } + }) + + // Wait for the load balancer and DNS to be ready. + if err := waitForIngressControllerCondition(t, kclient, 5*time.Minute, name, availableNotProgressingConditionsForIngressControllerWithLoadBalancer...); err != nil { + t.Fatalf("failed to observe expected conditions: %v", err) + } + + // Ensure the expected eipAllocation annotation is on the service. + waitForLBAnnotation(t, ic, awsLBEIPAllocationAnnotation, true, ingress.JoinAWSEIPAllocations(eipAllocations, ",")) + + // Verify the eipAllocations status field is configured to what we expect. + verifyIngressControllerEIPAllocationStatus(t, name) + + // Verify we can reach the NLB with the provided eipAllocations. + externalTestPodName := types.NamespacedName{Name: name.Name + "-external-verify", Namespace: name.Namespace} + testHostname := "apps." + ic.Spec.Domain + t.Logf("verifying external connectivity for ingresscontroller %q using an NLB with specified eipAllocations", ic.Name) + verifyExternalIngressController(t, externalTestPodName, testHostname, testHostname) + + // Now, update the IngressController to use invalid (non-existent) eipAllocations. + t.Logf("updating ingresscontroller %q to use invalid eipAllocations", ic.Name) + invalidEIPAllocations, err := createInvalidEIPAllocations(t, validEIPAllocations) + if err != nil { + t.Fatalf("failed to create invalid AWS EIPs: %v", err) + } + if err = updateIngressControllerWithRetryOnConflict(t, name, 5*time.Minute, func(ic *operatorv1.IngressController) { + ic.Spec.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.NetworkLoadBalancerParameters.EIPAllocations = invalidEIPAllocations + }); err != nil { + t.Fatalf("failed to update ingresscontroller: %v", err) + } + + // Since the eipAllocations are invalid, the load balancer will fail to provision and set LoadBalancerReady=False, but + // it shouldn't be Progressing either. + loadBalancerNotReadyNotProgressing := []operatorv1.OperatorCondition{ + { + Type: operatorv1.LoadBalancerReadyIngressConditionType, + Status: operatorv1.ConditionFalse, + }, + { + Type: operatorv1.OperatorStatusTypeProgressing, + Status: operatorv1.ConditionFalse, + }, + } + effectuateIngressControllerEIPAllocations(t, ic, invalidEIPAllocations, loadBalancerNotReadyNotProgressing...) + + // Now, update the IngressController to not specify eipAllocations, but let's use the + // auto-delete-load-balancer annotation, so we don't have to manually delete the service. + t.Logf("updating ingresscontroller %q to remove the eipAllocations while using the auto-delete-load-balancer annotation", ic.Name) + if err = updateIngressControllerWithRetryOnConflict(t, name, 5*time.Minute, func(ic *operatorv1.IngressController) { + if ic.Annotations == nil { + ic.Annotations = map[string]string{} + } + ic.Annotations["ingress.operator.openshift.io/auto-delete-load-balancer"] = "" + // Remove eipAllocations by not specifying them. + ic.Spec.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.NetworkLoadBalancerParameters.EIPAllocations = nil + }); err != nil { + t.Fatalf("failed to update ingresscontroller: %v", err) + } + + // Verify the eipAllocation annotation is removed on the service. + waitForLBAnnotation(t, ic, awsLBEIPAllocationAnnotation, false, "") + + // Expect the load balancer to provision successfully with the eipAllocations removed. + if err = waitForIngressControllerCondition(t, kclient, 10*time.Minute, name, availableNotProgressingConditionsForIngressControllerWithLoadBalancer...); err != nil { + t.Fatalf("failed to observe expected conditions: %v", err) + } + + // Verify the eipAllocations status field is configured to what we expect. + verifyIngressControllerEIPAllocationStatus(t, name) + + t.Logf("verifying external connectivity for ingresscontroller %q using an NLB with specified eipAllocations", ic.Name) + verifyExternalIngressController(t, externalTestPodName, testHostname, testHostname) +} + +// TestUnmanagedAWSEIPAllocations tests compatibility for unmanaged service.beta.kubernetes.io/aws-load-balancer-eip-allocations +// annotations on the IngressController service. This is done by directly configuring the annotation on the service +// and then updating the IngressController to match the unmanaged eipAllocation annotation. +func TestUnmanagedAWSEIPAllocations(t *testing.T) { + t.Parallel() + if infraConfig.Status.PlatformStatus == nil { + t.Skip("test skipped on nil platform") + } + if infraConfig.Status.PlatformStatus.Type != configv1.AWSPlatformType { + t.Skipf("test skipped on platform: %q", infraConfig.Status.PlatformStatus.Type) + } + if enabled, err := isFeatureGateEnabled(features.FeatureGateSetEIPForNLBIngressController); err != nil { + t.Fatalf("failed to get feature gate: %v", err) + } else if !enabled { + t.Skipf("test skipped because %q feature gate is not enabled", features.FeatureGateSetEIPForNLBIngressController) + } + + // Next, create a NLB IngressController. + icName := types.NamespacedName{Namespace: operatorNamespace, Name: "unmanaged-aws-eipallocations"} + t.Logf("create a NLB ingresscontroller: %q", icName.Name) + domain := icName.Name + "." + dnsConfig.Spec.BaseDomain + ic := newLoadBalancerController(icName, domain) + ic.Spec.EndpointPublishingStrategy.LoadBalancer = &operatorv1.LoadBalancerStrategy{ + Scope: operatorv1.ExternalLoadBalancer, + DNSManagementPolicy: operatorv1.ManagedLoadBalancerDNS, + ProviderParameters: &operatorv1.ProviderLoadBalancerParameters{ + Type: operatorv1.AWSLoadBalancerProvider, + AWS: &operatorv1.AWSLoadBalancerParameters{ + Type: operatorv1.AWSNetworkLoadBalancer, + }, + }, + } + + if err := kclient.Create(context.Background(), ic); err != nil { + t.Fatalf("expected ingresscontroller creation failed: %v", err) + } + t.Cleanup(func() { + assertIngressControllerDeleted(t, kclient, ic) + serviceName := controller.LoadBalancerServiceName(ic) + // Waits for the service to clean up so EIPs can be released in the next t.Cleanup. + if err := waitForIngressControllerServiceDeleted(t, ic, 4*time.Minute); err != nil { + t.Errorf("failed to delete IngressController service %s/%s due to error: %v", serviceName.Namespace, serviceName.Name, err) + } + }) + + // Wait for the load balancer and DNS to be ready. + if err := waitForIngressControllerCondition(t, kclient, 10*time.Minute, icName, availableNotProgressingConditionsForIngressControllerWithLoadBalancer...); err != nil { + t.Fatalf("failed to observe expected conditions: %v", err) + } + + // Ensure there is no eipAllocation annotation on the service. + waitForLBAnnotation(t, ic, awsLBEIPAllocationAnnotation, false, "") + + // Let's get the list of eipAllocations to use for the LB. + var eipAllocations []operatorv1.EIPAllocation + ec2ServiceClient := createEC2ServiceClient(t, infraConfig) + clusterName, err := getClusterName(infraConfig) + if err != nil { + t.Fatalf("cluster name not found due to error: %v", err) + } + vpcID, err := getVPCId(ec2ServiceClient, clusterName) + if err != nil { + t.Fatalf("failed to get VPC ID due to error: %v", err) + } + validEIPAllocations, err := createAWSEIPs(t, ec2ServiceClient, clusterName, vpcID) + if err != nil { + t.Fatalf("AWS EIPs failed to get created: %v", err) + } + t.Cleanup(func() { assertEIPAllocationDeleted(t, ec2ServiceClient, 5*time.Minute, clusterName) }) + + for _, validEIPAllocation := range validEIPAllocations { + eipAllocations = append(eipAllocations, operatorv1.EIPAllocation(validEIPAllocation)) + } + + // Now, update the eipAllocation annotation directly on the service. + serviceName := controller.LoadBalancerServiceName(ic) + t.Logf("updating service %s/%s directly add unmanaged eipAllocation annotation", serviceName.Namespace, serviceName.Name) + lbService := &corev1.Service{} + if err := kclient.Get(context.Background(), serviceName, lbService); err != nil { + t.Fatalf("failed to get service: %v", err) + } + lbService.Annotations[awsLBEIPAllocationAnnotation] = ingress.JoinAWSEIPAllocations(eipAllocations, ",") + if err := kclient.Update(context.Background(), lbService); err != nil { + t.Fatalf("failed to update service: %v", err) + } + + // LoadBalancerProgressing should become True because the eipAllocation annotation + // doesn't match the IngressController spec. + loadBalancerProgressingTrue := operatorv1.OperatorCondition{ + Type: ingress.IngressControllerLoadBalancerProgressingConditionType, + Status: operatorv1.ConditionTrue, + } + if err = waitForIngressControllerCondition(t, kclient, 5*time.Minute, icName, loadBalancerProgressingTrue); err != nil { + t.Fatalf("failed to observe expected conditions: %v", err) + } + + // Verify the eipAllocation annotation didn't get removed. + waitForLBAnnotation(t, ic, awsLBEIPAllocationAnnotation, true, ingress.JoinAWSEIPAllocations(eipAllocations, ",")) + + // Now, update the IngressController to specify the same unmanaged eipAllocations. + t.Logf("updating ingresscontroller %q to specify the eipAllocations in the unmanaged eipAllocations annotation", ic.Name) + if err = updateIngressControllerWithRetryOnConflict(t, icName, 5*time.Minute, func(ic *operatorv1.IngressController) { + ic.Spec.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.NetworkLoadBalancerParameters = &operatorv1.AWSNetworkLoadBalancerParameters{ + EIPAllocations: eipAllocations, + } + }); err != nil { + t.Fatalf("failed to update ingresscontroller: %v", err) + } + + // The LoadBalancerProgressing=True condition should be resolved and the IngressController should be available. + if err = waitForIngressControllerCondition(t, kclient, 5*time.Minute, icName, availableNotProgressingConditionsForIngressControllerWithLoadBalancer...); err != nil { + t.Fatalf("failed to observe expected conditions: %v", err) + } + + // Verify the eipAllocation annotation is still the same. + waitForLBAnnotation(t, ic, awsLBEIPAllocationAnnotation, true, ingress.JoinAWSEIPAllocations(eipAllocations, ",")) +} + +// verifyIngressControllerEIPAllocationStatus verifies that the IngressController eipAllocation status fields are +// equal to the appropriate eipAllocation spec fields when an IngressController is in a LoadBalancerProgressing=False state. +func verifyIngressControllerEIPAllocationStatus(t *testing.T, icName types.NamespacedName) { + t.Helper() + t.Logf("verifying ingresscontroller %q eipAllocation status field match the appropriate eipAllocation spec field", icName.Name) + // First, ensure the LoadBalancerProgressing is False. If LoadBalancerProgressing is True + // (due to a non-effectuated eipAllocation update), our eipAllocation status checks will surely fail. + loadBalancerProgressingFalse := operatorv1.OperatorCondition{ + Type: ingress.IngressControllerLoadBalancerProgressingConditionType, + Status: operatorv1.ConditionFalse, + } + if err := waitForIngressControllerCondition(t, kclient, 5*time.Minute, icName, loadBalancerProgressingFalse); err != nil { + t.Fatalf("failed to observe expected conditions: %v", err) + } + // Poll until the eipAllocation status is what we expect. + err := wait.PollUntilContextTimeout(context.Background(), 10*time.Second, 3*time.Minute, false, func(ctx context.Context) (bool, error) { + ic := &operatorv1.IngressController{} + if err := kclient.Get(context.Background(), icName, ic); err != nil { + t.Logf("failed to get ingresscontroller: %v, retrying...", err) + return false, nil + } + // Verify the eipAllocation status field is configured to what we expect. + var eipAllocationSpec, eipAllocationStatus []operatorv1.EIPAllocation + if networkLoadBalancerParametersSpecExists(ic) { + eipAllocationSpec = ic.Spec.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.NetworkLoadBalancerParameters.EIPAllocations + } + if networkLoadBalancerParametersStatusExist(ic) { + eipAllocationStatus = ic.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.NetworkLoadBalancerParameters.EIPAllocations + } + // Check if the eipAllocationSpec and eipAllocationStatus are equal. + if !reflect.DeepEqual(eipAllocationSpec, eipAllocationStatus) { + t.Logf("expected ingresscontroller eipAllocation status to be %q, got %q, retrying...", eipAllocationSpec, eipAllocationStatus) + return false, nil + } + return true, nil + }) + if err != nil { + t.Fatalf("error verifying eipAllocation status for IngressController %q: %v", icName.Name, err) + } +} + +// effectuateIngressControllerEIPAllocations manually effectuates updated IngressController eipAllocations by +// confirming IngressController is in a progressing state, deleting the service, waiting for +// the expected eipAllocations to appear on the service, and confirming the IngressController +// eipAllocation status is accurate. It waits for the provided operator conditions after the eipAllocations +// have been effectuated. +func effectuateIngressControllerEIPAllocations(t *testing.T, ic *operatorv1.IngressController, expectedEIPAllocations []operatorv1.EIPAllocation, expectedOperatorConditions ...operatorv1.OperatorCondition) { + t.Helper() + t.Logf("effectuating eipAllocations for IngressController %s", ic.Name) + icName := types.NamespacedName{Name: ic.Name, Namespace: ic.Namespace} + progressingTrue := operatorv1.OperatorCondition{ + Type: ingress.IngressControllerLoadBalancerProgressingConditionType, + Status: operatorv1.ConditionTrue, + } + if err := waitForIngressControllerCondition(t, kclient, 5*time.Minute, icName, progressingTrue); err != nil { + t.Fatalf("failed to observe expected conditions: %v", err) + } + + // Delete and recreate the IngressController service to effectuate. + t.Logf("recreating the service to effectuate the subnets: %s/%s", controller.LoadBalancerServiceName(ic).Namespace, controller.LoadBalancerServiceName(ic).Namespace) + if err := recreateIngressControllerService(t, ic); err != nil { + t.Fatalf("failed to delete and recreate service: %v", err) + } + + // Ensure the service's load-balancer status changes, and verify we get the expected eipAllocation annotation on the service. + waitForLBAnnotation(t, ic, awsLBEIPAllocationAnnotation, true, ingress.JoinAWSEIPAllocations(expectedEIPAllocations, ",")) + + // Expect the load balancer to provision successfully with the new eipAllocations. + if err := waitForIngressControllerCondition(t, kclient, 10*time.Minute, icName, expectedOperatorConditions...); err != nil { + t.Fatalf("failed to observe expected conditions: %v", err) + } + + // Verify the subnets status field is configured to what we expect. + verifyIngressControllerEIPAllocationStatus(t, icName) +} + +// createInvalidEIPAllocations fetches the public subnets and creates same number of invalid eipAllocations as that of public subnets. +func createInvalidEIPAllocations(t *testing.T, validEIPAllocations []string) ([]operatorv1.EIPAllocation, error) { + t.Helper() + invalidEIPAllocations := make([]operatorv1.EIPAllocation, len(validEIPAllocations)) + var hexString string + var err error + for i := 0; i < len(validEIPAllocations); i++ { + if hexString, err = randomHex(9); err != nil { + t.Fatalf("failed to create hexadecimal string for eipAllocation due to error: %v", err) + } + invalidEIPAllocations[i] = operatorv1.EIPAllocation(fmt.Sprintf("%s-%s", "eipalloc", hexString[:17])) + } + return invalidEIPAllocations, nil +} + +func randomHex(n int) (string, error) { + bytes := make([]byte, n) + if _, err := random.Read(bytes); err != nil { + return "", err + } + return hex.EncodeToString(bytes), nil +} + +// createAWSEIPs creates valid eipAllocations whose count is equal to the number of public subnets. +func createAWSEIPs(t *testing.T, ec2ServiceClient *ec2.EC2, clusterName string, vpcID string) ([]string, error) { + publicSubnets, err := getPublicSubnets(vpcID, ec2ServiceClient) + if err != nil { + t.Fatalf("failed to get public subnets due to error: %v", err) + } + // Set up random seed + rand.Seed(time.Now().UnixNano()) + + // Set tag key and value + tagKeyEIP, tagValueEIP := getTagKeyAndValue(t) + + // Allocate EIPs + var allocationIDs []string + for i := 0; i < len(publicSubnets); i++ { + // Allocate EIP + result, err := ec2ServiceClient.AllocateAddress(&ec2.AllocateAddressInput{ + Domain: aws.String("vpc"), + }) + if err != nil { + return nil, fmt.Errorf("Error allocating EIP: %v", err) + } + allocationID := aws.StringValue(result.AllocationId) + t.Logf("EIP allocated successfully. Allocation ID: %s", allocationID) + + // Generate random name + randomName := fmt.Sprintf("%s-EIP-%d", clusterName, rand.Int()) + + // Tag EIP + _, err = ec2ServiceClient.CreateTags(&ec2.CreateTagsInput{ + Resources: []*string{result.AllocationId}, + Tags: []*ec2.Tag{ + { + Key: aws.String("Name"), + Value: aws.String(randomName), + }, + { + Key: aws.String(tagKeyEIP), + Value: aws.String(tagValueEIP), + }, + }, + }) + if err != nil { + return nil, fmt.Errorf("Error tagging EIP Allocation ID: %v", allocationID) + } + allocationIDs = append(allocationIDs, allocationID) + } + return allocationIDs, nil +} + +// cleanupEIPAllocations clean all unassociated EIPs created during the e2e tests. It returns true if no associated eipAllocations tagged with cluster name are found. +func cleanupEIPAllocations(t *testing.T, svc *ec2.EC2, clusterName string) bool { + t.Log("Releasing unassociated EIPs") + + tagKeyEIP, tagValueEIP := getTagKeyAndValue(t) + + // Describe addresses to get unassociated EIPs + describeAddressesInput := &ec2.DescribeAddressesInput{} + describeAddressesOutput, err := svc.DescribeAddresses(describeAddressesInput) + if err != nil { + t.Fatalf("failed to describe addresses: %v", err) + } + + var unassociatedEIPs, associatedEIPs []*ec2.Address + + for _, address := range describeAddressesOutput.Addresses { + // Check if the EIP has the specified tag key and value + hasTag := false + for _, tag := range address.Tags { + if *tag.Key == tagKeyEIP && *tag.Value == tagValueEIP { + hasTag = true + break + } + } + if hasTag { + if address.AssociationId == nil { + unassociatedEIPs = append(unassociatedEIPs, address) + } else { + associatedEIPs = append(associatedEIPs, address) + } + } + } + + if len(unassociatedEIPs) == 0 { + t.Log("No unassociated EIPs found with the specified tag key and value.") + } else { + t.Log("Unassociated EIPs with the specified tag key and value:") + for _, eip := range unassociatedEIPs { + t.Logf("Public IP: %v, Allocation ID: %v", *eip.PublicIp, *eip.AllocationId) + } + } + + // Release each unassociated EIP + for _, eip := range unassociatedEIPs { + releaseAddressInput := &ec2.ReleaseAddressInput{ + AllocationId: eip.AllocationId, + } + _, err := svc.ReleaseAddress(releaseAddressInput) + if err != nil { + t.Errorf("Failed to release EIP %v with Allocation ID %v: %v", *eip.PublicIp, *eip.AllocationId, err) + } else { + t.Logf("Released EIP %v with Allocation ID %v", *eip.PublicIp, *eip.AllocationId) + } + } + + if len(associatedEIPs) == 0 { + t.Log("No associated EIPs found with the specified tag key and value.") + return true + } else { + t.Log("Associated EIPs with the specified tag key and value:") + for _, eip := range associatedEIPs { + t.Logf("Public IP: %v, Allocation ID: %v", *eip.PublicIp, *eip.AllocationId) + } + return false + } +} diff --git a/test/e2e/operator_test.go b/test/e2e/operator_test.go index 5c6baa5d4..08cd3a06d 100644 --- a/test/e2e/operator_test.go +++ b/test/e2e/operator_test.go @@ -12,6 +12,7 @@ import ( "encoding/json" "errors" "fmt" + "github.com/aws/aws-sdk-go/service/ec2" "io" "io/ioutil" "net" @@ -4250,6 +4251,22 @@ func waitForIngressControllerCondition(t *testing.T, cl client.Client, timeout t return err } +// assertEIPAllocationDeleted cleans the EIPs having a tag key and value and the polling to clean EIPs continues until all the unassociated EIPs are released. +func assertEIPAllocationDeleted(t *testing.T, svc *ec2.EC2, timeout time.Duration, clusterName string) { + t.Helper() + t.Log("Starting cleanup of EIPs") + if err := wait.PollUntilContextTimeout(context.Background(), 10*time.Second, timeout, true, func(ctx context.Context) (bool, error) { + if cleanupEIPAllocations(t, svc, clusterName) { + return true, nil + } else { + t.Log("failed to release the EIPs created...retrying") + return false, nil + } + }); err != nil { + t.Fatalf("failed to poll eipAllocations due to error: %v", err) + } +} + func assertIngressControllerDeleted(t *testing.T, cl client.Client, ing *operatorv1.IngressController) { t.Helper() if err := deleteIngressController(t, cl, ing, 4*time.Minute); err != nil { diff --git a/test/e2e/util_test.go b/test/e2e/util_test.go index eb1218318..68bbfe580 100644 --- a/test/e2e/util_test.go +++ b/test/e2e/util_test.go @@ -25,6 +25,7 @@ import ( "k8s.io/utils/pointer" "k8s.io/apimachinery/pkg/api/errors" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" @@ -1056,3 +1057,58 @@ func classicLoadBalancerParametersStatusExists(ic *operatorv1.IngressController) ic.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS != nil && ic.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.ClassicLoadBalancerParameters != nil } + +// waitForIngressControllerServiceDeleted checks if the service was deleted and return true if it is not found. +func waitForIngressControllerServiceDeleted(t *testing.T, ic *operatorv1.IngressController, timeout time.Duration) error { + lbService := &corev1.Service{} + serviceName := controller.LoadBalancerServiceName(ic) + + err := wait.PollUntilContextTimeout(context.Background(), 1*time.Second, timeout, false, func(ctx context.Context) (bool, error) { + if err := kclient.Get(context.TODO(), serviceName, lbService); err != nil { + if apierrors.IsNotFound(err) { + return true, nil + } + t.Logf("failed to get the load balancer service %s/%s for Ingress Controller %s due to error: %v", serviceName.Namespace, serviceName.Name, ic.Name, err) + return false, nil + } + t.Logf("waiting for service %s/%s to be deleted", serviceName.Namespace, serviceName.Name) + return false, nil + }) + if err != nil { + return fmt.Errorf("timed out waiting for the load balancer service to be deleted: %v", err) + } + return nil +} + +// waitForLBAnnotation waits for the provided annoation to appear on the LoadBalancer-type service for the +// given IngressController. It will return an error if it fails to observe the given annotation. +func waitForLBAnnotation(t *testing.T, ic *operatorv1.IngressController, expectedAnnotation string, expectedExist bool, expectedValue string) { + t.Helper() + + lbService := &corev1.Service{} + t.Logf("waiting for %q service with %q annotation of %q to exist: %t", controller.LoadBalancerServiceName(ic), expectedAnnotation, expectedValue, expectedExist) + err := wait.PollUntilContextTimeout(context.Background(), 10*time.Second, 5*time.Minute, false, func(ctx context.Context) (bool, error) { + if err := kclient.Get(ctx, controller.LoadBalancerServiceName(ic), lbService); err != nil { + t.Logf("failed to get %q service: %v, retrying ...", controller.LoadBalancerServiceName(ic), err) + return false, nil + } + val, ok := lbService.Annotations[expectedAnnotation] + // Handle the case where annotation should not exist. + if !expectedExist { + if ok { + t.Logf("expected %q annotation to be removed got %q, retrying...", expectedAnnotation, val) + return false, nil + } + return true, nil + } + // Handle the case where should exist and match. + if !ok || val != expectedValue { + t.Logf("expected %q annotation %q got %q, retrying...", expectedAnnotation, expectedValue, val) + return false, nil + } + return true, nil + }) + if err != nil { + t.Fatalf("error updating the %q service: %v", controller.LoadBalancerServiceName(ic), err) + } +}