Skip to content

Commit

Permalink
LB Type Should Reflect Actual LB Type
Browse files Browse the repository at this point in the history
  • Loading branch information
@gcs278
gcs278 committed Sep 19, 2024
1 parent 871b2b2 commit e0fe01d
Show file tree
Hide file tree
Showing 7 changed files with 180 additions and 62 deletions.
32 changes: 22 additions & 10 deletions pkg/operator/controller/ingress/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1069,7 +1069,19 @@ func (r *reconciler) ensureIngressController(ci *operatorv1.IngressController, d
haveClientCAConfigmap = true
}

haveDepl, deployment, err := r.ensureRouterDeployment(ci, infraConfig, ingressConfig, apiConfig, networkConfig, haveClientCAConfigmap, clientCAConfigmap, platformStatus, clusterProxyConfig)
_, currentLBService, err := r.currentLoadBalancerService(ci)
if err != nil {
errs = append(errs, fmt.Errorf("failed to get load balancer service for %s: %v", ci.Name, err))
return utilerrors.NewAggregate(errs)
}

proxyNeeded, err := IsProxyProtocolNeeded(ci, platformStatus, currentLBService)
if err != nil {
errs = append(errs, fmt.Errorf("failed to determine if proxy protocol is needed for ingresscontroller %s/%s: %v", ci.Namespace, ci.Name, err))
return utilerrors.NewAggregate(errs)
}

haveDepl, deployment, err := r.ensureRouterDeployment(ci, infraConfig, ingressConfig, apiConfig, networkConfig, haveClientCAConfigmap, clientCAConfigmap, platformStatus, clusterProxyConfig, proxyNeeded)
if err != nil {
errs = append(errs, fmt.Errorf("failed to ensure deployment: %v", err))
return utilerrors.NewAggregate(errs)
Expand All @@ -1088,7 +1100,7 @@ func (r *reconciler) ensureIngressController(ci *operatorv1.IngressController, d
}

var wildcardRecord *iov1.DNSRecord
haveLB, lbService, err := r.ensureLoadBalancerService(ci, deploymentRef, platformStatus)
haveLB, lbService, err := r.ensureLoadBalancerService(ci, deploymentRef, platformStatus, proxyNeeded)
if err != nil {
errs = append(errs, fmt.Errorf("failed to ensure load balancer service for %s: %v", ci.Name, err))
} else {
Expand Down Expand Up @@ -1183,23 +1195,23 @@ func IsStatusDomainSet(ingress *operatorv1.IngressController) bool {

// IsProxyProtocolNeeded checks whether proxy protocol is needed based
// upon the given ic and platform.
func IsProxyProtocolNeeded(ic *operatorv1.IngressController, platform *configv1.PlatformStatus) (bool, error) {
func IsProxyProtocolNeeded(ic *operatorv1.IngressController, platform *configv1.PlatformStatus, service *corev1.Service) (bool, error) {
if platform == nil {
return false, fmt.Errorf("platform status is missing; failed to determine if proxy protocol is needed for %s/%s",
ic.Namespace, ic.Name)
}
switch ic.Status.EndpointPublishingStrategy.Type {
case operatorv1.LoadBalancerServiceStrategyType:
// This can really be done for for any external [cloud] LBs that support the proxy protocol.
// This can really be done for any external [cloud] LBs that support the proxy protocol.
switch platform.Type {
case configv1.AWSPlatformType:
if ic.Status.EndpointPublishingStrategy.LoadBalancer == nil ||
ic.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters == nil ||
ic.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS == nil ||
ic.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.Type == operatorv1.AWSLoadBalancerProvider &&
ic.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.Type == operatorv1.AWSClassicLoadBalancer {
return true, nil
// LBType is only configured at service creation and won't be updated afterward. If the service already
// exists, we must use the actual LB Type from the service instead of the one specified in the spec.
lbType := getAWSLoadBalancerTypeInStatus(ic)
if service != nil {
lbType = getAWSLoadBalancerTypeFromServiceAnnotation(service)
}
return lbType == operatorv1.AWSClassicLoadBalancer, nil
case configv1.IBMCloudPlatformType:
if ic.Status.EndpointPublishingStrategy.LoadBalancer != nil &&
ic.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters != nil &&
Expand Down
25 changes: 24 additions & 1 deletion pkg/operator/controller/ingress/controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1437,11 +1437,20 @@ func Test_IsProxyProtocolNeeded(t *testing.T) {
Protocol: operatorv1.ProxyProtocol,
},
}
serviceWithELB = corev1.Service{}
serviceWithNLB = corev1.Service{
ObjectMeta: metav1.ObjectMeta{
Annotations: map[string]string{
AWSLBTypeAnnotation: AWSNLBAnnotation,
},
},
}
)
testCases := []struct {
description string
strategy *operatorv1.EndpointPublishingStrategy
platform *configv1.PlatformStatus
service *corev1.Service
expect bool
expectError bool
}{
Expand Down Expand Up @@ -1487,6 +1496,20 @@ func Test_IsProxyProtocolNeeded(t *testing.T) {
platform: &awsPlatform,
expect: false,
},
{
description: "loadbalancer strategy with NLB, but a service with ELB should use PROXY",
strategy: &loadBalancerStrategyWithNLB,
platform: &awsPlatform,
service: &serviceWithELB,
expect: true,
},
{
description: "loadbalancer strategy with ELB, but a service with NLB shouldn't use PROXY",
strategy: &loadBalancerStrategyWithNLB,
platform: &awsPlatform,
service: &serviceWithNLB,
expect: false,
},
{
description: "loadbalancer strategy shouldn't use PROXY on Azure",
strategy: &loadBalancerStrategy,
Expand Down Expand Up @@ -1568,7 +1591,7 @@ func Test_IsProxyProtocolNeeded(t *testing.T) {
EndpointPublishingStrategy: tc.strategy,
},
}
switch actual, err := IsProxyProtocolNeeded(ic, tc.platform); {
switch actual, err := IsProxyProtocolNeeded(ic, tc.platform, tc.service); {
case tc.expectError && err == nil:
t.Error("expected error, got nil")
case !tc.expectError && err != nil:
Expand Down
6 changes: 1 addition & 5 deletions pkg/operator/controller/ingress/deployment.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,15 +111,11 @@ const (

// ensureRouterDeployment ensures the router deployment exists for a given
// ingresscontroller.
func (r *reconciler) ensureRouterDeployment(ci *operatorv1.IngressController, infraConfig *configv1.Infrastructure, ingressConfig *configv1.Ingress, apiConfig *configv1.APIServer, networkConfig *configv1.Network, haveClientCAConfigmap bool, clientCAConfigmap *corev1.ConfigMap, platformStatus *configv1.PlatformStatus, clusterProxyConfig *configv1.Proxy) (bool, *appsv1.Deployment, error) {
func (r *reconciler) ensureRouterDeployment(ci *operatorv1.IngressController, infraConfig *configv1.Infrastructure, ingressConfig *configv1.Ingress, apiConfig *configv1.APIServer, networkConfig *configv1.Network, haveClientCAConfigmap bool, clientCAConfigmap *corev1.ConfigMap, platformStatus *configv1.PlatformStatus, clusterProxyConfig *configv1.Proxy, proxyNeeded bool) (bool, *appsv1.Deployment, error) {
haveDepl, current, err := r.currentRouterDeployment(ci)
if err != nil {
return false, nil, err
}
proxyNeeded, err := IsProxyProtocolNeeded(ci, platformStatus)
if err != nil {
return false, nil, fmt.Errorf("failed to determine if proxy protocol is needed for ingresscontroller %s/%s: %v", ci.Namespace, ci.Name, err)
}
desired, err := desiredRouterDeployment(ci, r.config.IngressControllerImage, ingressConfig, infraConfig, apiConfig, networkConfig, proxyNeeded, haveClientCAConfigmap, clientCAConfigmap, clusterProxyConfig, r.config.RouteExternalCertificateEnabled)
if err != nil {
return haveDepl, current, fmt.Errorf("failed to build router deployment: %v", err)
Expand Down
12 changes: 4 additions & 8 deletions pkg/operator/controller/ingress/deployment_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,7 @@ func getRouterDeploymentComponents(t *testing.T) (*operatorv1.IngressController,
},
},
}
proxyNeeded, err := IsProxyProtocolNeeded(ic, infraConfig.Status.PlatformStatus)
proxyNeeded, err := IsProxyProtocolNeeded(ic, infraConfig.Status.PlatformStatus, nil)
if err != nil {
t.Errorf("failed to determine infrastructure platform status for ingresscontroller %s/%s: %v", ic.Namespace, ic.Name, err)
}
Expand Down Expand Up @@ -670,7 +670,7 @@ func TestDesiredRouterDeploymentSpecAndNetwork(t *testing.T) {
ic.Status.Domain = "example.com"
ic.Status.EndpointPublishingStrategy.Type = operatorv1.LoadBalancerServiceStrategyType

proxyNeeded, err := IsProxyProtocolNeeded(ic, infraConfig.Status.PlatformStatus)
proxyNeeded, err := IsProxyProtocolNeeded(ic, infraConfig.Status.PlatformStatus, nil)
if err != nil {
t.Errorf("failed to determine infrastructure platform status for ingresscontroller %s/%s: %v", ic.Namespace, ic.Name, err)
}
Expand Down Expand Up @@ -766,7 +766,7 @@ func TestDesiredRouterDeploymentSpecAndNetwork(t *testing.T) {
},
},
}
proxyNeeded, err = IsProxyProtocolNeeded(ic, infraConfig.Status.PlatformStatus)
proxyNeeded, err = IsProxyProtocolNeeded(ic, infraConfig.Status.PlatformStatus, nil)
if err != nil {
t.Errorf("failed to determine infrastructure platform status for ingresscontroller %s/%s: %v", ic.Namespace, ic.Name, err)
}
Expand Down Expand Up @@ -874,7 +874,7 @@ func TestDesiredRouterDeploymentVariety(t *testing.T) {
networkConfig.Status.ClusterNetwork = []configv1.ClusterNetworkEntry{
{CIDR: "2620:0:2d0:200::7/32"},
}
proxyNeeded, err := IsProxyProtocolNeeded(ic, infraConfig.Status.PlatformStatus)
proxyNeeded, err := IsProxyProtocolNeeded(ic, infraConfig.Status.PlatformStatus, nil)
if err != nil {
t.Errorf("failed to determine infrastructure platform status for ingresscontroller %s/%s: %v", ic.Namespace, ic.Name, err)
}
Expand Down Expand Up @@ -992,10 +992,6 @@ func TestDesiredRouterDeploymentVariety(t *testing.T) {
func TestDesiredRouterDeploymentHostNetworkNil(t *testing.T) {
ic, ingressConfig, infraConfig, apiConfig, networkConfig, proxyNeeded, clusterProxyConfig := getRouterDeploymentComponents(t)
ic.Status.EndpointPublishingStrategy.Type = operatorv1.HostNetworkStrategyType
proxyNeeded, err := IsProxyProtocolNeeded(ic, infraConfig.Status.PlatformStatus)
if err != nil {
t.Fatal(err)
}
deployment, err := desiredRouterDeployment(ic, ingressControllerImage, ingressConfig, infraConfig, apiConfig, networkConfig, proxyNeeded, false, nil, clusterProxyConfig, false)
if err != nil {
t.Fatal(err)
Expand Down
66 changes: 46 additions & 20 deletions pkg/operator/controller/ingress/load_balancer_service.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -279,8 +279,8 @@ var (
// ensureLoadBalancerService creates an LB service if one is desired but absent.
// 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, r.config.IngressControllerEIPAllocationsAWSEnabled)
func (r *reconciler) ensureLoadBalancerService(ci *operatorv1.IngressController, deploymentRef metav1.OwnerReference, platformStatus *configv1.PlatformStatus, proxyNeeded bool) (bool, *corev1.Service, error) {
wantLBS, desiredLBService, err := desiredLoadBalancerService(ci, deploymentRef, platformStatus, r.config.IngressControllerLBSubnetsAWSEnabled, r.config.IngressControllerEIPAllocationsAWSEnabled, proxyNeeded)
if err != nil {
return false, nil, err
}
Expand Down Expand Up @@ -346,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, eipAllocationsAWSEnabled bool) (bool, *corev1.Service, error) {
func desiredLoadBalancerService(ci *operatorv1.IngressController, deploymentRef metav1.OwnerReference, platform *configv1.PlatformStatus, subnetsAWSEnabled bool, eipAllocationsAWSEnabled bool, proxyNeeded bool) (bool, *corev1.Service, error) {
if ci.Status.EndpointPublishingStrategy.Type != operatorv1.LoadBalancerServiceStrategyType {
return false, nil, nil
}
Expand All @@ -365,18 +365,13 @@ func desiredLoadBalancerService(ci *operatorv1.IngressController, deploymentRef

service.Spec.Selector = controller.IngressControllerDeploymentPodSelector(ci).MatchLabels

lb := ci.Status.EndpointPublishingStrategy.LoadBalancer
isInternal := lb != nil && lb.Scope == operatorv1.InternalLoadBalancer
lbStatus := ci.Status.EndpointPublishingStrategy.LoadBalancer
isInternal := lbStatus != nil && lbStatus.Scope == operatorv1.InternalLoadBalancer

if service.Annotations == nil {
service.Annotations = map[string]string{}
}

proxyNeeded, err := IsProxyProtocolNeeded(ci, platform)
if err != nil {
return false, nil, fmt.Errorf("failed to determine if proxy protocol is proxyNeeded for ingresscontroller %q: %v", ci.Name, err)
}

if platform != nil {
if isInternal {
annotation := InternalLBAnnotations[platform.Type]
Expand All @@ -386,10 +381,10 @@ func desiredLoadBalancerService(ci *operatorv1.IngressController, deploymentRef

// Set the GCP Global Access annotation for internal load balancers on GCP only
if platform.Type == configv1.GCPPlatformType {
if lb != nil && lb.ProviderParameters != nil &&
lb.ProviderParameters.Type == operatorv1.GCPLoadBalancerProvider &&
lb.ProviderParameters.GCP != nil {
globalAccessEnabled := lb.ProviderParameters.GCP.ClientAccess == operatorv1.GCPGlobalAccess
if lbStatus != nil && lbStatus.ProviderParameters != nil &&
lbStatus.ProviderParameters.Type == operatorv1.GCPLoadBalancerProvider &&
lbStatus.ProviderParameters.GCP != nil {
globalAccessEnabled := lbStatus.ProviderParameters.GCP.ClientAccess == operatorv1.GCPGlobalAccess
service.Annotations[GCPGlobalAccessAnnotation] = strconv.FormatBool(globalAccessEnabled)
}
}
Expand All @@ -405,8 +400,8 @@ func desiredLoadBalancerService(ci *operatorv1.IngressController, deploymentRef
if proxyNeeded {
service.Annotations[awsLBProxyProtocolAnnotation] = "*"
}
if lb != nil && lb.ProviderParameters != nil {
if aws := lb.ProviderParameters.AWS; aws != nil && lb.ProviderParameters.Type == operatorv1.AWSLoadBalancerProvider {
if lbStatus != nil && lbStatus.ProviderParameters != nil {
if aws := lbStatus.ProviderParameters.AWS; aws != nil && lbStatus.ProviderParameters.Type == operatorv1.AWSLoadBalancerProvider {
switch aws.Type {
case operatorv1.AWSNetworkLoadBalancer:
service.Annotations[AWSLBTypeAnnotation] = AWSNLBAnnotation
Expand Down Expand Up @@ -764,7 +759,11 @@ func loadBalancerServiceTagsModified(current, expected *corev1.Service) (bool, *
// 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, eipAllocationsAWSEnabled bool) error {
want, desired, err := desiredLoadBalancerService(ic, deploymentRef, platform, subnetsAWSEnabled, eipAllocationsAWSEnabled)
proxyNeeded, err := IsProxyProtocolNeeded(ic, platform, current)
if err != nil {
return fmt.Errorf("failed to determine if proxy protocol is needed for ingresscontroller %s/%s: %v", ic.Namespace, ic.Name, err)
}
want, desired, err := desiredLoadBalancerService(ic, deploymentRef, platform, subnetsAWSEnabled, eipAllocationsAWSEnabled, proxyNeeded)
if err != nil {
return err
}
Expand Down Expand Up @@ -804,7 +803,7 @@ func loadBalancerServiceIsProgressing(ic *operatorv1.IngressController, service
wantSubnets, haveSubnets *operatorv1.AWSSubnets
paramsFieldName string
)
switch getAWSLoadBalancerTypeInStatus(ic) {
switch getAWSLoadBalancerTypeFromServiceAnnotation(service) {
case operatorv1.AWSNetworkLoadBalancer:
if nlbParams := getAWSNetworkLoadBalancerParametersInSpec(ic); nlbParams != nil {
wantSubnets = nlbParams.Subnets
Expand Down Expand Up @@ -835,7 +834,7 @@ func loadBalancerServiceIsProgressing(ic *operatorv1.IngressController, service
}
}

if platform.Type == configv1.AWSPlatformType && eipAllocationsAWSEnabled && getAWSLoadBalancerTypeInStatus(ic) == operatorv1.AWSNetworkLoadBalancer {
if platform.Type == configv1.AWSPlatformType && eipAllocationsAWSEnabled && getAWSLoadBalancerTypeFromServiceAnnotation(service) == operatorv1.AWSNetworkLoadBalancer {
var (
wantEIPAllocations, haveEIPAllocations []operatorv1.EIPAllocation
)
Expand Down Expand Up @@ -966,6 +965,21 @@ func loadBalancerSourceRangesMatch(ic *operatorv1.IngressController, current *co
return fmt.Errorf("You have manually edited an operator-managed object. You must revert your modifications by removing the Spec.LoadBalancerSourceRanges field of LoadBalancer-typed service %q. You can use the new AllowedSourceRanges API field on the ingresscontroller to configure this setting instead.", current.Name)
}

// getAWSLoadBalancerTypeFromServiceAnnotation gets the effective load balancer type by looking at the
// service.beta.kubernetes.io/aws-load-balancer-type annotation of the LoadBalancer-type Service.
// If the annotation isn't specified, the function returns the default of Classic.
func getAWSLoadBalancerTypeFromServiceAnnotation(service *corev1.Service) operatorv1.AWSLoadBalancerType {
if service == nil {
return ""
}

if a, ok := service.Annotations[AWSLBTypeAnnotation]; ok && a == AWSNLBAnnotation {
return operatorv1.AWSNetworkLoadBalancer
}

return operatorv1.AWSClassicLoadBalancer
}

// getSubnetsFromServiceAnnotation gets the effective subnets by looking at the
// service.beta.kubernetes.io/aws-load-balancer-subnets annotation of the LoadBalancer-type Service.
// If no subnets are specified in the annotation, this function returns nil.
Expand Down Expand Up @@ -1180,6 +1194,18 @@ func JoinAWSEIPAllocations(eipAllocations []operatorv1.EIPAllocation, sep string
return buffer.String()
}

// getAWSLoadBalancerTypeInSpec gets the AWS Load Balancer Type reported in the status.
// If nothing is configured, then it returns the default of Classic.
func getAWSLoadBalancerTypeInSpec(ic *operatorv1.IngressController) operatorv1.AWSLoadBalancerType {
if ic.Spec.EndpointPublishingStrategy != nil &&
ic.Spec.EndpointPublishingStrategy.LoadBalancer != nil &&
ic.Spec.EndpointPublishingStrategy.LoadBalancer.ProviderParameters != nil &&
ic.Spec.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS != nil {
return ic.Spec.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.Type
}
return operatorv1.AWSClassicLoadBalancer
}

// getAWSLoadBalancerTypeInStatus gets the AWS Load Balancer Type reported in the status.
func getAWSLoadBalancerTypeInStatus(ic *operatorv1.IngressController) operatorv1.AWSLoadBalancerType {
if ic.Status.EndpointPublishingStrategy != nil &&
Expand All @@ -1188,7 +1214,7 @@ func getAWSLoadBalancerTypeInStatus(ic *operatorv1.IngressController) operatorv1
ic.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS != nil {
return ic.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.Type
}
return ""
return operatorv1.AWSClassicLoadBalancer
}

// getAWSClassicLoadBalancerParametersInSpec gets the ClassicLoadBalancerParameter struct
Expand Down
Loading

0 comments on commit e0fe01d

Please sign in to comment.