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

Add FloodProtectionProfile support #1025

Merged
merged 2 commits into from
Apr 4, 2024
Merged

Add FloodProtectionProfile support #1025

merged 2 commits into from
Apr 4, 2024

Conversation

GraysonWu
Copy link
Collaborator

Fix #880

@GraysonWu GraysonWu force-pushed the flood-protection-profile branch from f4064ff to 0f927ee Compare November 9, 2023 22:54
@GraysonWu
Copy link
Collaborator Author

/test-all

@GraysonWu GraysonWu force-pushed the flood-protection-profile branch from 0f927ee to ec09393 Compare November 9, 2023 22:56
@annakhm
Copy link
Collaborator

annakhm commented Nov 14, 2023

Is this object relevant for multitenancy use case?

@ksamoray
Copy link
Collaborator

Is this object relevant for multitenancy use case?

SDK has a multitenancy client implementation for this API, so it's worthwhile to implement this.
@GraysonWu LMK if you need help with this, generating a wrapper etc.

@GraysonWu GraysonWu force-pushed the flood-protection-profile branch from ec09393 to 38fe3a3 Compare November 15, 2023 23:55
@GraysonWu
Copy link
Collaborator Author

@annakhm @ksamoray Thanks for pointing it out. Added multi-tenancy support.

@GraysonWu GraysonWu force-pushed the flood-protection-profile branch from 38fe3a3 to 07e3483 Compare November 16, 2023 00:11
@GraysonWu
Copy link
Collaborator Author

/test-all

salv-orlando
salv-orlando reviewed Nov 16, 2023
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 am unsure about whether we need some addiitional step to consume these profiles.
Do you think we need to implement also the FloodProtectionProfileBindingMap?

My question specifically is that in theory we can create multiple profiles. How do we know which profile should be applied to the firewall?

nsxt/resource_nsxt_policy_gateway_flood_protection_profile.go Show resolved Hide resolved
@GraysonWu
Copy link
Collaborator Author

I am unsure about whether we need some addiitional step to consume these profiles. Do you think we need to implement also the FloodProtectionProfileBindingMap?

My question specifically is that in theory we can create multiple profiles. How do we know which profile should be applied to the firewall?

Ah, good catch. We do need that binding map to create group-profile binding to apply the profile to groups. Let me add it.

@GraysonWu GraysonWu force-pushed the flood-protection-profile branch 4 times, most recently from ba031ef to 9c0f133 Compare November 20, 2023 19:06
@GraysonWu
Copy link
Collaborator Author

/test-all

@GraysonWu GraysonWu force-pushed the flood-protection-profile branch 5 times, most recently from 392cd62 to afa2551 Compare November 21, 2023 04:07
@GraysonWu
Copy link
Collaborator Author

/test-all

@ksamoray
Copy link
Collaborator

LGTM, pls rebase this though as there are conflicts.

@salv-orlando salv-orlando modified the milestones: v3.5.0, v3.6.0 Feb 12, 2024
@annakhm
Copy link
Collaborator

annakhm commented Apr 3, 2024

Sorry I don't find the binding maps in this PR - should they be added as part of this PR?

@GraysonWu GraysonWu force-pushed the flood-protection-profile branch from afa2551 to 63c3a8b Compare April 3, 2024 18:43
@GraysonWu
Copy link
Collaborator Author

Sorry I don't find the binding maps in this PR - should they be added as part of this PR?

My bad. I thought I had pushed it...

@GraysonWu
Copy link
Collaborator Author

/test-all

@GraysonWu GraysonWu force-pushed the flood-protection-profile branch 2 times, most recently from b44a2fe to d181ecb Compare April 3, 2024 20:08
@GraysonWu
Copy link
Collaborator Author

/test-all

@GraysonWu GraysonWu force-pushed the flood-protection-profile branch from d181ecb to 13e596a Compare April 3, 2024 20:36
@GraysonWu
Copy link
Collaborator Author

/test-all

@GraysonWu GraysonWu force-pushed the flood-protection-profile branch from 13e596a to 88650fa Compare April 4, 2024 00:42
@GraysonWu
Copy link
Collaborator Author

/test-all

@GraysonWu GraysonWu force-pushed the flood-protection-profile branch 3 times, most recently from 7f2a855 to b4eab88 Compare April 4, 2024 04:39
@GraysonWu
Copy link
Collaborator Author

/test-all

@GraysonWu
Add binding map
92a21a4 
Signed-off-by: graysonwu <[email protected]>
@GraysonWu GraysonWu force-pushed the flood-protection-profile branch from b4eab88 to 92a21a4 Compare April 4, 2024 21:23
@GraysonWu GraysonWu merged commit 752aa5b into vmware:master Apr 4, 2024
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@annakhm annakhm annakhm left review comments

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

@ksamoray ksamoray ksamoray approved these changes

@wsquan171 wsquan171 Awaiting requested review from wsquan171

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.6.0
Development

Successfully merging this pull request may close these issues.

Add resource to manage the Distributed Firewall "Flood Protection Profiles"
4 participants
@GraysonWu @annakhm @ksamoray @salv-orlando