From 1bac752e552e1b009a235835c3c77098d1cd626f Mon Sep 17 00:00:00 2001 From: A10ss Date: Fri, 23 Jan 2026 15:30:47 +0800 Subject: [PATCH 01/13] feat: add support for load balancer, listener, and pool tags annotations --- pkg/openstack/loadbalancer.go | 117 +++++++++++++++++++++++++++++++++- 1 file changed, 114 insertions(+), 3 deletions(-) diff --git a/pkg/openstack/loadbalancer.go b/pkg/openstack/loadbalancer.go index 0a007e840a..38b547c839 100644 --- a/pkg/openstack/loadbalancer.go +++ b/pkg/openstack/loadbalancer.go @@ -96,6 +96,13 @@ const ( defaultProxyHostnameSuffix = "nip.io" ServiceAnnotationLoadBalancerID = "loadbalancer.openstack.org/load-balancer-id" + // ServiceAnnotationLoadBalancerTags The lb tags annotation is used to set tags on the loadbalancer resource itself(support json list). + ServiceAnnotationLoadBalancerTags = "loadbalancer.openstack.org/load-balancer-tags" + // ServiceAnnotationListenerTags The listener tags annotation is used to set tags on the loadbalancer listener resources(support json list). + ServiceAnnotationListenerTags = "loadbalancer.openstack.org/listener-tags" + // ServiceAnnotationPoolTags The pool tags annotation is used to set tags on the loadbalancer pool resources(support json list). + ServiceAnnotationPoolTags = "loadbalancer.openstack.org/pool-tags" + // Octavia resources name formats servicePrefix = "kube_service_" lbFormat = "%s%s_%s_%s" @@ -146,6 +153,9 @@ type serviceConfig struct { healthMonitorMaxRetries int healthMonitorMaxRetriesDown int preferredIPFamily corev1.IPFamily // preferred (the first) IP family indicated in service's `spec.ipFamilies` + lbTags string + listenerTags string + poolTags string } type listenerKey struct { @@ -197,6 +207,20 @@ func getLoadbalancerByName(ctx context.Context, client *gophercloud.ServiceClien return &validLBs[0], nil } +// mergeTags merges existedTags and desiredTags, returns true if all desiredTags are in existedTags. +func mergeTags(existedTags []string, desiredTags []string) (bool, []string) { + if len(existedTags) == 0 || existedTags == nil { + return false, desiredTags + } + desiredTagsSet := sets.NewString(desiredTags...) + tagSet := sets.NewString(existedTags...) + if tagSet.HasAll(desiredTags...) { + return true, nil + } else { + return false, tagSet.Union(desiredTagsSet).List() + } +} + func popListener(existingListeners []listeners.Listener, id string) []listeners.Listener { newListeners := []listeners.Listener{} for _, existingListener := range existingListeners { @@ -235,7 +259,13 @@ func (lbaas *LbaasV2) createOctaviaLoadBalancer(ctx context.Context, name, clust } if svcConf.supportLBTags { - createOpts.Tags = []string{svcConf.lbName} + var desiredTags []string + if len(svcConf.lbTags) == 0 { + klog.V(4).Infof("No load balancer tags found from service annotation key: %s", ServiceAnnotationLoadBalancerTags) + } else if err := json.Unmarshal([]byte(svcConf.lbTags), &desiredTags); err != nil { + klog.Warningf("unmarshal service annotation load balancer tags from key: %s, error: %s", ServiceAnnotationLoadBalancerTags, err) + } + createOpts.Tags = append([]string{svcConf.lbName}, desiredTags...) } if svcConf.flavorID != "" { @@ -923,16 +953,43 @@ func (lbaas *LbaasV2) ensureOctaviaPool(ctx context.Context, lbID string, name s } } + var desiredTags []string + if len(svcConf.poolTags) == 0 { + klog.V(4).Infof("No pools tags found from service annotation key: %s", ServiceAnnotationPoolTags) + } else if err := json.Unmarshal([]byte(svcConf.poolTags), &desiredTags); err != nil { + klog.Warningf("unmarshal service annotation pools tags from key: %s, error: %s", ServiceAnnotationPoolTags, err) + } + if pool == nil { createOpt := lbaas.buildPoolCreateOpt(listener.Protocol, service, svcConf, name) createOpt.ListenerID = listener.ID - + if svcConf.supportLBTags { + createOpt.Tags = desiredTags + } klog.InfoS("Creating pool", "listenerID", listener.ID, "protocol", createOpt.Protocol) + klog.V(4).Infof("Pool create options: %+v", createOpt) pool, err = openstackutil.CreatePool(ctx, lbaas.lb, createOpt, lbID) if err != nil { return nil, err } klog.V(2).Infof("Pool %s created for listener %s", pool.ID, listener.ID) + } else { + if svcConf.supportLBTags { + // Update tags if needed + if len(desiredTags) > 0 { + klog.V(4).Infof("Desired pools tags: %+v from service annotation key: %s", desiredTags, ServiceAnnotationPoolTags) + if ok, tags := mergeTags(pool.Tags, desiredTags); !ok { + klog.V(4).Infof("Will update pools' tags, current pools tags: %+v, desired tags: %+v", pool.Tags, tags) + updateOpts := v2pools.UpdateOpts{ + Tags: &tags, + } + klog.InfoS("Updating pool tags", "poolID", pool.ID, "listenerID", listener.ID, "lbID", lbID, "tags", desiredTags) + if err := openstackutil.UpdatePool(ctx, lbaas.lb, lbID, pool.ID, updateOpts); err != nil { + klog.Warningf("Error updating LB pool tags: %v", err) + } + } + } + } } if lbaas.opts.ProviderRequiresSerialAPICalls { @@ -1105,6 +1162,23 @@ func (lbaas *LbaasV2) ensureOctaviaListener(ctx context.Context, lbID string, na newTags = append(newTags, svcConf.lbName) updateOpts.Tags = &newTags listenerChanged = true + } else { + // Get desired tags from Service annotations + var desiredTags []string + if len(svcConf.listenerTags) == 0 { + klog.V(4).Infof("No listeners tags found from service annotation key: %s", ServiceAnnotationListenerTags) + } else if err := json.Unmarshal([]byte(svcConf.listenerTags), &desiredTags); err != nil { + klog.Warningf("unmarshal service annotation listeners tags from key: %s, error: %s", ServiceAnnotationListenerTags, err) + } + // Ensure listeners tags match the desired tags from Service annotations + if len(desiredTags) > 0 { + klog.Infof("Desired listener tags: %+v from service annotation key: %s", desiredTags, ServiceAnnotationListenerTags) + if ok, tags := mergeTags(listener.Tags, desiredTags); !ok { + klog.V(4).Infof("Will update listeners' tags, current listeners tags: %+v, desired tags: %+v", listener.Tags, tags) + updateOpts.Tags = &tags + listenerChanged = true + } + } } } @@ -1177,7 +1251,20 @@ func (lbaas *LbaasV2) buildListenerCreateOpt(ctx context.Context, port corev1.Se } if svcConf.supportLBTags { - listenerCreateOpt.Tags = []string{svcConf.lbName} + // Get desired tags from Service annotations + var desiredTags []string + if len(svcConf.listenerTags) == 0 { + klog.V(4).Infof("No listeners tags found from service annotation key: %s", ServiceAnnotationListenerTags) + } else if err := json.Unmarshal([]byte(svcConf.listenerTags), &desiredTags); err != nil { + klog.Warningf("unmarshal service annotation listeners tags from key: %s, error: %s", ServiceAnnotationListenerTags, err) + } + if len(desiredTags) > 0 { + klog.V(4).Infof("Desired listener tags: %+v from service annotation key: %s", desiredTags, ServiceAnnotationListenerTags) + if ok, tags := mergeTags([]string{svcConf.lbName}, desiredTags); !ok { + klog.V(4).Infof("Will update listeners' tags, current listeners tags: %s, desired tags: %+v", svcConf.lbName, tags) + listenerCreateOpt.Tags = tags + } + } } if openstackutil.IsOctaviaFeatureSupported(ctx, lbaas.lb, openstackutil.OctaviaFeatureTimeout, lbaas.opts.LBProvider) { @@ -1365,6 +1452,11 @@ func (lbaas *LbaasV2) checkService(ctx context.Context, service *corev1.Service, return fmt.Errorf("no service ports provided") } + annotations := service.GetAnnotations() + svcConf.lbTags = annotations[ServiceAnnotationLoadBalancerTags] + svcConf.listenerTags = annotations[ServiceAnnotationListenerTags] + svcConf.poolTags = annotations[ServiceAnnotationPoolTags] + if len(service.Spec.IPFamilies) > 0 { // Since OCCM does not support multiple load-balancers per service yet, // the first IP family will determine the IP family of the load-balancer @@ -1756,6 +1848,25 @@ func (lbaas *LbaasV2) ensureOctaviaLoadBalancer(ctx context.Context, clusterName // Make sure LB ID will be saved at this point. lbaas.updateServiceAnnotation(service, ServiceAnnotationLoadBalancerID, loadbalancer.ID) + if svcConf.supportLBTags { + var desiredTags []string + if len(svcConf.lbTags) == 0 { + klog.V(4).Infof("No load balancer tags found from service annotation key: %s", ServiceAnnotationLoadBalancerTags) + } else if err := json.Unmarshal([]byte(svcConf.lbTags), &desiredTags); err != nil { + klog.Warningf("unmarshal service annotation load balancer tags from key: %s, error: %s", ServiceAnnotationLoadBalancerTags, err) + } + // add the service annotation tags to load balancer tags if the tags don't match + if len(desiredTags) > 0 { + klog.V(4).Infof("Desired load balancer tags: %v from service annotation: %v", desiredTags, ServiceAnnotationLoadBalancerTags) + if ok, tags := mergeTags(loadbalancer.Tags, desiredTags); !ok { + klog.Infof("Will update load balancer's tags, current load balancer tags: %+v, desired tags: %+v", loadbalancer.Tags, tags) + if err := openstackutil.UpdateLoadBalancerTags(ctx, lbaas.lb, loadbalancer.ID, tags); err != nil { + klog.Warningf("failed to update load balancer %s tags: %v", loadbalancer.ID, err) + } + } + } + } + if loadbalancer.ProvisioningStatus != activeStatus { return nil, fmt.Errorf("load balancer %s is not ACTIVE, current provisioning status: %s", loadbalancer.ID, loadbalancer.ProvisioningStatus) } From dd53f2e77611a57b2b463dd703d9c74ddab60d85 Mon Sep 17 00:00:00 2001 From: A10ss Date: Tue, 27 Jan 2026 11:07:35 +0800 Subject: [PATCH 02/13] feat: refactor load balancer tag handling to use SplitTrim for better parsing --- pkg/openstack/loadbalancer.go | 46 +++++++++++++++++------------------ 1 file changed, 22 insertions(+), 24 deletions(-) diff --git a/pkg/openstack/loadbalancer.go b/pkg/openstack/loadbalancer.go index 38b547c839..bde13a5488 100644 --- a/pkg/openstack/loadbalancer.go +++ b/pkg/openstack/loadbalancer.go @@ -262,8 +262,8 @@ func (lbaas *LbaasV2) createOctaviaLoadBalancer(ctx context.Context, name, clust var desiredTags []string if len(svcConf.lbTags) == 0 { klog.V(4).Infof("No load balancer tags found from service annotation key: %s", ServiceAnnotationLoadBalancerTags) - } else if err := json.Unmarshal([]byte(svcConf.lbTags), &desiredTags); err != nil { - klog.Warningf("unmarshal service annotation load balancer tags from key: %s, error: %s", ServiceAnnotationLoadBalancerTags, err) + } else { + desiredTags = cpoutil.SplitTrim(svcConf.lbTags, ',') } createOpts.Tags = append([]string{svcConf.lbName}, desiredTags...) } @@ -956,8 +956,8 @@ func (lbaas *LbaasV2) ensureOctaviaPool(ctx context.Context, lbID string, name s var desiredTags []string if len(svcConf.poolTags) == 0 { klog.V(4).Infof("No pools tags found from service annotation key: %s", ServiceAnnotationPoolTags) - } else if err := json.Unmarshal([]byte(svcConf.poolTags), &desiredTags); err != nil { - klog.Warningf("unmarshal service annotation pools tags from key: %s, error: %s", ServiceAnnotationPoolTags, err) + } else { + desiredTags = cpoutil.SplitTrim(svcConf.poolTags, ',') } if pool == nil { @@ -973,20 +973,18 @@ func (lbaas *LbaasV2) ensureOctaviaPool(ctx context.Context, lbID string, name s return nil, err } klog.V(2).Infof("Pool %s created for listener %s", pool.ID, listener.ID) - } else { - if svcConf.supportLBTags { - // Update tags if needed - if len(desiredTags) > 0 { - klog.V(4).Infof("Desired pools tags: %+v from service annotation key: %s", desiredTags, ServiceAnnotationPoolTags) - if ok, tags := mergeTags(pool.Tags, desiredTags); !ok { - klog.V(4).Infof("Will update pools' tags, current pools tags: %+v, desired tags: %+v", pool.Tags, tags) - updateOpts := v2pools.UpdateOpts{ - Tags: &tags, - } - klog.InfoS("Updating pool tags", "poolID", pool.ID, "listenerID", listener.ID, "lbID", lbID, "tags", desiredTags) - if err := openstackutil.UpdatePool(ctx, lbaas.lb, lbID, pool.ID, updateOpts); err != nil { - klog.Warningf("Error updating LB pool tags: %v", err) - } + } else if svcConf.supportLBTags { + // Update tags if needed + if len(desiredTags) > 0 { + klog.V(4).Infof("Desired pools tags: %+v from service annotation key: %s", desiredTags, ServiceAnnotationPoolTags) + if ok, tags := mergeTags(pool.Tags, desiredTags); !ok { + klog.V(4).Infof("Will update pools' tags, current pools tags: %+v, desired tags: %+v", pool.Tags, tags) + updateOpts := v2pools.UpdateOpts{ + Tags: &tags, + } + klog.InfoS("Updating pool tags", "poolID", pool.ID, "listenerID", listener.ID, "lbID", lbID, "tags", desiredTags) + if err := openstackutil.UpdatePool(ctx, lbaas.lb, lbID, pool.ID, updateOpts); err != nil { + klog.Warningf("Error updating LB pool tags: %v", err) } } } @@ -1167,8 +1165,8 @@ func (lbaas *LbaasV2) ensureOctaviaListener(ctx context.Context, lbID string, na var desiredTags []string if len(svcConf.listenerTags) == 0 { klog.V(4).Infof("No listeners tags found from service annotation key: %s", ServiceAnnotationListenerTags) - } else if err := json.Unmarshal([]byte(svcConf.listenerTags), &desiredTags); err != nil { - klog.Warningf("unmarshal service annotation listeners tags from key: %s, error: %s", ServiceAnnotationListenerTags, err) + } else { + desiredTags = cpoutil.SplitTrim(svcConf.listenerTags, ',') } // Ensure listeners tags match the desired tags from Service annotations if len(desiredTags) > 0 { @@ -1255,8 +1253,8 @@ func (lbaas *LbaasV2) buildListenerCreateOpt(ctx context.Context, port corev1.Se var desiredTags []string if len(svcConf.listenerTags) == 0 { klog.V(4).Infof("No listeners tags found from service annotation key: %s", ServiceAnnotationListenerTags) - } else if err := json.Unmarshal([]byte(svcConf.listenerTags), &desiredTags); err != nil { - klog.Warningf("unmarshal service annotation listeners tags from key: %s, error: %s", ServiceAnnotationListenerTags, err) + } else { + desiredTags = cpoutil.SplitTrim(svcConf.listenerTags, ',') } if len(desiredTags) > 0 { klog.V(4).Infof("Desired listener tags: %+v from service annotation key: %s", desiredTags, ServiceAnnotationListenerTags) @@ -1852,8 +1850,8 @@ func (lbaas *LbaasV2) ensureOctaviaLoadBalancer(ctx context.Context, clusterName var desiredTags []string if len(svcConf.lbTags) == 0 { klog.V(4).Infof("No load balancer tags found from service annotation key: %s", ServiceAnnotationLoadBalancerTags) - } else if err := json.Unmarshal([]byte(svcConf.lbTags), &desiredTags); err != nil { - klog.Warningf("unmarshal service annotation load balancer tags from key: %s, error: %s", ServiceAnnotationLoadBalancerTags, err) + } else { + desiredTags = cpoutil.SplitTrim(svcConf.lbTags, ',') } // add the service annotation tags to load balancer tags if the tags don't match if len(desiredTags) > 0 { From 07ae20b84274a7ffd1dc9e5deb81528afee8efee Mon Sep 17 00:00:00 2001 From: A10ss Date: Tue, 27 Jan 2026 12:01:25 +0800 Subject: [PATCH 03/13] feat: streamline load balancer listener tag management for improved consistency --- pkg/openstack/loadbalancer.go | 42 ++++++++++++++--------------------- 1 file changed, 17 insertions(+), 25 deletions(-) diff --git a/pkg/openstack/loadbalancer.go b/pkg/openstack/loadbalancer.go index bde13a5488..809be1ec0c 100644 --- a/pkg/openstack/loadbalancer.go +++ b/pkg/openstack/loadbalancer.go @@ -1154,29 +1154,19 @@ func (lbaas *LbaasV2) ensureOctaviaListener(ctx context.Context, lbID string, na updateOpts := listeners.UpdateOpts{} if svcConf.supportLBTags { - if !slices.Contains(listener.Tags, svcConf.lbName) { - var newTags []string - copy(newTags, listener.Tags) - newTags = append(newTags, svcConf.lbName) - updateOpts.Tags = &newTags - listenerChanged = true + // Get desired tags from Service annotations + var desiredTags []string + if len(svcConf.listenerTags) == 0 { + klog.V(4).Infof("No listeners tags found from service annotation key: %s", ServiceAnnotationListenerTags) } else { - // Get desired tags from Service annotations - var desiredTags []string - if len(svcConf.listenerTags) == 0 { - klog.V(4).Infof("No listeners tags found from service annotation key: %s", ServiceAnnotationListenerTags) - } else { - desiredTags = cpoutil.SplitTrim(svcConf.listenerTags, ',') - } - // Ensure listeners tags match the desired tags from Service annotations - if len(desiredTags) > 0 { - klog.Infof("Desired listener tags: %+v from service annotation key: %s", desiredTags, ServiceAnnotationListenerTags) - if ok, tags := mergeTags(listener.Tags, desiredTags); !ok { - klog.V(4).Infof("Will update listeners' tags, current listeners tags: %+v, desired tags: %+v", listener.Tags, tags) - updateOpts.Tags = &tags - listenerChanged = true - } - } + desiredTags = cpoutil.SplitTrim(svcConf.listenerTags, ',') + } + + tagsToEnsure := append([]string{svcConf.lbName}, desiredTags...) + if ok, tags := mergeTags(listener.Tags, tagsToEnsure); !ok { + klog.V(4).Infof("Will update listeners' tags, current listeners tags: %+v, desired tags: %+v", listener.Tags, tags) + updateOpts.Tags = &tags + listenerChanged = true } } @@ -1256,13 +1246,15 @@ func (lbaas *LbaasV2) buildListenerCreateOpt(ctx context.Context, port corev1.Se } else { desiredTags = cpoutil.SplitTrim(svcConf.listenerTags, ',') } + + tags := []string{svcConf.lbName} if len(desiredTags) > 0 { klog.V(4).Infof("Desired listener tags: %+v from service annotation key: %s", desiredTags, ServiceAnnotationListenerTags) - if ok, tags := mergeTags([]string{svcConf.lbName}, desiredTags); !ok { - klog.V(4).Infof("Will update listeners' tags, current listeners tags: %s, desired tags: %+v", svcConf.lbName, tags) - listenerCreateOpt.Tags = tags + if ok, merged := mergeTags(tags, desiredTags); !ok { + tags = merged } } + listenerCreateOpt.Tags = tags } if openstackutil.IsOctaviaFeatureSupported(ctx, lbaas.lb, openstackutil.OctaviaFeatureTimeout, lbaas.opts.LBProvider) { From eb308332bc18a7f78fbf70d3785e6baa5515f9c8 Mon Sep 17 00:00:00 2001 From: Alexander Stephan Date: Fri, 24 Jul 2026 10:24:37 +0000 Subject: [PATCH 04/13] feat: address PR #3058 review feedback for LB tag annotations - Simplify mergeTags: drop unreachable nil check and redundant else - Collapse repeated SplitTrim guard blocks at all tag call sites - Correct annotation comments to comma-separated (not JSON) format - Add TestMergeTags and listener-tag cases to TestBuildListenerCreateOpt - Document load-balancer-tags, listener-tags and pool-tags annotations --- ...cations-using-loadbalancer-type-service.md | 12 +++ pkg/openstack/loadbalancer.go | 92 ++++++------------ pkg/openstack/loadbalancer_test.go | 93 +++++++++++++++++++ 3 files changed, 135 insertions(+), 62 deletions(-) diff --git a/docs/openstack-cloud-controller-manager/expose-applications-using-loadbalancer-type-service.md b/docs/openstack-cloud-controller-manager/expose-applications-using-loadbalancer-type-service.md index a74a3b44b3..e4806a99ed 100644 --- a/docs/openstack-cloud-controller-manager/expose-applications-using-loadbalancer-type-service.md +++ b/docs/openstack-cloud-controller-manager/expose-applications-using-loadbalancer-type-service.md @@ -239,6 +239,18 @@ Request Body: If this annotation is specified, the other annotations which define the load balancer features will be ignored. +- `loadbalancer.openstack.org/load-balancer-tags` + + A comma-separated list of tags to add to the load balancer resource in addition to the tags managed by OCCM. Example: `env=prod,team=network`. + +- `loadbalancer.openstack.org/listener-tags` + + A comma-separated list of tags to add to the load balancer listener resources in addition to the tags managed by OCCM. Example: `env=prod,team=network`. + +- `loadbalancer.openstack.org/pool-tags` + + A comma-separated list of tags to add to the load balancer pool resources. Example: `env=prod,team=network`. + - `loadbalancer.openstack.org/hostname` This annotations explicitly sets a hostname in the status of the load balancer service. diff --git a/pkg/openstack/loadbalancer.go b/pkg/openstack/loadbalancer.go index 809be1ec0c..e41e73b5b3 100644 --- a/pkg/openstack/loadbalancer.go +++ b/pkg/openstack/loadbalancer.go @@ -96,11 +96,11 @@ const ( defaultProxyHostnameSuffix = "nip.io" ServiceAnnotationLoadBalancerID = "loadbalancer.openstack.org/load-balancer-id" - // ServiceAnnotationLoadBalancerTags The lb tags annotation is used to set tags on the loadbalancer resource itself(support json list). + // ServiceAnnotationLoadBalancerTags is used to set additional tags on the loadbalancer resource itself (comma-separated list). ServiceAnnotationLoadBalancerTags = "loadbalancer.openstack.org/load-balancer-tags" - // ServiceAnnotationListenerTags The listener tags annotation is used to set tags on the loadbalancer listener resources(support json list). + // ServiceAnnotationListenerTags is used to set additional tags on the loadbalancer listener resources (comma-separated list). ServiceAnnotationListenerTags = "loadbalancer.openstack.org/listener-tags" - // ServiceAnnotationPoolTags The pool tags annotation is used to set tags on the loadbalancer pool resources(support json list). + // ServiceAnnotationPoolTags is used to set additional tags on the loadbalancer pool resources (comma-separated list). ServiceAnnotationPoolTags = "loadbalancer.openstack.org/pool-tags" // Octavia resources name formats @@ -207,18 +207,18 @@ func getLoadbalancerByName(ctx context.Context, client *gophercloud.ServiceClien return &validLBs[0], nil } -// mergeTags merges existedTags and desiredTags, returns true if all desiredTags are in existedTags. +// mergeTags merges existedTags and desiredTags, returns true if all desiredTags are already in existedTags. func mergeTags(existedTags []string, desiredTags []string) (bool, []string) { - if len(existedTags) == 0 || existedTags == nil { + if len(existedTags) == 0 { return false, desiredTags } - desiredTagsSet := sets.NewString(desiredTags...) + tagSet := sets.NewString(existedTags...) if tagSet.HasAll(desiredTags...) { return true, nil - } else { - return false, tagSet.Union(desiredTagsSet).List() } + + return false, tagSet.Union(sets.NewString(desiredTags...)).List() } func popListener(existingListeners []listeners.Listener, id string) []listeners.Listener { @@ -259,12 +259,7 @@ func (lbaas *LbaasV2) createOctaviaLoadBalancer(ctx context.Context, name, clust } if svcConf.supportLBTags { - var desiredTags []string - if len(svcConf.lbTags) == 0 { - klog.V(4).Infof("No load balancer tags found from service annotation key: %s", ServiceAnnotationLoadBalancerTags) - } else { - desiredTags = cpoutil.SplitTrim(svcConf.lbTags, ',') - } + desiredTags := cpoutil.SplitTrim(svcConf.lbTags, ',') createOpts.Tags = append([]string{svcConf.lbName}, desiredTags...) } @@ -953,12 +948,7 @@ func (lbaas *LbaasV2) ensureOctaviaPool(ctx context.Context, lbID string, name s } } - var desiredTags []string - if len(svcConf.poolTags) == 0 { - klog.V(4).Infof("No pools tags found from service annotation key: %s", ServiceAnnotationPoolTags) - } else { - desiredTags = cpoutil.SplitTrim(svcConf.poolTags, ',') - } + desiredTags := cpoutil.SplitTrim(svcConf.poolTags, ',') if pool == nil { createOpt := lbaas.buildPoolCreateOpt(listener.Protocol, service, svcConf, name) @@ -973,19 +963,17 @@ func (lbaas *LbaasV2) ensureOctaviaPool(ctx context.Context, lbID string, name s return nil, err } klog.V(2).Infof("Pool %s created for listener %s", pool.ID, listener.ID) - } else if svcConf.supportLBTags { + } else if svcConf.supportLBTags && len(desiredTags) > 0 { // Update tags if needed - if len(desiredTags) > 0 { - klog.V(4).Infof("Desired pools tags: %+v from service annotation key: %s", desiredTags, ServiceAnnotationPoolTags) - if ok, tags := mergeTags(pool.Tags, desiredTags); !ok { - klog.V(4).Infof("Will update pools' tags, current pools tags: %+v, desired tags: %+v", pool.Tags, tags) - updateOpts := v2pools.UpdateOpts{ - Tags: &tags, - } - klog.InfoS("Updating pool tags", "poolID", pool.ID, "listenerID", listener.ID, "lbID", lbID, "tags", desiredTags) - if err := openstackutil.UpdatePool(ctx, lbaas.lb, lbID, pool.ID, updateOpts); err != nil { - klog.Warningf("Error updating LB pool tags: %v", err) - } + klog.V(4).Infof("Desired pool tags: %+v from service annotation key: %s", desiredTags, ServiceAnnotationPoolTags) + if ok, tags := mergeTags(pool.Tags, desiredTags); !ok { + klog.V(4).Infof("Will update pool tags, current pool tags: %+v, desired tags: %+v", pool.Tags, tags) + updateOpts := v2pools.UpdateOpts{ + Tags: &tags, + } + klog.InfoS("Updating pool tags", "poolID", pool.ID, "listenerID", listener.ID, "lbID", lbID, "tags", tags) + if err := openstackutil.UpdatePool(ctx, lbaas.lb, lbID, pool.ID, updateOpts); err != nil { + klog.Warningf("Error updating LB pool tags: %v", err) } } } @@ -1154,17 +1142,11 @@ func (lbaas *LbaasV2) ensureOctaviaListener(ctx context.Context, lbID string, na updateOpts := listeners.UpdateOpts{} if svcConf.supportLBTags { - // Get desired tags from Service annotations - var desiredTags []string - if len(svcConf.listenerTags) == 0 { - klog.V(4).Infof("No listeners tags found from service annotation key: %s", ServiceAnnotationListenerTags) - } else { - desiredTags = cpoutil.SplitTrim(svcConf.listenerTags, ',') - } - + // Ensure the LB name tag plus any desired tags from the Service annotation. + desiredTags := cpoutil.SplitTrim(svcConf.listenerTags, ',') tagsToEnsure := append([]string{svcConf.lbName}, desiredTags...) if ok, tags := mergeTags(listener.Tags, tagsToEnsure); !ok { - klog.V(4).Infof("Will update listeners' tags, current listeners tags: %+v, desired tags: %+v", listener.Tags, tags) + klog.V(4).Infof("Will update listener tags, current listener tags: %+v, desired tags: %+v", listener.Tags, tags) updateOpts.Tags = &tags listenerChanged = true } @@ -1239,20 +1221,11 @@ func (lbaas *LbaasV2) buildListenerCreateOpt(ctx context.Context, port corev1.Se } if svcConf.supportLBTags { - // Get desired tags from Service annotations - var desiredTags []string - if len(svcConf.listenerTags) == 0 { - klog.V(4).Infof("No listeners tags found from service annotation key: %s", ServiceAnnotationListenerTags) - } else { - desiredTags = cpoutil.SplitTrim(svcConf.listenerTags, ',') - } - + // The LB name is always tagged, plus any desired tags from the Service annotation. + desiredTags := cpoutil.SplitTrim(svcConf.listenerTags, ',') tags := []string{svcConf.lbName} - if len(desiredTags) > 0 { - klog.V(4).Infof("Desired listener tags: %+v from service annotation key: %s", desiredTags, ServiceAnnotationListenerTags) - if ok, merged := mergeTags(tags, desiredTags); !ok { - tags = merged - } + if _, merged := mergeTags(tags, desiredTags); merged != nil { + tags = merged } listenerCreateOpt.Tags = tags } @@ -1839,17 +1812,12 @@ func (lbaas *LbaasV2) ensureOctaviaLoadBalancer(ctx context.Context, clusterName lbaas.updateServiceAnnotation(service, ServiceAnnotationLoadBalancerID, loadbalancer.ID) if svcConf.supportLBTags { - var desiredTags []string - if len(svcConf.lbTags) == 0 { - klog.V(4).Infof("No load balancer tags found from service annotation key: %s", ServiceAnnotationLoadBalancerTags) - } else { - desiredTags = cpoutil.SplitTrim(svcConf.lbTags, ',') - } - // add the service annotation tags to load balancer tags if the tags don't match + desiredTags := cpoutil.SplitTrim(svcConf.lbTags, ',') + // Add the Service annotation tags to the load balancer tags if they don't match. if len(desiredTags) > 0 { klog.V(4).Infof("Desired load balancer tags: %v from service annotation: %v", desiredTags, ServiceAnnotationLoadBalancerTags) if ok, tags := mergeTags(loadbalancer.Tags, desiredTags); !ok { - klog.Infof("Will update load balancer's tags, current load balancer tags: %+v, desired tags: %+v", loadbalancer.Tags, tags) + klog.Infof("Will update load balancer tags, current load balancer tags: %+v, desired tags: %+v", loadbalancer.Tags, tags) if err := openstackutil.UpdateLoadBalancerTags(ctx, lbaas.lb, loadbalancer.ID, tags); err != nil { klog.Warningf("failed to update load balancer %s tags: %v", loadbalancer.ID, err) } diff --git a/pkg/openstack/loadbalancer_test.go b/pkg/openstack/loadbalancer_test.go index 27304a45ad..462eb01307 100644 --- a/pkg/openstack/loadbalancer_test.go +++ b/pkg/openstack/loadbalancer_test.go @@ -152,6 +152,60 @@ type testGetRulesToCreateAndDelete struct { toDelete []rules.SecGroupRule } +func TestMergeTags(t *testing.T) { + testCases := []struct { + name string + existedTags []string + desiredTags []string + expectedOK bool + expectedTags []string + }{ + { + name: "nil existing tags returns desired tags", + existedTags: nil, + desiredTags: []string{"a", "b"}, + expectedOK: false, + expectedTags: []string{"a", "b"}, + }, + { + name: "empty existing tags returns desired tags", + existedTags: []string{}, + desiredTags: []string{"a"}, + expectedOK: false, + expectedTags: []string{"a"}, + }, + { + name: "all desired tags already present", + existedTags: []string{"a", "b", "c"}, + desiredTags: []string{"a", "b"}, + expectedOK: true, + expectedTags: nil, + }, + { + name: "some desired tags missing are merged and sorted", + existedTags: []string{"b", "d"}, + desiredTags: []string{"a", "c"}, + expectedOK: false, + expectedTags: []string{"a", "b", "c", "d"}, + }, + { + name: "empty desired tags with existing tags is a no-op", + existedTags: []string{"a"}, + desiredTags: []string{}, + expectedOK: true, + expectedTags: nil, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + ok, tags := mergeTags(tc.existedTags, tc.desiredTags) + assert.Equal(t, tc.expectedOK, ok) + assert.Equal(t, tc.expectedTags, tags) + }) + } +} + func TestGetRulesToCreateAndDelete(t *testing.T) { tests := []testGetRulesToCreateAndDelete{ { @@ -2485,6 +2539,45 @@ func TestBuildListenerCreateOpt(t *testing.T) { Tags: nil, }, }, + { + name: "Test with LB tags support and no listener-tags annotation", + port: corev1.ServicePort{ + Protocol: "TCP", + Port: 80, + }, + svcConf: &serviceConfig{ + connLimit: 100, + lbName: "my-lb", + supportLBTags: true, + }, + expectedCreateOpt: listeners.CreateOpts{ + Name: "Test with LB tags support and no listener-tags annotation", + Protocol: listeners.ProtocolTCP, + ProtocolPort: 80, + ConnLimit: &svcConf.connLimit, + Tags: []string{"my-lb"}, + }, + }, + { + name: "Test with LB tags support and listener-tags annotation", + port: corev1.ServicePort{ + Protocol: "TCP", + Port: 80, + }, + svcConf: &serviceConfig{ + connLimit: 100, + lbName: "my-lb", + supportLBTags: true, + listenerTags: "foo, bar", + }, + expectedCreateOpt: listeners.CreateOpts{ + Name: "Test with LB tags support and listener-tags annotation", + Protocol: listeners.ProtocolTCP, + ProtocolPort: 80, + ConnLimit: &svcConf.connLimit, + Tags: []string{"bar", "foo", "my-lb"}, + }, + }, } for _, tc := range testCases { From e20b4cc429378394adb0cc96058faee0b5b54697 Mon Sep 17 00:00:00 2001 From: Alexander Stephan Date: Fri, 24 Jul 2026 11:54:08 +0000 Subject: [PATCH 05/13] Address feedback --- pkg/openstack/loadbalancer.go | 103 +++++++++++++++++----------------- 1 file changed, 51 insertions(+), 52 deletions(-) diff --git a/pkg/openstack/loadbalancer.go b/pkg/openstack/loadbalancer.go index e41e73b5b3..120b24e22a 100644 --- a/pkg/openstack/loadbalancer.go +++ b/pkg/openstack/loadbalancer.go @@ -259,8 +259,8 @@ func (lbaas *LbaasV2) createOctaviaLoadBalancer(ctx context.Context, name, clust } if svcConf.supportLBTags { - desiredTags := cpoutil.SplitTrim(svcConf.lbTags, ',') - createOpts.Tags = append([]string{svcConf.lbName}, desiredTags...) + lbTags := cpoutil.SplitTrim(svcConf.lbTags, ',') + createOpts.Tags = append([]string{svcConf.lbName}, lbTags...) } if svcConf.flavorID != "" { @@ -927,7 +927,7 @@ func (lbaas *LbaasV2) ensureOctaviaPool(ctx context.Context, lbID string, name s pool = nil } - // If LBMethod changes, update the Pool with the new value + // Resolve the desired LB method, falling back to OCCM's default. var poolLbMethod string if svcConf.poolLbMethod != "" { poolLbMethod = svcConf.poolLbMethod @@ -935,26 +935,13 @@ func (lbaas *LbaasV2) ensureOctaviaPool(ctx context.Context, lbID string, name s // if LBMethod is not defined, fallback on default OCCM's default method poolLbMethod = lbaas.opts.LBMethod } - if pool != nil && pool.LBMethod != poolLbMethod { - klog.InfoS("Updating LoadBalancer LBMethod", "poolID", pool.ID, "listenerID", listener.ID, "lbID", lbID) - err = openstackutil.UpdatePool(ctx, lbaas.lb, lbID, pool.ID, v2pools.UpdateOpts{LBMethod: v2pools.LBMethod(poolLbMethod)}) - if err != nil { - err = PreserveGopherError(err) - msg := fmt.Sprintf("Error updating LB method for LoadBalancer: %v", err) - klog.Errorf(msg, "poolID", pool.ID, "listenerID", listener.ID, "lbID", lbID) - lbaas.eventRecorder.Event(service, corev1.EventTypeWarning, eventLBLbMethodUnknown, msg) - } else { - pool.LBMethod = poolLbMethod - } - } - - desiredTags := cpoutil.SplitTrim(svcConf.poolTags, ',') + poolTags := cpoutil.SplitTrim(svcConf.poolTags, ',') if pool == nil { createOpt := lbaas.buildPoolCreateOpt(listener.Protocol, service, svcConf, name) createOpt.ListenerID = listener.ID if svcConf.supportLBTags { - createOpt.Tags = desiredTags + createOpt.Tags = poolTags } klog.InfoS("Creating pool", "listenerID", listener.ID, "protocol", createOpt.Protocol) klog.V(4).Infof("Pool create options: %+v", createOpt) @@ -963,17 +950,41 @@ func (lbaas *LbaasV2) ensureOctaviaPool(ctx context.Context, lbID string, name s return nil, err } klog.V(2).Infof("Pool %s created for listener %s", pool.ID, listener.ID) - } else if svcConf.supportLBTags && len(desiredTags) > 0 { - // Update tags if needed - klog.V(4).Infof("Desired pool tags: %+v from service annotation key: %s", desiredTags, ServiceAnnotationPoolTags) - if ok, tags := mergeTags(pool.Tags, desiredTags); !ok { - klog.V(4).Infof("Will update pool tags, current pool tags: %+v, desired tags: %+v", pool.Tags, tags) - updateOpts := v2pools.UpdateOpts{ - Tags: &tags, + } else { + // Gather all desired changes to the existing pool into a single update. + updateOpts := v2pools.UpdateOpts{} + var newLBMethod string + var newTags []string + + // If LBMethod changes, update the Pool with the new value. + if pool.LBMethod != poolLbMethod { + newLBMethod = poolLbMethod + updateOpts.LBMethod = v2pools.LBMethod(poolLbMethod) + } + + // If tags differ, update them too. + if svcConf.supportLBTags && len(poolTags) > 0 { + klog.V(4).Infof("Desired pool tags: %+v from service annotation key: %s", poolTags, ServiceAnnotationPoolTags) + if ok, tags := mergeTags(pool.Tags, poolTags); !ok { + newTags = tags + updateOpts.Tags = &newTags } - klog.InfoS("Updating pool tags", "poolID", pool.ID, "listenerID", listener.ID, "lbID", lbID, "tags", tags) + } + + if newLBMethod != "" || newTags != nil { + klog.InfoS("Updating pool", "poolID", pool.ID, "listenerID", listener.ID, "lbID", lbID, "lbMethod", newLBMethod, "tags", newTags) if err := openstackutil.UpdatePool(ctx, lbaas.lb, lbID, pool.ID, updateOpts); err != nil { - klog.Warningf("Error updating LB pool tags: %v", err) + err = PreserveGopherError(err) + msg := fmt.Sprintf("Error updating pool for LoadBalancer: %v", err) + klog.Errorf(msg, "poolID", pool.ID, "listenerID", listener.ID, "lbID", lbID) + lbaas.eventRecorder.Event(service, corev1.EventTypeWarning, eventLBLbMethodUnknown, msg) + } else { + if newLBMethod != "" { + pool.LBMethod = newLBMethod + } + if newTags != nil { + pool.Tags = newTags + } } } } @@ -1143,8 +1154,8 @@ func (lbaas *LbaasV2) ensureOctaviaListener(ctx context.Context, lbID string, na if svcConf.supportLBTags { // Ensure the LB name tag plus any desired tags from the Service annotation. - desiredTags := cpoutil.SplitTrim(svcConf.listenerTags, ',') - tagsToEnsure := append([]string{svcConf.lbName}, desiredTags...) + listenerTags := cpoutil.SplitTrim(svcConf.listenerTags, ',') + tagsToEnsure := append([]string{svcConf.lbName}, listenerTags...) if ok, tags := mergeTags(listener.Tags, tagsToEnsure); !ok { klog.V(4).Infof("Will update listener tags, current listener tags: %+v, desired tags: %+v", listener.Tags, tags) updateOpts.Tags = &tags @@ -1222,9 +1233,9 @@ func (lbaas *LbaasV2) buildListenerCreateOpt(ctx context.Context, port corev1.Se if svcConf.supportLBTags { // The LB name is always tagged, plus any desired tags from the Service annotation. - desiredTags := cpoutil.SplitTrim(svcConf.listenerTags, ',') + listenerTags := cpoutil.SplitTrim(svcConf.listenerTags, ',') tags := []string{svcConf.lbName} - if _, merged := mergeTags(tags, desiredTags); merged != nil { + if ok, merged := mergeTags(tags, listenerTags); !ok && len(merged) > 0 { tags = merged } listenerCreateOpt.Tags = tags @@ -1812,16 +1823,16 @@ func (lbaas *LbaasV2) ensureOctaviaLoadBalancer(ctx context.Context, clusterName lbaas.updateServiceAnnotation(service, ServiceAnnotationLoadBalancerID, loadbalancer.ID) if svcConf.supportLBTags { - desiredTags := cpoutil.SplitTrim(svcConf.lbTags, ',') - // Add the Service annotation tags to the load balancer tags if they don't match. - if len(desiredTags) > 0 { - klog.V(4).Infof("Desired load balancer tags: %v from service annotation: %v", desiredTags, ServiceAnnotationLoadBalancerTags) - if ok, tags := mergeTags(loadbalancer.Tags, desiredTags); !ok { - klog.Infof("Will update load balancer tags, current load balancer tags: %+v, desired tags: %+v", loadbalancer.Tags, tags) - if err := openstackutil.UpdateLoadBalancerTags(ctx, lbaas.lb, loadbalancer.ID, tags); err != nil { - klog.Warningf("failed to update load balancer %s tags: %v", loadbalancer.ID, err) - } + // Ensure the LB name tag plus any tags from the Service annotation in a single update. + lbTags := cpoutil.SplitTrim(svcConf.lbTags, ',') + desiredLBTags := append([]string{lbName}, lbTags...) + klog.V(4).Infof("Desired load balancer tags: %v (LB name plus annotation %s)", desiredLBTags, ServiceAnnotationLoadBalancerTags) + if ok, tags := mergeTags(loadbalancer.Tags, desiredLBTags); !ok { + klog.InfoS("Updating load balancer tags", "lbID", loadbalancer.ID, "tags", tags) + if err := openstackutil.UpdateLoadBalancerTags(ctx, lbaas.lb, loadbalancer.ID, tags); err != nil { + return nil, fmt.Errorf("failed to update load balancer %s tags: %w", loadbalancer.ID, err) } + loadbalancer.Tags = tags } } @@ -1895,18 +1906,6 @@ func (lbaas *LbaasV2) ensureOctaviaLoadBalancer(ctx context.Context, clusterName // save address into the annotation lbaas.updateServiceAnnotation(service, ServiceAnnotationLoadBalancerAddress, addr) - // add LB name to load balancer tags. - if svcConf.supportLBTags { - lbTags := loadbalancer.Tags - if !slices.Contains(lbTags, lbName) { - lbTags = append(lbTags, lbName) - klog.InfoS("Updating load balancer tags", "lbID", loadbalancer.ID, "tags", lbTags) - if err := openstackutil.UpdateLoadBalancerTags(ctx, lbaas.lb, loadbalancer.ID, lbTags); err != nil { - return nil, err - } - } - } - // Create status the load balancer status := lbaas.createLoadBalancerStatus(service, svcConf, addr) From 35d8c76445399f66fc224c045b68286c554a1fe3 Mon Sep 17 00:00:00 2001 From: Alexander Stephan Date: Fri, 24 Jul 2026 12:04:27 +0000 Subject: [PATCH 06/13] Add withLBNameTag helper --- pkg/openstack/loadbalancer.go | 24 ++++++++++-------------- pkg/openstack/loadbalancer_test.go | 2 +- 2 files changed, 11 insertions(+), 15 deletions(-) diff --git a/pkg/openstack/loadbalancer.go b/pkg/openstack/loadbalancer.go index 120b24e22a..ef9f9f4b33 100644 --- a/pkg/openstack/loadbalancer.go +++ b/pkg/openstack/loadbalancer.go @@ -207,6 +207,12 @@ func getLoadbalancerByName(ctx context.Context, client *gophercloud.ServiceClien return &validLBs[0], nil } +// withLBNameTag returns the LB name (OCCM's ownership tag) followed by the +// comma-separated tags from the given Service annotation value. +func withLBNameTag(lbName, annotation string) []string { + return append([]string{lbName}, cpoutil.SplitTrim(annotation, ',')...) +} + // mergeTags merges existedTags and desiredTags, returns true if all desiredTags are already in existedTags. func mergeTags(existedTags []string, desiredTags []string) (bool, []string) { if len(existedTags) == 0 { @@ -259,8 +265,7 @@ func (lbaas *LbaasV2) createOctaviaLoadBalancer(ctx context.Context, name, clust } if svcConf.supportLBTags { - lbTags := cpoutil.SplitTrim(svcConf.lbTags, ',') - createOpts.Tags = append([]string{svcConf.lbName}, lbTags...) + createOpts.Tags = withLBNameTag(svcConf.lbName, svcConf.lbTags) } if svcConf.flavorID != "" { @@ -956,13 +961,11 @@ func (lbaas *LbaasV2) ensureOctaviaPool(ctx context.Context, lbID string, name s var newLBMethod string var newTags []string - // If LBMethod changes, update the Pool with the new value. if pool.LBMethod != poolLbMethod { newLBMethod = poolLbMethod updateOpts.LBMethod = v2pools.LBMethod(poolLbMethod) } - // If tags differ, update them too. if svcConf.supportLBTags && len(poolTags) > 0 { klog.V(4).Infof("Desired pool tags: %+v from service annotation key: %s", poolTags, ServiceAnnotationPoolTags) if ok, tags := mergeTags(pool.Tags, poolTags); !ok { @@ -1154,8 +1157,7 @@ func (lbaas *LbaasV2) ensureOctaviaListener(ctx context.Context, lbID string, na if svcConf.supportLBTags { // Ensure the LB name tag plus any desired tags from the Service annotation. - listenerTags := cpoutil.SplitTrim(svcConf.listenerTags, ',') - tagsToEnsure := append([]string{svcConf.lbName}, listenerTags...) + tagsToEnsure := withLBNameTag(svcConf.lbName, svcConf.listenerTags) if ok, tags := mergeTags(listener.Tags, tagsToEnsure); !ok { klog.V(4).Infof("Will update listener tags, current listener tags: %+v, desired tags: %+v", listener.Tags, tags) updateOpts.Tags = &tags @@ -1233,12 +1235,7 @@ func (lbaas *LbaasV2) buildListenerCreateOpt(ctx context.Context, port corev1.Se if svcConf.supportLBTags { // The LB name is always tagged, plus any desired tags from the Service annotation. - listenerTags := cpoutil.SplitTrim(svcConf.listenerTags, ',') - tags := []string{svcConf.lbName} - if ok, merged := mergeTags(tags, listenerTags); !ok && len(merged) > 0 { - tags = merged - } - listenerCreateOpt.Tags = tags + listenerCreateOpt.Tags = withLBNameTag(svcConf.lbName, svcConf.listenerTags) } if openstackutil.IsOctaviaFeatureSupported(ctx, lbaas.lb, openstackutil.OctaviaFeatureTimeout, lbaas.opts.LBProvider) { @@ -1824,8 +1821,7 @@ func (lbaas *LbaasV2) ensureOctaviaLoadBalancer(ctx context.Context, clusterName if svcConf.supportLBTags { // Ensure the LB name tag plus any tags from the Service annotation in a single update. - lbTags := cpoutil.SplitTrim(svcConf.lbTags, ',') - desiredLBTags := append([]string{lbName}, lbTags...) + desiredLBTags := withLBNameTag(lbName, svcConf.lbTags) klog.V(4).Infof("Desired load balancer tags: %v (LB name plus annotation %s)", desiredLBTags, ServiceAnnotationLoadBalancerTags) if ok, tags := mergeTags(loadbalancer.Tags, desiredLBTags); !ok { klog.InfoS("Updating load balancer tags", "lbID", loadbalancer.ID, "tags", tags) diff --git a/pkg/openstack/loadbalancer_test.go b/pkg/openstack/loadbalancer_test.go index 462eb01307..7f6b04766f 100644 --- a/pkg/openstack/loadbalancer_test.go +++ b/pkg/openstack/loadbalancer_test.go @@ -2575,7 +2575,7 @@ func TestBuildListenerCreateOpt(t *testing.T) { Protocol: listeners.ProtocolTCP, ProtocolPort: 80, ConnLimit: &svcConf.connLimit, - Tags: []string{"bar", "foo", "my-lb"}, + Tags: []string{"my-lb", "foo", "bar"}, }, }, } From 93035b50f8d05471dae4cc6d9f45d18ec29cccd8 Mon Sep 17 00:00:00 2001 From: Alexander Stephan Date: Fri, 24 Jul 2026 12:35:44 +0000 Subject: [PATCH 07/13] Rename desiredTags completely --- pkg/openstack/loadbalancer.go | 37 +++++++++++------------------- pkg/openstack/loadbalancer_test.go | 14 +++++------ 2 files changed, 20 insertions(+), 31 deletions(-) diff --git a/pkg/openstack/loadbalancer.go b/pkg/openstack/loadbalancer.go index ef9f9f4b33..82ec1450f2 100644 --- a/pkg/openstack/loadbalancer.go +++ b/pkg/openstack/loadbalancer.go @@ -213,18 +213,18 @@ func withLBNameTag(lbName, annotation string) []string { return append([]string{lbName}, cpoutil.SplitTrim(annotation, ',')...) } -// mergeTags merges existedTags and desiredTags, returns true if all desiredTags are already in existedTags. -func mergeTags(existedTags []string, desiredTags []string) (bool, []string) { +// mergeTags merges existedTags and newTags, returns true if all newTags are already in existedTags. +func mergeTags(existedTags []string, newTags []string) (bool, []string) { if len(existedTags) == 0 { - return false, desiredTags + return false, newTags } tagSet := sets.NewString(existedTags...) - if tagSet.HasAll(desiredTags...) { + if tagSet.HasAll(newTags...) { return true, nil } - return false, tagSet.Union(sets.NewString(desiredTags...)).List() + return false, tagSet.Union(sets.NewString(newTags...)).List() } func popListener(existingListeners []listeners.Listener, id string) []listeners.Listener { @@ -958,36 +958,25 @@ func (lbaas *LbaasV2) ensureOctaviaPool(ctx context.Context, lbID string, name s } else { // Gather all desired changes to the existing pool into a single update. updateOpts := v2pools.UpdateOpts{} - var newLBMethod string - var newTags []string if pool.LBMethod != poolLbMethod { - newLBMethod = poolLbMethod updateOpts.LBMethod = v2pools.LBMethod(poolLbMethod) } if svcConf.supportLBTags && len(poolTags) > 0 { klog.V(4).Infof("Desired pool tags: %+v from service annotation key: %s", poolTags, ServiceAnnotationPoolTags) if ok, tags := mergeTags(pool.Tags, poolTags); !ok { - newTags = tags - updateOpts.Tags = &newTags + updateOpts.Tags = &tags } } - if newLBMethod != "" || newTags != nil { - klog.InfoS("Updating pool", "poolID", pool.ID, "listenerID", listener.ID, "lbID", lbID, "lbMethod", newLBMethod, "tags", newTags) + if updateOpts != (v2pools.UpdateOpts{}) { + klog.InfoS("Updating pool", "poolID", pool.ID, "listenerID", listener.ID, "lbID", lbID, "lbMethod", updateOpts.LBMethod, "tags", updateOpts.Tags) if err := openstackutil.UpdatePool(ctx, lbaas.lb, lbID, pool.ID, updateOpts); err != nil { err = PreserveGopherError(err) msg := fmt.Sprintf("Error updating pool for LoadBalancer: %v", err) klog.Errorf(msg, "poolID", pool.ID, "listenerID", listener.ID, "lbID", lbID) lbaas.eventRecorder.Event(service, corev1.EventTypeWarning, eventLBLbMethodUnknown, msg) - } else { - if newLBMethod != "" { - pool.LBMethod = newLBMethod - } - if newTags != nil { - pool.Tags = newTags - } } } } @@ -1157,8 +1146,8 @@ func (lbaas *LbaasV2) ensureOctaviaListener(ctx context.Context, lbID string, na if svcConf.supportLBTags { // Ensure the LB name tag plus any desired tags from the Service annotation. - tagsToEnsure := withLBNameTag(svcConf.lbName, svcConf.listenerTags) - if ok, tags := mergeTags(listener.Tags, tagsToEnsure); !ok { + listenerTags := withLBNameTag(svcConf.lbName, svcConf.listenerTags) + if ok, tags := mergeTags(listener.Tags, listenerTags); !ok { klog.V(4).Infof("Will update listener tags, current listener tags: %+v, desired tags: %+v", listener.Tags, tags) updateOpts.Tags = &tags listenerChanged = true @@ -1821,9 +1810,9 @@ func (lbaas *LbaasV2) ensureOctaviaLoadBalancer(ctx context.Context, clusterName if svcConf.supportLBTags { // Ensure the LB name tag plus any tags from the Service annotation in a single update. - desiredLBTags := withLBNameTag(lbName, svcConf.lbTags) - klog.V(4).Infof("Desired load balancer tags: %v (LB name plus annotation %s)", desiredLBTags, ServiceAnnotationLoadBalancerTags) - if ok, tags := mergeTags(loadbalancer.Tags, desiredLBTags); !ok { + lbTags := withLBNameTag(lbName, svcConf.lbTags) + klog.V(4).Infof("Desired load balancer tags: %v (LB name plus annotation %s)", lbTags, ServiceAnnotationLoadBalancerTags) + if ok, tags := mergeTags(loadbalancer.Tags, lbTags); !ok { klog.InfoS("Updating load balancer tags", "lbID", loadbalancer.ID, "tags", tags) if err := openstackutil.UpdateLoadBalancerTags(ctx, lbaas.lb, loadbalancer.ID, tags); err != nil { return nil, fmt.Errorf("failed to update load balancer %s tags: %w", loadbalancer.ID, err) diff --git a/pkg/openstack/loadbalancer_test.go b/pkg/openstack/loadbalancer_test.go index 7f6b04766f..f3f82f9d91 100644 --- a/pkg/openstack/loadbalancer_test.go +++ b/pkg/openstack/loadbalancer_test.go @@ -156,42 +156,42 @@ func TestMergeTags(t *testing.T) { testCases := []struct { name string existedTags []string - desiredTags []string + newTags []string expectedOK bool expectedTags []string }{ { name: "nil existing tags returns desired tags", existedTags: nil, - desiredTags: []string{"a", "b"}, + newTags: []string{"a", "b"}, expectedOK: false, expectedTags: []string{"a", "b"}, }, { name: "empty existing tags returns desired tags", existedTags: []string{}, - desiredTags: []string{"a"}, + newTags: []string{"a"}, expectedOK: false, expectedTags: []string{"a"}, }, { name: "all desired tags already present", existedTags: []string{"a", "b", "c"}, - desiredTags: []string{"a", "b"}, + newTags: []string{"a", "b"}, expectedOK: true, expectedTags: nil, }, { name: "some desired tags missing are merged and sorted", existedTags: []string{"b", "d"}, - desiredTags: []string{"a", "c"}, + newTags: []string{"a", "c"}, expectedOK: false, expectedTags: []string{"a", "b", "c", "d"}, }, { name: "empty desired tags with existing tags is a no-op", existedTags: []string{"a"}, - desiredTags: []string{}, + newTags: []string{}, expectedOK: true, expectedTags: nil, }, @@ -199,7 +199,7 @@ func TestMergeTags(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - ok, tags := mergeTags(tc.existedTags, tc.desiredTags) + ok, tags := mergeTags(tc.existedTags, tc.newTags) assert.Equal(t, tc.expectedOK, ok) assert.Equal(t, tc.expectedTags, tags) }) From 182bdb91aab9dcbff855a4c65fbaba1ae0378a2b Mon Sep 17 00:00:00 2001 From: Alexander Stephan Date: Fri, 24 Jul 2026 12:59:01 +0000 Subject: [PATCH 08/13] Change structure --- pkg/openstack/loadbalancer.go | 64 +++++++++++++++++------------------ 1 file changed, 32 insertions(+), 32 deletions(-) diff --git a/pkg/openstack/loadbalancer.go b/pkg/openstack/loadbalancer.go index 82ec1450f2..bd51aee941 100644 --- a/pkg/openstack/loadbalancer.go +++ b/pkg/openstack/loadbalancer.go @@ -942,37 +942,22 @@ func (lbaas *LbaasV2) ensureOctaviaPool(ctx context.Context, lbID string, name s } poolTags := cpoutil.SplitTrim(svcConf.poolTags, ',') - if pool == nil { - createOpt := lbaas.buildPoolCreateOpt(listener.Protocol, service, svcConf, name) - createOpt.ListenerID = listener.ID - if svcConf.supportLBTags { - createOpt.Tags = poolTags - } - klog.InfoS("Creating pool", "listenerID", listener.ID, "protocol", createOpt.Protocol) - klog.V(4).Infof("Pool create options: %+v", createOpt) - pool, err = openstackutil.CreatePool(ctx, lbaas.lb, createOpt, lbID) - if err != nil { - return nil, err - } - klog.V(2).Infof("Pool %s created for listener %s", pool.ID, listener.ID) - } else { - // Gather all desired changes to the existing pool into a single update. + // If the LBMethod or tags change, update the existing pool in a single call. + if pool != nil { updateOpts := v2pools.UpdateOpts{} - if pool.LBMethod != poolLbMethod { updateOpts.LBMethod = v2pools.LBMethod(poolLbMethod) } - if svcConf.supportLBTags && len(poolTags) > 0 { klog.V(4).Infof("Desired pool tags: %+v from service annotation key: %s", poolTags, ServiceAnnotationPoolTags) if ok, tags := mergeTags(pool.Tags, poolTags); !ok { updateOpts.Tags = &tags } } - if updateOpts != (v2pools.UpdateOpts{}) { klog.InfoS("Updating pool", "poolID", pool.ID, "listenerID", listener.ID, "lbID", lbID, "lbMethod", updateOpts.LBMethod, "tags", updateOpts.Tags) - if err := openstackutil.UpdatePool(ctx, lbaas.lb, lbID, pool.ID, updateOpts); err != nil { + err = openstackutil.UpdatePool(ctx, lbaas.lb, lbID, pool.ID, updateOpts) + if err != nil { err = PreserveGopherError(err) msg := fmt.Sprintf("Error updating pool for LoadBalancer: %v", err) klog.Errorf(msg, "poolID", pool.ID, "listenerID", listener.ID, "lbID", lbID) @@ -981,6 +966,21 @@ func (lbaas *LbaasV2) ensureOctaviaPool(ctx context.Context, lbID string, name s } } + if pool == nil { + createOpt := lbaas.buildPoolCreateOpt(listener.Protocol, service, svcConf, name) + createOpt.ListenerID = listener.ID + if svcConf.supportLBTags { + createOpt.Tags = poolTags + } + klog.InfoS("Creating pool", "listenerID", listener.ID, "protocol", createOpt.Protocol) + klog.V(4).Infof("Pool create options: %+v", createOpt) + pool, err = openstackutil.CreatePool(ctx, lbaas.lb, createOpt, lbID) + if err != nil { + return nil, err + } + klog.V(2).Infof("Pool %s created for listener %s", pool.ID, listener.ID) + } + if lbaas.opts.ProviderRequiresSerialAPICalls { klog.V(2).Infof("Using serial API calls to update members for pool %s", pool.ID) var nodePort = int(port.NodePort) @@ -1808,19 +1808,6 @@ func (lbaas *LbaasV2) ensureOctaviaLoadBalancer(ctx context.Context, clusterName // Make sure LB ID will be saved at this point. lbaas.updateServiceAnnotation(service, ServiceAnnotationLoadBalancerID, loadbalancer.ID) - if svcConf.supportLBTags { - // Ensure the LB name tag plus any tags from the Service annotation in a single update. - lbTags := withLBNameTag(lbName, svcConf.lbTags) - klog.V(4).Infof("Desired load balancer tags: %v (LB name plus annotation %s)", lbTags, ServiceAnnotationLoadBalancerTags) - if ok, tags := mergeTags(loadbalancer.Tags, lbTags); !ok { - klog.InfoS("Updating load balancer tags", "lbID", loadbalancer.ID, "tags", tags) - if err := openstackutil.UpdateLoadBalancerTags(ctx, lbaas.lb, loadbalancer.ID, tags); err != nil { - return nil, fmt.Errorf("failed to update load balancer %s tags: %w", loadbalancer.ID, err) - } - loadbalancer.Tags = tags - } - } - if loadbalancer.ProvisioningStatus != activeStatus { return nil, fmt.Errorf("load balancer %s is not ACTIVE, current provisioning status: %s", loadbalancer.ID, loadbalancer.ProvisioningStatus) } @@ -1891,6 +1878,19 @@ func (lbaas *LbaasV2) ensureOctaviaLoadBalancer(ctx context.Context, clusterName // save address into the annotation lbaas.updateServiceAnnotation(service, ServiceAnnotationLoadBalancerAddress, addr) + // Ensure the LB name tag plus any tags from the Service annotation in a single update. + if svcConf.supportLBTags { + lbTags := withLBNameTag(lbName, svcConf.lbTags) + klog.V(4).Infof("Desired load balancer tags: %v (LB name plus annotation %s)", lbTags, ServiceAnnotationLoadBalancerTags) + if ok, tags := mergeTags(loadbalancer.Tags, lbTags); !ok { + klog.InfoS("Updating load balancer tags", "lbID", loadbalancer.ID, "tags", tags) + if err := openstackutil.UpdateLoadBalancerTags(ctx, lbaas.lb, loadbalancer.ID, tags); err != nil { + return nil, fmt.Errorf("failed to update load balancer %s tags: %w", loadbalancer.ID, err) + } + loadbalancer.Tags = tags + } + } + // Create status the load balancer status := lbaas.createLoadBalancerStatus(service, svcConf, addr) From 351921b9c7aed5a85e3ae906b79f89a34dda3044 Mon Sep 17 00:00:00 2001 From: Alexander Stephan Date: Fri, 24 Jul 2026 13:04:03 +0000 Subject: [PATCH 09/13] Outdated comments --- pkg/openstack/loadbalancer.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/openstack/loadbalancer.go b/pkg/openstack/loadbalancer.go index bd51aee941..9c6a335a0a 100644 --- a/pkg/openstack/loadbalancer.go +++ b/pkg/openstack/loadbalancer.go @@ -932,7 +932,7 @@ func (lbaas *LbaasV2) ensureOctaviaPool(ctx context.Context, lbID string, name s pool = nil } - // Resolve the desired LB method, falling back to OCCM's default. + // If LBMethod changes, update the Pool with the new value var poolLbMethod string if svcConf.poolLbMethod != "" { poolLbMethod = svcConf.poolLbMethod @@ -942,7 +942,6 @@ func (lbaas *LbaasV2) ensureOctaviaPool(ctx context.Context, lbID string, name s } poolTags := cpoutil.SplitTrim(svcConf.poolTags, ',') - // If the LBMethod or tags change, update the existing pool in a single call. if pool != nil { updateOpts := v2pools.UpdateOpts{} if pool.LBMethod != poolLbMethod { From 932048799c77f349c6c6f7c1f366aa1dc6c9a613 Mon Sep 17 00:00:00 2001 From: Alexander Stephan Date: Fri, 24 Jul 2026 14:13:04 +0000 Subject: [PATCH 10/13] Fix mergeTags to treat empty/empty as no-op --- pkg/openstack/loadbalancer.go | 4 ---- pkg/openstack/loadbalancer_test.go | 7 +++++++ 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/pkg/openstack/loadbalancer.go b/pkg/openstack/loadbalancer.go index 9c6a335a0a..8228e76374 100644 --- a/pkg/openstack/loadbalancer.go +++ b/pkg/openstack/loadbalancer.go @@ -215,10 +215,6 @@ func withLBNameTag(lbName, annotation string) []string { // mergeTags merges existedTags and newTags, returns true if all newTags are already in existedTags. func mergeTags(existedTags []string, newTags []string) (bool, []string) { - if len(existedTags) == 0 { - return false, newTags - } - tagSet := sets.NewString(existedTags...) if tagSet.HasAll(newTags...) { return true, nil diff --git a/pkg/openstack/loadbalancer_test.go b/pkg/openstack/loadbalancer_test.go index f3f82f9d91..b212647804 100644 --- a/pkg/openstack/loadbalancer_test.go +++ b/pkg/openstack/loadbalancer_test.go @@ -195,6 +195,13 @@ func TestMergeTags(t *testing.T) { expectedOK: true, expectedTags: nil, }, + { + name: "empty existing and empty desired tags is a no-op", + existedTags: []string{}, + newTags: []string{}, + expectedOK: true, + expectedTags: nil, + }, } for _, tc := range testCases { From a673385812f388285618aa5d9fac5530711a8594 Mon Sep 17 00:00:00 2001 From: Alexander Stephan Date: Mon, 27 Jul 2026 07:51:25 +0000 Subject: [PATCH 11/13] Prevent duplicate tags --- pkg/openstack/loadbalancer.go | 11 +++++++++-- pkg/openstack/loadbalancer_test.go | 20 ++++++++++++++++++++ 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/pkg/openstack/loadbalancer.go b/pkg/openstack/loadbalancer.go index 8228e76374..29d81877b8 100644 --- a/pkg/openstack/loadbalancer.go +++ b/pkg/openstack/loadbalancer.go @@ -208,9 +208,16 @@ func getLoadbalancerByName(ctx context.Context, client *gophercloud.ServiceClien } // withLBNameTag returns the LB name (OCCM's ownership tag) followed by the -// comma-separated tags from the given Service annotation value. +// comma-separated tags from the given Service annotation value, skipping any +// annotation tag that duplicates the LB name so it cannot appear twice. func withLBNameTag(lbName, annotation string) []string { - return append([]string{lbName}, cpoutil.SplitTrim(annotation, ',')...) + tags := []string{lbName} + for _, t := range cpoutil.SplitTrim(annotation, ',') { + if t != lbName { + tags = append(tags, t) + } + } + return tags } // mergeTags merges existedTags and newTags, returns true if all newTags are already in existedTags. diff --git a/pkg/openstack/loadbalancer_test.go b/pkg/openstack/loadbalancer_test.go index b212647804..cf84426106 100644 --- a/pkg/openstack/loadbalancer_test.go +++ b/pkg/openstack/loadbalancer_test.go @@ -2585,6 +2585,26 @@ func TestBuildListenerCreateOpt(t *testing.T) { Tags: []string{"my-lb", "foo", "bar"}, }, }, + { + name: "Test with listener-tags annotation duplicating the LB name", + port: corev1.ServicePort{ + Protocol: "TCP", + Port: 80, + }, + svcConf: &serviceConfig{ + connLimit: 100, + lbName: "my-lb", + supportLBTags: true, + listenerTags: "foo, my-lb, bar", + }, + expectedCreateOpt: listeners.CreateOpts{ + Name: "Test with listener-tags annotation duplicating the LB name", + Protocol: listeners.ProtocolTCP, + ProtocolPort: 80, + ConnLimit: &svcConf.connLimit, + Tags: []string{"my-lb", "foo", "bar"}, + }, + }, } for _, tc := range testCases { From 3b70b4c79de7f64bd50b8176f97a5749fbab65a5 Mon Sep 17 00:00:00 2001 From: Alexander Stephan Date: Mon, 27 Jul 2026 08:03:20 +0000 Subject: [PATCH 12/13] Also within annotations --- pkg/openstack/loadbalancer.go | 28 ++++++++++++++++++---------- pkg/openstack/loadbalancer_test.go | 20 ++++++++++++++++++++ 2 files changed, 38 insertions(+), 10 deletions(-) diff --git a/pkg/openstack/loadbalancer.go b/pkg/openstack/loadbalancer.go index 29d81877b8..4cfe49f92e 100644 --- a/pkg/openstack/loadbalancer.go +++ b/pkg/openstack/loadbalancer.go @@ -207,17 +207,25 @@ func getLoadbalancerByName(ctx context.Context, client *gophercloud.ServiceClien return &validLBs[0], nil } -// withLBNameTag returns the LB name (OCCM's ownership tag) followed by the -// comma-separated tags from the given Service annotation value, skipping any -// annotation tag that duplicates the LB name so it cannot appear twice. -func withLBNameTag(lbName, annotation string) []string { - tags := []string{lbName} - for _, t := range cpoutil.SplitTrim(annotation, ',') { - if t != lbName { - tags = append(tags, t) +// dedupTags returns tags with duplicates removed, preserving first-seen order. +func dedupTags(tags []string) []string { + seen := sets.NewString() + result := make([]string, 0, len(tags)) + for _, t := range tags { + if !seen.Has(t) { + seen.Insert(t) + result = append(result, t) } } - return tags + return result +} + +// withLBNameTag returns the LB name (OCCM's ownership tag) followed by the +// comma-separated tags from the given Service annotation value, with duplicate +// tags removed (including ones that duplicate the LB name) so no tag can appear +// twice. +func withLBNameTag(lbName, annotation string) []string { + return dedupTags(append([]string{lbName}, cpoutil.SplitTrim(annotation, ',')...)) } // mergeTags merges existedTags and newTags, returns true if all newTags are already in existedTags. @@ -943,7 +951,7 @@ func (lbaas *LbaasV2) ensureOctaviaPool(ctx context.Context, lbID string, name s // if LBMethod is not defined, fallback on default OCCM's default method poolLbMethod = lbaas.opts.LBMethod } - poolTags := cpoutil.SplitTrim(svcConf.poolTags, ',') + poolTags := dedupTags(cpoutil.SplitTrim(svcConf.poolTags, ',')) if pool != nil { updateOpts := v2pools.UpdateOpts{} diff --git a/pkg/openstack/loadbalancer_test.go b/pkg/openstack/loadbalancer_test.go index cf84426106..c456318eb0 100644 --- a/pkg/openstack/loadbalancer_test.go +++ b/pkg/openstack/loadbalancer_test.go @@ -2605,6 +2605,26 @@ func TestBuildListenerCreateOpt(t *testing.T) { Tags: []string{"my-lb", "foo", "bar"}, }, }, + { + name: "Test with duplicate tags within the listener-tags annotation", + port: corev1.ServicePort{ + Protocol: "TCP", + Port: 80, + }, + svcConf: &serviceConfig{ + connLimit: 100, + lbName: "my-lb", + supportLBTags: true, + listenerTags: "foo, foo, bar, foo", + }, + expectedCreateOpt: listeners.CreateOpts{ + Name: "Test with duplicate tags within the listener-tags annotation", + Protocol: listeners.ProtocolTCP, + ProtocolPort: 80, + ConnLimit: &svcConf.connLimit, + Tags: []string{"my-lb", "foo", "bar"}, + }, + }, } for _, tc := range testCases { From affd31a232cb27ed9273ec0f8af56d595d5d82d7 Mon Sep 17 00:00:00 2001 From: Alexander Stephan Date: Mon, 27 Jul 2026 09:17:06 +0000 Subject: [PATCH 13/13] Make sure tags are always populated --- pkg/openstack/loadbalancer.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/openstack/loadbalancer.go b/pkg/openstack/loadbalancer.go index 4cfe49f92e..50f7273b1d 100644 --- a/pkg/openstack/loadbalancer.go +++ b/pkg/openstack/loadbalancer.go @@ -1422,11 +1422,6 @@ func (lbaas *LbaasV2) checkService(ctx context.Context, service *corev1.Service, return fmt.Errorf("no service ports provided") } - annotations := service.GetAnnotations() - svcConf.lbTags = annotations[ServiceAnnotationLoadBalancerTags] - svcConf.listenerTags = annotations[ServiceAnnotationListenerTags] - svcConf.poolTags = annotations[ServiceAnnotationPoolTags] - if len(service.Spec.IPFamilies) > 0 { // Since OCCM does not support multiple load-balancers per service yet, // the first IP family will determine the IP family of the load-balancer @@ -1605,6 +1600,11 @@ func (lbaas *LbaasV2) makeSvcConf(ctx context.Context, serviceName string, servi svcConf.poolLbMethod = getStringFromServiceAnnotation(service, ServiceAnnotationLoadBalancerLbMethod, "") svcConf.supportLBTags = openstackutil.IsOctaviaFeatureSupported(ctx, lbaas.lb, openstackutil.OctaviaFeatureTags, lbaas.opts.LBProvider) + annotations := service.GetAnnotations() + svcConf.lbTags = annotations[ServiceAnnotationLoadBalancerTags] + svcConf.listenerTags = annotations[ServiceAnnotationListenerTags] + svcConf.poolTags = annotations[ServiceAnnotationPoolTags] + // Get service node-selector annotations svcConf.nodeSelectors = getKeyValueFromServiceAnnotation(service, ServiceAnnotationLoadBalancerNodeSelector, lbaas.opts.NodeSelector) for key, value := range svcConf.nodeSelectors {