OpenStack LBaaS fix: must use ID, not name, of the node security group

[multi-io]

This is a bugfix for the OpenStack LBaaS cloud provider security group management.

A bit of context: When creating a load balancer for a given type: LoadBalancer service, the provider will try to:

(see pkg/cloudprovider/providers/openstack/openstack_loadbalancer.go/EnsureLoadBalancer)

1. create a load balancer (LB) in Openstack with listeners corresponding to the service's ports
2. attach a floating IP to the LB's network port

If manage-security-groups is enabled in controller-manager's cloud.conf:

3. create a security group with ingress rules corresponding to the LB's listeners, and attach it to the LB's network port
4. for all nodes of the cluster, pick an existing security group for the nodes ("node security group") and add ingress rules to it exposing the service's NodePorts to the security group created in step 3.

In the current upstream master, steps 1 through 3 work fine, step 4 fails, leading to a service that's not accessible via the LB without further manual intervention.

The bug is in the "pick an existing security group" operation (func getNodeSecurityGroupIDForLB), which, contrary to its name, will return the security group's name rather than its ID (actually it returns a list of names rather than IDs, apparently to cover some corner cases where you might have more than one node security group, but anyway). This will then be used when trying to add the ingress rules to the group, which the Openstack API will reject with a 404 (at least on our (fairly standard) Openstack Ocata installation) because we're giving it a name where it expects an ID.

The PR adds a "get ID given a name" lookup to the getNodeSecurityGroupIDForLB function, so it actually returns IDs. That's it. I'm not sure if the upstream code wasn't really tested, or maybe other people use other Openstacks with more lenient APIs. The bug and the fix is always reproducible on our installation.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):

Fixes #58145

Special notes for your reviewer:

Should we turn getNodeSecurityGroupIDForLB into a method with the lbaas as its receiver because it now requires two of the lbaas's attributes? I'm not sure what the conventions are here, if any.

Release note:

Properly manage security groups for loadbalancer services on OpenStack.

[k8s-ci-robot]

Hi @multi-io. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[anguslees]

groups.IDFromName does an API lookup. Given that almost all uses of this loop are going to be performing many lookups for a very small (almost always 1) number of names, it would be nice to do this lookup after de-duplication...

iow: something like:

Assuming s/nodeSecurityGroupIDs/sgNames/g

for _, node := range nodes {
   ...
   sgNames.Insert(securityGroupName.(string))
}

ids := make([]string, sgNames.Len())
for i, name := range sgNames.List() {
    var err error
    ids[i], err = groups.IDFromName(network, name)
    if err != nil {
        return []string{}, err
    }
}

return ids, nil

[multi-io]

Yep, thanks. Fix applied. (force push -- is that OK?)

What do you think about turning the function into a method?

[anguslees]

/lgtm

(and yes, force pushing is fine - it's the github way. For long/controversial PRs it's sometimes nice to build up incremental commits to make the review easier, and then squash the whole lot after you get a "verbal" lgtm.)

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: anguslees, multi-io

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• pkg/cloudprovider/providers/openstack/OWNERS [anguslees]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[anguslees]

/cc @hogepodge - this looks like an important gap in our testing (Service type=LoadBalancer + lbaas without explicit node security group config), if you're looking for a motivational example...

[k8s-github-robot]

Automatic merge from submit-queue (batch tested with PRs 65449, 65373, 49410). If you want to cherry-pick this change to another branch, please follow the instructions here.

[anguslees]

If the previous code was broken, then it was broken in 1.9-1.11, and we should backport this to (at least) 1.11 imo. I hesitate to suggest this code has been non-functional for that long however, so more testing reports from people with/without this patch would be useful to help inform that backport discussion.

(tagging @dims to track backporting - if required)

[bashofmann]

We can verify this issue with 1.9.7 as well as 1.10.3.

[dims]

Thanks @bashofmann @anguslees. i've filed reverts.