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) + } +}