Copilot commented on code in PR #282:
URL:
https://github.com/apache/cloudstack-terraform-provider/pull/282#discussion_r3240432639
##########
cloudstack/resource_cloudstack_network.go:
##########
@@ -306,6 +378,13 @@ func resourceCloudStackNetworkRead(d *schema.ResourceData,
meta interface{}) err
d.Set("network_domain", n.Networkdomain)
d.Set("vpc_id", n.Vpcid)
+ // Always set IPv6 fields to detect drift when IPv6 is removed
server-side
+ d.Set("ip6cidr", n.Ip6cidr)
+ d.Set("ip6gateway", n.Ip6gateway)
+
+ // Note: CloudStack API may not return startipv6 and endipv6 fields
+ // These are typically only set during network creation
+
Review Comment:
The Read function only refreshes `ip6cidr` and `ip6gateway` from the API,
but never refreshes `startipv6` and `endipv6`. The inline comment acknowledges
this, but the consequence is that drift in the IPv6 IP range will go
undetected, and any out-of-band changes (or differences between user-supplied
values and what CloudStack actually stored) will never be reconciled into
state. Since these schema fields are `Computed: true`, Terraform also has no
way to populate them on initial import. Consider attempting to read the
network's IPv6 range (e.g., via `listVlanIpRanges` / `listIpv6FirewallRules` or
whichever CloudStack API surfaces it) and setting these fields, or at minimum
documenting that drift detection is intentionally not supported.
##########
cloudstack/resource_cloudstack_network_test.go:
##########
@@ -165,6 +175,90 @@ func TestAccCloudStackNetwork_importProject(t *testing.T) {
})
}
+// testAccPreCheckIPv6Support checks if IPv6 tests should run.
+// IPv6 tests are skipped on the CloudStack simulator unless explicitly enabled
+// via the CLOUDSTACK_ENABLE_IPV6_TESTS environment variable.
+func testAccPreCheckIPv6Support(t *testing.T) {
+ testAccPreCheck(t)
+
+ // Allow explicit override to enable IPv6 tests
+ if os.Getenv("CLOUDSTACK_ENABLE_IPV6_TESTS") == "true" {
+ return
+ }
+
+ // Try to detect if we're running on the simulator by checking the API
URL
+ apiURL := os.Getenv("CLOUDSTACK_API_URL")
+ if strings.Contains(apiURL, "localhost") || strings.Contains(apiURL,
"127.0.0.1") {
+ t.Skip("Skipping IPv6 test: CloudStack simulator does not
support IPv6 for isolated networks. Set CLOUDSTACK_ENABLE_IPV6_TESTS=true to
force-enable.")
+ }
Review Comment:
The simulator detection here is fragile: it relies on the literal substrings
`localhost` or `127.0.0.1` appearing in `CLOUDSTACK_API_URL`. Real
(non-simulator) CloudStack deployments accessed via an SSH tunnel, a reverse
proxy, or any other `localhost`/`127.0.0.1` URL will incorrectly skip these
tests, and conversely a simulator reachable via a hostname or different
loopback (e.g. `::1`) will run them and likely fail. Consider gating on an
explicit opt-in env var (`CLOUDSTACK_ENABLE_IPV6_TESTS`) without the URL-based
heuristic, or by querying CloudStack for IPv6 capability/zone support before
running the test.
##########
cloudstack/resource_cloudstack_network.go:
##########
@@ -471,3 +550,93 @@ func parseCIDR(d *schema.ResourceData, specifyiprange
bool) (map[string]string,
return m, nil
}
+
+// addToIPv6 adds an integer offset to an IPv6 address with proper carry
across all bytes.
+// Returns a new net.IP with the result.
+func addToIPv6(ip net.IP, offset uint64) net.IP {
+ result := make(net.IP, len(ip))
+ copy(result, ip)
+
+ carry := offset
+ // Start from the least significant byte (rightmost) and work backwards
+ for i := len(result) - 1; i >= 0 && carry > 0; i-- {
+ sum := uint64(result[i]) + carry
+ result[i] = byte(sum & 0xff)
+ carry = sum >> 8
+ }
+
+ return result
+}
+
+func parseCIDRv6(d *schema.ResourceData, specifyiprange bool)
(map[string]string, error) {
+ m := make(map[string]string, 4)
+
+ cidr := d.Get("ip6cidr").(string)
+ ip, ipnet, err := net.ParseCIDR(cidr)
+ if err != nil {
+ return nil, fmt.Errorf("Unable to parse cidr %s: %s", cidr, err)
+ }
+
+ // Validate that this is actually an IPv6 CIDR
+ if ip.To4() != nil {
+ return nil, fmt.Errorf("ip6cidr must be an IPv6 CIDR, got IPv4:
%s", cidr)
+ }
+ if len(ipnet.Mask) != net.IPv6len {
+ return nil, fmt.Errorf("ip6cidr must be an IPv6 CIDR with
16-byte mask, got %d bytes: %s", len(ipnet.Mask), cidr)
+ }
+
+ // Validate prefix length to ensure we have enough addresses for
gateway/start/end
+ ones, _ := ipnet.Mask.Size()
+ if specifyiprange {
+ // When specifyiprange is true, we need at least 3 addresses:
+ // - gateway (network + 1)
+ // - start IP (network + 2)
+ // - end IP (network + 3 or more)
+ // This requires a /126 or larger prefix (4 addresses minimum)
+ if ones > 126 {
+ return nil, fmt.Errorf("ip6cidr prefix /%d is too small
for automatic IP range generation; minimum is /126 (4 addresses)", ones)
+ }
+ } else {
+ // When specifyiprange is false, we only need the gateway
(network + 1)
+ // This requires a /127 or larger prefix (2 addresses minimum)
+ if ones > 127 {
+ return nil, fmt.Errorf("ip6cidr prefix /%d is too small
for automatic gateway generation; minimum is /127 (2 addresses)", ones)
+ }
+ }
+
+ if gateway, ok := d.GetOk("ip6gateway"); ok {
+ m["ip6gateway"] = gateway.(string)
+ } else {
+ // Default gateway to network address + 1 (e.g., 2001:db8::1)
+ gwip := addToIPv6(ipnet.IP, 1)
+ m["ip6gateway"] = gwip.String()
+ }
+
+ if startipv6, ok := d.GetOk("startipv6"); ok {
+ m["startipv6"] = startipv6.(string)
+ } else if specifyiprange {
+ // Default start IP to network address + 2
+ startip := addToIPv6(ipnet.IP, 2)
+ m["startipv6"] = startip.String()
+ }
+
+ if endip, ok := d.GetOk("endipv6"); ok {
+ m["endipv6"] = endip.(string)
Review Comment:
User-supplied `ip6gateway`, `startipv6`, and `endipv6` are passed through to
the CloudStack API without verifying that they fall within the `ip6cidr`
subnet. If a user provides values outside the CIDR (e.g., a typo or wrong
prefix), the failure will only surface as an opaque CloudStack API error.
Consider validating with `ipnet.Contains(net.ParseIP(...))` and returning a
clear local error before invoking the API, matching the spirit of the new
prefix-length validation above.
##########
website/docs/r/network.html.markdown:
##########
@@ -43,6 +55,19 @@ The following arguments are supported:
* `endip` - (Optional) End of the IP block that will be available on the
network. Defaults to the last available IP in the range.
+* `ip6cidr` - (Optional) The IPv6 CIDR block for the network. Changing this
+ forces a new resource to be created.
Review Comment:
The new prefix-length validation (`/127` minimum without IP range, `/126`
minimum with IP range) and the rejection of IPv4 CIDRs are not mentioned in the
user-facing documentation for `ip6cidr`. Users who pass a `/128` or an IPv4
string will see only a local error message at apply time with no documented
constraint. Consider documenting the allowed prefix range and that the value
must be an IPv6 CIDR.
##########
website/docs/r/network.html.markdown:
##########
@@ -43,6 +55,19 @@ The following arguments are supported:
* `endip` - (Optional) End of the IP block that will be available on the
network. Defaults to the last available IP in the range.
+* `ip6cidr` - (Optional) The IPv6 CIDR block for the network. Changing this
+ forces a new resource to be created.
+
+* `ip6gateway` - (Optional) IPv6 Gateway that will be provided to the instances
+ in this network. Defaults to the second address in the subnet (network
address + 1,
+ e.g., 2001:db8::1 for 2001:db8::/64).
+
+* `startipv6` - (Optional) Start of the IPv6 block that will be available on
the
+ network. Defaults to the second available IP in the range.
+
+* `endipv6` - (Optional) End of the IPv6 block that will be available on the
+ network. Defaults to the last available IP in the range.
Review Comment:
The documentation for `startipv6` and `endipv6` states they "Default to the
second/last available IP in the range," but the implementation only assigns
these defaults when the network offering has `specifyipranges` enabled.
Otherwise, no default is computed and these attributes are simply not sent to
the API (and will be empty in state). Please clarify in the docs that the
default is conditional on the network offering's `specifyipranges` setting,
similar to the behavior of `startip`/`endip`.
##########
cloudstack/resource_cloudstack_network_unit_test.go:
##########
@@ -0,0 +1,321 @@
+//
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+//
+
+package cloudstack
+
+import (
+ "strings"
+ "testing"
+
+ "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
+)
+
+func TestParseCIDRv6_DefaultGateway(t *testing.T) {
+ d := schema.TestResourceDataRaw(t, resourceCloudStackNetwork().Schema,
map[string]interface{}{
+ "ip6cidr": "2001:db8::/64",
+ })
+
+ result, err := parseCIDRv6(d, false)
+ if err != nil {
+ t.Fatalf("parseCIDRv6 failed: %v", err)
+ }
+
+ // Default gateway should be network address + 1
+ expectedGateway := "2001:db8::1"
+ if result["ip6gateway"] != expectedGateway {
+ t.Errorf("Expected gateway %s, got %s", expectedGateway,
result["ip6gateway"])
+ }
+
+ // When specifyiprange is false, startipv6 and endipv6 should not be set
+ if _, ok := result["startipv6"]; ok {
+ t.Errorf("startipv6 should not be set when specifyiprange is
false")
+ }
+ if _, ok := result["endipv6"]; ok {
+ t.Errorf("endipv6 should not be set when specifyiprange is
false")
+ }
+}
+
+func TestParseCIDRv6_CustomGateway(t *testing.T) {
+ d := schema.TestResourceDataRaw(t, resourceCloudStackNetwork().Schema,
map[string]interface{}{
+ "ip6cidr": "2001:db8::/64",
+ "ip6gateway": "2001:db8::1",
+ })
+
+ result, err := parseCIDRv6(d, false)
+ if err != nil {
+ t.Fatalf("parseCIDRv6 failed: %v", err)
+ }
+
+ expectedGateway := "2001:db8::1"
+ if result["ip6gateway"] != expectedGateway {
+ t.Errorf("Expected gateway %s, got %s", expectedGateway,
result["ip6gateway"])
+ }
+}
+
+func TestParseCIDRv6_WithIPRange(t *testing.T) {
+ d := schema.TestResourceDataRaw(t, resourceCloudStackNetwork().Schema,
map[string]interface{}{
+ "ip6cidr": "2001:db8::/64",
+ })
+
+ result, err := parseCIDRv6(d, true)
+ if err != nil {
+ t.Fatalf("parseCIDRv6 failed: %v", err)
+ }
+
+ // Check gateway (should be network address + 1)
+ expectedGateway := "2001:db8::1"
+ if result["ip6gateway"] != expectedGateway {
+ t.Errorf("Expected gateway %s, got %s", expectedGateway,
result["ip6gateway"])
+ }
+
+ // Check start IP (should be network address + 2)
+ expectedStartIP := "2001:db8::2"
+ if result["startipv6"] != expectedStartIP {
+ t.Errorf("Expected start IP %s, got %s", expectedStartIP,
result["startipv6"])
+ }
+
+ // Check end IP (should be the last address in the /64 range)
+ expectedEndIP := "2001:db8::ffff:ffff:ffff:ffff"
+ if result["endipv6"] != expectedEndIP {
+ t.Errorf("Expected end IP %s, got %s", expectedEndIP,
result["endipv6"])
+ }
+}
+
+func TestParseCIDRv6_CustomIPRange(t *testing.T) {
+ d := schema.TestResourceDataRaw(t, resourceCloudStackNetwork().Schema,
map[string]interface{}{
+ "ip6cidr": "2001:db8:1::/64",
+ "startipv6": "2001:db8:1::100",
+ "endipv6": "2001:db8:1::200",
+ })
+
+ result, err := parseCIDRv6(d, true)
+ if err != nil {
+ t.Fatalf("parseCIDRv6 failed: %v", err)
+ }
+
+ // Check that custom values are used
+ if result["startipv6"] != "2001:db8:1::100" {
+ t.Errorf("Expected custom start IP 2001:db8:1::100, got %s",
result["startipv6"])
+ }
+ if result["endipv6"] != "2001:db8:1::200" {
+ t.Errorf("Expected custom end IP 2001:db8:1::200, got %s",
result["endipv6"])
+ }
+}
+
+func TestParseCIDRv6_SmallerPrefix(t *testing.T) {
+ d := schema.TestResourceDataRaw(t, resourceCloudStackNetwork().Schema,
map[string]interface{}{
+ "ip6cidr": "2001:db8::/48",
+ })
+
+ result, err := parseCIDRv6(d, true)
+ if err != nil {
+ t.Fatalf("parseCIDRv6 failed: %v", err)
+ }
+
+ // For a /48, the end IP should have the last 80 bits set to 1
+ expectedEndIP := "2001:db8:0:ffff:ffff:ffff:ffff:ffff"
+ if result["endipv6"] != expectedEndIP {
+ t.Errorf("Expected end IP %s, got %s", expectedEndIP,
result["endipv6"])
+ }
+}
+
+func TestParseCIDRv6_RejectsIPv4(t *testing.T) {
+ d := schema.TestResourceDataRaw(t, resourceCloudStackNetwork().Schema,
map[string]interface{}{
+ "ip6cidr": "10.0.0.0/24",
+ })
+
+ _, err := parseCIDRv6(d, false)
+ if err == nil {
+ t.Fatal("parseCIDRv6 should reject IPv4 CIDR")
+ }
+
+ expectedError := "ip6cidr must be an IPv6 CIDR, got IPv4"
+ if !strings.HasPrefix(err.Error(), expectedError) {
+ t.Errorf("Expected error message to start with '%s', got '%s'",
expectedError, err.Error())
+ }
+}
+
+func TestParseCIDRv6_Prefix128_NoIPRange(t *testing.T) {
+ // /128 is a single address - should fail even without IP range
+ d := schema.TestResourceDataRaw(t, resourceCloudStackNetwork().Schema,
map[string]interface{}{
+ "ip6cidr": "2001:db8::1/128",
+ })
+
+ _, err := parseCIDRv6(d, false)
+ if err == nil {
+ t.Fatal("parseCIDRv6 should reject /128 prefix (single
address)")
+ }
+
+ expectedError := "ip6cidr prefix /128 is too small"
+ if !strings.HasPrefix(err.Error(), expectedError) {
+ t.Errorf("Expected error message to start with '%s', got '%s'",
expectedError, err.Error())
+ }
+}
+
+func TestParseCIDRv6_Prefix127_NoIPRange(t *testing.T) {
+ // /127 has 2 addresses - should work without IP range (only needs
gateway)
+ d := schema.TestResourceDataRaw(t, resourceCloudStackNetwork().Schema,
map[string]interface{}{
+ "ip6cidr": "2001:db8::/127",
+ })
+
+ result, err := parseCIDRv6(d, false)
+ if err != nil {
+ t.Fatalf("parseCIDRv6 should accept /127 prefix without IP
range: %v", err)
+ }
+
+ // Should have gateway
+ if _, ok := result["ip6gateway"]; !ok {
+ t.Error("Expected ip6gateway to be set")
+ }
+
+ // Should not have start/end IP
+ if _, ok := result["startipv6"]; ok {
+ t.Error("startipv6 should not be set when specifyiprange is
false")
+ }
+}
+
+func TestParseCIDRv6_Prefix127_WithIPRange(t *testing.T) {
+ // /127 has only 2 addresses - should fail with IP range (needs 3+
addresses)
+ d := schema.TestResourceDataRaw(t, resourceCloudStackNetwork().Schema,
map[string]interface{}{
+ "ip6cidr": "2001:db8::/127",
+ })
+
+ _, err := parseCIDRv6(d, true)
+ if err == nil {
+ t.Fatal("parseCIDRv6 should reject /127 prefix with IP range
(only 2 addresses)")
+ }
+
+ expectedError := "ip6cidr prefix /127 is too small for automatic IP
range generation"
+ if !strings.HasPrefix(err.Error(), expectedError) {
+ t.Errorf("Expected error message to start with '%s', got '%s'",
expectedError, err.Error())
+ }
+}
+
+func TestParseCIDRv6_Prefix126_WithIPRange(t *testing.T) {
+ // /126 has 4 addresses - should work with IP range
+ d := schema.TestResourceDataRaw(t, resourceCloudStackNetwork().Schema,
map[string]interface{}{
+ "ip6cidr": "2001:db8::/126",
+ })
+
+ result, err := parseCIDRv6(d, true)
+ if err != nil {
+ t.Fatalf("parseCIDRv6 should accept /126 prefix with IP range:
%v", err)
+ }
+
+ // Should have gateway, start, and end
+ if _, ok := result["ip6gateway"]; !ok {
+ t.Error("Expected ip6gateway to be set")
+ }
+ if _, ok := result["startipv6"]; !ok {
+ t.Error("Expected startipv6 to be set")
+ }
+ if _, ok := result["endipv6"]; !ok {
+ t.Error("Expected endipv6 to be set")
+ }
+
+ // Verify the end IP is correct for /126 (last 2 bits set to 1)
+ expectedEndIP := "2001:db8::3"
+ if result["endipv6"] != expectedEndIP {
+ t.Errorf("Expected end IP %s for /126, got %s", expectedEndIP,
result["endipv6"])
+ }
+}
+
+func TestParseCIDRv6_NonZeroNetworkAddress(t *testing.T) {
+ // Test with a CIDR where the network address doesn't end in ::0
+ // This tests the fix for proper IPv6 address arithmetic with carry
+ d := schema.TestResourceDataRaw(t, resourceCloudStackNetwork().Schema,
map[string]interface{}{
+ "ip6cidr": "2001:db8::4/126",
+ })
+
+ result, err := parseCIDRv6(d, true)
+ if err != nil {
+ t.Fatalf("parseCIDRv6 failed: %v", err)
+ }
+
+ // For 2001:db8::4/126, the network is 2001:db8::4
+ // Gateway should be network + 1 = 2001:db8::5
+ expectedGateway := "2001:db8::5"
+ if result["ip6gateway"] != expectedGateway {
+ t.Errorf("Expected gateway %s, got %s", expectedGateway,
result["ip6gateway"])
+ }
+
+ // Start IP should be network + 2 = 2001:db8::6
+ expectedStartIP := "2001:db8::6"
+ if result["startipv6"] != expectedStartIP {
+ t.Errorf("Expected start IP %s, got %s", expectedStartIP,
result["startipv6"])
+ }
+
+ // End IP should be network + 3 = 2001:db8::7 (last address in /126)
+ expectedEndIP := "2001:db8::7"
+ if result["endipv6"] != expectedEndIP {
+ t.Errorf("Expected end IP %s, got %s", expectedEndIP,
result["endipv6"])
+ }
+}
+
+func TestParseCIDRv6_NonAlignedPrefix(t *testing.T) {
+ // Test with a /124 prefix where network address has non-zero low-order
bits
+ d := schema.TestResourceDataRaw(t, resourceCloudStackNetwork().Schema,
map[string]interface{}{
+ "ip6cidr": "2001:db8::f0/124",
+ })
+
+ result, err := parseCIDRv6(d, true)
+ if err != nil {
+ t.Fatalf("parseCIDRv6 failed: %v", err)
+ }
+
+ // For 2001:db8::f0/124, the network is 2001:db8::f0
+ // Gateway should be network + 1 = 2001:db8::f1
+ expectedGateway := "2001:db8::f1"
+ if result["ip6gateway"] != expectedGateway {
+ t.Errorf("Expected gateway %s, got %s", expectedGateway,
result["ip6gateway"])
+ }
+
+ // Start IP should be network + 2 = 2001:db8::f2
+ expectedStartIP := "2001:db8::f2"
+ if result["startipv6"] != expectedStartIP {
+ t.Errorf("Expected start IP %s, got %s", expectedStartIP,
result["startipv6"])
+ }
+
+ // End IP should be 2001:db8::ff (last address in /124)
+ expectedEndIP := "2001:db8::ff"
+ if result["endipv6"] != expectedEndIP {
+ t.Errorf("Expected end IP %s, got %s", expectedEndIP,
result["endipv6"])
+ }
+}
+
+func TestParseCIDRv6_CarryAcrossBytes(t *testing.T) {
+ // Test carry across byte boundaries
+ // Use a network address ending in ::ff to test carry to next byte
+ d := schema.TestResourceDataRaw(t, resourceCloudStackNetwork().Schema,
map[string]interface{}{
+ "ip6cidr": "2001:db8::ff/120",
+ })
+
+ result, err := parseCIDRv6(d, true)
+ if err != nil {
+ t.Fatalf("parseCIDRv6 failed: %v", err)
+ }
+
+ // For 2001:db8::ff/120, network is 2001:db8::0 (masked)
+ // But let's test with an address that will actually carry
+ // Gateway should be network + 1
+ expectedGateway := "2001:db8::1"
+ if result["ip6gateway"] != expectedGateway {
+ t.Errorf("Expected gateway %s, got %s", expectedGateway,
result["ip6gateway"])
+ }
+}
Review Comment:
This test name and comment describe testing "carry across byte boundaries,"
but the CIDR `2001:db8::ff/120` is masked to network `2001:db8::00`, so the
resulting `gateway = network + 1 = ::1` never exercises any carry. The body
comment even admits this ("But let's test with an address that will actually
carry"). To actually verify `addToIPv6`'s carry behavior, use a CIDR whose
masked network address ends in a byte boundary that causes a real carry when
adding 1 or 2 (e.g., a `/120` whose unmasked input wouldn't help — instead
construct a unit test that calls `addToIPv6` directly with an address ending in
`...:ff` and offset 1, or use a CIDR like `2001:db8::ff00/120` if testing
through the wrapper).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
