-
Notifications
You must be signed in to change notification settings - Fork 23.9k
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
new windows module, win_audit_rule #30473
Conversation
The test
The test
The test
|
457b20c
to
dc698cc
Compare
@LiranNis @SamLiu79 @timothyvandenbrande @andrewsaraceni @angstwad @ar7z1 @blakfeld @brianlloyd @chrishoffman @daBONDi @elventear @if-meaton @joshludwig @nwchandler @nwsparks @petemounce @rndmh3ro @schwartzmx @smadam813 As a maintainer of a module in the same namespace this new module has been submitted to, your vote counts for shipits. Please review this module and add |
CI failure in integration tests: https://app.shippable.com/github/ansible/ansible/runs/37813/12/tests |
1ec3aa0
to
871a389
Compare
fixed now. ready_for_review |
I am not in favor of adding a special test_ module for testing. Use assert for verifying if the module works correctly, and test idempotency and check-mode support to ensure these policies are correctly created and removed. We don't want to end up in the situation where a bug in the test module negates bugs in the module. For every operation (add, modify remove) you would usually do:
|
|
||
$result = @{ | ||
changed = $false | ||
identity_reference = $identity_reference |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I don't see the benefit of returning the same input as return values. The user has these values already so there is no need to return ito back to them. Worse even, it makes it harder to find the real interesting return values.
Please only return new/modified useful information.
PS Yes, older modules also did this, but we don't want to make the same mistakes for newer modules.
} | ||
|
||
#Make sure target path is valid | ||
If (-not (Test-Path $path) ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We tend to add option names to cmdlets, like:
Test-Path -Path $path
#Make sure target path is valid | ||
If (-not (Test-Path $path) ) | ||
{ | ||
Fail-Json $result "defined path ($path) is not found/invalid" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here:
Fail-Json -obj $result -message "defined path '$path' is not found or is invalid"
|
||
$result.current_audit_rules = Get-CurrentAuditRules $path | ||
$result.changed = $true | ||
exit-json $result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would go here as well for:
Exit-Json -obj $result
If (-Not $ToRemove) | ||
{ | ||
$result.current_audit_rules = Get-CurrentAuditRules $path | ||
Exit-Json $result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here:
Exit-Json -obj $result
- For absent, only path, identity_reference, and state are required | ||
required: true | ||
default: present | ||
choices: [ Present, Absent ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use lower-case for these options. YAML is case-sensitive, so it will appear in the docs as Title-case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review. Do you want this done for all the options or only present/absent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and if all the options, how should something like NoPropagateInherit appear? I was trying to just leave it like that so I could directly pass it rather than having to convert something like no_propagate_inherit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I am mostly concerned with absent/present because that is a generic option used by other modules. Besides your examples use it as lowercase anyway. For the others, if the casing is important, leave as-is, but ensure you validate using strict casing as well, or users with casing-issues may get an ugly exception.
@dagwieers for the testing module. i wasn't sure how to approach. in the primary module (win_audit_rule) i'm returning the current auditing rules to the $results object before exiting...initially I was just going to test against that, but it was suggested that out of band testing would be better. The code to retrieve the rules was a bit much for an inline win_shell option so I did the module to retrieve them. I figured since I was already doing a comparison in the testing module I could just use its results rather than asserts in the test. I guess my question is, what is the most preferable way of approaching this? Do you have issue with just using the module in general no matter how it is used? Or do you have issue with the comparisons to check expected results occurring withing the testing module rather than in the .yml file? Do you prefer I just compare against my returned $results from the main module? |
I don't mind having a test module to get the results of the stat instead of relying on crazy shell calls each time you want to assert against something. @dagwieers are your concerns around using the test module to check the assumptions or a test module to get results that we then assert on? |
So we discussed this during the Windows Working Group meeting, and it seems my unease with the test module is unwarranted. So forget anything I said about it :-) |
@jborean93 thanks. Updates the suggested changes. It failed because $inheritance_flags had to be set to none for a file. I think setting audit rules for a specific file is something of an edge case so I like leaving the default value there to automatically make sub-objects of a folder inherit....but, I think that obviously now it needs to be addressed in some way as well as mentioned in the docs. Do you have a preference between these 2 options?
I'll update integration testing to target single files as well. |
and registry keys. Split testing files apart to be more organized. Updated powershell for better handling when targetting file objects and optimized a bit. Removed duplicated sections that got there from a previous merge I think.
Seemed like there would be less change of accidentally using the wrong variable when copy/pasting that way, and not much upside to having unique names. Did final cleanup and fixed a few errors in the integration testing.
e5e004d
to
ce0c81f
Compare
@jborean93 went ahead and just made it automatically set it to 'none' if the path type is a file and made the other requested changes as well. Also added individual file testing to integration. LMK if you spot anything else. |
Fixed a bug where removal was failing if multiple rules existed due to inheritance from higher level objects.
70e601f
to
ba1077d
Compare
for get-acl. Changed from setauditrule to addauditrule, see comment in script for reasoning. Fixed state absent to be able to remove multiple entries if they exist.
The test
The test
The test
The test
The test
|
I know you have already made the change but I'm too much a fan of automatically setting values based on the type. It would be preferable to check if the None flag is set when it is a file and throw an error. It would be good to at least document an example of how to do this as well. |
No worries. It's been updated to fail rather than warn and docs are updated as well. ready_for_review |
Thanks for this @nwsparks, when you get a chance , are you able to update the CHANGELOG.md and add this to the list of new modules for 2.5. |
SUMMARY
New module PR for win_audit_rule
Allows management of auditing rules on Windows objects (add, modify, remove)
ISSUE TYPE
COMPONENT NAME
win_audit_rule