discoverspaces: Get subnets when provider doesn't support space discovery

apiserver/common/networkingcommon/shims.go

@@ -110,7 +110,7 @@ func (s *stateShim) AllSpaces() ([]BackingSpace, error) {
 }
 
 func (s *stateShim) AddSubnet(info BackingSubnetInfo) (BackingSubnet, error) {
-	// TODO(dimitern): Add multiple AZs per subnet in state.
+	// TODO(xtian): cargo culting taking the first zone - why was this done?
 	var firstZone string
 	if len(info.AvailabilityZones) > 0 {
 		firstZone = info.AvailabilityZones[0]

apiserver/common/networkingcommon/subnets.go

@@ -414,12 +414,19 @@ func ListSubnets(api NetworkBacking, args params.SubnetsFilters) (results params
 			)
 			continue
 		}
+		// TODO(babbageclunk): make the empty string a valid space
+		// name, rather than treating blank as "doesn't have a space".
+		// lp:1672888
+		var spaceTag string
+		if subnet.SpaceName() != "" {
+			spaceTag = names.NewSpaceTag(subnet.SpaceName()).String()
+		}
 		result := params.Subnet{
 			CIDR:       subnet.CIDR(),
 			ProviderId: string(subnet.ProviderId()),
 			VLANTag:    subnet.VLANTag(),
 			Life:       subnet.Life(),
-			SpaceTag:   names.NewSpaceTag(subnet.SpaceName()).String(),
+			SpaceTag:   spaceTag,
 			Zones:      subnet.AvailabilityZones(),
 			Status:     subnet.Status(),
 		}

apiserver/common/networkingcommon/subnets_test.go

@@ -789,6 +789,26 @@ func (s *SubnetsSuite) TestListSubnetsInvalidSpaceTag(c *gc.C) {
 	c.Assert(err, gc.ErrorMatches, `"invalid" is not a valid tag`)
 }
 
+func (s *SubnetsSuite) TestListSubnetsBlankSpaceTag(c *gc.C) {
+	args := params.SubnetsFilters{}
+	apiservertesting.BackingInstance.Subnets = []networkingcommon.BackingSubnet{
+		&apiservertesting.FakeSubnet{Info: networkingcommon.BackingSubnetInfo{
+			CIDR:              "10.0.10.0/24",
+			ProviderId:        "1",
+			AvailabilityZones: []string{"zone1"},
+			SpaceName:         "",
+		}},
+	}
+	results, err := networkingcommon.ListSubnets(apiservertesting.BackingInstance, args)
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(results, gc.DeepEquals, params.ListSubnetsResults{Results: []params.Subnet{{
+		CIDR:       "10.0.10.0/24",
+		ProviderId: "1",
+		SpaceTag:   "",
+		Zones:      []string{"zone1"},
+	}}})
+}
+
 func (s *SubnetsSuite) TestListSubnetsAllSubnetError(c *gc.C) {
 	boom := errors.New("no subnets for you")
 	apiservertesting.BackingInstance.SetErrors(boom)

apiserver/discoverspaces/discoverspaces.go

@@ -5,43 +5,48 @@ package discoverspaces
 
 import (
 	"github.com/juju/errors"
+	"github.com/juju/utils/set"
+	"gopkg.in/juju/names.v2"
 
 	"github.com/juju/juju/apiserver/common"
 	"github.com/juju/juju/apiserver/common/networkingcommon"
 	"github.com/juju/juju/apiserver/facade"
 	"github.com/juju/juju/apiserver/params"
+	"github.com/juju/juju/network"
 	"github.com/juju/juju/state"
 )
 
 func init() {
-	common.RegisterStandardFacade("DiscoverSpaces", 2, NewDiscoverSpacesAPI)
+	common.RegisterStandardFacade("DiscoverSpaces", 2, NewAPI)
 }
 
-// DiscoverSpacesAPI implements the API used by the discoverspaces worker.
-type DiscoverSpacesAPI struct {
+// API implements the API used by the discoverspaces worker.
+type API struct {
 	st         networkingcommon.NetworkBacking
 	resources  facade.Resources
 	authorizer facade.Authorizer
 }
 
-// NewDiscoverSpacesAPI creates a new instance of the DiscoverSpaces API.
-func NewDiscoverSpacesAPI(st *state.State, resources facade.Resources, authorizer facade.Authorizer) (*DiscoverSpacesAPI, error) {
-	return NewDiscoverSpacesAPIWithBacking(networkingcommon.NewStateShim(st), resources, authorizer)
+// NewAPI creates a new instance of the DiscoverSpaces API.
+func NewAPI(st *state.State, resources facade.Resources, authorizer facade.Authorizer) (*API, error) {
+	return NewAPIWithBacking(networkingcommon.NewStateShim(st), resources, authorizer)
 }
 
-func NewDiscoverSpacesAPIWithBacking(st networkingcommon.NetworkBacking, resources facade.Resources, authorizer facade.Authorizer) (*DiscoverSpacesAPI, error) {
+// NewAPIWithBacking creates an API instance from the given network
+// backing (primarily useful from tests).
+func NewAPIWithBacking(st networkingcommon.NetworkBacking, resources facade.Resources, authorizer facade.Authorizer) (*API, error) {
 	if !authorizer.AuthController() {
 		return nil, common.ErrPerm
 	}
-	return &DiscoverSpacesAPI{
+	return &API{
 		st:         st,
 		authorizer: authorizer,
 		resources:  resources,
 	}, nil
 }
 
 // ModelConfig returns the current model's configuration.
-func (api *DiscoverSpacesAPI) ModelConfig() (params.ModelConfigResult, error) {
+func (api *API) ModelConfig() (params.ModelConfigResult, error) {
 	result := params.ModelConfigResult{}
 
 	config, err := api.st.ModelConfig()
@@ -57,12 +62,12 @@ func (api *DiscoverSpacesAPI) ModelConfig() (params.ModelConfigResult, error) {
 
 // CreateSpaces creates a new Juju network space, associating the
 // specified subnets with it (optional; can be empty).
-func (api *DiscoverSpacesAPI) CreateSpaces(args params.CreateSpacesParams) (results params.ErrorResults, err error) {
+func (api *API) CreateSpaces(args params.CreateSpacesParams) (results params.ErrorResults, err error) {
 	return networkingcommon.CreateSpaces(api.st, args)
 }
 
 // ListSpaces lists all the available spaces and their associated subnets.
-func (api *DiscoverSpacesAPI) ListSpaces() (results params.DiscoverSpacesResults, err error) {
+func (api *API) ListSpaces() (results params.DiscoverSpacesResults, err error) {
 	spaces, err := api.st.AllSpaces()
 	if err != nil {
 		return results, errors.Trace(err)
@@ -91,13 +96,62 @@ func (api *DiscoverSpacesAPI) ListSpaces() (results params.DiscoverSpacesResults
 	return results, nil
 }
 
-// AddSubnets is defined on the API interface.
-func (api *DiscoverSpacesAPI) AddSubnets(args params.AddSubnetsParams) (params.ErrorResults, error) {
-	return networkingcommon.AddSubnets(api.st, args)
+// AddSubnets adds the passed subnet info to the backing store.
+func (api *API) AddSubnets(args params.AddSubnetsParams) (params.ErrorResults, error) {
+	var empty params.ErrorResults
+	if len(args.Subnets) == 0 {
+		return empty, nil
+	}
+	spaces, err := api.st.AllSpaces()
+	if err != nil {
+		return empty, errors.Trace(err)
+	}
+	spaceNames := set.NewStrings()
+	for _, space := range spaces {
+		spaceNames.Add(space.Name())
+	}
+
+	addOneSubnet := func(arg params.AddSubnetParams) error {
+		if arg.SubnetProviderId == "" {
+			return errors.Trace(errors.New("SubnetProviderId is required"))
+		}
+		spaceName := ""
+		if arg.SpaceTag != "" {
+			spaceTag, err := names.ParseSpaceTag(arg.SpaceTag)
+			if err != nil {
+				return errors.Annotate(err, "SpaceTag is invalid")
+			}
+			spaceName = spaceTag.Id()
+			if !spaceNames.Contains(spaceName) {
+				return errors.NotFoundf("space %q", spaceName)
+			}
+		}
+		if arg.SubnetTag == "" {
+			return errors.New("SubnetTag is required")
+		}
+		subnetTag, err := names.ParseSubnetTag(arg.SubnetTag)
+		if err != nil {
+			return errors.Annotate(err, "SubnetTag is invalid")
+		}
+		_, err = api.st.AddSubnet(networkingcommon.BackingSubnetInfo{
+			ProviderId:        network.Id(arg.SubnetProviderId),
+			CIDR:              subnetTag.Id(),
+			VLANTag:           arg.VLANTag,
+			AvailabilityZones: arg.Zones,
+			SpaceName:         spaceName,
+		})
+		return errors.Trace(err)
+	}
+
+	results := make([]params.ErrorResult, len(args.Subnets))
+	for i, arg := range args.Subnets {
+		results[i].Error = common.ServerError(addOneSubnet(arg))
+	}
+	return params.ErrorResults{Results: results}, nil
 }
 
 // ListSubnets lists all the available subnets or only those matching
 // all given optional filters.
-func (api *DiscoverSpacesAPI) ListSubnets(args params.SubnetsFilters) (results params.ListSubnetsResults, err error) {
+func (api *API) ListSubnets(args params.SubnetsFilters) (results params.ListSubnetsResults, err error) {
 	return networkingcommon.ListSubnets(api.st, args)
 }

apiserver/discoverspaces/discoverspaces_test.go

@@ -4,13 +4,13 @@
 package discoverspaces_test
 
 import (
-	"errors"
-
+	"github.com/juju/errors"
 	jc "github.com/juju/testing/checkers"
 	gc "gopkg.in/check.v1"
 	"gopkg.in/juju/names.v2"
 
 	"github.com/juju/juju/apiserver/common"
+	"github.com/juju/juju/apiserver/common/networkingcommon"
 	"github.com/juju/juju/apiserver/discoverspaces"
 	"github.com/juju/juju/apiserver/params"
 	apiservertesting "github.com/juju/juju/apiserver/testing"
@@ -23,7 +23,7 @@ type DiscoverSpacesSuite struct {
 
 	resources  *common.Resources
 	authorizer apiservertesting.FakeAuthorizer
-	facade     *discoverspaces.DiscoverSpacesAPI
+	facade     *discoverspaces.API
 }
 
 var _ = gc.Suite(&DiscoverSpacesSuite{})
@@ -53,7 +53,7 @@ func (s *DiscoverSpacesSuite) SetUpTest(c *gc.C) {
 	}
 
 	var err error
-	s.facade, err = discoverspaces.NewDiscoverSpacesAPIWithBacking(
+	s.facade, err = discoverspaces.NewAPIWithBacking(
 		apiservertesting.BackingInstance, s.resources, s.authorizer,
 	)
 	c.Assert(err, jc.ErrorIsNil)
@@ -132,3 +132,141 @@ func (s *DiscoverSpacesSuite) TestListSpacesFailure(c *gc.C) {
 
 	apiservertesting.BackingInstance.CheckCallNames(c, "AllSpaces")
 }
+
+func (s *DiscoverSpacesSuite) TestAddSubnetsParamsCombinations(c *gc.C) {
+	apiservertesting.BackingInstance.SetUp(
+		c,
+		apiservertesting.StubNetworkingEnvironName,
+		apiservertesting.WithZones,
+		apiservertesting.WithSpaces,
+		apiservertesting.WithSubnets)
+
+	args := params.AddSubnetsParams{Subnets: []params.AddSubnetParams{{
+		// No ProviderId
+		SubnetProviderId: "",
+		SubnetTag:        "subnet-10.10.0.0/24",
+		VLANTag:          3,
+		Zones:            []string{"a", "b", "c"},
+		SpaceTag:         "space-dmz",
+	}, {
+		// No subnet tag
+		SubnetProviderId: "1",
+		SubnetTag:        "",
+		VLANTag:          3,
+		Zones:            []string{"a", "b", "c"},
+		SpaceTag:         "space-dmz",
+	}, {
+		// Invalid subnet
+		SubnetProviderId: "1",
+		SubnetTag:        "subnet-10.10.10.10",
+		VLANTag:          3,
+		Zones:            []string{"a", "b", "c"},
+		SpaceTag:         "space-dmz",
+	}, {
+		// Invalid space
+		SubnetProviderId: "1",
+		SubnetTag:        "subnet-10.10.10.10",
+		VLANTag:          3,
+		Zones:            []string{"a", "b", "c"},
+		SpaceTag:         "application-blemp",
+	}, {
+		// Non-existent space
+		SubnetProviderId: "1",
+		SubnetTag:        "subnet-10.10.10.0/24",
+		VLANTag:          3,
+		Zones:            []string{"a", "b", "c"},
+		SpaceTag:         "space-thing",
+	}, {
+		// Successful - ipv6
+		SubnetProviderId: "sn-ipv6",
+		SubnetTag:        "subnet-2001:db8::/32",
+		VLANTag:          0,
+		Zones:            []string{"a", "b", "c"},
+		SpaceTag:         "space-dmz",
+	}, {
+		// Successful - no zones
+		SubnetProviderId: "sn-no-zone",
+		SubnetTag:        "subnet-10.10.10.0/24",
+		VLANTag:          3,
+		Zones:            nil,
+		SpaceTag:         "space-dmz",
+	}, {
+		// Successful - no space
+		SubnetProviderId: "sn-no-space",
+		SubnetTag:        "subnet-10.10.10.0/24",
+		VLANTag:          3,
+		Zones:            []string{"a", "b", "c"},
+		SpaceTag:         "",
+	}}}
+
+	expectedErrors := []struct {
+		message   string
+		satisfier func(error) bool
+	}{
+		{"SubnetProviderId is required", nil},
+		{"SubnetTag is required", nil},
+		{`SubnetTag is invalid: "subnet-10.10.10.10" is not a valid subnet tag`, nil},
+		{`SpaceTag is invalid: "application-blemp" is not a valid space tag`, nil},
+		{`space "thing" not found`, params.IsCodeNotFound},
+		{"", nil},
+		{"", nil},
+		{"", nil},
+	}
+	expectedBackingInfos := []networkingcommon.BackingSubnetInfo{{
+		ProviderId:        "sn-ipv6",
+		CIDR:              "2001:db8::/32",
+		VLANTag:           0,
+		AvailabilityZones: []string{"a", "b", "c"},
+		SpaceName:         "dmz",
+	}, {
+		ProviderId:        "sn-no-zone",
+		CIDR:              "10.10.10.0/24",
+		VLANTag:           3,
+		AvailabilityZones: nil,
+		SpaceName:         "dmz",
+	}, {
+		ProviderId:        "sn-no-space",
+		CIDR:              "10.10.10.0/24",
+		VLANTag:           3,
+		AvailabilityZones: []string{"a", "b", "c"},
+		SpaceName:         "",
+	}}
+	c.Check(expectedErrors, gc.HasLen, len(args.Subnets))
+	results, err := s.facade.AddSubnets(args)
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(len(results.Results), gc.Equals, len(args.Subnets))
+	for i, result := range results.Results {
+		c.Logf("result #%d: expected: %q", i, expectedErrors[i].message)
+		if expectedErrors[i].message == "" {
+			if !c.Check(result.Error, gc.IsNil) {
+				c.Logf("unexpected error: %v; args: %#v", result.Error, args.Subnets[i])
+			}
+			continue
+		}
+		if !c.Check(result.Error, gc.NotNil) {
+			c.Logf("unexpected success; args: %#v", args.Subnets[i])
+			continue
+		}
+		c.Check(result.Error.Message, gc.Equals, expectedErrors[i].message)
+		if expectedErrors[i].satisfier != nil {
+			c.Check(result.Error, jc.Satisfies, expectedErrors[i].satisfier)
+		} else {
+			c.Check(result.Error.Code, gc.Equals, "")
+		}
+	}
+
+	apiservertesting.CheckMethodCalls(c, apiservertesting.SharedStub,
+		apiservertesting.BackingCall("AllSpaces"),
+		apiservertesting.BackingCall("AddSubnet", expectedBackingInfos[0]),
+		apiservertesting.BackingCall("AddSubnet", expectedBackingInfos[1]),
+		apiservertesting.BackingCall("AddSubnet", expectedBackingInfos[2]),
+	)
+	apiservertesting.ResetStub(apiservertesting.SharedStub)
+
+	// Finally, check that no params yields no results.
+	results, err = s.facade.AddSubnets(params.AddSubnetsParams{})
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(results.Results, gc.HasLen, 0)
+
+	apiservertesting.CheckMethodCalls(c, apiservertesting.SharedStub)
+}

apiserver/params/network.go

@@ -580,6 +580,7 @@ type AddSubnetParams struct {
 	SubnetTag        string   `json:"subnet-tag,omitempty"`
 	SubnetProviderId string   `json:"subnet-provider-id,omitempty"`
 	SpaceTag         string   `json:"space-tag"`
+	VLANTag          int      `json:"vlan-tag,omitempty"`
 	Zones            []string `json:"zones,omitempty"`
 }