Skip to content

Support constraint resource #1583

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 1 commit into
base: master
Choose a base branch
from
Open

Support constraint resource #1583

wants to merge 1 commit into from

Conversation

annakhm
Copy link
Contributor

@annakhm annakhm commented Mar 25, 2025

No description provided.

@annakhm annakhm force-pushed the constraint-resource branch 2 times, most recently from 9c66200 to 08c6e8a Compare March 25, 2025 20:14
@annakhm
Copy link
Contributor Author

annakhm commented Mar 26, 2025

/test-all

@annakhm annakhm force-pushed the constraint-resource branch 2 times, most recently from 55d0970 to a95e930 Compare April 1, 2025 18:44
@annakhm annakhm requested review from ksamoray and salv-orlando April 1, 2025 20:35
@annakhm
Copy link
Contributor Author

annakhm commented Apr 1, 2025

/test-all

@annakhm annakhm force-pushed the constraint-resource branch from a95e930 to 8335adf Compare April 2, 2025 01:03
@annakhm
Copy link
Contributor Author

annakhm commented Apr 2, 2025

/test-all

@annakhm annakhm force-pushed the constraint-resource branch 3 times, most recently from ee8bfdd to 3491de5 Compare April 3, 2025 16:56
@annakhm
Copy link
Contributor Author

annakhm commented Apr 3, 2025

/test-all

@annakhm annakhm requested a review from aruntony005 April 3, 2025 17:47
@annakhm annakhm force-pushed the constraint-resource branch from 3491de5 to 13c8872 Compare April 10, 2025 18:11
@annakhm
Copy link
Contributor Author

annakhm commented Apr 11, 2025

/test-all

salv-orlando
salv-orlando reviewed Apr 18, 2025
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.

Everything looks good, just a question inline

nsxt/resource_nsxt_vpc_test.go
@@ -178,7 +178,7 @@ func testAccNsxtVpcExists(displayName string, resourceName string) resource.Test
return fmt.Errorf("Policy Vpc resource ID not set in resources")
}

exists, err := resourceNsxtVpcExists(testAccGetProjectContext(), resourceID, connector)
exists, err := resourceNsxtVpcExists(testAccGetMultitenancyContext(), resourceID, connector)
Copy link
Member

Choose a reason for hiding this comment

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

Is this change (and the other similar ones) unrelated to this commit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've renamed this helper as part of PR, for improved code readability.

@annakhm annakhm force-pushed the constraint-resource branch from 13c8872 to 8ae9025 Compare April 22, 2025 22:37
ksamoray
ksamoray reviewed Apr 24, 2025
View reviewed changes
Copy link
Contributor

@ksamoray ksamoray left a comment

Choose a reason for hiding this comment

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

Few nits

api/infra/constraint.go
@@ -0,0 +1,137 @@
//nolint:revive
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we store the updated api_list.yaml anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the generator repository

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, I guess that the correct thing would be merging the one in the provider repo with the one in the generator repo (they are different), and delete the one in the provider repo. Makes sense?

docs/resources/policy_constraint.md Outdated
* `description` - (Optional) Description of the resource.
* `message` - (Optional) User friendly message to be shown to users upon violation.
* `target` - (Optional) Targets for the constraints to be enforced
* `path_prefix` - (Optional) Prefix match to the path, needs to end with `\`
Copy link
Contributor

Choose a reason for hiding this comment

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

Examples above start with /. So I'm guessing that one of these is wrong.

nsxt/resource_nsxt_policy_constraint.go
Type: schema.TypeList,
Elem: &metadata.ExtendedResource{
Schema: map[string]*metadata.ExtendedSchema{
"path_prefix": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we validate that / or \ suffix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need to add the slash anymore, as per your suggestion below

Copy link
Contributor

Choose a reason for hiding this comment

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

So now we can validatePolicyPath here, I think.

Copy link
Contributor Author

@annakhm annakhm Apr 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'd rather give the user an option to configure with trailing slash, in case they would want to follow the API exactly. Unless we want to deviate from the API and also rename this attribute to vpc_path, assuming that NSX will not extend functionality to support other types of path prefixes

Copy link
Contributor

Choose a reason for hiding this comment

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

To follow the API, they have to look into the API spec, I doubt that anyone will bother. But yeah it's not important.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Users sometimes probe the API and copy values from response

docs/resources/policy_constraint.md

## Target resource types

|Object|project + VPC|project only|VPC only|
Copy link
Contributor

Choose a reason for hiding this comment

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

How do we maintain this list? Do we gather this from some doc?

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 is not documented anywhere as far as I know. I followed the UI to build the table

ksamoray
ksamoray reviewed Apr 24, 2025
View reviewed changes
nsxt/resource_nsxt_policy_constraint_test.go
message = "%s"

target {
path_prefix = "${data.nsxt_vpc.test.path}/"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit ugly - if users will have to manipulate paths in that manner. Can we do that automatically if not suffixed by /?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, done

@annakhm annakhm force-pushed the constraint-resource branch 2 times, most recently from 2f2b05e to 7d957c1 Compare April 24, 2025 23:03
@annakhm
Copy link
Contributor Author

annakhm commented Apr 24, 2025

/test-all

@annakhm annakhm force-pushed the constraint-resource branch from 7d957c1 to a6ff05a Compare April 25, 2025 18:31
@annakhm
Copy link
Contributor Author

annakhm commented Apr 25, 2025

/test-all

ksamoray
ksamoray reviewed Apr 27, 2025
View reviewed changes
docs/resources/policy_constraint.md Outdated
message = "too many objects mate"

target {
path_prefix = "/orgs/default/projects/demo/"
Copy link
Contributor

Choose a reason for hiding this comment

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

So now we don't need the trailing slash in the example, right?

ksamoray
ksamoray reviewed Apr 27, 2025
View reviewed changes
Copy link
Contributor

@ksamoray ksamoray left a comment

Choose a reason for hiding this comment

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

Few other nits

docs/resources/policy_constraint.md Outdated
display_name = "demo1-quota"

target {
path_prefix = "/orgs/default/projects/demo/vpcs/demo1/"
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

nsxt/resource_nsxt_policy_constraint.go
Type: schema.TypeList,
Elem: &metadata.ExtendedResource{
Schema: map[string]*metadata.ExtendedSchema{
"path_prefix": {
Copy link
Contributor

Choose a reason for hiding this comment

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

So now we can validatePolicyPath here, I think.

@annakhm annakhm force-pushed the constraint-resource branch from a6ff05a to 7d68b46 Compare April 28, 2025 16:53
salv-orlando
salv-orlando approved these changes May 1, 2025
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.

LGTM

@annakhm
Support constraint resource
3be751f 
Signed-off-by: Anna Khmelnitsky <[email protected]>
@annakhm annakhm force-pushed the constraint-resource branch from 7d68b46 to 3be751f Compare May 3, 2025 00:26
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 approved these changes

@ksamoray ksamoray ksamoray approved these changes

@aruntony005 aruntony005 Awaiting requested review from aruntony005

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
@annakhm @salv-orlando @ksamoray