Skip to content

Add cidr_list attribute to nsxt_policy_ip_block #1611

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add cidr_list attribute to nsxt_policy_ip_block #1611

wants to merge 2 commits into from

Conversation

ksamoray
Copy link
Contributor

This attribute is added for NSX v9.1.0.

annakhm
annakhm reviewed Apr 17, 2025
View reviewed changes
nsxt/resource_nsxt_policy_ip_block.go
@@ -55,6 +55,15 @@ func resourceNsxtPolicyIPBlock() *schema.Resource {
Optional: true,
ValidateFunc: validation.StringInSlice(visibilityTypes, false),
},
"cidr_list": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two additional new attributes - RangeList and ReservedIps. Not sure about SyncRealization one (its not new)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. We don't have to incldue sync_realization in the Terraform provider.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have to include sync_realization but maybe it's worthwhile to turn it on to save users some realization issues?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understood from the docs it is auto-assigned to true, but only in some cases.. not sure why

annakhm
annakhm reviewed Apr 17, 2025
View reviewed changes
nsxt/resource_nsxt_policy_ip_block.go
Elem: &schema.Schema{
Type: schema.TypeString,
ValidateFunc: validateCidr(),
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add ConflictsWIth with cidr

salv-orlando
salv-orlando reviewed Apr 18, 2025
View reviewed changes
nsxt/resource_nsxt_policy_edge_cluster.go
@@ -376,7 +376,7 @@ func resourceNsxtPolicyEdgeClusterDelete(d *schema.ResourceData, m interface{})

connector := getPolicyConnector(m)
client := enforcement_points.NewEdgeClustersClient(connector)
err = client.Delete(siteID, epID, id)
err = client.Delete(siteID, epID, id, nil)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you making this change? Is it due to SDK update? (If so, what should the 4th argument be)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is derived from PR #1610, and its:

deleteMemberEdgeNodesParam Flag to specify whether to delete edge transport nodes within edge cluster. (optional, default to false)

nsxt/resource_nsxt_policy_ip_block.go
@@ -46,7 +46,7 @@ func resourceNsxtPolicyIPBlock() *schema.Resource {
"cidr": {
Type: schema.TypeString,
Description: "Network address and the prefix length which will be associated with a layer-2 broadcast domain",
Required: true,
Optional: true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cidr should also be deprecated

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We mark it as deprecated even though it's the only option for v9.0 and below?

nsxt/resource_nsxt_policy_ip_block.go
@@ -55,6 +55,15 @@ func resourceNsxtPolicyIPBlock() *schema.Resource {
Optional: true,
ValidateFunc: validation.StringInSlice(visibilityTypes, false),
},
"cidr_list": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. We don't have to incldue sync_realization in the Terraform provider.

@ksamoray ksamoray force-pushed the ip-block-9.1 branch 4 times, most recently from 495f16a to 83b84df Compare April 21, 2025 09:44
annakhm
annakhm reviewed Apr 22, 2025
View reviewed changes
website/docs/r/policy_ip_block.html.markdown Outdated
* `cidr` - (Required) Network address and the prefix length which will be associated with a layer-2 broadcast domain.
* `cidr` - (Optional) Network address and the prefix length which will be associated with a layer-2 broadcast domain.
* `cidr_list` - (Optional) Array of contiguous IP address spaces represented by network address and prefix length. This attribute is supported with NSX 9.1.0 onwards.
* `range_list` - (Optional) Represents list of IP address ranges in the form of start and end IPs.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two attributes should also be marked as supported with NSX 9.1.0 onwards

annakhm
annakhm reviewed Apr 22, 2025
View reviewed changes
nsxt/resource_nsxt_policy_ip_block_test.go
@@ -42,6 +42,40 @@ func TestAccResourceNsxtPolicyIPBlock_minimal(t *testing.T) {
})
}

func TestAccResourceNsxtPolicyIPBlock_v910(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it make sense to add a test that switches from cidr to cidr_list with an update?

@ksamoray ksamoray force-pushed the ip-block-9.1 branch 2 times, most recently from 5afbe8b to ed5f00c Compare April 24, 2025 10:41
@ksamoray ksamoray force-pushed the ip-block-9.1 branch 2 times, most recently from eee575e to cca2d01 Compare May 7, 2025 10:55
@ksamoray
Copy link
Contributor Author

ksamoray commented May 7, 2025

/test-all

@ksamoray
Use dev branch of SDK for 9.1 development
203438c 
Use the dev branch of the SDK to obtain the latest API changes.

Signed-off-by: Kobi Samoray <[email protected]>
@ksamoray
Add cidr_list attribute to nsxt_policy_ip_block
58784e2 
This attribute is added for NSX v9.1.0.

Signed-off-by: Kobi Samoray <[email protected]>
@ksamoray
Copy link
Contributor Author

ksamoray commented May 8, 2025

Tests pass with v9.1.0

=== RUN   TestAccResourceNsxtPolicyIPBlock_minimal
=== PAUSE TestAccResourceNsxtPolicyIPBlock_minimal
=== CONT  TestAccResourceNsxtPolicyIPBlock_minimal
--- PASS: TestAccResourceNsxtPolicyIPBlock_minimal (26.49s)
=== RUN   TestAccResourceNsxtPolicyIPBlock_v910
=== PAUSE TestAccResourceNsxtPolicyIPBlock_v910
=== CONT  TestAccResourceNsxtPolicyIPBlock_v910
--- PASS: TestAccResourceNsxtPolicyIPBlock_v910 (26.73s)
=== RUN   TestAccResourceNsxtPolicyIPBlock_v910_migrate
=== PAUSE TestAccResourceNsxtPolicyIPBlock_v910_migrate
=== CONT  TestAccResourceNsxtPolicyIPBlock_v910_migrate
--- PASS: TestAccResourceNsxtPolicyIPBlock_v910_migrate (42.13s)
=== RUN   TestAccResourceNsxtPolicyIPBlock_basic
=== PAUSE TestAccResourceNsxtPolicyIPBlock_basic
=== CONT  TestAccResourceNsxtPolicyIPBlock_basic
--- PASS: TestAccResourceNsxtPolicyIPBlock_basic (42.87s)
=== RUN   TestAccResourceNsxtPolicyIPBlock_visibility
=== PAUSE TestAccResourceNsxtPolicyIPBlock_visibility
=== CONT  TestAccResourceNsxtPolicyIPBlock_visibility
--- PASS: TestAccResourceNsxtPolicyIPBlock_visibility (41.77s)
=== RUN   TestAccResourceNsxtPolicyIPBlock_multitenancy
=== PAUSE TestAccResourceNsxtPolicyIPBlock_multitenancy
=== CONT  TestAccResourceNsxtPolicyIPBlock_multitenancy
    utils_test.go:343: This test requires a multitenancy environment
--- SKIP: TestAccResourceNsxtPolicyIPBlock_multitenancy (3.54s)

Test ignored.
=== RUN   TestAccResourceNsxtPolicyIPBlock_importBasic
=== PAUSE TestAccResourceNsxtPolicyIPBlock_importBasic
=== CONT  TestAccResourceNsxtPolicyIPBlock_importBasic
--- PASS: TestAccResourceNsxtPolicyIPBlock_importBasic (29.49s)
=== RUN   TestAccResourceNsxtPolicyIPBlock_importVisibility
=== PAUSE TestAccResourceNsxtPolicyIPBlock_importVisibility
=== CONT  TestAccResourceNsxtPolicyIPBlock_importVisibility
--- PASS: TestAccResourceNsxtPolicyIPBlock_importVisibility (29.93s)
=== RUN   TestAccResourceNsxtPolicyIPBlock_importBasic_multitenancy
=== PAUSE TestAccResourceNsxtPolicyIPBlock_importBasic_multitenancy
=== CONT  TestAccResourceNsxtPolicyIPBlock_importBasic_multitenancy
    utils_test.go:343: This test requires a multitenancy environment
--- SKIP: TestAccResourceNsxtPolicyIPBlock_importBasic_multitenancy (3.27s)

Test ignored.
PASS

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@salv-orlando salv-orlando salv-orlando left review comments

@annakhm annakhm annakhm left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ksamoray @salv-orlando @annakhm