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
+562 −233
Open

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

Show file tree
Hide file tree
Changes from 1 commit
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
4 changes: 4 additions & 0 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
76 changes: 64 additions & 12 deletions pkg/operator/controller/ingress/load_balancer_service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,8 +228,6 @@ var (
// local-with-fallback annotation for kube-proxy (see
// <https://bugzilla.redhat.com/show_bug.cgi?id=1960284>).
localWithFallbackAnnotation,
// AWS load balancer type annotation to set either CLB/ELB or NLB
AWSLBTypeAnnotation,
// awsLBProxyProtocolAnnotation is used to enable the PROXY protocol on any
// AWS load balancer services created.
//
Expand Down Expand Up @@ -279,6 +277,11 @@ var (
managedAtServiceCreationLBServiceAnnotations = func() map[configv1.PlatformType]sets.Set[string] {
result := map[configv1.PlatformType]sets.Set[string]{
configv1.AWSPlatformType: sets.New[string](
// Annotation to set either CLB/ELB or NLB.
// This annotation was previously fully managed (https://issues.redhat.com/browse/NE-865),
// but now required service recreation due to issues with AWS leaking resources when updating
// LB Type without recreating the service (see https://issues.redhat.com/browse/OCPBUGS-16728).
AWSLBTypeAnnotation,
// Annotation for configuring load balancer subnets.
// This requires service recreation because NLBs do not support updates to subnets,
// the operator cannot detect errors if the subnets are invalid, and to prevent
Expand Down Expand Up @@ -801,29 +804,61 @@ func loadBalancerServiceIsUpgradeable(ic *operatorv1.IngressController, deployme
return nil
}

// effectuateMessage returns a message describing how to effectuate a
// change that requires the service to be deleted.
func effectuateMessage(changedMsg, ocPatchRevertCmd string, service *corev1.Service) string {
return fmt.Sprintf(
"%[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`. "+
"Direct updates to the service annotations are not supported.",
changedMsg, service.Namespace, service.Name, ocPatchRevertCmd,
)
}

// loadBalancerServiceIsProgressing returns an error value indicating if the
// load balancer service is in progressing status.
// Tricky: Status fields have mixed functions. Older status fields often represent the
// *desired* state, while some newer fields reflect the *actual* state of the object.
func loadBalancerServiceIsProgressing(ic *operatorv1.IngressController, service *corev1.Service, platform *configv1.PlatformStatus, subnetsAWSEnabled bool, eipAllocationsAWSEnabled bool) error {
var errs []error
// Tricky: Scope in the status indicates the *desired* scope (user desire + defaulting).
wantScope := ic.Status.EndpointPublishingStrategy.LoadBalancer.Scope
haveScope := operatorv1.ExternalLoadBalancer
if IsServiceInternal(service) {
haveScope = operatorv1.InternalLoadBalancer
}
if wantScope != haveScope {
err := fmt.Errorf("The IngressController scope was changed from %q to %q.", haveScope, wantScope)
changedMsg := fmt.Sprintf("The IngressController scope was changed from %q to %q.", haveScope, wantScope)
err := fmt.Errorf(changedMsg)
Comment on lines +832 to +833
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we want double format expansion, do we?

Suggested change
changedMsg := fmt.Sprintf("The IngressController scope was changed from %q to %q.", haveScope, wantScope)
err := fmt.Errorf(changedMsg)
err := fmt.Errorf("The IngressController scope was changed from %q to %q.", haveScope, wantScope)
changedMsg := err.Error()

or

Suggested change
changedMsg := fmt.Sprintf("The IngressController scope was changed from %q to %q.", haveScope, wantScope)
err := fmt.Errorf(changedMsg)
changedMsg := fmt.Sprintf("The IngressController scope was changed from %q to %q.", haveScope, wantScope)
err := fmt.Errorf("%s", changedMsg)

or import "errors" and then

Suggested change
changedMsg := fmt.Sprintf("The IngressController scope was changed from %q to %q.", haveScope, wantScope)
err := fmt.Errorf(changedMsg)
changedMsg := fmt.Sprintf("The IngressController scope was changed from %q to %q.", haveScope, wantScope)
err := errors.New(changedMsg)

if _, ok := platformsWithMutableScope[platform.Type]; !ok {
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 from the old one's. Alternatively, you can revert the change to the IngressController: `oc -n openshift-ingress-operator patch ingresscontrollers/%[4]s --type=merge --patch='{\"spec\":{\"endpointPublishingStrategy\":{\"loadBalancer\":{\"scope\":\"%[5]s\"}}}}'`", err.Error(), service.Namespace, service.Name, ic.Name, haveScope)
ocPatchRevertCmd := fmt.Sprintf("oc -n openshift-ingress-operator patch ingresscontrollers/%[1]s --type=merge --patch='{\"spec\":{\"endpointPublishingStrategy\":{\"loadBalancer\":{\"scope\":\"%[2]s\"}}}}'", ic.Name, haveScope)
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
ocPatchRevertCmd := fmt.Sprintf("oc -n openshift-ingress-operator patch ingresscontrollers/%[1]s --type=merge --patch='{\"spec\":{\"endpointPublishingStrategy\":{\"loadBalancer\":{\"scope\":\"%[2]s\"}}}}'", ic.Name, haveScope)
ocPatchRevertCmd := fmt.Sprintf(`oc -n openshift-ingress-operator patch ingresscontrollers/%[1]s --type=merge --patch='{"spec":{"endpointPublishingStrategy":{"loadBalancer":{"scope":"%[2]s"}}}}'`, ic.Name, haveScope)

err = fmt.Errorf(effectuateMessage(changedMsg, ocPatchRevertCmd, service))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you have extra format expansion again, here and everywhere where you have fmt.Errorf(effectuateMessage(...)).

}
errs = append(errs, err)
}

haveLBType := getAWSLoadBalancerTypeFromServiceAnnotation(service)
if platform.Type == configv1.AWSPlatformType {
// Tricky: LB Type in the status indicates the *desired* scope (user desire + defaulting).
wantLBType := getAWSLoadBalancerTypeInStatus(ic)
Comment on lines +841 to +844
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic is confusing. Why is haveLBType computed outside the if statement and wantLBType inside? I think haveLBType will give operatorv1.AWSClassicLoadBalancer on non-AWS platforms, which is kind of silly. Moreover, it seems to me that all the AWS-specific code (for LB type, subnets, and EIP) should be inside a single if platform.Type == configv1.AWSPlatformType { block.


if wantLBType != haveLBType {
changedMsg := fmt.Sprintf("The IngressController load balancer type was changed from %q to %q.", haveLBType, wantLBType)
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\"}}}}}}'", ic.Name, haveLBType)
err := fmt.Errorf(effectuateMessage(changedMsg, ocPatchRevertCmd, service))
errs = append(errs, err)
}
}

if platform.Type == configv1.AWSPlatformType && subnetsAWSEnabled {
var (
// Tricky: Subnets in the status indicates the *actual* scope.
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar question. Can we read subnets from the service annotation directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Subnets and EIPs status do reflect the actual state, but yes, we can also read it from the service annotation directly.

It doesn't really matter either way. I implemented this way because I liked the idea of putting the annotation into the IC status value, then using the status as a single point to access the actual value.

wantSubnets, haveSubnets *operatorv1.AWSSubnets
paramsFieldName string
)
switch getAWSLoadBalancerTypeInStatus(ic) {
switch haveLBType {
case operatorv1.AWSNetworkLoadBalancer:
if nlbParams := getAWSNetworkLoadBalancerParametersInSpec(ic); nlbParams != nil {
wantSubnets = nlbParams.Subnets
Expand All @@ -848,14 +883,15 @@ func loadBalancerServiceIsProgressing(ic *operatorv1.IngressController, service
haveSubnetsPrettyJson := convertAWSSubnetListToPatchJson(haveSubnets, "{}", "[]")
wantSubnetsPrettyJson := convertAWSSubnetListToPatchJson(wantSubnets, "{}", "[]")
changedMsg := fmt.Sprintf("The IngressController subnets were changed from %q to %q.", haveSubnetsPrettyJson, wantSubnetsPrettyJson)
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\":{\"subnets\":%[4]s}}}}}}}'", ic.Name, getAWSLoadBalancerTypeInStatus(ic), paramsFieldName, haveSubnetsPatchJson)
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)
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\":{\"subnets\":%[4]s}}}}}}}'", ic.Name, haveLBType, paramsFieldName, haveSubnetsPatchJson)
err := fmt.Errorf(effectuateMessage(changedMsg, ocPatchRevertCmd, service))
errs = append(errs, err)
}
}

if platform.Type == configv1.AWSPlatformType && eipAllocationsAWSEnabled && getAWSLoadBalancerTypeInStatus(ic) == operatorv1.AWSNetworkLoadBalancer {
if platform.Type == configv1.AWSPlatformType && eipAllocationsAWSEnabled && haveLBType == operatorv1.AWSNetworkLoadBalancer {
var (
// Tricky: EIP Allocations in the status indicate the *actual* scope.
wantEIPAllocations, haveEIPAllocations []operatorv1.EIPAllocation
)
if nlbParams := getAWSNetworkLoadBalancerParametersInSpec(ic); nlbParams != nil {
Expand All @@ -871,20 +907,21 @@ func loadBalancerServiceIsProgressing(ic *operatorv1.IngressController, service
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)
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, haveLBType, "networkLoadBalancer", haveEIPAllocationsPatchJson)
err := fmt.Errorf(effectuateMessage(changedMsg, ocPatchRevertCmd, service))
errs = append(errs, err)
}
}

if platform.Type == configv1.OpenStackPlatformType {
// Tricky: FloatingIP in the status indicate the *actual* scope.
wantFloatingIP := getOpenStackFloatingIPInSpec(ic)
haveFloatingIP := getOpenStackFloatingIPInStatus(ic)
// OpenStack CCM does not support updating Service.Spec.LoadBalancerIP after creation, the load balancer will never be updated.
if wantFloatingIP != haveFloatingIP {
changeMsg := fmt.Sprintf("The IngressController floatingIP was changed from %q to %q.", haveFloatingIP, wantFloatingIP)
changedMsg := fmt.Sprintf("The IngressController floatingIP was changed from %q to %q.", haveFloatingIP, wantFloatingIP)
ocPatchRevertCmd := fmt.Sprintf(`oc -n %[1]s patch ingresscontrollers/%[2]s --type=merge --patch='{"spec":{"endpointPublishingStrategy":{"type":"LoadBalancerService","loadBalancer":{"providerParameters":{"type":"OpenStack","openstack":{"floatingIP":"%[3]s"}}}}}}'`, ic.Namespace, ic.Name, haveFloatingIP)
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`", changeMsg, service.Namespace, service.Name, ocPatchRevertCmd)
err := fmt.Errorf(effectuateMessage(changedMsg, ocPatchRevertCmd, service))
errs = append(errs, err)
}
}
Expand Down Expand Up @@ -997,6 +1034,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
9 changes: 8 additions & 1 deletion pkg/operator/controller/ingress/load_balancer_service_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1200,7 +1200,14 @@ func Test_loadBalancerServiceChanged(t *testing.T) {
svc.Annotations["service.beta.kubernetes.io/aws-load-balancer-subnets"] = "foo-subnet"
svc.Annotations["service.beta.kubernetes.io/aws-load-balancer-type"] = "NLB"
},
expect: true,
expect: false,
},
{
description: "if the service.beta.kubernetes.io/aws-load-balancer-type is added",
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
description: "if the service.beta.kubernetes.io/aws-load-balancer-type is added",
description: "if the service.beta.kubernetes.io/aws-load-balancer-type annotation is added",

mutate: func(svc *corev1.Service) {
svc.Annotations["service.beta.kubernetes.io/aws-load-balancer-type"] = "NLB"
},
expect: false,
},
{
description: "if the service.beta.kubernetes.io/aws-load-balancer-eip-allocations annotation added",
Expand Down
4 changes: 2 additions & 2 deletions pkg/operator/controller/ingress/status.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -954,7 +954,7 @@ func computeLoadBalancerProgressingStatus(ic *operatorv1.IngressController, serv
func updateIngressControllerAWSSubnetStatus(ic *operatorv1.IngressController, service *corev1.Service) {
// Set the subnets status based on the actual service annotation and the load balancer type,
// as NLBs and CLBs have separate subnet configuration fields.
switch getAWSLoadBalancerTypeInStatus(ic) {
switch getAWSLoadBalancerTypeFromServiceAnnotation(service) {
case operatorv1.AWSNetworkLoadBalancer:
// NetworkLoadBalancerParameters should be initialized by setDefaultPublishingStrategy
// when an IngressController is admitted, so we don't need to initialize here.
Expand All @@ -974,7 +974,7 @@ func updateIngressControllerAWSSubnetStatus(ic *operatorv1.IngressController, se
// 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) {
switch getAWSLoadBalancerTypeFromServiceAnnotation(service) {
case operatorv1.AWSNetworkLoadBalancer:
// NetworkLoadBalancerParameters should be initialized by setDefaultPublishingStrategy
// when an IngressController is admitted, so we don't need to initialize here.
Expand Down
Loading