Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

[occm] feat : add load balancer listener tag using service annotation #2439

Closed
wants to merge 1 commit into from
Closed

[occm] feat : add load balancer listener tag using service annotation #2439

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions ...cloud-controller-manager/expose-applications-using-loadbalancer-type-service.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,11 @@ Request Body:
This annotation is automatically added and it contains the floating ip address of the load balancer service.
When using `loadbalancer.openstack.org/hostname` annotation it is the only place to see the real address of the load balancer.

- `loadbalancer.openstack.org/custom-tags`

Allows to specify custom tags that all load balancer resources for that Service will be tagged with.
Tags are arbitrary strings, to specify multiple tags separate them using a comma `,` in the annotation.

### Switching between Floating Subnets by using preconfigured Classes

If you have multiple `FloatingIPPools` and/or `FloatingIPSubnets` it might be desirable to offer the user logical meanings for `LoadBalancers` like `internetFacing` or `DMZ` instead of requiring the user to select a dedicated network or subnet ID at the service object level as an annotation.
Expand Down
77 changes: 65 additions & 12 deletions pkg/openstack/loadbalancer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ const (
ServiceAnnotationLoadBalancerXForwardedFor = "loadbalancer.openstack.org/x-forwarded-for"
ServiceAnnotationLoadBalancerFlavorID = "loadbalancer.openstack.org/flavor-id"
ServiceAnnotationLoadBalancerAvailabilityZone = "loadbalancer.openstack.org/availability-zone"
ServiceAnnotationLoadBalancerCustomTags = "loadbalancer.openstack.org/custom-tags"
// ServiceAnnotationLoadBalancerEnableHealthMonitor defines whether to create health monitor for the load balancer
// pool, if not specified, use 'create-monitor' config. The health monitor can be created or deleted dynamically.
ServiceAnnotationLoadBalancerEnableHealthMonitor = "loadbalancer.openstack.org/enable-health-monitor"
Expand Down Expand Up @@ -469,6 +470,7 @@ func (lbaas *LbaasV2) createOctaviaLoadBalancer(name, clusterName string, servic

if svcConf.supportLBTags {
createOpts.Tags = []string{svcConf.lbName}
createOpts.Tags = append(createOpts.Tags, getCustomLoadBalancerTags(service, svcConf)...)
}

if svcConf.flavorID != "" {
Expand Down Expand Up @@ -508,8 +510,8 @@ func (lbaas *LbaasV2) createOctaviaLoadBalancer(name, clusterName string, servic

if !lbaas.opts.ProviderRequiresSerialAPICalls {
for portIndex, port := range service.Spec.Ports {
listenerCreateOpt := lbaas.buildListenerCreateOpt(port, svcConf, cpoutil.Sprintf255(listenerFormat, portIndex, name))
members, newMembers, err := lbaas.buildBatchUpdateMemberOpts(port, nodes, svcConf)
listenerCreateOpt := lbaas.buildListenerCreateOpt(port, svcConf, service, cpoutil.Sprintf255(listenerFormat, portIndex, name))
members, newMembers, err := lbaas.buildBatchUpdateMemberOpts(port, nodes, svcConf, service)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -606,6 +608,23 @@ func (lbaas *LbaasV2) getLoadBalancerLegacyName(_ context.Context, _ string, ser
return cloudprovider.DefaultLoadBalancerName(service)
}

// Returns a list of custom loadbalancer tags for the supported service resources.
func getCustomLoadBalancerTags(service *corev1.Service, svcConf *serviceConfig) []string {
if !svcConf.supportLBTags {
return nil
}

annotationVal := getStringFromServiceAnnotation(service, ServiceAnnotationLoadBalancerCustomTags, "")

tags := strings.Split(annotationVal, ",")

for i, tag := range tags {
tags[i] = strings.TrimSpace(tag)
}

return tags
}

// The LB needs to be configured with instance addresses on the same
// subnet as the LB (aka opts.SubnetID). Currently, we're just
// guessing that the node's InternalIP is the right address.
Expand Down Expand Up @@ -739,7 +758,7 @@ func isPortMember(port PortWithPortSecurity, IP string, subnetID string) bool {
}

// applyNodeSecurityGroupIDForLB associates the security group with the ports being members of the LB on the nodes.
func applyNodeSecurityGroupIDForLB(network *gophercloud.ServiceClient, svcConf *serviceConfig, nodes []*corev1.Node, sg string) error {
func applyNodeSecurityGroupIDForLB(network *gophercloud.ServiceClient, service *corev1.Service, svcConf *serviceConfig, nodes []*corev1.Node, sg string) error {
for _, node := range nodes {
serverID, _, err := instanceIDFromProviderID(node.Spec.ProviderID)
if err != nil {
Expand Down Expand Up @@ -785,6 +804,20 @@ func applyNodeSecurityGroupIDForLB(network *gophercloud.ServiceClient, svcConf *
if mc.ObserveRequest(res.Err) != nil {
return fmt.Errorf("failed to update security group for port %s: %v", port.ID, res.Err)
}

tags := getCustomLoadBalancerTags(service, svcConf)

mc = metrics.NewMetricContext("security_group_tag", "replace")
_, err := neutrontags.ReplaceAll(network, "security_groups", port.ID, neutrontags.ReplaceAllOpts{Tags: tags}).Extract()
if mc.ObserveRequest(err) != nil {
return fmt.Errorf("failed to add tag %s to port %s: %v", tags, port.ID, err)
}
Comment on lines +810 to +814
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is wrong placement, this method applies the security group to a certain port. For now we should limit the SG tagging to the place we're creating it.

Also this won't ever work, you specify port.ID which will not match a security group.


mc = metrics.NewMetricContext("floating_ip_tag", "replace")
_, err = neutrontags.ReplaceAll(network, "floatingips", port.ID, neutrontags.ReplaceAllOpts{Tags: tags}).Extract()
if mc.ObserveRequest(err) != nil {
return fmt.Errorf("failed to add tag %s to port %s of floating_ips: %v", tags, port.ID, err)
}
Comment on lines +816 to +820
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be here, this function doesn't have anything to do with floating IPs.

Also this won't ever work, you specify port.ID which will not match a floating IP.

}
}

Expand Down Expand Up @@ -896,10 +929,16 @@ func (lbaas *LbaasV2) deleteOctaviaListeners(lbID string, listenerList []listene
return nil
}

func (lbaas *LbaasV2) createFloatingIP(msg string, floatIPOpts floatingips.CreateOpts) (*floatingips.FloatingIP, error) {
func (lbaas *LbaasV2) createFloatingIP(msg string, floatIPOpts floatingips.CreateOpts, service *corev1.Service, svcConf *serviceConfig) (*floatingips.FloatingIP, error) {
klog.V(4).Infof("%s floating ip with opts %+v", msg, floatIPOpts)
mc := metrics.NewMetricContext("floating_ip", "create")
floatIP, err := floatingips.Create(lbaas.network, floatIPOpts).Extract()

tags := getCustomLoadBalancerTags(service, svcConf)

if _, err := neutrontags.ReplaceAll(lbaas.network, "floatingips", floatIP.ID, neutrontags.ReplaceAllOpts{Tags: tags}).Extract(); err != nil {
return nil, fmt.Errorf("failed to add custom tags %s to floatingIPs %s with a projectID (%s)", tags, floatIP.ID, floatIP.ProjectID)
}
Comment on lines +939 to +941
Copy link
Contributor

Choose a reason for hiding this comment

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

This is missing the metrics addition. This is where you should add mc = metrics.NewMetricContext("floating_ip_tag", "replace") and all the other stuff.

Copy link
Contributor

Choose a reason for hiding this comment

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

I also realized we should only call this code when len(tags) > 0.

err = PreserveGopherError(err)
if mc.ObserveRequest(err) != nil {
return floatIP, fmt.Errorf("error creating LB floatingip: %s", err)
Expand Down Expand Up @@ -1037,7 +1076,7 @@ func (lbaas *LbaasV2) ensureFloatingIP(clusterName string, service *corev1.Servi
svcConf.lbPublicSubnetSpec, svcConf.lbPublicNetworkID)
for _, subnet := range foundSubnets {
floatIPOpts.SubnetID = subnet.ID
floatIP, err = lbaas.createFloatingIP(fmt.Sprintf("Trying subnet %s for creating", subnet.Name), floatIPOpts)
floatIP, err = lbaas.createFloatingIP(fmt.Sprintf("Trying subnet %s for creating", subnet.Name), floatIPOpts, service, svcConf)
if err == nil {
foundSubnet = subnet
break
Expand All @@ -1054,7 +1093,7 @@ func (lbaas *LbaasV2) ensureFloatingIP(clusterName string, service *corev1.Servi
floatIPOpts.SubnetID = svcConf.lbPublicSubnetSpec.subnetID
}
floatIPOpts.FloatingIP = loadBalancerIP
floatIP, err = lbaas.createFloatingIP("Creating", floatIPOpts)
floatIP, err = lbaas.createFloatingIP("Creating", floatIPOpts, service, svcConf)
if err != nil {
return "", err
}
Expand Down Expand Up @@ -1237,7 +1276,7 @@ func (lbaas *LbaasV2) ensureOctaviaPool(lbID string, name string, listener *list
curMembers.Insert(fmt.Sprintf("%s-%s-%d-%d", m.Name, m.Address, m.ProtocolPort, m.MonitorPort))
}

members, newMembers, err := lbaas.buildBatchUpdateMemberOpts(port, nodes, svcConf)
members, newMembers, err := lbaas.buildBatchUpdateMemberOpts(port, nodes, svcConf, service)
if err != nil {
return nil, err
}
Expand All @@ -1254,6 +1293,8 @@ func (lbaas *LbaasV2) ensureOctaviaPool(lbID string, name string, listener *list
}

func (lbaas *LbaasV2) buildPoolCreateOpt(listenerProtocol string, service *corev1.Service, svcConf *serviceConfig, name string) v2pools.CreateOpts {
customTags := getCustomLoadBalancerTags(service, svcConf)

// By default, use the protocol of the listener
poolProto := v2pools.Protocol(listenerProtocol)
if svcConf.enableProxyProtocol {
Expand Down Expand Up @@ -1284,14 +1325,17 @@ func (lbaas *LbaasV2) buildPoolCreateOpt(listenerProtocol string, service *corev
Protocol: poolProto,
LBMethod: lbmethod,
Persistence: persistence,
Tags: customTags,
}
}

// buildBatchUpdateMemberOpts returns v2pools.BatchUpdateMemberOpts array for Services and Nodes alongside a list of member names
func (lbaas *LbaasV2) buildBatchUpdateMemberOpts(port corev1.ServicePort, nodes []*corev1.Node, svcConf *serviceConfig) ([]v2pools.BatchUpdateMemberOpts, sets.Set[string], error) {
func (lbaas *LbaasV2) buildBatchUpdateMemberOpts(port corev1.ServicePort, nodes []*corev1.Node, svcConf *serviceConfig, service *corev1.Service) ([]v2pools.BatchUpdateMemberOpts, sets.Set[string], error) {
var members []v2pools.BatchUpdateMemberOpts
newMembers := sets.New[string]()

customTags := getCustomLoadBalancerTags(service, svcConf)

for _, node := range nodes {
addr, err := nodeAddressForLB(node, svcConf.preferredIPFamily)
if err != nil {
Expand All @@ -1315,6 +1359,7 @@ func (lbaas *LbaasV2) buildBatchUpdateMemberOpts(port corev1.ServicePort, nodes
ProtocolPort: int(port.NodePort),
Name: &node.Name,
SubnetID: memberSubnetID,
Tags: customTags,
}
if svcConf.healthCheckNodePort > 0 && lbaas.canUseHTTPMonitor(port) {
member.MonitorPort = &svcConf.healthCheckNodePort
Expand All @@ -1327,13 +1372,13 @@ func (lbaas *LbaasV2) buildBatchUpdateMemberOpts(port corev1.ServicePort, nodes
}

// Make sure the listener is created for Service
func (lbaas *LbaasV2) ensureOctaviaListener(lbID string, name string, curListenerMapping map[listenerKey]*listeners.Listener, port corev1.ServicePort, svcConf *serviceConfig, _ *corev1.Service) (*listeners.Listener, error) {
func (lbaas *LbaasV2) ensureOctaviaListener(lbID string, name string, curListenerMapping map[listenerKey]*listeners.Listener, port corev1.ServicePort, svcConf *serviceConfig, service *corev1.Service) (*listeners.Listener, error) {
listener, isPresent := curListenerMapping[listenerKey{
Protocol: getListenerProtocol(port.Protocol, svcConf),
Port: int(port.Port),
}]
if !isPresent {
listenerCreateOpt := lbaas.buildListenerCreateOpt(port, svcConf, name)
listenerCreateOpt := lbaas.buildListenerCreateOpt(port, svcConf, service, name)
listenerCreateOpt.LoadbalancerID = lbID

klog.V(2).Infof("Creating listener for port %d using protocol %s", int(port.Port), listenerCreateOpt.Protocol)
Expand Down Expand Up @@ -1419,7 +1464,7 @@ func (lbaas *LbaasV2) ensureOctaviaListener(lbID string, name string, curListene
}

// buildListenerCreateOpt returns listeners.CreateOpts for a specific Service port and configuration
func (lbaas *LbaasV2) buildListenerCreateOpt(port corev1.ServicePort, svcConf *serviceConfig, name string) listeners.CreateOpts {
func (lbaas *LbaasV2) buildListenerCreateOpt(port corev1.ServicePort, svcConf *serviceConfig, service *corev1.Service, name string) listeners.CreateOpts {
listenerCreateOpt := listeners.CreateOpts{
Name: name,
Protocol: listeners.Protocol(port.Protocol),
Expand All @@ -1429,6 +1474,8 @@ func (lbaas *LbaasV2) buildListenerCreateOpt(port corev1.ServicePort, svcConf *s

if svcConf.supportLBTags {
listenerCreateOpt.Tags = []string{svcConf.lbName}
// add custom tags to LB listener
listenerCreateOpt.Tags = append(listenerCreateOpt.Tags, getCustomLoadBalancerTags(service, svcConf)...)
}

if openstackutil.IsOctaviaFeatureSupported(lbaas.lb, openstackutil.OctaviaFeatureTimeout, lbaas.opts.LBProvider) {
Expand Down Expand Up @@ -2317,6 +2364,12 @@ func (lbaas *LbaasV2) ensureAndUpdateOctaviaSecurityGroup(clusterName string, ap
return fmt.Errorf("failed to create Security Group for loadbalancer service %s/%s: %v", apiService.Namespace, apiService.Name, err)
}
lbSecGroupID = lbSecGroup.ID

tags := getCustomLoadBalancerTags(apiService, svcConf)

if _, err := neutrontags.ReplaceAll(lbaas.network, "security-groups", lbSecGroupID, neutrontags.ReplaceAllOpts{Tags: tags}).Extract(); err != nil {
return fmt.Errorf("failed to add custom tags %s to security group %s (%s)", tags, lbSecGroupID, lbSecGroupName)
}
Comment on lines +2370 to +2372
Copy link
Contributor

Choose a reason for hiding this comment

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

This is missing the mc = metrics.NewMetricContext("security_group_tag", "replace") and then the subsequent ObserveRequest().

Copy link
Contributor

Choose a reason for hiding this comment

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

Same here should only do tagging when len(tags) > 0.

}

mc := metrics.NewMetricContext("subnet", "get")
Expand Down Expand Up @@ -2409,7 +2462,7 @@ func (lbaas *LbaasV2) ensureAndUpdateOctaviaSecurityGroup(clusterName string, ap
}
}

if err := applyNodeSecurityGroupIDForLB(lbaas.network, svcConf, nodes, lbSecGroupID); err != nil {
if err := applyNodeSecurityGroupIDForLB(lbaas.network, apiService, svcConf, nodes, lbSecGroupID); err != nil {
return err
}
return nil
Expand Down
111 changes: 109 additions & 2 deletions pkg/openstack/loadbalancer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -708,6 +708,7 @@ func TestLbaasV2_checkListenerPorts(t *testing.T) {
})
}
}

func TestLbaasV2_createLoadBalancerStatus(t *testing.T) {
type fields struct {
LoadBalancer LoadBalancer
Expand Down Expand Up @@ -1831,6 +1832,7 @@ func TestBuildBatchUpdateMemberOpts(t *testing.T) {
nodes []*corev1.Node
port corev1.ServicePort
svcConf *serviceConfig
service *corev1.Service
expectedLen int
expectedNewMembersCount int
}{
Expand Down Expand Up @@ -1909,7 +1911,7 @@ func TestBuildBatchUpdateMemberOpts(t *testing.T) {
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
lbaas := &LbaasV2{}
members, newMembers, err := lbaas.buildBatchUpdateMemberOpts(tc.port, tc.nodes, tc.svcConf)
members, newMembers, err := lbaas.buildBatchUpdateMemberOpts(tc.port, tc.nodes, tc.svcConf, tc.service)
assert.Len(t, members, tc.expectedLen)
assert.NoError(t, err)

Expand Down Expand Up @@ -2298,6 +2300,7 @@ func TestBuildListenerCreateOpt(t *testing.T) {
name string
port corev1.ServicePort
svcConf *serviceConfig
service *corev1.Service
expectedCreateOpt listeners.CreateOpts
}{
{
Expand Down Expand Up @@ -2400,9 +2403,113 @@ func TestBuildListenerCreateOpt(t *testing.T) {
},
},
}
createOpt := lbaas.buildListenerCreateOpt(tc.port, tc.svcConf, tc.name)
createOpt := lbaas.buildListenerCreateOpt(tc.port, tc.svcConf, tc.service, tc.name)
assert.Equal(t, tc.expectedCreateOpt, createOpt)
})
}
}

func TestLbaasV2_customLoadBalancerListenerTag(t *testing.T) {
type testArgs struct {
service *corev1.Service
svcConf *serviceConfig
}
tests := []struct {
name string
testArgs testArgs
want []string
}{
{
name: "Single Custom Tag in Annotation With Disabled 'svcconfig.supportLBTags'",
testArgs: testArgs{
service: &corev1.Service{
ObjectMeta: v1.ObjectMeta{
Annotations: map[string]string{ServiceAnnotationLoadBalancerCustomTags: "single-custom-tag"},
},
},
svcConf: &serviceConfig{
supportLBTags: false,
},
},
want: nil,
},
{
name: "Empty Custom Tag in Annotation With Disabled 'svcconfig.supportLBTags'",
testArgs: testArgs{
service: &corev1.Service{
ObjectMeta: v1.ObjectMeta{
Annotations: map[string]string{ServiceAnnotationLoadBalancerCustomTags: ""},
},
},
svcConf: &serviceConfig{
supportLBTags: false,
},
},
want: nil,
},
{
name: "Multiple Custom Tag in Annotation With Disabled 'svcconfig.supportLBTags'",
testArgs: testArgs{
service: &corev1.Service{
ObjectMeta: v1.ObjectMeta{
Annotations: map[string]string{ServiceAnnotationLoadBalancerCustomTags: "tag1, tag2, tag3, tag4, multiple-custom-tag"},
},
},
svcConf: &serviceConfig{
supportLBTags: false,
},
},
want: nil,
},
{
name: "Empty Custom Tag in Annotation With Enabled 'svcconfig.supportLBTags'",
testArgs: testArgs{
service: &corev1.Service{
ObjectMeta: v1.ObjectMeta{
Annotations: map[string]string{ServiceAnnotationLoadBalancerCustomTags: ""},
},
},
svcConf: &serviceConfig{
supportLBTags: true,
},
},
want: []string{""},
},
{
name: "Valid Single Custom Tag in Annotation With Enabled 'svcconfig.supportLBTags'",
testArgs: testArgs{
service: &corev1.Service{
ObjectMeta: v1.ObjectMeta{
Annotations: map[string]string{ServiceAnnotationLoadBalancerCustomTags: "single-custom-tag"},
},
},
svcConf: &serviceConfig{
supportLBTags: true,
},
},
want: []string{"single-custom-tag"},
},
{
name: "Multiple Custom Tag in Annotation With Enabled 'svcconfig.supportLBTags'",
testArgs: testArgs{
service: &corev1.Service{
ObjectMeta: v1.ObjectMeta{
Annotations: map[string]string{ServiceAnnotationLoadBalancerCustomTags: "tag1, tag2, tag3, tag4, multiple-custom-tag"},
},
},
svcConf: &serviceConfig{
supportLBTags: true,
},
},
want: []string{"tag1", "tag2", "tag3", "tag4", "multiple-custom-tag"},
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got := getCustomLoadBalancerTags(tt.testArgs.service, tt.testArgs.svcConf)

assert.ElementsMatch(t, tt.want, got)
})
}
}