[rfr] Allow subnets to have no gateway #553
Conversation
| @@ -108,6 +108,7 @@ type CreateOpts struct { | |||
| TenantID string | |||
| AllocationPools []AllocationPool | |||
| GatewayIP string | |||
| NoGateway bool | |||
There was a problem hiding this comment.
Should this be *bool? I don't understand why EnableDHCP is *bool.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😄
| @@ -139,6 +140,8 @@ func (opts CreateOpts) ToSubnetCreateMap() (map[string]interface{}, error) { | |||
| } | |||
| if opts.GatewayIP != "" { | |||
| s["gateway_ip"] = opts.GatewayIP | |||
| } else if opts.NoGateway { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😉
|
@jrperritt Invalid gateway config checks added. |
|
@jrperritt err.. just noticed my commit message was incorrect ( |
|
+2 |
|
Thank you! |
If the
gateway_ipparameter is omitted, Neutron assigns a default gateway of.1. However, ifgateway_ipexists, but is null, then no gateway is assigned.This patch is trying to emulate the
--no-gatewaycommand-line option of thepython-neutronclientapp:https://github.com/openstack/python-neutronclient/blob/master/neutronclient/neutron/v2_0/subnet.py#L120-L123