[occm] empty SubnetID for members transmitted and fails in octavia API

[danfai]

Is this a BUG REPORT or FEATURE REQUEST?:

Uncomment only one, leave it on its own line:

/kind bug /kind feature

What happened: When creating fully populated loadbalancers without any subnet, the members have an empty subnet ID ("subnet_id": "") in the API request. The empty string is an invalid UUID and thus the validation in the octavia API fails with: [...]error message: {\"faultcode\": \"Client\", \"faultstring\": \"Invalid input for field/attribute subnet_id. Value: ''. Value should be UUID format\" [...]

What you expected to happen: The subnet_id field for members should not be sent at all and octavia will select the correct subnet(s) corresponding to the subnet.

How to reproduce it: Configure a subnet-id as empty string or none at all for the service and try to create a fully populated LB.

Anything else we need to know?: The problem exist since gophercloud moved the BatchMemberUpdateOpts to allow the removal of options in an update request. However, doing so the data structure for creating fully populated loadbalancer with BatchMemberUpdateOpts is not correctly parsed with the omitempty option, and will transfer use an empty string instead of removing the field from the API request. The relevant gophercloud issue and PR: https://github.com/gophercloud/gophercloud/issues/2559 https://github.com/gophercloud/gophercloud/pull/2560

Potential fix would be to use the SubnetID field in buildBatchUpdateMemberOpts only if there is a subnet to be used. In case the subnet is an empty string, do not set the value (or set it to nil, since the evaluation of omitempty works for nil for the pointer in BatchUpdateMemberOpts).

Environment:

• openstack-cloud-controller-manager(or other related binary) version: 1.23
• OpenStack version: 11.0.0/zed
• Others:

[jichenjc]

I Think you are proposing following logic? @danfai

if svcConf.lbMemberSubnetID != ""
{
member := v2pools.BatchUpdateMemberOpts{
                                Address:      addr,
                                ProtocolPort: int(port.NodePort),
                                Name:         &node.Name,
                                SubnetID:     &svcConf.lbMemberSubnetID,
                        }
}
else
{
member := v2pools.BatchUpdateMemberOpts{
                                Address:      addr,
                                ProtocolPort: int(port.NodePort),
                                Name:         &node.Name,
                        }
}

[danfai]

Yes I was thinking about this logic. Other way to solve it, would be to specify nil, e.g. like this.

@@ -1329,11 +1329,16 @@ func (lbaas *LbaasV2) buildBatchUpdateMemberOpts(port corev1.ServicePort, nodes
                        }
                }

+               sn := &svcConf.lbMemberSubnetID
+               if sn != nil && len(*sn) == 0 {
+                       sn = nil
+               }
+
                member := v2pools.BatchUpdateMemberOpts{
                        Address:      addr,
                        ProtocolPort: int(port.NodePort),
                        Name:         &node.Name,
-                       SubnetID:     &svcConf.lbMemberSubnetID,
+                       SubnetID:     sn,
                }
                if svcConf.healthCheckNodePort > 0 {
                        member.MonitorPort = &svcConf.healthCheckNodePort

[jichenjc]

also fine, do you want to submit a PR for it ?

[danfai]

I can make a PR, sure.