Skip to content

Commit

Permalink
Implement DiscoverSpaces.AddSubnets API method
Browse files Browse the repository at this point in the history 
Implemented the method standalone rather than using
networkingcommon.AddSubnets. That has far more cleverness than is needed
for this case and it overcomplicated how this worker behaves in tests.
  • Loading branch information
@babbageclunk
babbageclunk committed Mar 14, 2017
1 parent 1c547b2 commit 8e87c5b
Show file tree
Hide file tree
Showing 3 changed files with 212 additions and 20 deletions.
2 changes: 1 addition & 1 deletion apiserver/common/networkingcommon/shims.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
84 changes: 69 additions & 15 deletions apiserver/discoverspaces/discoverspaces.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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)
Expand Down Expand Up @@ -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)
}
146 changes: 142 additions & 4 deletions apiserver/discoverspaces/discoverspaces_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -23,7 +23,7 @@ type DiscoverSpacesSuite struct {

resources *common.Resources
authorizer apiservertesting.FakeAuthorizer
facade *discoverspaces.DiscoverSpacesAPI
facade *discoverspaces.API
}

var _ = gc.Suite(&DiscoverSpacesSuite{})
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
}

0 comments on commit 8e87c5b

Please sign in to comment.