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.

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

VPC E-W security policy resource #1217

Merged
merged 1 commit into from
Jun 18, 2024
Merged

VPC E-W security policy resource #1217

merged 1 commit into from
Jun 18, 2024

Conversation

ksamoray
Copy link
Collaborator

@ksamoray ksamoray commented May 27, 2024 •
edited
Loading

Implement the resource for east-west VPC security policy.

@ksamoray ksamoray changed the title VPC E-W security policy resource [WIP] VPC E-W security policy resource May 27, 2024
@ksamoray
Copy link
Collaborator Author

/test-all

@ksamoray ksamoray force-pushed the vpc-ew-policy branch 2 times, most recently from c653eb5 to 9977b84 Compare May 27, 2024 13:26
@ksamoray
Copy link
Collaborator Author

/test-all

@ksamoray
Copy link
Collaborator Author

/test-all

salv-orlando
salv-orlando reviewed May 28, 2024
View reviewed changes
nsxt/provider.go Outdated
ctxPtr := d.Get("context")
if ctxPtr != nil {
contexts := ctxPtr.([]interface{})
for _, context := range contexts {
data := context.(map[string]interface{})
vpcID := ""
if data["vpi_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.

perhaps you have a typo here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed

@ksamoray ksamoray mentioned this pull request May 28, 2024
Merged
@ksamoray
Copy link
Collaborator Author

/test-all

@ksamoray ksamoray force-pushed the vpc-ew-policy branch 5 times, most recently from 0889a1b to 1aaaf4f Compare May 30, 2024 11:28
@ksamoray ksamoray changed the title [WIP] VPC E-W security policy resource VPC E-W security policy resource May 30, 2024
@ksamoray ksamoray force-pushed the vpc-ew-policy branch 3 times, most recently from 7c83f69 to 5ece116 Compare June 2, 2024 14:25
@ksamoray
Copy link
Collaborator Author

ksamoray commented Jun 2, 2024

/test-all

@annakhm
Copy link
Collaborator

annakhm commented Jun 4, 2024

This PR does not cover the provider-defined VPC scope yet, correct?

@ksamoray
Copy link
Collaborator Author

ksamoray commented Jun 4, 2024

This PR does not cover the provider-defined VPC scope yet, correct?

Indeed - the provider-level VPC specification is a global, non per-resource change.

@ksamoray ksamoray force-pushed the vpc-ew-policy branch 3 times, most recently from 8134a41 to 77d6a6b Compare June 4, 2024 11:39
@ksamoray
Copy link
Collaborator Author

ksamoray commented Jun 4, 2024

/test-all

annakhm
annakhm reviewed Jun 4, 2024
View reviewed changes
nsxt/provider.go Outdated
@@ -496,6 +496,7 @@ func Provider() *schema.Provider {
"nsxt_policy_gateway_flood_protection_profile_binding": resourceNsxtPolicyGatewayFloodProtectionProfileBinding(),
"nsxt_policy_compute_sub_cluster": resourceNsxtPolicyComputeSubCluster(),
"nsxt_policy_tier0_inter_vrf_routing": resourceNsxtPolicyTier0InterVRFRouting(),
"nsxt_policy_vpc_security_policy": resourceNsxtPolicyVPCSecurityPolicy(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we remove the first policy for all VPC resources? There is no other VPC.. Would love to hear also from @salv-orlando and @tvigneron

Copy link
Member

Choose a reason for hiding this comment

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

I agree, also proposed that in design review.

Copy link
Collaborator

@tvigneron tvigneron Jun 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It seems a great idea - for vpc resources let's get rid of the Policy. We don't want the name to be unnecessarily long

@qiyueyao
Copy link
Collaborator

qiyueyao commented Jun 6, 2024

Hi, a quick maybe dumb question: does this series of change support data_source_nsxt_policy_security_policy and data_source_nsxt_policy_gateway_policy? Looks like for Get with ID, it's handled, but without ID, it still lists the domain 🤔

@ksamoray
Copy link
Collaborator Author

ksamoray commented Jun 9, 2024

Hi, a quick maybe dumb question: does this series of change support data_source_nsxt_policy_security_policy and data_source_nsxt_policy_gateway_policy? Looks like for Get with ID, it's handled, but without ID, it still lists the domain 🤔

@qiyueyao I haven't got to handle these data sources yet but yeah - unless they use search API, they'll be affected by this change as the API wrapper is modified.
I believe that I should add a check to the DS, that the VPC is not specified - as these datasources aren't supposed to serve VPC.
VPC should have a pair of data_source_nsxt_policy_vpc.* datasources.

salv-orlando
salv-orlando reviewed Jun 10, 2024
View reviewed changes
Copy link
Member

@salv-orlando salv-orlando left a comment

Choose a reason for hiding this comment

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

I have no comment, and looks good to me.
Let's decide if we want to include also VPC support for parent_security_policy and security_policy_rule

nsxt/provider.go Outdated
@@ -496,6 +496,7 @@ func Provider() *schema.Provider {
"nsxt_policy_gateway_flood_protection_profile_binding": resourceNsxtPolicyGatewayFloodProtectionProfileBinding(),
"nsxt_policy_compute_sub_cluster": resourceNsxtPolicyComputeSubCluster(),
"nsxt_policy_tier0_inter_vrf_routing": resourceNsxtPolicyTier0InterVRFRouting(),
"nsxt_policy_vpc_security_policy": resourceNsxtPolicyVPCSecurityPolicy(),
Copy link
Member

Choose a reason for hiding this comment

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

I agree, also proposed that in design review.

@ksamoray
Copy link
Collaborator Author

/test-all

@ksamoray
Copy link
Collaborator Author

/test-all

ksamoray
ksamoray commented Jun 13, 2024
View reviewed changes
nsxt/resource_nsxt_vpc_security_policy_test.go Outdated
},
Providers: testAccProviders,
CheckDestroy: func(state *terraform.State) error {
return testAccNsxtPolicySecurityPolicyCheckDestroy(state, updatedName, defaultDomain)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should pass the resource name or use a VPC func here

@ksamoray ksamoray force-pushed the vpc-ew-policy branch 2 times, most recently from 0321054 to 0e0bd77 Compare June 13, 2024 15:47
annakhm
annakhm reviewed Jun 14, 2024
View reviewed changes
nsxt/resource_nsxt_policy_predefined_security_policy.go
Children: []*data.StructValue{childVPC},
}

client := nsxt.NewOrgRootClient(connector)
Copy link
Collaborator

@annakhm annakhm Jun 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This would work with VPC admin privilege?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Tested with v4.2.0, it does.

annakhm
annakhm reviewed Jun 17, 2024
View reviewed changes
website/docs/r/vpc_security_policy.html.markdown Outdated
terraform import nsxt_vpc_security_policy.policy1 PATH
```

The above command imports the VPC security policy named `policy1` under NSX domain `domain` with the NSX Policy path `PATH`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

domain is irrelevant here

@annakhm
Copy link
Collaborator

annakhm commented Jun 17, 2024

LGTM. There are some doc fixes needed, but feel free to address them in one of the following PRs if you prefer

@ksamoray
VPC E-W security policy resource
e29eb44 
Implement the resource for east-west VPC security policy.

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

/test-all

@ksamoray ksamoray merged commit d77898b into vmware:master Jun 18, 2024
7 checks passed
@ksamoray ksamoray deleted the vpc-ew-policy branch June 18, 2024 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tvigneron tvigneron tvigneron left review comments

@annakhm annakhm annakhm approved these changes

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

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

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