Merge pull request juju#11187 from SimonRichardson/openstack-subnets-…

…spaces juju#11187 ### Checklist - [x] Checked if it requires a [pylibjuju](https://github.com/juju/python-libjuju) change? - [x] Added [integration tests](https://github.com/juju/juju/tree/develop/tests) for the PR? - [x] Added or updated [doc.go](https://discourse.jujucharms.com/t/readme-in-packages/451) related to packages changed? - [x] Do comments answer the question of why design decisions were made? ---- ## Description of change The following is a mechanical change to move the ability to find subnet ids from available zones. This is a very generic function, one that can be extracted to core and be tested in isolation. ## QA steps Bootstrapping to aws should be fine. ```sh juju bootstrap aws test --no-gui --debug ```
nammn · Feb 6, 2020 · 614dc71 · 614dc71
2 parents 40c7f02 + cd34c08
commit 614dc71
Show file tree

Hide file tree

Showing 5 changed files with 96 additions and 45 deletions.
diff --git a/core/network/subnet.go b/core/network/subnet.go
@@ -6,6 +6,7 @@ package network
 import (
 	"net"
 
+	"github.com/juju/collections/set"
 	"github.com/juju/errors"
 )
 
@@ -136,3 +137,23 @@ func IsValidCidr(cidr string) bool {
 	}
 	return false
 }
+
+// FindSubnetIDsForAvailabilityZone returns a series of subnet IDs from a series
+// of zones, if zones match the zoneName.
+//
+// Returns an error if no matching subnets match the zoneName.
+func FindSubnetIDsForAvailabilityZone(zoneName string, subnetsToZones map[Id][]string) ([]string, error) {
+	matchingSubnetIDs := set.NewStrings()
+	for subnetID, zones := range subnetsToZones {
+		zonesSet := set.NewStrings(zones...)
+		if zonesSet.Contains(zoneName) {
+			matchingSubnetIDs.Add(string(subnetID))
+		}
+	}
+
+	if matchingSubnetIDs.IsEmpty() {
+		return nil, errors.NotFoundf("subnets in AZ %q", zoneName)
+	}
+
+	return matchingSubnetIDs.SortedValues(), nil
+}
diff --git a/core/network/subnet_test.go b/core/network/subnet_test.go
@@ -0,0 +1,74 @@
+// Copyright 2020 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package network_test
+
+import (
+	"github.com/juju/errors"
+	"github.com/juju/juju/core/network"
+	"github.com/juju/testing"
+	jc "github.com/juju/testing/checkers"
+	gc "gopkg.in/check.v1"
+)
+
+type subnetSuite struct {
+	testing.IsolationSuite
+}
+
+var _ = gc.Suite(&subnetSuite{})
+
+func (*subnetSuite) TestFindSubnetIDsForAZ(c *gc.C) {
+	testCases := []struct {
+		name           string
+		zoneName       string
+		subnetsToZones map[network.Id][]string
+		expected       []string
+		expectedErr    func(error) bool
+	}{
+		{
+			name:           "empty",
+			zoneName:       "",
+			subnetsToZones: make(map[network.Id][]string),
+			expected:       make([]string, 0),
+			expectedErr:    errors.IsNotFound,
+		},
+		{
+			name:     "no match",
+			zoneName: "fuzz",
+			subnetsToZones: map[network.Id][]string{
+				"bar": {"foo", "baz"},
+			},
+			expected:    make([]string, 0),
+			expectedErr: errors.IsNotFound,
+		},
+		{
+			name:     "match",
+			zoneName: "foo",
+			subnetsToZones: map[network.Id][]string{
+				"bar": {"foo", "baz"},
+			},
+			expected: []string{"bar"},
+		},
+		{
+			name:     "multi-match",
+			zoneName: "foo",
+			subnetsToZones: map[network.Id][]string{
+				"bar":   {"foo", "baz"},
+				"other": {"aaa", "foo", "xxx"},
+			},
+			expected: []string{"bar", "other"},
+		},
+	}
+
+	for i, t := range testCases {
+		c.Logf("test %d: %s", i, t.name)
+
+		res, err := network.FindSubnetIDsForAvailabilityZone(t.zoneName, t.subnetsToZones)
+		if t.expectedErr != nil {
+			c.Check(t.expectedErr(err), jc.IsTrue)
+		} else {
+			c.Assert(err, gc.IsNil)
+			c.Check(res, gc.DeepEquals, t.expected)
+		}
+	}
+}
diff --git a/provider/ec2/environ.go b/provider/ec2/environ.go
@@ -579,7 +579,7 @@ func (e *environ) StartInstance(ctx context.ProviderCallContext, args environs.S
 		}
 		subnetIDsForZone, subnetErr = getVPCSubnetIDsForAvailabilityZone(e.ec2, ctx, e.ecfg().vpcID(), availabilityZone, allowedSubnetIDs)
 	} else if args.Constraints.HasSpaces() {
-		subnetIDsForZone, subnetErr = findSubnetIDsForAvailabilityZone(availabilityZone, args.SubnetsToZones)
+		subnetIDsForZone, subnetErr = corenetwork.FindSubnetIDsForAvailabilityZone(availabilityZone, args.SubnetsToZones)
 		if subnetErr == nil && placementSubnetID != "" {
 			asSet := set.NewStrings(subnetIDsForZone...)
 			if asSet.Contains(placementSubnetID) {

diff --git a/provider/ec2/environ_vpc.go b/provider/ec2/environ_vpc.go
@@ -11,7 +11,6 @@ import (
 	"github.com/juju/errors"
 	"gopkg.in/amz.v3/ec2"
 
-	"github.com/juju/juju/core/network"
 	"github.com/juju/juju/environs"
 	"github.com/juju/juju/environs/context"
 )
@@ -491,22 +490,6 @@ func getVPCSubnetIDsForAvailabilityZone(
 	return sortedIDs, nil
 }
 
-func findSubnetIDsForAvailabilityZone(zoneName string, subnetsToZones map[network.Id][]string) ([]string, error) {
-	matchingSubnetIDs := set.NewStrings()
-	for subnetID, zones := range subnetsToZones {
-		zonesSet := set.NewStrings(zones...)
-		if zonesSet.Contains(zoneName) {
-			matchingSubnetIDs.Add(string(subnetID))
-		}
-	}
-
-	if matchingSubnetIDs.IsEmpty() {
-		return nil, errors.NotFoundf("subnets in AZ %q", zoneName)
-	}
-
-	return matchingSubnetIDs.SortedValues(), nil
-}
-
 func isVPCIDSetButInvalid(vpcID string) bool {
 	return isVPCIDSet(vpcID) && !strings.HasPrefix(vpcID, "vpc-")
 }

diff --git a/provider/ec2/environ_vpc_test.go b/provider/ec2/environ_vpc_test.go
@@ -12,7 +12,6 @@ import (
 	"gopkg.in/amz.v3/ec2"
 	gc "gopkg.in/check.v1"
 
-	"github.com/juju/juju/core/network"
 	"github.com/juju/juju/environs/context"
 	envtesting "github.com/juju/juju/environs/testing"
 	"github.com/juju/juju/provider/common"
@@ -714,32 +713,6 @@ func (s *vpcSuite) TestGetVPCSubnetIDsForAvailabilityZoneSuccess(c *gc.C) {
 	s.stubAPI.CheckSingleSubnetsCall(c, anyVPC)
 }
 
-var fakeSubnetsToZones = map[network.Id][]string{
-	"subnet-foo": {"az1", "az2"},
-	"subnet-bar": {"az1"},
-	"subnet-oof": {"az3"},
-}
-
-func (s *vpcSuite) TestFindSubnetIDsForAvailabilityZoneNoneFound(c *gc.C) {
-	subnetIDs, err := findSubnetIDsForAvailabilityZone("unknown-zone", fakeSubnetsToZones)
-	c.Assert(err, gc.ErrorMatches, `subnets in AZ "unknown-zone" not found`)
-	c.Check(err, jc.Satisfies, errors.IsNotFound)
-	c.Check(subnetIDs, gc.IsNil)
-}
-
-func (s *vpcSuite) TestFindSubnetIDsForAvailabilityOneMatched(c *gc.C) {
-	subnetIDs, err := findSubnetIDsForAvailabilityZone("az3", fakeSubnetsToZones)
-	c.Assert(err, jc.ErrorIsNil)
-	c.Check(subnetIDs, gc.DeepEquals, []string{"subnet-oof"})
-}
-
-func (s *vpcSuite) TestFindSubnetIDsForAvailabilityMultipleMatched(c *gc.C) {
-	subnetIDs, err := findSubnetIDsForAvailabilityZone("az1", fakeSubnetsToZones)
-	c.Assert(err, jc.ErrorIsNil)
-	// Result slice of IDs is always sorted.
-	c.Check(subnetIDs, gc.DeepEquals, []string{"subnet-bar", "subnet-foo"})
-}
-
 const (
 	notDefaultVPC = false
 	defaultVPC    = true