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 1 commit
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
43 changes: 41 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,44 @@ 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)
}

func TestBatchCreate(t *testing.T) {
Expand Down
6 changes: 6 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 Down Expand Up @@ -139,6 +140,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 +187,7 @@ type UpdateOptsBuilder interface {
type UpdateOpts struct {
Name string
GatewayIP string
NoGateway bool
DNSNameservers []string
HostRoutes []HostRoute
EnableDHCP *bool
Expand All @@ -201,6 +205,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
121 changes: 121 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,91 @@ 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 TestRequiredCreateOpts(t *testing.T) {
res := Create(fake.ServiceClient(), CreateOpts{})
if res.Err == nil {
Expand Down