fix: make cidr optional for L2 network creation#289
fix: make cidr optional for L2 network creation#289Damans227 wants to merge 3 commits intoapache:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the cloudstack_network Terraform resource to support creating L2 networks without requiring a CIDR, aligning the provider schema with CloudStack’s createNetwork API requirements.
Changes:
- Made
cidroptional in thecloudstack_networkschema. - Applied gateway/netmask/IP-range parameter logic only when
cidris provided. - Added an acceptance test that creates an L2 network without specifying
cidr.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
cloudstack/resource_cloudstack_network.go |
Makes cidr optional and gates CIDR-derived network parameter setting behind a presence check. |
cloudstack/resource_cloudstack_network_test.go |
Adds an acceptance test for L2 network creation without CIDR. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if _, ok := d.GetOk("cidr"); ok { | ||
| m, err := parseCIDR(d, no.Specifyipranges) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Now that cidr is optional, users can set gateway/startip/endip without setting cidr. In that case this block is skipped and those arguments are silently ignored, which can lead to confusing behavior and perpetual diffs (e.g., config sets gateway but API/state stays empty). Consider validating that gateway, startip, and endip may only be set when cidr is also set (e.g., schema RequiredWith/ConflictsWith or an explicit check in Create returning a clear error).
| if _, ok := d.GetOk("cidr"); ok { | ||
| m, err := parseCIDR(d, no.Specifyipranges) | ||
| if err != nil { |
There was a problem hiding this comment.
With cidr optional, GetNetworkOfferingByID is only needed when cidr is present (to read Specifyipranges for parseCIDR). To avoid an extra API call on L2/no-cidr networks, consider moving the network-offering lookup inside the cidr conditional block.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if err != nil { | ||
| return err | ||
| } | ||
| if _, ok := d.GetOk("cidr"); ok { |
There was a problem hiding this comment.
With cidr now optional, users can set gateway/startip/endip without setting cidr, but this branch will skip parseCIDR and those user-supplied values will be silently ignored (and netmask can’t be derived). Consider validating that cidr must be set whenever any of gateway, startip, or endip is configured (return a clear error), or otherwise handle those fields explicitly when cidr is absent.
| if _, ok := d.GetOk("cidr"); ok { | |
| _, cidrConfigured := d.GetOk("cidr") | |
| if !cidrConfigured { | |
| if _, ok := d.GetOk("gateway"); ok { | |
| return fmt.Errorf("cidr must be specified when gateway is configured") | |
| } | |
| if _, ok := d.GetOk("startip"); ok { | |
| return fmt.Errorf("cidr must be specified when startip is configured") | |
| } | |
| if _, ok := d.GetOk("endip"); ok { | |
| return fmt.Errorf("cidr must be specified when endip is configured") | |
| } | |
| } | |
| if cidrConfigured { |
The
cidrfield was marked asRequiredin the Terraform provider's schema forcloudstack_network, but the CloudStackcreateNetworkAPI only requiresname,networkOfferingId, andzoneId.gatewayandnetmaskare optional and unnecessary for L2 networks.This change makes
cidroptional and conditionally applies the CIDR/gateway/netmask logic only when a CIDR is provided.An acceptance test has been added to verify L2 network creation without CIDR.