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

Implement feature to flatten group members (closes #128) #132

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

Yannik
Copy link

@Yannik Yannik commented Jul 1, 2024

SUMMARY

Implements a feature to flatten group members (closes #128).

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

microsoft.ad.group

Copy link

@Yannik Yannik force-pushed the group-member-flatten branch 2 times, most recently from 6ed84ed to fcbf6b5 Compare July 1, 2024 21:57
Copy link

github-actions bot commented Jul 1, 2024

Docs Build 📝

Thank you for contribution!✨

The docsite for this PR is available for download as an artifact from this run:
https://github.com/ansible-collections/microsoft.ad/actions/runs/10257208553

You can compare to the docs for the main branch here:
https://ansible-collections.github.io/microsoft.ad/branch/main

File changes:

  • M collections/microsoft/ad/group_module.html
Click to see the diff comparison.

NOTE: only file modifications are shown here. New and deleted files are excluded.
See the file list and check the published docs to see those files.

diff --git a/home/runner/work/microsoft.ad/microsoft.ad/docsbuild/base/collections/microsoft/ad/group_module.html b/home/runner/work/microsoft.ad/microsoft.ad/docsbuild/head/collections/microsoft/ad/group_module.html
index 7cd516c..788449e 100644
--- a/home/runner/work/microsoft.ad/microsoft.ad/docsbuild/base/collections/microsoft/ad/group_module.html
+++ b/home/runner/work/microsoft.ad/microsoft.ad/docsbuild/head/collections/microsoft/ad/group_module.html
@@ -322,6 +322,19 @@ see <a class="reference internal" href="#ansible-collections-microsoft-ad-group-
 </div></td>
 </tr>
 <tr class="row-even"><td><div class="ansible-option-cell">
+<div class="ansibleOptionAnchor" id="parameter-flatten"></div><p class="ansible-option-title" id="ansible-collections-microsoft-ad-group-module-parameter-flatten"><strong>flatten</strong></p>
+<a class="ansibleOptionLink" href="#parameter-flatten" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">boolean</span></p>
+<p><em class="ansible-option-versionadded">added in microsoft.ad 1.7.0</em></p>
+</div></td>
+<td><div class="ansible-option-cell"><p>Flattens nested groups.</p>
+<p class="ansible-option-line"><strong class="ansible-option-choices">Choices:</strong></p>
+<ul class="simple">
+<li><p><code class="ansible-option-default-bold docutils literal notranslate"><strong><span class="pre">false</span></strong></code> <span class="ansible-option-choices-default-mark">← (default)</span></p></li>
+<li><p><code class="ansible-option-choices-entry docutils literal notranslate"><span class="pre">true</span></code></p></li>
+</ul>
+</div></td>
+</tr>
+<tr class="row-odd"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-homepage"></div><p class="ansible-option-title" id="ansible-collections-microsoft-ad-group-module-parameter-homepage"><strong>homepage</strong></p>
 <a class="ansibleOptionLink" href="#parameter-homepage" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">string</span></p>
 </div></td>
@@ -329,7 +342,7 @@ see <a class="reference internal" href="#ansible-collections-microsoft-ad-group-
 <p>This is the value set on the <code class="docutils literal notranslate"><span class="pre">wWWHomePage</span></code> LDAP attribute.</p>
 </div></td>
 </tr>
-<tr class="row-odd"><td><div class="ansible-option-cell">
+<tr class="row-even"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-identity"></div><p class="ansible-option-title" id="ansible-collections-microsoft-ad-group-module-parameter-identity"><strong>identity</strong></p>
 <a class="ansibleOptionLink" href="#parameter-identity" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">string</span></p>
 </div></td>
@@ -340,7 +353,7 @@ see <a class="reference internal" href="#ansible-collections-microsoft-ad-group-
 <p>When using the <a class="reference internal" href="computer_module.html#ansible-collections-microsoft-ad-computer-module"><span class="std std-ref">microsoft.ad.computer</span></a> module, the identity will automatically append <code class="docutils literal notranslate"><span class="pre">$</span></code> to the end of the <code class="docutils literal notranslate"><span class="pre">sAMAccountName</span></code> if the provided value did not result in a match and did not already have a <code class="docutils literal notranslate"><span class="pre">$</span></code> at the end.</p>
 </div></td>
 </tr>
-<tr class="row-even"><td><div class="ansible-option-cell">
+<tr class="row-odd"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-managed_by"></div><p class="ansible-option-title" id="ansible-collections-microsoft-ad-group-module-parameter-managed-by"><strong>managed_by</strong></p>
 <a class="ansibleOptionLink" href="#parameter-managed_by" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">any</span></p>
 </div></td>
@@ -350,7 +363,7 @@ see <a class="reference internal" href="#ansible-collections-microsoft-ad-group-
 <p>See <a class="reference internal" href="docsite/guide_attributes.html#ansible-collections-microsoft-ad-docsite-guide-attributes-dn-lookup-attributes"><span class="std std-ref">DN Lookup Attributes</span></a> for more information on how DN lookups work.</p>
 </div></td>
 </tr>
-<tr class="row-odd"><td><div class="ansible-option-cell">
+<tr class="row-even"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-members"></div><p class="ansible-option-title" id="ansible-collections-microsoft-ad-group-module-parameter-members"><strong>members</strong></p>
 <a class="ansibleOptionLink" href="#parameter-members" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">dictionary</span></p>
 </div></td>
@@ -361,14 +374,14 @@ see <a class="reference internal" href="#ansible-collections-microsoft-ad-group-
 <p>See <a class="reference internal" href="docsite/guide_attributes.html#ansible-collections-microsoft-ad-docsite-guide-attributes-dn-lookup-attributes"><span class="std std-ref">DN Lookup Attributes</span></a> for more information.</p>
 </div></td>
 </tr>
-<tr class="row-even"><td><div class="ansible-option-indent"></div><div class="ansible-option-cell">
+<tr class="row-odd"><td><div class="ansible-option-indent"></div><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-members/add"></div><p class="ansible-option-title" id="ansible-collections-microsoft-ad-group-module-parameter-members-add"><strong>add</strong></p>
 <a class="ansibleOptionLink" href="#parameter-members/add" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">list</span> / <span class="ansible-option-elements">elements=any</span></p>
 </div></td>
 <td><div class="ansible-option-indent-desc"></div><div class="ansible-option-cell"><p>Adds the principals specified as members of the group, keeping the existing membership if they are not specified.</p>
 </div></td>
 </tr>
-<tr class="row-odd"><td><div class="ansible-option-indent"></div><div class="ansible-option-cell">
+<tr class="row-even"><td><div class="ansible-option-indent"></div><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-members/lookup_failure_action"></div><p class="ansible-option-title" id="ansible-collections-microsoft-ad-group-module-parameter-members-lookup-failure-action"><strong>lookup_failure_action</strong></p>
 <a class="ansibleOptionLink" href="#parameter-members/lookup_failure_action" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">string</span></p>
 </div></td>
@@ -384,14 +397,14 @@ see <a class="reference internal" href="#ansible-collections-microsoft-ad-group-
 </ul>
 </div></td>
 </tr>
-<tr class="row-even"><td><div class="ansible-option-indent"></div><div class="ansible-option-cell">
+<tr class="row-odd"><td><div class="ansible-option-indent"></div><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-members/remove"></div><p class="ansible-option-title" id="ansible-collections-microsoft-ad-group-module-parameter-members-remove"><strong>remove</strong></p>
 <a class="ansibleOptionLink" href="#parameter-members/remove" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">list</span> / <span class="ansible-option-elements">elements=any</span></p>
 </div></td>
 <td><div class="ansible-option-indent-desc"></div><div class="ansible-option-cell"><p>Removes the principals specified as members of the group, keeping the existing membership if they are not specified.</p>
 </div></td>
 </tr>
-<tr class="row-odd"><td><div class="ansible-option-indent"></div><div class="ansible-option-cell">
+<tr class="row-even"><td><div class="ansible-option-indent"></div><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-members/set"></div><p class="ansible-option-title" id="ansible-collections-microsoft-ad-group-module-parameter-members-set"><strong>set</strong></p>
 <a class="ansibleOptionLink" href="#parameter-members/set" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">list</span> / <span class="ansible-option-elements">elements=any</span></p>
 </div></td>
@@ -400,7 +413,7 @@ see <a class="reference internal" href="#ansible-collections-microsoft-ad-group-
 <p>Set this to an empty list to remove all members from a group.</p>
 </div></td>
 </tr>
-<tr class="row-even"><td><div class="ansible-option-cell">
+<tr class="row-odd"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-name"></div><p class="ansible-option-title" id="ansible-collections-microsoft-ad-group-module-parameter-name"><strong>name</strong></p>
 <a class="ansibleOptionLink" href="#parameter-name" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">string</span></p>
 </div></td>
@@ -409,7 +422,7 @@ see <a class="reference internal" href="#ansible-collections-microsoft-ad-group-
 <p>This must be specified if <em>identity</em> is not set.</p>
 </div></td>
 </tr>
-<tr class="row-odd"><td><div class="ansible-option-cell">
+<tr class="row-even"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-path"></div><p class="ansible-option-title" id="ansible-collections-microsoft-ad-group-module-parameter-path"><strong>path</strong></p>
 <a class="ansibleOptionLink" href="#parameter-path" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">string</span></p>
 </div></td>
@@ -420,7 +433,7 @@ see <a class="reference internal" href="#ansible-collections-microsoft-ad-group-
 <p>This can be set to the literal value <code class="docutils literal notranslate"><span class="pre">microsoft.ad.default_path</span></code> which will equal the default value used when creating a new object.</p>
 </div></td>
 </tr>
-<tr class="row-even"><td><div class="ansible-option-cell">
+<tr class="row-odd"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-protect_from_deletion"></div><p class="ansible-option-title" id="ansible-collections-microsoft-ad-group-module-parameter-protect-from-deletion"><strong>protect_from_deletion</strong></p>
 <a class="ansibleOptionLink" href="#parameter-protect_from_deletion" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">boolean</span></p>
 </div></td>
@@ -434,7 +447,7 @@ see <a class="reference internal" href="#ansible-collections-microsoft-ad-group-
 </ul>
 </div></td>
 </tr>
-<tr class="row-odd"><td><div class="ansible-option-cell">
+<tr class="row-even"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-sam_account_name"></div><p class="ansible-option-title" id="ansible-collections-microsoft-ad-group-module-parameter-sam-account-name"><strong>sam_account_name</strong></p>
 <a class="ansibleOptionLink" href="#parameter-sam_account_name" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">string</span></p>
 </div></td>
@@ -442,7 +455,7 @@ see <a class="reference internal" href="#ansible-collections-microsoft-ad-group-
 <p>If omitted, the <em>name</em> value is used when creating a new group.</p>
 </div></td>
 </tr>
-<tr class="row-even"><td><div class="ansible-option-cell">
+<tr class="row-odd"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-scope"></div><p class="ansible-option-title" id="ansible-collections-microsoft-ad-group-module-parameter-scope"><strong>scope</strong></p>
 <a class="ansibleOptionLink" href="#parameter-scope" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">string</span></p>
 </div></td>
@@ -458,7 +471,7 @@ see <a class="reference internal" href="#ansible-collections-microsoft-ad-group-
 </ul>
 </div></td>
 </tr>
-<tr class="row-odd"><td><div class="ansible-option-cell">
+<tr class="row-even"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-state"></div><p class="ansible-option-title" id="ansible-collections-microsoft-ad-group-module-parameter-state"><strong>state</strong></p>
 <a class="ansibleOptionLink" href="#parameter-state" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">string</span></p>
 </div></td>

Copy link

Copy link

Copy link

@jborean93
Copy link
Collaborator

Sorry I'm not ignoring you on purpose with this PR. I've gotten back from some leave this week so have been trying to play catch up with everything there.

@Yannik
Copy link
Author

Yannik commented Jul 11, 2024

@jborean93 Don't worry about it! Take all the time you need. I appreciate your work very much!

(For now, I am using an internal fork of this collection, which is fine for until you get around to reviewing this :))

Copy link
Collaborator

@jborean93 jborean93 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, it's nice to see Get-ADGroupMember has a -Recursive option that can do all the logic needed to flatten the results.

I'm wondering whether we can try and make this a bit more generic and to avoid some of the module specific options in the module util function. What I was potentially thinking:

  • Add a new AllowFlatten property in the PropertyInfo entry
  • This option adds flatten as a sub option for that field
  • It will also pass along -NestedGroupFlatten to ConvertTo-AnsibleADDistinguishedName
  • The members entry in group.ps1 will set AllowFlatten = $true

This way we can potentially reimplement the same logic but for other options that also use the add/remove/set and DN lookup logic. It also removes the module specific code in the module util code as now it's controlled by a special property on the property info. The option is also now implemented as part of a sub option for members rather than it being global on the module itself, e.g.

- microsoft.ad.group:
    name: MyGroup
    members:
      add:
      - user1
      - user2
      flatten: true

Another option which I think might be the most flexible is to add it as a new sub member for each DN you want to flatten. This is similar to the server sub option that was added with #117. For example

- microsoft.ad.group:
    name: MyGroup
    members:
      add:
      # Won't be flattened
      - user1
      # Will be flattened
      - name: user2
        flatten: true

I find this a potentially better approach as

  • We don't need to mess with the code to check for the flatten and set it for each option
  • The caller has control over whether each individual entry is to be flattened or not, rather than the all or nothing approach
  • This will automatically allow any existing option that uses the current add_remove_set behaviour to have this ability

The only downside I see is that it has to be set on each option which might be annoying but we could look into filter plugins to make that easier in the future if that's a problem.

What are your thoughts on that approach?

plugins/module_utils/_ADObject.psm1 Outdated Show resolved Hide resolved
@@ -1016,6 +1026,13 @@ Function Invoke-AnsibleADObject {
}
)

if ($ModuleNoun -eq "ADGroup") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should not be part of the shared module util function but an option inside the group module itself. Unfortunately it probably means that the logic for the members option needs to split out of the util.

Copy link
Author

Choose a reason for hiding this comment

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

Can you explain this in more detail? I don't really understand what to do here.

plugins/module_utils/_ADObject.psm1 Show resolved Hide resolved
plugins/modules/group.py Outdated Show resolved Hide resolved
Copy link

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.
Warning:
Error merging github.com/ansible-collections/microsoft.ad for 132,dd160c202c6891e37c70ac35093ea3741c98c2ac

Copy link

@Yannik
Copy link
Author

Yannik commented Aug 5, 2024

I'm wondering whether we can try and make this a bit more generic and to avoid some of the module specific options in the module util function. What I was potentially thinking:

* Add a new `AllowFlatten` property in the `PropertyInfo` entry

* This option adds `flatten` as a sub option for that field

* It will also pass along `-NestedGroupFlatten` to `ConvertTo-AnsibleADDistinguishedName`

* The `members` entry in `group.ps1` will set `AllowFlatten = $true`

This way we can potentially reimplement the same logic but for other options that also use the add/remove/set and DN lookup logic.

Do you see any other usecases where a flatten option would be useful? I'm not sure if adding a general AllowFlatten abstraction (instead of keeping this specific to nested groups) really makes sense, unless it will be used for anything else than flattening nested groups.

It also removes the module specific code in the module util code as now it's controlled by a special property on the property info. The option is also now implemented as part of a sub option for members rather than it being global on the module itself, e.g.

- microsoft.ad.group:
    name: MyGroup
    members:
      add:
      - user1
      - user2
      flatten: true

Another option which I think might be the most flexible is to add it as a new sub member for each DN you want to flatten. This is similar to the server sub option that was added with #117. For example

- microsoft.ad.group:
    name: MyGroup
    members:
      add:
      # Won't be flattened
      - user1
      # Will be flattened
      - name: user2
        flatten: true

I find this a potentially better approach as

* We don't need to mess with the code to check for the `flatten` and set it for each option

* The caller has control over whether each individual entry is to be flattened or not, rather than the all or nothing approach

* This will automatically allow any existing option that uses the current `add_remove_set` behaviour to have this ability

The only downside I see is that it has to be set on each option which might be annoying but we could look into filter plugins to make that easier in the future if that's a problem.

What are your thoughts on that approach?

Being able to set flatten on set/add/remove level, or even on item level is more flexible. On the other hand, having to set it on set/add/remove or even item level causes code repetition in all those cases where you want to use this option on a module level. Not sure how a filter would fix this, since this would only case addThis is how I would weigh each option.

I tried thinking of usecases where flattening on set/add/remove level would - in practice - be used, and had a hard time coming up with anything, even more so for doing this on item level.

Unless there are compelling usecases for being able to do this on set/add/remove or item level, the module level seems to be the most straightforward and simple to use option for me, and I would currently prefer this.

(Sorry for taking so long to respond, I was on holiday)

Copy link

Copy link

@jborean93
Copy link
Collaborator

The main reason why I was suggesting setting on each item level is because the code is already setup to support options on an item so would be the easiest thing to implement without major changes. I will agree it can be somewhat annoying to have to set it on each item and I cannot think of a reason why you might want to flatten one member but not the others.

The more I've been thinking about it I think setting the option under the group option makes a bit more sense to me. It explicitly scopes the option to just the group option by it's nature of being a sub option. It should also be possible to make it a generic ability where the PropertyInfo PSCustomObject has an option like ExposeFlatten where the module util code automtically adds this. This removes the explicit group and cmdlet noun checks in the common code in favour of a pluggable option I've been trying to work with.

@Yannik
Copy link
Author

Yannik commented Oct 2, 2024

@jborean93 How can we move forward with this? Are you ready to make a decision where to put this option? :)

@Yannik
Copy link
Author

Yannik commented Oct 29, 2024

@jborean93 Hi there, quick update: I will only be on this project until EOY, as such I would appreciate being able to finish this up before rather sooner than later :)

@jborean93
Copy link
Collaborator

Sorry @Yannik I had to go on leave quite abruptly so couldn't work on this. I think putting the option under the members key is the way I would work on this, for example

- microsoft.ad.group:
    name: MyGroup
    members:
      add:
      - user1
      - user2
      flatten: true

This way has a few advantages in my mind:

  • The flatten is associated with the option it is going to flatten
  • Any new root option added won't be affected by a top level flatten option
  • We can implement a generic "flatten" logic under the PSCustomObject entry like SupportsFlatten
  • With the above any future option where we want to expose a way to flatten the add/remove/set members can be done automatically
  • No need for hardcoding the flatten logic for the module or option name now that it's part of the property spec

Please let me know if that makes sense to you or not or if you have any concerns about this approach.

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.

Feature request: "flatten" option for microsoft.ad.group
2 participants