Skip to content
This repository has been archived by the owner on Aug 1, 2023. It is now read-only.

[rfr] Allow subnets to have no gateway #553

Merged
merged 2 commits into from
Apr 7, 2016
Merged

[rfr] Allow subnets to have no gateway #553

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f92ae6c
Allow subnets to have no gateway
jtopjian Apr 7, 2016
2524d11
Ensure both GatewayIP and NoGateway cannot be set in subnets
jtopjian Apr 7, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 57 additions & 2 deletions acceptance/openstack/networking/v2/subnet_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import (
th "github.com/rackspace/gophercloud/testhelper"
)

func TestList(t *testing.T) {
func TestSubnetList(t *testing.T) {
Setup(t)
defer Teardown()

Expand All @@ -32,7 +32,7 @@ func TestList(t *testing.T) {
th.CheckNoErr(t, err)
}

func TestCRUD(t *testing.T) {
func TestSubnetCRUD(t *testing.T) {
Setup(t)
defer Teardown()

Expand Down Expand Up @@ -61,6 +61,7 @@ func TestCRUD(t *testing.T) {
th.AssertEquals(t, s.IPVersion, 4)
th.AssertEquals(t, s.Name, "my_subnet")
th.AssertEquals(t, s.EnableDHCP, false)
th.AssertEquals(t, s.GatewayIP, "192.168.199.1")
subnetID := s.ID

// Get subnet
Expand All @@ -79,6 +80,60 @@ func TestCRUD(t *testing.T) {
t.Log("Delete subnet")
res := subnets.Delete(Client, subnetID)
th.AssertNoErr(t, res.Err)

// Create subnet with no gateway
t.Log("Create subnet with no gateway")
opts = subnets.CreateOpts{
NetworkID: networkID,
CIDR: "192.168.199.0/24",
IPVersion: subnets.IPv4,
Name: "my_subnet",
EnableDHCP: &enable,
NoGateway: true,
}
s, err = subnets.Create(Client, opts).Extract()
th.AssertNoErr(t, err)

th.AssertEquals(t, s.NetworkID, networkID)
th.AssertEquals(t, s.CIDR, "192.168.199.0/24")
th.AssertEquals(t, s.IPVersion, 4)
th.AssertEquals(t, s.Name, "my_subnet")
th.AssertEquals(t, s.EnableDHCP, false)
th.AssertEquals(t, s.GatewayIP, "")
subnetID = s.ID

// Get subnet
t.Log("Getting subnet with no gateway")
s, err = subnets.Get(Client, subnetID).Extract()
th.AssertNoErr(t, err)
th.AssertEquals(t, s.ID, subnetID)

// Update subnet
t.Log("Update subnet with no gateway")
s, err = subnets.Update(Client, subnetID, subnets.UpdateOpts{Name: "new_subnet_name"}).Extract()
th.AssertNoErr(t, err)
th.AssertEquals(t, s.Name, "new_subnet_name")

// Delete subnet
t.Log("Delete subnet with no gateway")
res = subnets.Delete(Client, subnetID)
th.AssertNoErr(t, res.Err)

// Create subnet with invalid gateway configuration
t.Log("Create subnet with invalid gateway configuration")
opts = subnets.CreateOpts{
NetworkID: networkID,
CIDR: "192.168.199.0/24",
IPVersion: subnets.IPv4,
Name: "my_subnet",
EnableDHCP: &enable,
NoGateway: true,
GatewayIP: "192.168.199.1",
}
_, err = subnets.Create(Client, opts).Extract()
if err == nil {
t.Fatalf("Expected an error, got none")
}
}

func TestBatchCreate(t *testing.T) {
Expand Down
7 changes: 4 additions & 3 deletions openstack/networking/v2/subnets/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ func err(str string) error {
}

var (
errNetworkIDRequired = err("A network ID is required")
errCIDRRequired = err("A valid CIDR is required")
errInvalidIPType = err("An IP type must either be 4 or 6")
errNetworkIDRequired = err("A network ID is required")
errCIDRRequired = err("A valid CIDR is required")
errInvalidIPType = err("An IP type must either be 4 or 6")
errInvalidGatewayConfig = err("Both disabling the gateway and specifying a gateway is not allowed")
)
16 changes: 16 additions & 0 deletions openstack/networking/v2/subnets/requests.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ type CreateOpts struct {
TenantID string
AllocationPools []AllocationPool
GatewayIP string
NoGateway bool
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be *bool? I don't understand why EnableDHCP is *bool.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The zero-value for bool is false, so there's no way to check if a user set the field to false or just didn't provide it. With a *bool, we know that if the value is true or false it was provided by the user, because otherwise it'd be nil.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. The issue I ran into is that an omission of EnableDHCP was turning DHCP on. I did a scan through the Neutron code and couldn't find where DHCP would be turned on if enable_dhcp was omitted, but it was.

By having the direct value of EnableDHCP passed through, that would have been avoided. See here: https://github.com/hashicorp/terraform/pull/6052/files

So I'm wondering if NoGateway should be bool or *bool. IMO, the former makes sense and is more intuitive, but having a bool and *bool attribute in the struct seems to complicate the structure.

Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the default is reasonable, bool is fine, and we use it elsewhere in the library like that. The Neturon OpenStack docs aren't good, so I'm sure we just didn't know that enabling DHCP was the default. Otherwise we probably would've named it DisableDHCP and made it a bool.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I'll keep NoGateway at bool. I think the zero-value shouldn't cause any issues with how this is implemented.

Re DHCP: I had no idea, either. I was just inquiring to see if I was overlooking something, there was past history, if I was going crazy, etc 😄

IPVersion int
EnableDHCP *bool
DNSNameservers []string
Expand All @@ -128,6 +129,11 @@ func (opts CreateOpts) ToSubnetCreateMap() (map[string]interface{}, error) {
return nil, errInvalidIPType
}

// Both GatewayIP and NoGateway should not be set
if opts.GatewayIP != "" && opts.NoGateway {
return nil, errInvalidGatewayConfig
}

s["network_id"] = opts.NetworkID
s["cidr"] = opts.CIDR

Expand All @@ -139,6 +145,8 @@ func (opts CreateOpts) ToSubnetCreateMap() (map[string]interface{}, error) {
}
if opts.GatewayIP != "" {
s["gateway_ip"] = opts.GatewayIP
} else if opts.NoGateway {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be an error to provide NoGateway == true and GatewayIP != "". We should probably check for that at the beginning of this if block. Also the one in Update.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you're right. I was taking the cheap way out by thinking if a user has both set, they have more problems to worry about 😉

s["gateway_ip"] = nil
}
if opts.TenantID != "" {
s["tenant_id"] = opts.TenantID
Expand Down Expand Up @@ -184,6 +192,7 @@ type UpdateOptsBuilder interface {
type UpdateOpts struct {
Name string
GatewayIP string
NoGateway bool
DNSNameservers []string
HostRoutes []HostRoute
EnableDHCP *bool
Expand All @@ -193,6 +202,11 @@ type UpdateOpts struct {
func (opts UpdateOpts) ToSubnetUpdateMap() (map[string]interface{}, error) {
s := make(map[string]interface{})

// Both GatewayIP and NoGateway should not be set
if opts.GatewayIP != "" && opts.NoGateway {
return nil, errInvalidGatewayConfig
}

if opts.EnableDHCP != nil {
s["enable_dhcp"] = &opts.EnableDHCP
}
Expand All @@ -201,6 +215,8 @@ func (opts UpdateOpts) ToSubnetUpdateMap() (map[string]interface{}, error) {
}
if opts.GatewayIP != "" {
s["gateway_ip"] = opts.GatewayIP
} else if opts.NoGateway {
s["gateway_ip"] = nil
}
if opts.DNSNameservers != nil {
s["dns_nameservers"] = opts.DNSNameservers
Expand Down
175 changes: 175 additions & 0 deletions openstack/networking/v2/subnets/requests_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,24 @@ func TestList(t *testing.T) {
"gateway_ip": "192.0.0.1",
"cidr": "192.0.0.0/8",
"id": "54d6f61d-db07-451c-9ab3-b9609b6b6f0b"
},
{
"name": "my_gatewayless_subnet",
"enable_dhcp": true,
"network_id": "d32019d3-bc6e-4319-9c1d-6722fc136a23",
"tenant_id": "4fd44f30292945e481c7b8a0c8908869",
"dns_nameservers": [],
"allocation_pools": [
{
"start": "192.168.1.2",
"end": "192.168.1.254"
}
],
"host_routes": [],
"ip_version": 4,
"gateway_ip": null,
"cidr": "192.168.1.0/24",
"id": "54d6f61d-db07-451c-9ab3-b9609b6b6f0c"
}
]
}
Expand Down Expand Up @@ -112,6 +130,24 @@ func TestList(t *testing.T) {
CIDR: "192.0.0.0/8",
ID: "54d6f61d-db07-451c-9ab3-b9609b6b6f0b",
},
Subnet{
Name: "my_gatewayless_subnet",
EnableDHCP: true,
NetworkID: "d32019d3-bc6e-4319-9c1d-6722fc136a23",
TenantID: "4fd44f30292945e481c7b8a0c8908869",
DNSNameservers: []string{},
AllocationPools: []AllocationPool{
AllocationPool{
Start: "192.168.1.2",
End: "192.168.1.254",
},
},
HostRoutes: []HostRoute{},
IPVersion: 4,
GatewayIP: "",
CIDR: "192.168.1.0/24",
ID: "54d6f61d-db07-451c-9ab3-b9609b6b6f0c",
},
}

th.CheckDeepEquals(t, expected, actual)
Expand Down Expand Up @@ -270,6 +306,145 @@ func TestCreate(t *testing.T) {
th.AssertEquals(t, s.ID, "3b80198d-4f7b-4f77-9ef5-774d54e17126")
}

func TestCreateNoGateway(t *testing.T) {
th.SetupHTTP()
defer th.TeardownHTTP()

th.Mux.HandleFunc("/v2.0/subnets", func(w http.ResponseWriter, r *http.Request) {
th.TestMethod(t, r, "POST")
th.TestHeader(t, r, "X-Auth-Token", fake.TokenID)
th.TestHeader(t, r, "Content-Type", "application/json")
th.TestHeader(t, r, "Accept", "application/json")
th.TestJSONRequest(t, r, `
{
"subnet": {
"network_id": "d32019d3-bc6e-4319-9c1d-6722fc136a23",
"ip_version": 4,
"cidr": "192.168.1.0/24",
"gateway_ip": null,
"allocation_pools": [
{
"start": "192.168.1.2",
"end": "192.168.1.254"
}
]
}
}
`)

w.Header().Add("Content-Type", "application/json")
w.WriteHeader(http.StatusCreated)

fmt.Fprintf(w, `
{
"subnet": {
"name": "",
"enable_dhcp": true,
"network_id": "d32019d3-bc6e-4319-9c1d-6722fc136a23",
"tenant_id": "4fd44f30292945e481c7b8a0c8908869",
"allocation_pools": [
{
"start": "192.168.1.2",
"end": "192.168.1.254"
}
],
"host_routes": [],
"ip_version": 4,
"gateway_ip": null,
"cidr": "192.168.1.0/24",
"id": "54d6f61d-db07-451c-9ab3-b9609b6b6f0c"
}
}
`)
})

opts := CreateOpts{
NetworkID: "d32019d3-bc6e-4319-9c1d-6722fc136a23",
IPVersion: 4,
CIDR: "192.168.1.0/24",
NoGateway: true,
AllocationPools: []AllocationPool{
AllocationPool{
Start: "192.168.1.2",
End: "192.168.1.254",
},
},
DNSNameservers: []string{},
}
s, err := Create(fake.ServiceClient(), opts).Extract()
th.AssertNoErr(t, err)

th.AssertEquals(t, s.Name, "")
th.AssertEquals(t, s.EnableDHCP, true)
th.AssertEquals(t, s.NetworkID, "d32019d3-bc6e-4319-9c1d-6722fc136a23")
th.AssertEquals(t, s.TenantID, "4fd44f30292945e481c7b8a0c8908869")
th.AssertDeepEquals(t, s.AllocationPools, []AllocationPool{
AllocationPool{
Start: "192.168.1.2",
End: "192.168.1.254",
},
})
th.AssertDeepEquals(t, s.HostRoutes, []HostRoute{})
th.AssertEquals(t, s.IPVersion, 4)
th.AssertEquals(t, s.GatewayIP, "")
th.AssertEquals(t, s.CIDR, "192.168.1.0/24")
th.AssertEquals(t, s.ID, "54d6f61d-db07-451c-9ab3-b9609b6b6f0c")
}

func TestCreateInvalidGatewayConfig(t *testing.T) {
th.SetupHTTP()
defer th.TeardownHTTP()

th.Mux.HandleFunc("/v2.0/subnets", func(w http.ResponseWriter, r *http.Request) {
th.TestMethod(t, r, "POST")
th.TestHeader(t, r, "X-Auth-Token", fake.TokenID)
th.TestHeader(t, r, "Content-Type", "application/json")
th.TestHeader(t, r, "Accept", "application/json")
th.TestJSONRequest(t, r, `
{
"subnet": {
"network_id": "d32019d3-bc6e-4319-9c1d-6722fc136a23",
"ip_version": 4,
"cidr": "192.168.1.0/24",
"gateway_ip": "192.168.1.1",
"allocation_pools": [
{
"start": "192.168.1.2",
"end": "192.168.1.254"
}
]
}
}
`)

w.Header().Add("Content-Type", "application/json")
w.WriteHeader(http.StatusCreated)
})

opts := CreateOpts{
NetworkID: "d32019d3-bc6e-4319-9c1d-6722fc136a23",
IPVersion: 4,
CIDR: "192.168.1.0/24",
NoGateway: true,
GatewayIP: "192.168.1.1",
AllocationPools: []AllocationPool{
AllocationPool{
Start: "192.168.1.2",
End: "192.168.1.254",
},
},
DNSNameservers: []string{},
}
_, err := Create(fake.ServiceClient(), opts).Extract()
if err == nil {
t.Fatalf("Expected an error, got none")
}

if err != errInvalidGatewayConfig {
t.Fatalf("Exected errInvalidGateway but got: %s", err)
}
}

func TestRequiredCreateOpts(t *testing.T) {
res := Create(fake.ServiceClient(), CreateOpts{})
if res.Err == nil {
Expand Down