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

Add group_type attribue to policy group #857

Merged
merged 1 commit into from
Apr 11, 2023
Merged

Conversation

ksamoray
Copy link
Collaborator

Fixes: #855

@ksamoray
Copy link
Collaborator Author

@annakhm note that in the API, group_type is a list with a maximum of one member.
I implemented this as a string property as I thought that it'll make more sense. If you believe that implementing this as a list also in the provider, I can change that.

@ksamoray ksamoray requested a review from annakhm March 23, 2023 09:49
@@ -184,6 +184,7 @@ The following arguments are supported:
* `distinguished_name` (Required for an `identity_group`) LDAP distinguished name (DN). A valid fully qualified distinguished name should be provided here. This value is valid only if it matches to exactly 1 LDAP object on the LDAP server.
* `domain_base_distinguished_name` (Required for an `identity_group`) Identity (Directory) domain base distinguished name. This is the base distinguished name for the domain where this identity group resides. (e.g. dc=example,dc=com)
* `sid` (Optional) Identity (Directory) Group SID (security identifier). A security identifier (SID) is a unique value of variable length used to identify a trustee. This field is only populated for Microsoft Active Directory identity store.
* `group_type` - (Optional) Group type can be specified during create and update of a group. Empty group type indicates a 'generic' group, ie group can be one of IPAddress, ANTERA.
Copy link
Collaborator

Choose a reason for hiding this comment

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

ANTERA -> ANTREA?

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. Thanks

obj := model.Group{
DisplayName: &displayName,
Description: &description,
Tags: tags,
Expression: expressionData,
ExtendedExpression: extendedExpressionList,
GroupType: groupTypes,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe version check is needed here since GroupType support was added recently in NSX

@@ -844,12 +855,18 @@ func resourceNsxtPolicyGroupCreate(d *schema.ResourceData, m interface{}) error
description := d.Get("description").(string)
tags := getPolicyTagsFromSchema(d)

var groupTypes []string
groupType := d.Get("group_type").(string)
if groupType != "" && nsxVersionHigherOrEqual("3.2.0") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this version check would work with NSX version <3.2, since empty list of group_types would still be sent to the platform, and NSX would not recognize this property. The version check needs to define whether GroupType is set in the struct.

@ksamoray ksamoray force-pushed the issue_855 branch 3 times, most recently from 2913f2a to 2d9d998 Compare April 3, 2023 14:49
obj := model.Group{
DisplayName: &displayName,
Description: &description,
Tags: tags,
Expression: expressionData,
ExtendedExpression: extendedExpressionList,
GroupType: groupTypes,
Copy link
Collaborator

Choose a reason for hiding this comment

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

In Update code path Group type is still set regardless of version

testResourceName := "nsxt_policy_group.test"

resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
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 add a version check here?

Fixes: vmware#855
Signed-off-by: Kobi Samoray <ksamoray@vmware.com>
@annakhm
Copy link
Collaborator

annakhm commented Apr 10, 2023

/test-all

@ksamoray ksamoray merged commit 8bd9241 into vmware:master Apr 11, 2023
@ksamoray ksamoray deleted the issue_855 branch April 11, 2023 13:26
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.

Missing group_type definition for nsxt_policy_group
2 participants