Add support for internal LB - #317
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for internal load balancer rules by allowing cloudstack_loadbalancer_rule to be created with only network_id (no ip_address_id / public IP), aligning the Terraform provider behavior with CloudStack’s API semantics.
Changes:
- Make
ip_address_idoptional in the load balancer rule schema and enforce “ip_address_idornetwork_idmust be set” viaverifyLoadBalancerRule. - Omit
publicipidfrom the create request whenip_address_idis not provided. - Add an acceptance test covering the internal (network-only) LB use case.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| cloudstack/resource_cloudstack_loadbalancer_rule.go | Makes ip_address_id optional, conditionally sends/sets it, and validates that either ip_address_id or network_id is provided. |
| cloudstack/resource_cloudstack_loadbalancer_rule_test.go | Adds an acceptance test for internal (network-only) load balancer rules. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| resource.TestCheckResourceAttr( | ||
| "cloudstack_loadbalancer_rule.foo", "ip_address_id", ""), | ||
| resource.TestCheckResourceAttr( |
…r into support-internal-lb
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
cloudstack/resource_cloudstack_loadbalancer_rule_test.go:233
- The internal LB acceptance test currently asserts
ip_address_idis present in state with an empty string. With the updated Read() behavior (only setting ip_address_id when it was configured) and schema Optional (no Computed), the attribute will typically be absent from state, so this check is likely to fail. Prefer asserting the attribute is not set.
resource.TestCheckResourceAttr(
"cloudstack_loadbalancer_rule.foo", "name", "terraform-ilb"),
resource.TestCheckResourceAttr(
"cloudstack_loadbalancer_rule.foo", "ip_address_id", ""),
resource.TestCheckResourceAttr(
cloudstack/resource_cloudstack_loadbalancer_rule.go:62
- Now that both ip_address_id and network_id are Optional, Terraform will allow configs that set neither and only fail later during Create/Update via verifyLoadBalancerRule. You can enforce this at plan time by adding AtLeastOneOf to both schema attributes (keeping verifyLoadBalancerRule as a defensive check if desired).
"ip_address_id": {
Type: schema.TypeString,
Optional: true,
ForceNew: true,
},
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
cloudstack/resource_cloudstack_loadbalancer_rule_test.go:233
- The new internal-LB acceptance test asserts
ip_address_id == "", but the resource now intentionally does not setip_address_idin state when the user omitted it (see the guardedd.Set("ip_address_id", ...)in Read). In that case the attribute will typically be absent from state rather than present-and-empty, so this check is likely to fail.
resource.TestCheckResourceAttr(
"cloudstack_loadbalancer_rule.foo", "name", "terraform-ilb"),
resource.TestCheckResourceAttr(
"cloudstack_loadbalancer_rule.foo", "ip_address_id", ""),
resource.TestCheckResourceAttr(
cloudstack/resource_cloudstack_loadbalancer_rule.go:62
verifyLoadBalancerRulenow enforces that at least one ofip_address_idornetwork_idis set, but that validation only runs during apply (Create/Update). It’s better to enforce this at schema level as well soterraform planfails fast with a config error, similar to other schema-level relationship validations used in the provider (e.g.,RequiredWith).
"ip_address_id": {
Type: schema.TypeString,
Optional: true,
ForceNew: true,
},
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
cloudstack/resource_cloudstack_loadbalancer_test.go:60
- The acceptance test doesn’t assert
networkid/sourceipaddressnetworkid, which are central to the internal LB wiring and (in this PR) to the state-reading fix. AddingTestCheckResourceAttrPairchecks here makes the test actually validate these IDs are persisted correctly.
resource.TestCheckResourceAttr(
"cloudstack_loadbalancer.foo", "name", "terraform-ilb"),
resource.TestCheckResourceAttr(
"cloudstack_loadbalancer.foo", "algorithm", "roundrobin"),
resource.TestCheckResourceAttr(
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
cloudstack/resource_cloudstack_loadbalancer.go:162
d.Set(...)can return an error (e.g., when the schema key is wrong). This resource previously had a typo here ("network_id" vs "networkid"), and because errors are ignored it would have failed silently. Consider checking and returning theseSeterrors to prevent state-refresh bugs from being masked in the future.
d.Set("algorithm", r.Algorithm)
d.Set("name", r.Name)
d.Set("networkid", r.Networkid)
d.Set("sourceipaddressnetworkid", r.Sourceipaddressnetworkid)
No description provided.