Adds the SubnetsByCIDR method, previously removed from the spaces

facade, to the subnets facade where it belongs.

The AllSpaces method, not used by any client functionality, is deprecated
for all new versions of the subnets facade. This method should never
have resided here.

apiserver/facades/client/subnets/shims.go

@@ -67,6 +67,24 @@ func (s *stateShim) SubnetByCIDR(cidr string) (networkingcommon.BackingSubnet, e
 	return networkingcommon.NewSubnetShim(result), nil
 }
 
+// SubnetsByCIDR wraps each result of a call to state.SubnetsByCIDR
+// in a subnet shim and returns the result.
+func (s *stateShim) SubnetsByCIDR(cidr string) ([]networkingcommon.BackingSubnet, error) {
+	subnets, err := s.State.SubnetsByCIDR(cidr)
+	if err != nil {
+		return nil, errors.Trace(err)
+	}
+	if len(subnets) == 0 {
+		return nil, nil
+	}
+
+	result := make([]networkingcommon.BackingSubnet, len(subnets))
+	for i, subnet := range subnets {
+		result[i] = networkingcommon.NewSubnetShim(subnet)
+	}
+	return result, nil
+}
+
 func (s *stateShim) AvailabilityZones() ([]providercommon.AvailabilityZone, error) {
 	// TODO (hml) 2019-09-13
 	// now available... include.

apiserver/facades/client/subnets/subnets.go

@@ -16,6 +16,7 @@ import (
 	"github.com/juju/juju/apiserver/common/networkingcommon"
 	"github.com/juju/juju/apiserver/facade"
 	"github.com/juju/juju/apiserver/params"
+	"github.com/juju/juju/core/network"
 	"github.com/juju/juju/core/permission"
 	"github.com/juju/juju/environs"
 	"github.com/juju/juju/environs/context"
@@ -43,8 +44,12 @@ type Backing interface {
 	// AllSubnets returns all backing subnets.
 	AllSubnets() ([]networkingcommon.BackingSubnet, error)
 
+	// SubnetByCIDR returns a unique subnet based on the input CIDR.
 	SubnetByCIDR(cidr string) (networkingcommon.BackingSubnet, error)
 
+	// SubnetsByCIDR returns any subnets with the input CIDR.
+	SubnetsByCIDR(cidr string) ([]networkingcommon.BackingSubnet, error)
+
 	// AllSpaces returns all known Juju network spaces.
 	AllSpaces() ([]networkingcommon.BackingSpace, error)
 
@@ -54,10 +59,15 @@ type Backing interface {
 
 // APIv2 provides the subnets API facade for versions < 3.
 type APIv2 struct {
+	*APIv3
+}
+
+// APIv3 provides the subnets API facade for versions 3.
+type APIv3 struct {
 	*API
 }
 
-// API provides the subnets API facade for version 3.
+// API provides the subnets API facade for version 4.
 type API struct {
 	backing    Backing
 	resources  facade.Resources
@@ -67,21 +77,30 @@ type API struct {
 
 // NewAPIv2 is a wrapper that creates a V2 subnets API.
 func NewAPIv2(st *state.State, res facade.Resources, auth facade.Authorizer) (*APIv2, error) {
-	api, err := NewAPI(st, res, auth)
+	api, err := NewAPIv3(st, res, auth)
 	if err != nil {
 		return nil, errors.Trace(err)
 	}
 	return &APIv2{api}, nil
 }
 
+// NewAPIv3 is a wrapper that creates a V3 subnets API.
+func NewAPIv3(st *state.State, res facade.Resources, auth facade.Authorizer) (*APIv3, error) {
+	api, err := NewAPI(st, res, auth)
+	if err != nil {
+		return nil, errors.Trace(err)
+	}
+	return &APIv3{api}, nil
+}
+
 // NewAPI creates a new Subnets API server-side facade with a
 // state.State backing.
 func NewAPI(st *state.State, res facade.Resources, auth facade.Authorizer) (*API, error) {
-	stateshim, err := NewStateShim(st)
+	stateShim, err := NewStateShim(st)
 	if err != nil {
 		return nil, errors.Trace(err)
 	}
-	return newAPIWithBacking(stateshim, state.CallContext(st), res, auth)
+	return newAPIWithBacking(stateShim, state.CallContext(st), res, auth)
 }
 
 func (api *API) checkCanRead() error {
@@ -132,7 +151,10 @@ func (api *API) AllZones() (params.ZoneResults, error) {
 }
 
 // AllSpaces returns the tags of all network spaces known to Juju.
-func (api *API) AllSpaces() (params.SpaceResults, error) {
+// This is not recruited by any Juju client code and should not actually exist.
+// It is preserved for versions 3 and below of the facade,
+// but blanked for later versions to ensure it is not used.
+func (api *APIv3) AllSpaces() (params.SpaceResults, error) {
 	if err := api.checkCanRead(); err != nil {
 		return params.SpaceResults{}, err
 	}
@@ -146,14 +168,16 @@ func (api *API) AllSpaces() (params.SpaceResults, error) {
 
 	results.Results = make([]params.SpaceResult, len(spaces))
 	for i, space := range spaces {
-		// TODO(dimitern): Add a Tag() a method and use it here. Too
-		// early to do it now as it will just complicate the tests.
 		tag := names.NewSpaceTag(space.Name())
 		results.Results[i].Tag = tag.String()
 	}
 	return results, nil
 }
 
+// AllSpaces does not belong on this API facade.
+// If you need it, use the spaces facade.
+func (api *API) AllSpaces(_, _ struct{}) {}
+
 // AddSubnets adds existing subnets to Juju.
 func (api *API) AddSubnets(args params.AddSubnetsParams) (params.ErrorResults, error) {
 	if err := api.checkCanWrite(); err != nil {
@@ -162,8 +186,8 @@ func (api *API) AddSubnets(args params.AddSubnetsParams) (params.ErrorResults, e
 	return api.addSubnets(args)
 }
 
-// AddSubnets adds existing subnets to Juju.  Args are converted to
-// the new form for compatibility
+// AddSubnets adds existing subnets to Juju.
+// Args are converted to the new form for compatibility.
 func (api *APIv2) AddSubnets(args params.AddSubnetsParamsV2) (params.ErrorResults, error) {
 	if err := api.checkCanWrite(); err != nil {
 		return params.ErrorResults{}, err
@@ -241,6 +265,37 @@ func (api *API) ListSubnets(args params.SubnetsFilters) (results params.ListSubn
 	return results, nil
 }
 
+// SubnetsByCIDR returns the collection of subnets matching each CIDR in the input.
+func (api *API) SubnetsByCIDR(arg params.CIDRParams) (params.SubnetsResults, error) {
+	result := params.SubnetsResults{}
+
+	if err := api.checkCanRead(); err != nil {
+		return result, err
+	}
+
+	results := make([]params.SubnetsResult, len(arg.CIDRS))
+	for i, cidr := range arg.CIDRS {
+		if !network.IsValidCidr(cidr) {
+			results[i].Error = common.ServerError(errors.NotValidf("CIDR %q", cidr))
+			continue
+		}
+
+		subnets, err := api.backing.SubnetsByCIDR(cidr)
+		if err != nil {
+			results[i].Error = common.ServerError(err)
+			continue
+		}
+
+		subnetResults := make([]params.SubnetV2, len(subnets))
+		for j, subnet := range subnets {
+			subnetResults[j] = networkingcommon.BackingSubnetToParamsSubnetV2(subnet)
+		}
+		results[i].Subnets = subnetResults
+	}
+	result.Results = results
+	return result, nil
+}
+
 func convertToAddSubnetsParams(old params.AddSubnetsParamsV2) (params.AddSubnetsParams, int, error) {
 	subnetsParams := params.AddSubnetsParams{
 		Subnets: make([]params.AddSubnetParams, len(old.Subnets)),

apiserver/facades/client/subnets/subnets_test.go

@@ -7,6 +7,7 @@ import (
 	"github.com/golang/mock/gomock"
 	"github.com/juju/collections/set"
 	"github.com/juju/errors"
+	"github.com/juju/juju/core/life"
 	jc "github.com/juju/testing/checkers"
 	gc "gopkg.in/check.v1"
 	"gopkg.in/juju/names.v3"
@@ -26,8 +27,9 @@ import (
 	coretesting "github.com/juju/juju/testing"
 )
 
-// This suite shows the new mocking suite. While below shows the old suite we want to migrate from.
-type SubnetTestMockSuite struct {
+// SubnetSuite uses mocks for testing.
+// All future facade tests should be added to this suite.
+type SubnetSuite struct {
 	mockBacking          *mocks.MockBacking
 	mockResource         *facademocks.MockResources
 	mockCloudCallContext *context.CloudCallContext
@@ -36,31 +38,50 @@ type SubnetTestMockSuite struct {
 	api *subnets.API
 }
 
-var _ = gc.Suite(&SubnetTestMockSuite{})
+var _ = gc.Suite(&SubnetSuite{})
 
-func (s *SubnetTestMockSuite) TearDownTest(c *gc.C) {
+func (s *SubnetSuite) TearDownTest(c *gc.C) {
 	s.api = nil
 }
 
-func (s *SubnetTestMockSuite) TestAllSpaces(c *gc.C) {
+func (s *SubnetSuite) TestSubnetsByCIDR(c *gc.C) {
 	ctrl := s.setupSubnetsAPI(c)
 	defer ctrl.Finish()
-	tag := names.NewSpaceTag("myspace")
 
-	spacesMock := networkcommonmocks.NewMockBackingSpace(ctrl)
-	spacesMock.EXPECT().Name().Return(tag.Id())
-
-	s.mockBacking.EXPECT().AllSpaces().Return([]networkingcommon.BackingSpace{spacesMock}, nil)
+	cidrs := []string{"10.10.10.0/24", "10.10.20.0/24", "not-a-cidr"}
+
+	subnet := networkcommonmocks.NewMockBackingSubnet(ctrl)
+	sExp := subnet.EXPECT()
+	sExp.ID().Return("1")
+	sExp.CIDR().Return("10.10.20.0/24")
+	sExp.SpaceName().Return("space")
+	sExp.VLANTag().Return(0)
+	sExp.ProviderId().Return(network.Id("0"))
+	sExp.ProviderNetworkId().Return(network.Id("1"))
+	sExp.AvailabilityZones().Return([]string{"bar", "bam"})
+	sExp.Status().Return("in-use")
+	sExp.Life().Return(life.Value("alive"))
+
+	bExp := s.mockBacking.EXPECT()
+	gomock.InOrder(
+		bExp.SubnetsByCIDR(cidrs[0]).Return(nil, errors.New("bad-mongo")),
+		bExp.SubnetsByCIDR(cidrs[1]).Return([]networkingcommon.BackingSubnet{subnet}, nil),
+		// No call for cidrs[2]; the input is invalidated.
+	)
 
-	spaces, err := s.api.AllSpaces()
+	arg := params.CIDRParams{CIDRS: cidrs}
+	res, err := s.api.SubnetsByCIDR(arg)
 	c.Assert(err, jc.ErrorIsNil)
-	c.Assert(spaces.Results, gc.HasLen, 1)
-	c.Assert(spaces.Results[0].Tag, gc.Equals, tag.String())
-	c.Assert(spaces.Results[0].Error, gc.IsNil)
 
+	results := res.Results
+	c.Assert(results, gc.HasLen, 3)
+
+	c.Check(results[0].Error.Message, gc.Equals, "bad-mongo")
+	c.Check(results[1].Subnets, gc.HasLen, 1)
+	c.Check(results[2].Error.Message, gc.Equals, `CIDR "not-a-cidr" not valid`)
 }
 
-func (s *SubnetTestMockSuite) setupSubnetsAPI(c *gc.C) *gomock.Controller {
+func (s *SubnetSuite) setupSubnetsAPI(c *gc.C) *gomock.Controller {
 	ctrl := gomock.NewController(c)
 	s.mockResource = facademocks.NewMockResources(ctrl)
 	s.mockCloudCallContext = context.NewCloudCallContext()
@@ -78,6 +99,10 @@ func (s *SubnetTestMockSuite) setupSubnetsAPI(c *gc.C) *gomock.Controller {
 	return ctrl
 }
 
+// SubnetsSuite is the old testing suite based on testing stubs.
+// This should be phased out in favour of mock-based tests.
+// The testing network infrastructure should also be removed as soon as can be
+// managed.
 type SubnetsSuite struct {
 	coretesting.BaseSuite
 	apiservertesting.StubNetwork
@@ -93,6 +118,10 @@ type stubBacking struct {
 	*apiservertesting.StubBacking
 }
 
+func (sb *stubBacking) SubnetsByCIDR(_ string) ([]networkingcommon.BackingSubnet, error) {
+	panic("should not be called")
+}
+
 var _ = gc.Suite(&SubnetsSuite{})
 
 func (s *SubnetsSuite) SetUpSuite(c *gc.C) {
@@ -356,9 +385,15 @@ func (s *SubnetsSuite) TestAllSpacesNoExistingSuccess(c *gc.C) {
 }
 
 func (s *SubnetsSuite) testAllSpacesSuccess(c *gc.C, withBackingSpaces apiservertesting.SetUpFlag) {
-	apiservertesting.BackingInstance.SetUp(c, apiservertesting.StubZonedEnvironName, apiservertesting.WithZones, withBackingSpaces, apiservertesting.WithSubnets)
+	apiservertesting.BackingInstance.SetUp(c,
+		apiservertesting.StubZonedEnvironName,
+		apiservertesting.WithZones,
+		withBackingSpaces,
+		apiservertesting.WithSubnets,
+	)
 
-	results, err := s.facade.AllSpaces()
+	api := &subnets.APIv3{API: s.facade}
+	results, err := api.AllSpaces()
 	c.Assert(err, jc.ErrorIsNil)
 	s.AssertAllSpacesResult(c, results, apiservertesting.BackingInstance.Spaces)
 
@@ -370,7 +405,8 @@ func (s *SubnetsSuite) testAllSpacesSuccess(c *gc.C, withBackingSpaces apiserver
 func (s *SubnetsSuite) TestAllSpacesFailure(c *gc.C) {
 	apiservertesting.SharedStub.SetErrors(errors.NotFoundf("boom"))
 
-	results, err := s.facade.AllSpaces()
+	api := &subnets.APIv3{API: s.facade}
+	results, err := api.AllSpaces()
 	c.Assert(err, gc.ErrorMatches, "boom not found")
 	// Verify the cause is not obscured.
 	c.Assert(err, jc.Satisfies, errors.IsNotFound)
@@ -569,7 +605,7 @@ func (s *SubnetsSuite) TestAddSubnetAPI(c *gc.C) {
 func (s *SubnetsSuite) TestAddSubnetAPIv2(c *gc.C) {
 	apiservertesting.BackingInstance.SetUp(c, apiservertesting.StubNetworkingEnvironName,
 		apiservertesting.WithZones, apiservertesting.WithSpaces, apiservertesting.WithSubnets)
-	apiV2 := &subnets.APIv2{s.facade}
+	apiV2 := &subnets.APIv2{APIv3: &subnets.APIv3{API: s.facade}}
 	results, err := apiV2.AddSubnets(params.AddSubnetsParamsV2{
 		Subnets: []params.AddSubnetParamsV2{
 			{