[release-1.36] feat: add support for load balancer, listener, and pool tags annotations (#3154)

* feat: add support for load balancer, listener, and pool tags annotations

* feat: refactor load balancer tag handling to use SplitTrim for better parsing

* feat: streamline load balancer listener tag management for improved consistency

* 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

* Address feedback

* Add withLBNameTag helper

* Rename desiredTags completely

* Change structure

* Outdated comments

* Fix mergeTags to treat empty/empty as no-op

* Prevent duplicate tags

* Also within annotations

* Make sure tags are always populated

* Sanitize user tags

* Reserved service prefix

* Introduce generic helpers

---------

Co-authored-by: A10ss <la10ss@163.com>
Co-authored-by: Alexander Stephan <alexander.stephan@sap.com>

Author: 3 people

docs/openstack-cloud-controller-manager/expose-applications-using-loadbalancer-type-service.md

@@ -239,6 +239,20 @@ 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`.
+
+  > NOTE: Tags starting with `kube_service_` are reserved for OCCM's own ownership and shared-load-balancer tracking. Any such tag supplied through this annotation is ignored (a warning is logged) to avoid conflicting with the tags OCCM manages. This applies to all three tag annotations.
+
+- `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`. Tags starting with the reserved `kube_service_` prefix are ignored (see the note above).
+
+- `loadbalancer.openstack.org/pool-tags`
+
+  A comma-separated list of tags to add to the load balancer pool resources. Example: `env=prod,team=network`. Tags starting with the reserved `kube_service_` prefix are ignored (see the note above).
+
 - `loadbalancer.openstack.org/hostname`
 
   This annotations explicitly sets a hostname in the status of the load balancer service.

pkg/openstack/loadbalancer.go

@@ -96,6 +96,13 @@ const (
 	defaultProxyHostnameSuffix      = "nip.io"
 	ServiceAnnotationLoadBalancerID = "loadbalancer.openstack.org/load-balancer-id"
 
+	// ServiceAnnotationLoadBalancerTags is used to set additional tags on the loadbalancer resource itself (comma-separated list).
+	ServiceAnnotationLoadBalancerTags = "loadbalancer.openstack.org/load-balancer-tags"
+	// ServiceAnnotationListenerTags is used to set additional tags on the loadbalancer listener resources (comma-separated list).
+	ServiceAnnotationListenerTags = "loadbalancer.openstack.org/listener-tags"
+	// 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
 	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,43 @@ func getLoadbalancerByName(ctx context.Context, client *gophercloud.ServiceClien
 	return &validLBs[0], nil
 }
 
+// stripReservedTags drops any user-supplied tag that begins with servicePrefix.
+//
+// Tags with this prefix are how OCCM marks resource ownership: the ownership tag
+// written to a resource is always a GetLoadBalancerName value, which always
+// starts with servicePrefix. Every ownership check in this file matches tags
+// exclusively on that form -- either an exact match against the prefixed lbName
+// (e.g. slices.Contains(tags, lbName)) or a strings.HasPrefix(tag, servicePrefix)
+// scan (used for shared-LB counting and deletion decisions).
+//
+// Because of that, keeping user tags out of the servicePrefix subspace fully
+// partitions the tag namespace: ownership tags live in the kube_service_* space,
+// user tags live in its complement, and no user tag can ever satisfy an ownership
+// check. This prevents a Service from injecting a tag matching another (possibly
+// cross-tenant) load balancer's name to hijack ownership, shared-LB limits, or
+// deletion. INVARIANT: any new tag-based ownership check must also key on
+// servicePrefix, or this guarantee no longer holds.
+func stripReservedTags(tags []string) []string {
+	result := make([]string, 0, len(tags))
+	for _, t := range tags {
+		if strings.HasPrefix(t, servicePrefix) {
+			klog.Warningf("Ignoring reserved tag %q: tags starting with %q are reserved for OCCM ownership tracking", t, servicePrefix)
+			continue
+		}
+		result = append(result, t)
+	}
+	return result
+}
+
+// withLBNameTag returns the LB name (OCCM's ownership tag) followed by the
+// comma-separated tags from the given Service annotation value. Any user tag
+// using the reserved servicePrefix is stripped, and duplicate tags are removed
+// (including ones that duplicate the LB name) so no tag can appear twice.
+func withLBNameTag(lbName, annotation string) []string {
+	userTags := stripReservedTags(cpoutil.SplitTrim(annotation, ','))
+	return cpoutil.Unique(append([]string{lbName}, userTags...))
+}
+
 func popListener(existingListeners []listeners.Listener, id string) []listeners.Listener {
 	newListeners := []listeners.Listener{}
 	for _, existingListener := range existingListeners {
@@ -235,7 +282,7 @@ func (lbaas *LbaasV2) createOctaviaLoadBalancer(ctx context.Context, name, clust
 	}
 
 	if svcConf.supportLBTags {
-		createOpts.Tags = []string{svcConf.lbName}
+		createOpts.Tags = withLBNameTag(svcConf.lbName, svcConf.lbTags)
 	}
 
 	if svcConf.flavorID != "" {
@@ -910,24 +957,39 @@ 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
+	poolTags := cpoutil.Unique(stripReservedTags(cpoutil.SplitTrim(svcConf.poolTags, ',')))
+
+	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 tags, changed := cpoutil.Merge(pool.Tags, poolTags); changed {
+				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)
+			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)
+				lbaas.eventRecorder.Event(service, corev1.EventTypeWarning, eventLBLbMethodUnknown, msg)
+			}
 		}
 	}
 
 	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
@@ -1099,11 +1161,11 @@ 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
+			// Ensure the LB name tag plus any desired tags from the Service annotation.
+			listenerTags := withLBNameTag(svcConf.lbName, svcConf.listenerTags)
+			if tags, changed := cpoutil.Merge(listener.Tags, listenerTags); changed {
+				klog.V(4).Infof("Will update listener tags, current listener tags: %+v, desired tags: %+v", listener.Tags, tags)
+				updateOpts.Tags = &tags
 				listenerChanged = true
 			}
 		}
@@ -1177,7 +1239,8 @@ func (lbaas *LbaasV2) buildListenerCreateOpt(ctx context.Context, port corev1.Se
 	}
 
 	if svcConf.supportLBTags {
-		listenerCreateOpt.Tags = []string{svcConf.lbName}
+		// The LB name is always tagged, plus any desired tags from the Service annotation.
+		listenerCreateOpt.Tags = withLBNameTag(svcConf.lbName, svcConf.listenerTags)
 	}
 
 	if openstackutil.IsOctaviaFeatureSupported(ctx, lbaas.lb, openstackutil.OctaviaFeatureTimeout, lbaas.opts.LBProvider) {
@@ -1543,6 +1606,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 {
@@ -1826,15 +1894,16 @@ 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.
+	// Ensure the LB name tag plus any tags from the Service annotation in a single update.
 	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
+		lbTags := withLBNameTag(lbName, svcConf.lbTags)
+		klog.V(4).Infof("Desired load balancer tags: %v (LB name plus annotation %s)", lbTags, ServiceAnnotationLoadBalancerTags)
+		if tags, changed := cpoutil.Merge(loadbalancer.Tags, lbTags); changed {
+			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
 		}
 	}
 

pkg/openstack/loadbalancer_test.go

@@ -152,6 +152,72 @@ type testGetRulesToCreateAndDelete struct {
 	toDelete      []rules.SecGroupRule
 }
 
+func TestWithLBNameTag(t *testing.T) {
+	lbName := servicePrefix + "cluster_ns_svc"
+	testCases := []struct {
+		name       string
+		annotation string
+		expected   []string
+	}{
+		{
+			name:       "empty annotation returns only the LB name",
+			annotation: "",
+			expected:   []string{lbName},
+		},
+		{
+			name:       "user tags are appended after the LB name",
+			annotation: "team=foo,env=prod",
+			expected:   []string{lbName, "team=foo", "env=prod"},
+		},
+		{
+			name:       "user tag using the reserved prefix is stripped",
+			annotation: servicePrefix + "cluster_ns_other,team=foo",
+			expected:   []string{lbName, "team=foo"},
+		},
+		{
+			name:       "user tag equal to this LB name is stripped, not duplicated",
+			annotation: lbName + ",team=foo",
+			expected:   []string{lbName, "team=foo"},
+		},
+		{
+			name:       "duplicate user tags are removed",
+			annotation: "team=foo,team=foo",
+			expected:   []string{lbName, "team=foo"},
+		},
+		{
+			name:       "whitespace-padded reserved tag is trimmed then stripped",
+			annotation: "  " + servicePrefix + "cluster_ns_other  ,team=foo",
+			expected:   []string{lbName, "team=foo"},
+		},
+		{
+			name:       "bare reserved prefix is stripped",
+			annotation: servicePrefix + ",team=foo",
+			expected:   []string{lbName, "team=foo"},
+		},
+		{
+			name:       "multiple reserved tags are all stripped",
+			annotation: servicePrefix + "a," + servicePrefix + "b,team=foo",
+			expected:   []string{lbName, "team=foo"},
+		},
+		{
+			name:       "prefix appearing mid-string is not stripped",
+			annotation: "owner=" + servicePrefix + "x,team=foo",
+			expected:   []string{lbName, "owner=" + servicePrefix + "x", "team=foo"},
+		},
+		{
+			name:       "only reserved tags leaves just the LB name",
+			annotation: servicePrefix + "a," + servicePrefix + "b",
+			expected:   []string{lbName},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			assert.Equal(t, tc.expected, withLBNameTag(lbName, tc.annotation))
+		})
+	}
+}
+
 func TestGetRulesToCreateAndDelete(t *testing.T) {
 	tests := []testGetRulesToCreateAndDelete{
 		{
@@ -2485,6 +2551,85 @@ 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{"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"},
+			},
+		},
+		{
+			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 {