Skip to content
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.

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

Allow IP CIDR Range or Any as Source and/or Destination Policy Groups #589

Merged
merged 1 commit into from
Mar 18, 2021

Conversation

akgiesler
Copy link

@akgiesler akgiesler commented Mar 9, 2021

Adds a new validator specific to source and destination policy groups.
NSX-T can accept an IP, Range, CIDR, or a Group Path as a source
and/or destination group through the security policy interface.

Updates the getSecurityPolicyAndGatewayRulesSchema function to use the
new validator.

Resolves: Issue #584

@annakhm
Copy link
Collaborator

annakhm commented Mar 11, 2021

Thank you @akgiesler! This looks great, but a documentation update is also needed, as well as acc tests. If you can add those it will be great, otherwise we'll follow up with those on top of your change.

@akgiesler
Copy link
Author

Sure thing @annakhm , we are working on those now.

@akgiesler akgiesler force-pushed the ip_as_policy_source_destination branch from 30953d9 to 23b612d Compare March 12, 2021 18:59
@akgiesler
Copy link
Author

akgiesler commented Mar 12, 2021

Hi @annakhm,
I have updated the PR to include tests for Security Policy and Gateway Policy as they share the same function to get & validate their schema. I have also updated the docs for both to reference the availability of IP, CIDR, or IP ranges.

During our build-out of the tests, it was discovered that the provider already supports using "ANY" as a source or destination in security policy. The way to do this was to map the source_groups or destination_groups to an empty set.

for example:

  rule {
    display_name       = "rule1"
    source_groups      = [ nsxt_policy_group.group1.path ]
    destination_groups = [ ]
    action             = "ALLOW"
    services           = [data.nsxt_policy_service.icmp.path]
  }

This was tested in NSX-T 3.1 using NSX-T Provider v3.1.0. I added this feature into the Security policy documentation. I did not add this feature to the gateway policy documentation as I was unable to test it. I was able to test this feature in a Gateway policy and it also worked fine. I have updated the gateway policy docs as well.

},
Steps: []resource.TestStep{
{
Config: testAccNsxtPolicyGatewayPolicyWithIPCidrRangeDestCreate(name, policyIP, policyCidr, policyRange),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would suggest to use a single function to test both Create and Update use cases (just pass different values in params), and also I think one test should be enough to cover both source and dest use cases. While our goal is to have good test coverage, we also try to keep CI running time reasonable :)

Copy link
Author

Choose a reason for hiding this comment

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

I went ahead and collapsed the tests into a single test for both security policy and gateway policy. I also reduced the terraform generating functions down to single functions as you requested.

@@ -93,7 +93,7 @@ The following arguments are supported:
* `rule` (Optional) A repeatable block to specify rules for the Gateway Policy. Each rule includes the following fields:
* `display_name` - (Required) Display name of the resource.
* `description` - (Optional) Description of the resource.
* `destination_groups` - (Optional) A list of destination group paths to use for the policy.
* `destination_groups` - (Optional) Set of group paths, IPs, IP ranges, or CIDRs that serve as the destination for this rule.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It should be helpful specifying here that IP/ranges/cidrs are only supported starting NSX 3.0.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @annakhm, I have updated all of the documentation as requested.

@annakhm
Copy link
Collaborator

annakhm commented Mar 13, 2021

Thanks for those changes @akgiesler! You are correct, "ANY" is equivalent to empty set. I've ran your tests on gateway policy and they pass fine. I noticed that docs for gateway policy are updated in your change.
I've left couple of small comments above, another nit would be to also update documentation in resource_nsxt_policy_predefined_security_policy and resource_nsxt_policy_predefined_gateway_policy.

Adds a new validator specific to source and destination policy groups.
NSX-T can accept an IP, Range, CIDR, or a Group Path as a source
and/or destination group through the security policy interface.

Updates the getSecurityPolicyAndGatewayRulesSchema function to use the
new validator.

Updates docs to reference IP, Range, and CIDR as valid source /
destination groups. Also include reference for using empty set
to specify "ANY".

Resolves: Issue vmware#584
@akgiesler akgiesler force-pushed the ip_as_policy_source_destination branch from 23b612d to b55fdb6 Compare March 15, 2021 21:38
@annakhm
Copy link
Collaborator

annakhm commented Mar 18, 2021

Thank you @akgiesler, merging this!

@annakhm annakhm merged commit 7a8bc99 into vmware:master Mar 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants