Skip to content

Commit 0678597

Browse files
hrakclaude
hrak
and
claude
committed
feat: Clean up load balancer annotations in EnsureLoadBalancerDeleted
When a service switches from type LoadBalancer to ClusterIP, Kubernetes calls EnsureLoadBalancerDeleted. Previously this left stale CloudStack annotations on the service. Now all 6 LB annotations are removed on successful deletion via the service patcher, but only when the service is not being deleted (DeletionTimestamp is zero) to avoid unnecessary API calls on garbage-collected objects. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent dd42351 commit 0678597

File tree

2 files changed

+245
-36
lines changed

2 files changed

+245
-36
lines changed

‎cloudstack/cloudstack_loadbalancer.go‎

Lines changed: 40 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -324,9 +324,13 @@ func isFirewallSupported(services []cloudstack.NetworkServiceInternal) bool {
324324

325325
// EnsureLoadBalancerDeleted deletes the specified load balancer if it exists, returning
326326
// nil if the load balancer specified either didn't exist or was successfully deleted.
327-
func (cs *CSCloud) EnsureLoadBalancerDeleted(ctx context.Context, clusterName string, service *corev1.Service) error {
327+
func (cs *CSCloud) EnsureLoadBalancerDeleted(ctx context.Context, clusterName string, service *corev1.Service) (err error) {
328328
klog.V(4).InfoS("EnsureLoadBalancerDeleted", "cluster", clusterName, "service", klog.KObj(service))
329329

330+
// Patch the service to remove annotations after EnsureLoadBalancerDeleted finishes.
331+
patcher := newServicePatcher(cs.kclient, service)
332+
defer func() { err = patcher.Patch(ctx, err) }()
333+
330334
// Get the load balancer details and existing rules.
331335
name := cs.GetLoadBalancerName(ctx, clusterName, service)
332336
legacyName := cs.getLoadBalancerLegacyName(ctx, clusterName, service)
@@ -340,7 +344,17 @@ func (cs *CSCloud) EnsureLoadBalancerDeleted(ctx context.Context, clusterName st
340344
if len(lb.rules) == 0 {
341345
klog.V(4).Infof("No load balancer rules found for service, checking annotation for orphaned IP")
342346

343-
return cs.releaseOrphanedIPIfNeeded(lb, service)
347+
if err := cs.releaseOrphanedIPIfNeeded(lb, service); err != nil {
348+
return err
349+
}
350+
351+
// If the service is not marked for deletion (f.e. when switching from type
352+
// LoadBalancer to ClusterIP), remove our annotations.
353+
if service.DeletionTimestamp.IsZero() {
354+
deleteLoadBalancerAnnotations(service)
355+
}
356+
357+
return nil
344358
}
345359

346360
serviceName := fmt.Sprintf("%s/%s", service.Namespace, service.Name)
@@ -427,6 +441,12 @@ func (cs *CSCloud) EnsureLoadBalancerDeleted(ctx context.Context, clusterName st
427441
return fmt.Errorf("load balancer deletion completed with errors: %w", deletionErrors[0])
428442
}
429443

444+
// If the service is not marked for deletion (f.e. when switching from type
445+
// LoadBalancer to ClusterIP), remove our annotations.
446+
if service.DeletionTimestamp.IsZero() {
447+
deleteLoadBalancerAnnotations(service)
448+
}
449+
430450
msg := "Successfully deleted load balancer for service " + serviceName
431451
cs.eventRecorder.Event(service, corev1.EventTypeNormal, "DeletedLoadBalancer", msg)
432452
klog.Info(msg)
@@ -1421,3 +1441,21 @@ func setServiceAnnotation(service *corev1.Service, key, value string) {
14211441
}
14221442
service.ObjectMeta.Annotations[key] = value
14231443
}
1444+
1445+
// deleteServiceAnnotation removes an annotation from the Service object.
1446+
func deleteServiceAnnotation(service *corev1.Service, key string) {
1447+
if service.ObjectMeta.Annotations == nil {
1448+
return
1449+
}
1450+
delete(service.ObjectMeta.Annotations, key)
1451+
}
1452+
1453+
// deleteLoadBalancerAnnotations removes all CloudStack load balancer annotations from the service.
1454+
func deleteLoadBalancerAnnotations(service *corev1.Service) {
1455+
deleteServiceAnnotation(service, ServiceAnnotationLoadBalancerProxyProtocol)
1456+
deleteServiceAnnotation(service, ServiceAnnotationLoadBalancerLoadbalancerHostname)
1457+
deleteServiceAnnotation(service, ServiceAnnotationLoadBalancerAddress)
1458+
deleteServiceAnnotation(service, ServiceAnnotationLoadBalancerKeepIP)
1459+
deleteServiceAnnotation(service, ServiceAnnotationLoadBalancerID)
1460+
deleteServiceAnnotation(service, ServiceAnnotationLoadBalancerNetworkID)
1461+
}

‎cloudstack/cloudstack_loadbalancer_test.go‎

Lines changed: 205 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -3305,14 +3305,6 @@ func TestEnsureLoadBalancerDeletedOrphanedIP(t *testing.T) {
33053305
mockAddress.EXPECT().NewDisassociateIpAddressParams("ip-orphan").Return(&cloudstack.DisassociateIpAddressParams{})
33063306
mockAddress.EXPECT().DisassociateIpAddress(gomock.Any()).Return(&cloudstack.DisassociateIpAddressResponse{}, nil)
33073307

3308-
cs := &CSCloud{
3309-
client: &cloudstack.CloudStackClient{
3310-
LoadBalancer: mockLB,
3311-
Address: mockAddress,
3312-
},
3313-
eventRecorder: record.NewFakeRecorder(10),
3314-
}
3315-
33163308
service := &corev1.Service{
33173309
ObjectMeta: metav1.ObjectMeta{
33183310
Name: "foo",
@@ -3323,6 +3315,15 @@ func TestEnsureLoadBalancerDeletedOrphanedIP(t *testing.T) {
33233315
},
33243316
}
33253317

3318+
cs := &CSCloud{
3319+
client: &cloudstack.CloudStackClient{
3320+
LoadBalancer: mockLB,
3321+
Address: mockAddress,
3322+
},
3323+
kclient: fake.NewSimpleClientset(service),
3324+
eventRecorder: record.NewFakeRecorder(10),
3325+
}
3326+
33263327
err := cs.EnsureLoadBalancerDeleted(t.Context(), "cluster", service)
33273328
if err != nil {
33283329
t.Fatalf("unexpected error: %v", err)
@@ -3336,19 +3337,21 @@ func TestEnsureLoadBalancerDeletedOrphanedIP(t *testing.T) {
33363337
mockLB := cloudstack.NewMockLoadBalancerServiceIface(ctrl)
33373338
setupGetLoadBalancerByNameEmpty(mockLB)
33383339

3339-
cs := &CSCloud{
3340-
client: &cloudstack.CloudStackClient{
3341-
LoadBalancer: mockLB,
3342-
},
3343-
}
3344-
33453340
service := &corev1.Service{
33463341
ObjectMeta: metav1.ObjectMeta{
33473342
Name: "foo",
33483343
Namespace: "default",
33493344
},
33503345
}
33513346

3347+
cs := &CSCloud{
3348+
client: &cloudstack.CloudStackClient{
3349+
LoadBalancer: mockLB,
3350+
},
3351+
kclient: fake.NewSimpleClientset(service),
3352+
eventRecorder: record.NewFakeRecorder(10),
3353+
}
3354+
33523355
err := cs.EnsureLoadBalancerDeleted(t.Context(), "cluster", service)
33533356
if err != nil {
33543357
t.Fatalf("unexpected error: %v", err)
@@ -3370,13 +3373,6 @@ func TestEnsureLoadBalancerDeletedOrphanedIP(t *testing.T) {
33703373
Count: 0, PublicIpAddresses: []*cloudstack.PublicIpAddress{},
33713374
}, nil)
33723375

3373-
cs := &CSCloud{
3374-
client: &cloudstack.CloudStackClient{
3375-
LoadBalancer: mockLB,
3376-
Address: mockAddress,
3377-
},
3378-
}
3379-
33803376
service := &corev1.Service{
33813377
ObjectMeta: metav1.ObjectMeta{
33823378
Name: "foo",
@@ -3387,6 +3383,15 @@ func TestEnsureLoadBalancerDeletedOrphanedIP(t *testing.T) {
33873383
},
33883384
}
33893385

3386+
cs := &CSCloud{
3387+
client: &cloudstack.CloudStackClient{
3388+
LoadBalancer: mockLB,
3389+
Address: mockAddress,
3390+
},
3391+
kclient: fake.NewSimpleClientset(service),
3392+
eventRecorder: record.NewFakeRecorder(10),
3393+
}
3394+
33903395
err := cs.EnsureLoadBalancerDeleted(t.Context(), "cluster", service)
33913396
if err != nil {
33923397
t.Fatalf("unexpected error: %v", err)
@@ -3421,13 +3426,6 @@ func TestEnsureLoadBalancerDeletedOrphanedIP(t *testing.T) {
34213426
mockAddress.EXPECT().NewDisassociateIpAddressParams("ip-orphan").Return(&cloudstack.DisassociateIpAddressParams{})
34223427
mockAddress.EXPECT().DisassociateIpAddress(gomock.Any()).Return(nil, errors.New("release failed"))
34233428

3424-
cs := &CSCloud{
3425-
client: &cloudstack.CloudStackClient{
3426-
LoadBalancer: mockLB,
3427-
Address: mockAddress,
3428-
},
3429-
}
3430-
34313429
service := &corev1.Service{
34323430
ObjectMeta: metav1.ObjectMeta{
34333431
Name: "foo",
@@ -3438,6 +3436,15 @@ func TestEnsureLoadBalancerDeletedOrphanedIP(t *testing.T) {
34383436
},
34393437
}
34403438

3439+
cs := &CSCloud{
3440+
client: &cloudstack.CloudStackClient{
3441+
LoadBalancer: mockLB,
3442+
Address: mockAddress,
3443+
},
3444+
kclient: fake.NewSimpleClientset(service),
3445+
eventRecorder: record.NewFakeRecorder(10),
3446+
}
3447+
34413448
err := cs.EnsureLoadBalancerDeleted(t.Context(), "cluster", service)
34423449
if err == nil {
34433450
t.Fatalf("expected error")
@@ -3466,28 +3473,192 @@ func TestEnsureLoadBalancerDeletedOrphanedIP(t *testing.T) {
34663473
}, nil)
34673474
// shouldReleaseLoadBalancerIP returns false because keep-ip annotation is set
34683475

3476+
service := &corev1.Service{
3477+
ObjectMeta: metav1.ObjectMeta{
3478+
Name: "foo",
3479+
Namespace: "default",
3480+
Annotations: map[string]string{
3481+
ServiceAnnotationLoadBalancerAddress: "10.0.0.1",
3482+
ServiceAnnotationLoadBalancerKeepIP: "true",
3483+
},
3484+
},
3485+
}
3486+
34693487
cs := &CSCloud{
34703488
client: &cloudstack.CloudStackClient{
34713489
LoadBalancer: mockLB,
34723490
Address: mockAddress,
34733491
},
3492+
kclient: fake.NewSimpleClientset(service),
3493+
eventRecorder: record.NewFakeRecorder(10),
34743494
}
34753495

3496+
err := cs.EnsureLoadBalancerDeleted(t.Context(), "cluster", service)
3497+
if err != nil {
3498+
t.Fatalf("unexpected error: %v", err)
3499+
}
3500+
})
3501+
}
3502+
3503+
func TestEnsureLoadBalancerDeletedAnnotationCleanup(t *testing.T) {
3504+
// allLBAnnotations returns a map with all 6 CloudStack LB annotations set.
3505+
allLBAnnotations := func() map[string]string {
3506+
return map[string]string{
3507+
ServiceAnnotationLoadBalancerProxyProtocol: "true",
3508+
ServiceAnnotationLoadBalancerLoadbalancerHostname: "lb.example.com",
3509+
ServiceAnnotationLoadBalancerAddress: "10.0.0.1",
3510+
ServiceAnnotationLoadBalancerKeepIP: "false",
3511+
ServiceAnnotationLoadBalancerID: "ip-1",
3512+
ServiceAnnotationLoadBalancerNetworkID: "net-1",
3513+
}
3514+
}
3515+
3516+
lbAnnotationKeys := []string{
3517+
ServiceAnnotationLoadBalancerProxyProtocol,
3518+
ServiceAnnotationLoadBalancerLoadbalancerHostname,
3519+
ServiceAnnotationLoadBalancerAddress,
3520+
ServiceAnnotationLoadBalancerKeepIP,
3521+
ServiceAnnotationLoadBalancerID,
3522+
ServiceAnnotationLoadBalancerNetworkID,
3523+
}
3524+
3525+
t.Run("annotations removed after successful deletion", func(t *testing.T) {
3526+
ctrl := gomock.NewController(t)
3527+
t.Cleanup(ctrl.Finish)
3528+
3529+
mockLB := cloudstack.NewMockLoadBalancerServiceIface(ctrl)
3530+
mockAddress := cloudstack.NewMockAddressServiceIface(ctrl)
3531+
mockFirewall := cloudstack.NewMockFirewallServiceIface(ctrl)
3532+
3533+
// getLoadBalancerByName returns one rule
3534+
mockLB.EXPECT().NewListLoadBalancerRulesParams().Return(&cloudstack.ListLoadBalancerRulesParams{})
3535+
mockLB.EXPECT().ListLoadBalancerRules(gomock.Any()).Return(&cloudstack.ListLoadBalancerRulesResponse{
3536+
Count: 1,
3537+
LoadBalancerRules: []*cloudstack.LoadBalancerRule{
3538+
{
3539+
Id: "rule-1", Name: "K8s_svc_cluster_default_foo-tcp-80",
3540+
Publicip: "10.0.0.1", Publicipid: "ip-1", Publicport: "80",
3541+
Protocol: "tcp", Networkid: "net-1",
3542+
},
3543+
},
3544+
}, nil)
3545+
3546+
// deleteFirewallRule
3547+
mockFirewall.EXPECT().NewListFirewallRulesParams().Return(&cloudstack.ListFirewallRulesParams{})
3548+
mockFirewall.EXPECT().ListFirewallRules(gomock.Any()).Return(&cloudstack.ListFirewallRulesResponse{
3549+
Count: 0, FirewallRules: []*cloudstack.FirewallRule{},
3550+
}, nil)
3551+
3552+
// deleteLoadBalancerRule
3553+
mockLB.EXPECT().NewDeleteLoadBalancerRuleParams("rule-1").Return(&cloudstack.DeleteLoadBalancerRuleParams{})
3554+
mockLB.EXPECT().DeleteLoadBalancerRule(gomock.Any()).Return(&cloudstack.DeleteLoadBalancerRuleResponse{}, nil)
3555+
3556+
// shouldReleaseLoadBalancerIP: no keep-ip, no other rules
3557+
mockLB.EXPECT().NewListLoadBalancerRulesParams().Return(&cloudstack.ListLoadBalancerRulesParams{})
3558+
mockLB.EXPECT().ListLoadBalancerRules(gomock.Any()).Return(&cloudstack.ListLoadBalancerRulesResponse{
3559+
Count: 0, LoadBalancerRules: []*cloudstack.LoadBalancerRule{},
3560+
}, nil)
3561+
3562+
// releaseLoadBalancerIP
3563+
mockAddress.EXPECT().NewDisassociateIpAddressParams("ip-1").Return(&cloudstack.DisassociateIpAddressParams{})
3564+
mockAddress.EXPECT().DisassociateIpAddress(gomock.Any()).Return(&cloudstack.DisassociateIpAddressResponse{}, nil)
3565+
34763566
service := &corev1.Service{
34773567
ObjectMeta: metav1.ObjectMeta{
3478-
Name: "foo",
3479-
Namespace: "default",
3480-
Annotations: map[string]string{
3481-
ServiceAnnotationLoadBalancerAddress: "10.0.0.1",
3482-
ServiceAnnotationLoadBalancerKeepIP: "true",
3483-
},
3568+
Name: "foo",
3569+
Namespace: "default",
3570+
Annotations: allLBAnnotations(),
34843571
},
34853572
}
34863573

3574+
cs := &CSCloud{
3575+
client: &cloudstack.CloudStackClient{
3576+
LoadBalancer: mockLB,
3577+
Address: mockAddress,
3578+
Firewall: mockFirewall,
3579+
},
3580+
kclient: fake.NewSimpleClientset(service),
3581+
eventRecorder: record.NewFakeRecorder(10),
3582+
}
3583+
34873584
err := cs.EnsureLoadBalancerDeleted(t.Context(), "cluster", service)
34883585
if err != nil {
34893586
t.Fatalf("unexpected error: %v", err)
34903587
}
3588+
3589+
for _, key := range lbAnnotationKeys {
3590+
if _, ok := service.Annotations[key]; ok {
3591+
t.Errorf("annotation %q should have been removed", key)
3592+
}
3593+
}
3594+
})
3595+
3596+
t.Run("annotations preserved when deletion fails", func(t *testing.T) {
3597+
ctrl := gomock.NewController(t)
3598+
t.Cleanup(ctrl.Finish)
3599+
3600+
mockLB := cloudstack.NewMockLoadBalancerServiceIface(ctrl)
3601+
mockAddress := cloudstack.NewMockAddressServiceIface(ctrl)
3602+
mockFirewall := cloudstack.NewMockFirewallServiceIface(ctrl)
3603+
3604+
// getLoadBalancerByName returns one rule
3605+
mockLB.EXPECT().NewListLoadBalancerRulesParams().Return(&cloudstack.ListLoadBalancerRulesParams{}).Times(2)
3606+
mockLB.EXPECT().ListLoadBalancerRules(gomock.Any()).Return(&cloudstack.ListLoadBalancerRulesResponse{
3607+
Count: 1,
3608+
LoadBalancerRules: []*cloudstack.LoadBalancerRule{
3609+
{
3610+
Id: "rule-1", Name: "K8s_svc_cluster_default_foo-tcp-80",
3611+
Publicip: "10.0.0.1", Publicipid: "ip-1", Publicport: "80",
3612+
Protocol: "tcp", Networkid: "net-1",
3613+
},
3614+
},
3615+
}, nil).Times(1)
3616+
3617+
// deleteFirewallRule fails
3618+
mockFirewall.EXPECT().NewListFirewallRulesParams().Return(&cloudstack.ListFirewallRulesParams{})
3619+
mockFirewall.EXPECT().ListFirewallRules(gomock.Any()).Return(nil, errors.New("firewall error"))
3620+
3621+
// deleteLoadBalancerRule fails
3622+
mockLB.EXPECT().NewDeleteLoadBalancerRuleParams("rule-1").Return(&cloudstack.DeleteLoadBalancerRuleParams{})
3623+
mockLB.EXPECT().DeleteLoadBalancerRule(gomock.Any()).Return(nil, errors.New("delete rule error"))
3624+
3625+
// shouldReleaseLoadBalancerIP is still called (IP cleanup attempted even on rule errors)
3626+
mockLB.EXPECT().ListLoadBalancerRules(gomock.Any()).Return(&cloudstack.ListLoadBalancerRulesResponse{
3627+
Count: 0, LoadBalancerRules: []*cloudstack.LoadBalancerRule{},
3628+
}, nil)
3629+
3630+
// releaseLoadBalancerIP also fails
3631+
mockAddress.EXPECT().NewDisassociateIpAddressParams("ip-1").Return(&cloudstack.DisassociateIpAddressParams{})
3632+
mockAddress.EXPECT().DisassociateIpAddress(gomock.Any()).Return(nil, errors.New("release failed"))
3633+
3634+
service := &corev1.Service{
3635+
ObjectMeta: metav1.ObjectMeta{
3636+
Name: "foo",
3637+
Namespace: "default",
3638+
Annotations: allLBAnnotations(),
3639+
},
3640+
}
3641+
3642+
cs := &CSCloud{
3643+
client: &cloudstack.CloudStackClient{
3644+
LoadBalancer: mockLB,
3645+
Address: mockAddress,
3646+
Firewall: mockFirewall,
3647+
},
3648+
kclient: fake.NewSimpleClientset(service),
3649+
eventRecorder: record.NewFakeRecorder(10),
3650+
}
3651+
3652+
err := cs.EnsureLoadBalancerDeleted(t.Context(), "cluster", service)
3653+
if err == nil {
3654+
t.Fatalf("expected error")
3655+
}
3656+
3657+
for _, key := range lbAnnotationKeys {
3658+
if _, ok := service.Annotations[key]; !ok {
3659+
t.Errorf("annotation %q should have been preserved on error", key)
3660+
}
3661+
}
34913662
})
34923663
}
34933664

0 commit comments

Comments
 (0)