Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OCPBUGS-16728: Require Service Deletion for LB Type Updates #1142

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

OCPBUGS-16728: Require Service Deletion for LB Type Updates #1142

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c25c6c1
Update Broken Annotation Documentation Links
gcs278 Oct 18, 2024
3dcff76
Introduce `managedAtServiceCreationLBServiceAnnotations` Map
gcs278 Oct 18, 2024
0d249ce
OCPBUGS-16728: Require Service Deletion for LB Type Updates
gcs278 Sep 9, 2024
5862310
[WILL SQUASH] Refactor IsProxyProtocolNeeded
gcs278 Sep 18, 2024
1a57afd
[WILL SQUASH] Move inactive LB parameters clearing to status.go
gcs278 Oct 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 29 additions & 12 deletions pkg/operator/controller/ingress/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -440,6 +440,10 @@ func setDefaultDomain(ic *operatorv1.IngressController, ingressConfig *configv1.
}

func setDefaultPublishingStrategy(ic *operatorv1.IngressController, platformStatus *configv1.PlatformStatus, domainMatchesBaseDomain bool, ingressConfig *configv1.Ingress, alreadyAdmitted bool) bool {
// WARNING: setDefaultPublishingStrategy follows an outdated pattern of using spec and status. It uses the status
// to represent the *desired* state (spec + defaulting), but per Kubernetes standards, the status should reflect
// the *actual* state. As a result, this function is considered legacy. For new API fields, use spec as the desired
// state and syncIngressControllerStatus to populate the status with the actual state.
effectiveStrategy := ic.Spec.EndpointPublishingStrategy.DeepCopy()
if effectiveStrategy == nil {
var strategyType operatorv1.EndpointPublishingStrategyType
Expand Down Expand Up @@ -577,7 +581,6 @@ func setDefaultPublishingStrategy(ic *operatorv1.IngressController, platformStat
changed = true
}
if statusLB.ProviderParameters.AWS.Type == operatorv1.AWSClassicLoadBalancer {
statusLB.ProviderParameters.AWS.NetworkLoadBalancerParameters = nil
if statusLB.ProviderParameters.AWS.ClassicLoadBalancerParameters == nil {
statusLB.ProviderParameters.AWS.ClassicLoadBalancerParameters = &operatorv1.AWSClassicLoadBalancerParameters{}
}
Expand All @@ -599,7 +602,6 @@ func setDefaultPublishingStrategy(ic *operatorv1.IngressController, platformStat
}
}
if statusLB.ProviderParameters.AWS.Type == operatorv1.AWSNetworkLoadBalancer {
statusLB.ProviderParameters.AWS.ClassicLoadBalancerParameters = nil
if statusLB.ProviderParameters.AWS.NetworkLoadBalancerParameters == nil {
statusLB.ProviderParameters.AWS.NetworkLoadBalancerParameters = &operatorv1.AWSNetworkLoadBalancerParameters{}
}
Expand Down Expand Up @@ -1106,7 +1108,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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
errs = append(errs, fmt.Errorf("failed to determine if proxy protocol is needed for ingresscontroller %s/%s: %v", ci.Namespace, ci.Name, err))
errs = append(errs, fmt.Errorf("failed to determine if proxy protocol is needed for ingresscontroller %s/%s: %w", 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 @@ -1125,7 +1139,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 @@ -1220,23 +1234,26 @@ 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
// TRICKY: If the service exists, use the LB Type annotation from the service,
// as status will be inaccurate if a LB Type update is pending.
// If the service does not exist, the status WILL be what determines the next LB Type.
var lbType operatorv1.AWSLoadBalancerType
if service != nil {
lbType = getAWSLoadBalancerTypeFromServiceAnnotation(service)
} else {
lbType = getAWSLoadBalancerTypeInStatus(ic)
}
return lbType == operatorv1.AWSClassicLoadBalancer, nil
case configv1.IBMCloudPlatformType:
if ic.Status.EndpointPublishingStrategy.LoadBalancer != nil &&
ic.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters != nil &&
Expand Down
39 changes: 24 additions & 15 deletions pkg/operator/controller/ingress/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -652,20 +652,6 @@ func TestSetDefaultPublishingStrategyHandlesUpdates(t *testing.T) {
expectedIC: makeIC(spec(elb()), status(elbWithNullParameters())),
domainMatchesBaseDomain: true,
},
{
name: "loadbalancer type changed from ELB to NLB, with old ELB parameters removal",
ic: makeIC(spec(nlb()), status(elbWithNullParameters())),
expectedResult: true,
expectedIC: makeIC(spec(nlb()), status(nlbWithNullParameters())),
domainMatchesBaseDomain: true,
},
{
name: "loadbalancer type changed from NLB to ELB, with old NLB parameters removal",
ic: makeIC(spec(elb()), status(nlbWithNullParameters())),
expectedResult: true,
expectedIC: makeIC(spec(elb()), status(elbWithNullParameters())),
domainMatchesBaseDomain: true,
},
Comment on lines -655 to -668
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason you removed these test cases rather than changing them to match the new behavior? Granted, TestSetDefaultPublishingStrategyHandlesUpdates is already a bit unwieldy, and you did add plenty of test coverage in this commit, and that should be adequate.

{
name: "loadbalancer type changed from NLB to unset, with Classic LB as default",
ic: makeIC(spec(eps(lbs(operatorv1.ExternalLoadBalancer, &managedDNS))), status(nlb())),
Expand Down Expand Up @@ -1465,11 +1451,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 @@ -1515,6 +1510,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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
strategy: &loadBalancerStrategyWithNLB,
strategy: &loadBalancerStrategy,

based on the description of the test case.

platform: &awsPlatform,
service: &serviceWithNLB,
expect: false,
},
{
description: "loadbalancer strategy shouldn't use PROXY on Azure",
strategy: &loadBalancerStrategy,
Expand Down Expand Up @@ -1596,7 +1605,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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,15 +117,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, ingressConfig, infraConfig, apiConfig, networkConfig, proxyNeeded, haveClientCAConfigmap, clientCAConfigmap, clusterProxyConfig)
if err != nil {
return haveDepl, current, fmt.Errorf("failed to build router deployment: %v", err)
Expand Down
10 changes: 5 additions & 5 deletions pkg/operator/controller/ingress/deployment_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,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 @@ -683,7 +683,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 @@ -781,7 +781,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 @@ -891,7 +891,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 @@ -1009,7 +1009,7 @@ 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)
proxyNeeded, err := IsProxyProtocolNeeded(ic, infraConfig.Status.PlatformStatus, nil)
if err != nil {
t.Fatal(err)
}
Expand Down
Loading