Regression: Specifying subnet filter without network fails when it previously did not

[mdbooth]

/kind bug

What steps did you take and what happened:
In OpenShift we add default networks to machines by subnet tag, e.g.:

      networks:
      - filter: {}
        subnets:
        - filter:
            name: cluster-dsal-59p8x-nodes
            tags: openshiftClusterID=cluster-dsal-59p8x

This now returns an error. It previously succeeded.

What did you expect to happen:

We should find the requested subnet in any network, which was the previous behaviour.

Anything else you would like to add:

This was regressed by #935 in an interesting way. It turns out that the previous behaviour in this case was to list networks with an empty filter, which returns all networks, then execute a separate subnet query per-network and add all matches subnets. This worked, but was obviously also incredibly inefficient.

#935 was principally about adding a mock interface, but incidentally also modified GetSubnetsByFilter and GetNetworksByFilter to return an error if there were no results. This breaks us, because we're querying every network individually, but only one of them contains the subnet we're looking for. The first network we query which doesn't contain the target subnet will return an error.

I'm not 100% convinced this is a desirable change, but it did highlight a very inefficient use of the OpenStack REST API. We can address this just by making a more efficient API call.

Environment:

• Cluster API Provider OpenStack version (Or git rev-parse HEAD if manually built): ac9c503

[macaptain]

Here's the old GetNetworksByFilter on the parent of #935's merge commit on master:

cluster-api-provider-openstack/pkg/cloud/services/networking/network.go

Lines 342 to 364 in 649fcf5

	// GetSubnetsByFilter gets the id of a subnet by querying openstack with filters.
	func (s *Service) GetSubnetsByFilter(opts subnets.ListOptsBuilder) ([]subnets.Subnet, error) {
	if opts == nil {
	return []subnets.Subnet{}, fmt.Errorf("no Filters were passed")
	}
	mc := metrics.NewMetricPrometheusContext("subnet", "list")
	pager := subnets.List(s.client, opts)
	var snets []subnets.Subnet
	err := pager.EachPage(func(page pagination.Page) (bool, error) {
	subnetList, err := subnets.ExtractSubnets(page)
	if mc.ObserveRequest(err) != nil {
	return false, err
	} else if len(subnetList) == 0 {
	return false, fmt.Errorf("no subnets could be found with the filters provided")
	}
	snets = append(snets, subnetList...)
	return true, nil
	})
	if err != nil {
	return []subnets.Subnet{}, err
	}
	return snets, nil
	}

On line 354, if subnetList extracted from the current page has length zero, we return false, fmt.Errorf(...), which stops iteration of pager.EachPage. Then on line 361 the function returns that error. So I think we also used to return an error here when listing subnets with no results before #935, but I'm squinting at the code here, haven't tested it and could be mistaken.

In any case, I don't think it matters how, when or why this stopped working. Sounds like we should support this use case to find a subnet without specifying its network, and even better when there's both API usage efficiency gains and the unit tests to guard against this getting missed in future.

[mdbooth]

Ouch! That code's a headscratcher.

You may well be right. It definitely did regress, but I may have git blamed the wrong person 🙄