Skip to content

Commit

Permalink
Allow subnet without network
Browse files Browse the repository at this point in the history 
When specifying a subnet filter without a network filter we would first
execute a network list with no filter, which returns all networks, then
execute the subnet filter for each network. With this change we:

* Don't filter subnets by network if there is no network filter
* Don't execute a network query if we have a network uuid
* Don't execute a subnet query if we have a subnet uuid

The changes to unit tests are:
* Don't expect gophercloud calls which we no longer make
* "Subnet without network" now returns subnets instead of error
  • Loading branch information
@mdbooth
mdbooth committed Sep 13, 2021
1 parent dac689a commit 85fe929
Show file tree
Hide file tree
Showing 2 changed files with 57 additions and 104 deletions.
76 changes: 51 additions & 25 deletions pkg/cloud/services/compute/instance.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -382,38 +382,64 @@ func (s *Service) getSecurityGroups(securityGroupParams []infrav1.SecurityGroupP

func (s *Service) getServerNetworks(networkParams []infrav1.NetworkParam) ([]infrav1.Network, error) {
var nets []infrav1.Network

addSubnet := func(netID, subnetID string) {
nets = append(nets, infrav1.Network{
ID: netID,
Subnet: &infrav1.Subnet{
ID: subnetID,
},
})
}

addSubnets := func(networkParam infrav1.NetworkParam, netID string) error {
if len(networkParam.Subnets) == 0 && netID != "" {
addSubnet(netID, "")
return nil
}

for _, subnet := range networkParam.Subnets {
// Don't lookup subnet if it was specified explicitly by UUID
if subnet.UUID != "" {
addSubnet(netID, subnet.UUID)
continue
}
subnetOpts := subnets.ListOpts(subnet.Filter)
if netID != "" {
subnetOpts.NetworkID = netID
}
subnetsByFilter, err := s.networkingService.GetSubnetsByFilter(&subnetOpts)
if err != nil {
return err
}
for _, subnetByFilter := range subnetsByFilter {
addSubnet(subnetByFilter.NetworkID, subnetByFilter.ID)
}
}

return nil
}

for _, networkParam := range networkParams {
// Don't lookup network if we specified one explicitly by UUID
// Don't lookup network if we didn't specify a network filter
// If there is no explicit network UUID and no network filter,
// we will look for subnets matching any given subnet filters in
// all networks.
if networkParam.UUID != "" || networkParam.Filter == (infrav1.Filter{}) {
if err := addSubnets(networkParam, networkParam.UUID); err != nil {
return nil, err
}
continue
}
opts := networks.ListOpts(networkParam.Filter)
opts.ID = networkParam.UUID
ids, err := s.networkingService.GetNetworkIDsByFilter(&opts)
if err != nil {
return nil, err
}
for _, netID := range ids {
if networkParam.Subnets == nil {
nets = append(nets, infrav1.Network{
ID: netID,
Subnet: &infrav1.Subnet{},
})
continue
}

for _, subnet := range networkParam.Subnets {
subnetOpts := subnets.ListOpts(subnet.Filter)
subnetOpts.ID = subnet.UUID
subnetOpts.NetworkID = netID
subnetsByFilter, err := s.networkingService.GetSubnetsByFilter(&subnetOpts)
if err != nil {
return nil, err
}
for _, subnetByFilter := range subnetsByFilter {
nets = append(nets, infrav1.Network{
ID: subnetByFilter.NetworkID,
Subnet: &infrav1.Subnet{
ID: subnetByFilter.ID,
},
})
}
if err := addSubnets(networkParam, netID); err != nil {
return nil, err
}
}
}
Expand Down
85 changes: 6 additions & 79 deletions pkg/cloud/services/compute/instance_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,11 +108,6 @@ func TestService_getServerNetworks(t *testing.T) {
Subnets: []string{subnetB1UUID},
Tags: []string{testClusterTag},
}
testNetworkC := networks.Network{
ID: networkCUUID,
Name: "network-c",
Subnets: []string{subnetC1UUID},
}

testSubnetA1 := subnets.Subnet{
ID: subnetA1UUID,
Expand All @@ -126,22 +121,12 @@ func TestService_getServerNetworks(t *testing.T) {
NetworkID: networkAUUID,
Tags: []string{testClusterTag},
}
testSubnetA3 := subnets.Subnet{
ID: subnetA3UUID,
Name: "subnet-a3",
NetworkID: networkAUUID,
}
testSubnetB1 := subnets.Subnet{
ID: subnetB1UUID,
Name: "subnet-b1",
NetworkID: networkBUUID,
Tags: []string{testClusterTag},
}
testSubnetC1 := subnets.Subnet{
ID: subnetC1UUID,
Name: "subnet-c1",
NetworkID: networkCUUID,
}

// Define arbitrary test network and subnet filters for use in multiple tests,
// the gophercloud ListOpts they should translate to, and the arbitrary returned networks/subnets.
Expand All @@ -150,18 +135,6 @@ func TestService_getServerNetworks(t *testing.T) {
testSubnetFilter := infrav1.SubnetFilter{Tags: testClusterTag}
testSubnetListOpts := subnets.ListOpts{Tags: testClusterTag}

// Expect a list query by network UUID which returns the network with the same UUID
expectNetworkListByUUID := func(m *mock_networking.MockNetworkClientMockRecorder, network *networks.Network) {
m.ListNetwork(&networks.ListOpts{ID: network.ID}).
Return([]networks.Network{*network}, nil)
}

// Expect a list query by subnet and network UUID which returns the subnet with the same UUID
expectSubnetListByUUID := func(m *mock_networking.MockNetworkClientMockRecorder, subnet *subnets.Subnet) {
m.ListSubnet(&subnets.ListOpts{ID: subnet.ID, NetworkID: subnet.NetworkID}).
Return([]subnets.Subnet{*subnet}, nil)
}

tests := []struct {
name string
networkParams []infrav1.NetworkParam
Expand All @@ -178,7 +151,6 @@ func TestService_getServerNetworks(t *testing.T) {
{ID: networkAUUID, Subnet: &infrav1.Subnet{}},
},
expect: func(m *mock_networking.MockNetworkClientMockRecorder) {
expectNetworkListByUUID(m, &testNetworkA)
},
wantErr: false,
},
Expand All @@ -205,39 +177,17 @@ func TestService_getServerNetworks(t *testing.T) {
Subnets: []infrav1.SubnetParam{{Filter: testSubnetFilter}},
},
},
want: nil,
/* We expect this to return all tagged subnets in any network, but it returns error
[]infrav1.Network{
want: []infrav1.Network{
{ID: networkAUUID, Subnet: &infrav1.Subnet{ID: subnetA1UUID}},
{ID: networkAUUID, Subnet: &infrav1.Subnet{ID: subnetA2UUID}},
{ID: networkAUUID, Subnet: &infrav1.Subnet{ID: subnetB1UUID}},
{ID: networkBUUID, Subnet: &infrav1.Subnet{ID: subnetB1UUID}},
},
*/
expect: func(m *mock_networking.MockNetworkClientMockRecorder) {
// Empty list query returns all networks
m.ListNetwork(&networks.ListOpts{}).
Return([]networks.Network{testNetworkA, testNetworkB, testNetworkC}, nil)

// List tagged subnets in network A (A1 & A2)
networkAFilter := testSubnetListOpts
networkAFilter.NetworkID = networkAUUID
m.ListSubnet(&networkAFilter).
Return([]subnets.Subnet{testSubnetA1, testSubnetA2}, nil)

// List tagged subnets in network B (B1)
networkBFilter := testSubnetListOpts
networkBFilter.NetworkID = networkBUUID
m.ListSubnet(&networkBFilter).
Return([]subnets.Subnet{testSubnetB1}, nil)

// List tagged subnets in network C (none)
networkCFilter := testSubnetListOpts
networkCFilter.NetworkID = networkCUUID
m.ListSubnet(&networkCFilter).
Return([]subnets.Subnet{}, nil)
// List all tagged subnets in any network (A1, A2, and B1)
m.ListSubnet(&testSubnetListOpts).
Return([]subnets.Subnet{testSubnetA1, testSubnetA2, testSubnetB1}, nil)
},
wantErr: true,
wantErr: false,
},
{
name: "Network UUID and subnet filter",
Expand All @@ -254,9 +204,6 @@ func TestService_getServerNetworks(t *testing.T) {
{ID: networkAUUID, Subnet: &infrav1.Subnet{ID: subnetA2UUID}},
},
expect: func(m *mock_networking.MockNetworkClientMockRecorder) {
// List network A by UUID
expectNetworkListByUUID(m, &testNetworkA)

// List tagged subnets in network A (A1 & A2)
networkAFilter := testSubnetListOpts
networkAFilter.NetworkID = networkAUUID
Expand All @@ -279,11 +226,6 @@ func TestService_getServerNetworks(t *testing.T) {
{ID: networkAUUID, Subnet: &infrav1.Subnet{ID: subnetA1UUID}},
},
expect: func(m *mock_networking.MockNetworkClientMockRecorder) {
// List network A by uuid
expectNetworkListByUUID(m, &testNetworkA)

// List subnet A1 by uuid
expectSubnetListByUUID(m, &testSubnetA1)
},
wantErr: false,
},
Expand All @@ -304,12 +246,6 @@ func TestService_getServerNetworks(t *testing.T) {
{ID: networkAUUID, Subnet: &infrav1.Subnet{ID: subnetA2UUID}},
},
expect: func(m *mock_networking.MockNetworkClientMockRecorder) {
// List network A by uuid
expectNetworkListByUUID(m, &testNetworkA)

// List subnet A3 by uuid
expectSubnetListByUUID(m, &testSubnetA3)

// List tagged subnets in network A
networkAFilter := testSubnetListOpts
networkAFilter.NetworkID = networkAUUID
Expand Down Expand Up @@ -341,12 +277,6 @@ func TestService_getServerNetworks(t *testing.T) {
{ID: networkBUUID, Subnet: &infrav1.Subnet{ID: subnetB1UUID}},
},
expect: func(m *mock_networking.MockNetworkClientMockRecorder) {
// List network C by uuid
expectNetworkListByUUID(m, &testNetworkC)

// List subnet C1 by uuid
expectSubnetListByUUID(m, &testSubnetC1)

// List tagged networks (A & B)
m.ListNetwork(&testNetworkListOpts).
Return([]networks.Network{testNetworkA, testNetworkB}, nil)
Expand Down Expand Up @@ -391,9 +321,6 @@ func TestService_getServerNetworks(t *testing.T) {
},
want: nil,
expect: func(m *mock_networking.MockNetworkClientMockRecorder) {
// List network A by UUID
expectNetworkListByUUID(m, &testNetworkA)

// List tagged subnets in network A
networkAFilter := testSubnetListOpts
networkAFilter.NetworkID = networkAUUID
Expand Down

0 comments on commit 85fe929

Please sign in to comment.