win_nssm: tests and several bug fixes

[ksubileau]

SUMMARY

The win_nssm module is full of bugs and is almost not working at all. This PR adds or changes the following to win_nssm:

• integration tests, including indempotence tests
• fix service not started when state=started
• fix error with the dependencies option when using a comma-separated list of dependent services
• resolve several non-indempotence issues
• drop support of YAML dict for app_parameters option as it's broken since 2.3 release
• add missing space between arguments with app_parameters
• fix incorrect argument line with app_parameters or app_parameters_free_form when a parameter contains spaces, quotes or backslashes
• fix extra space added in argument line with app_parameters or app_parameters_free_form when a parameter start by a dash and is followed by a period (e.g. -Dcom.test)

ISSUE TYPE

• Bugfix Pull Request
• Docs Pull Request

COMPONENT NAME

win_nssm

ANSIBLE VERSION

devel

ADDITIONAL INFORMATION

It's a bit an all-in-one PR because I tried to write a complete set of tests and get it to work.
There are still some issues on special cases that are not solved here and may be dealt with later (for example, it's not possible to set user to LocalSystem without password, as the module throw an error if no password is given).
The module also still lacks check-mode support, but this may be part of another PR after this one.

Fixes #25265
Fixes #44079
Fixes #35442
Fixes #38557
Fixes #20625
And other issues not yet reported

[ansibot]

cc @dagwieers @jborean93 @jhawkesworth @mattclay @nitzmahone @smadam813 @themiwi
click here for bot help

[dagwieers]

Great! This was one of the important modules to require a rewrite and was missing integration tests.
See https://github.com/ansible/community/wiki/Windows:-progress-tracker

I remember it had idempotency issues, do you remember fixing any ?

[dagwieers]

Also, it could be useful to check the remaining reported issue to win_nssm, as they might be fixed already, like #35442:

• win_nssm needs to support more parameters. #44176 -- win_nssm needs to support more parameters
• Unable to install service via win_nssm.  #38557 -- Unable to install service via win_nssm
• win_nssm: service doesn't start #35442 -- win_nssm: service doesn't start
• Idempotence problems with win_nssm's app_parameters #20625 -- Idempotence problems with win_nssm's app_parameters
• Win_nssm module doesn't honour the order of the app_parameters #20032 -- Win_nssm module doesn't honour the order of the app_parameters

Now that the codebase is fresh, maybe you can pinpoint issues to logic just from memory ;-)

[dagwieers]

The only thing that would be nice to have is check-mode support.

[ksubileau]

I tried to list existing issues for this module and I found some, but not the ones you listed. But indeed while using this module and doing this PR, I encountered some of these issues and fix them:

#35442: covered by tests and fixed (see test named install and start service).

#38557: I can't test on Windows 7, but the part of code pointed in the issue has been replaced by 20a0d90, so this should be fixed.

#20625: I also encountered the same issues (and several other parameters was not indempotent too). It's now covered by tests and should be fixed. The module is now idempotent.

I choose to implement this behavior, let me know if you didn't agree with that:
-app_parameters_free_form: the string passed in the playbook should be kept as is, regardless of its content (except yaml escaping)
-app_parameters: the key or value is escaped and/or surround with quotes if needed (if it contain spaces, quotes, backslashes...), so that the service should receive the same string as parameter

#20032 : I saw that but I didn't fixed, as we can see in the test install service with dict parameters. But I think this cannot be fixed easily, as the hashtable type doesn't preserve order. This has already been discussed here: #25265 (comment). I think this need either bigger changes (maybe including core changes) or rather a redesign of the module UI.
=> I suggest removing app_parameters_free_form, have app_parameters accept both a simple string or a list, as it's done in other modules like win_package, and abandon the dict support (IMHO not really helpful).

#44176 isn't covered. There is also #33159. In my use case I also encountered the need to add more parameters (like DisplayName and Description). I plan to add this and open other PR if I have time (and also for the check mode).
Nevertheless, the nssm's options list is quite long and I wonder if it would not be simpler to have a catch-all parameter that would support all of them at once, like this:

---
- win_nssm:
    name: nssm_test
    application: 'C:\Windows\System32\cmd.exe'
    options:
        AppAffinity: All
        AppThrottle: 1500
        ....

[jborean93]

Thanks for having a look at this, a few breaking changes we need to fix up for backwards compatibility. We should be looking to simplify a lot more in this module that I spoke about in the comments but I don't want to hold up this PR if you want to tackle that later on.

[jborean93]

I've never used this module before but after looking at the existing UI I'm quite confused. Honestly I think we should look into simplifying it a bit more. My thoughts are;

• app_parameters is designed to take in a dict/hashtable according to the docs and that seems wrong
• app_parameters_free_form is meant to be a string of parameters which is more what I expect but the name of the option isn't that great
• Ultimately a dict/hashtable is a poor way of defining some args considering the YAML syntax makes it hard to start keys with -

I'll leave it up to you whether you want to address this in this change or in a future PR but I think we should do is;

• deprecate the app_parameters_free_form option
• have app_parameters accept a string, list, or dict/hashtable but deprecate the dict/hashtable form as it is confusing
• potentially change app_parameters to be just arguments or parameters and have app_parameters as an alias for this

Let me know your thoughts, obviously you used or have a need for this module so would know more.

[ksubileau]

It's also exactly what I think, as I wrote in my previous comment in a summary form:

=> I suggest removing app_parameters_free_form, have app_parameters accept both a simple string or a list, as it's done in other modules like win_package, and abandon the dict support (IMHO not really helpful).

However, for the renaming of app_parameters, I'm less convinced because this name match the internal name used by nssm, but this can be discussed later:

The goal of this first PR remains to fix the current codebase in the simplest and fastest way so that it can be merged quickly and maybe backported.

I totally agree that this module would require a lot more changes, improvements, and features addition (including check mode support), but I prefer to address this in futures PRs to avoid mixing bug fixes, feature requests and breaking changes and delay this first PR ;)

[jborean93]

Cool if the nssm refers to it as AppParameters then keeping the name sounds good, I wasn't aware of this. Perfectly fine for all this to be resolved in future PRs just wanted to bring out my concerns with the current UI.

[jborean93]

As stated in https://github.com/jborean93, let's add an explicit check and error our if the value is a Hashtable and not a string. We should also deprecate this option and remove in 2.12.

[jborean93]

As much as I would like to force it being a dict/hashtable we still need to support the older option (string representing a dict). We should be adding a deprecation warning saying that this format will be removed in 4 releases. In reality we should remove the whole dict/hashtable part as well but see my comment above.

[ksubileau]

I saw a similar comment on issue #25265 (comment), but I don't understand what is it exactly.

The module documentation doesn't give any example of this usage. Is it really something that is supposed to be supported ? Where does it come from ?

That said, from my experience, currently the app_parameters parameter doesn't work at all on any of the supported Ansible versions (see #25265), so maybe it's not a big deal to break a little while fixing almost everything else, as a first step before doing what is said in the comment above...

Also, if we keep this support, and in the perspective of merging the app_parameters_free_form and app_parameters options as said before, so that app_parameters support a simple string, how can we distinguish a chain supposed to be a dict/hashtable and a simple string ?

[jborean93]

I'm honestly not sure, I need to play around with it tomorrow and have a closer look at the code. Weird things are happening in this module so I'll get back to you tomorrow about this.

[ksubileau]

Any update on this? Have you had the time to look into this?

[jborean93]

Ok, sorry @ksubileau for taking so long but here is what I found out

• The docs for win_nssm gives examples on specifying app_parameters as a literal YMAL dict which worked in previous versions of Ansible
• The commit ff1efb2#diff-5ba1945628b98dfd2ff1c1893772b39a added -type "str" to the value which meant a the value would have been converted using .ToString(), in this case a hashtable ToString value is System.Collections.Hashtable
• The commit above effective broke compatibility with someone specifying a literal dict but app_parameters still supports the key=value format
• This was added in the 2.3.0 release so this example has been broken since then

What I think we should do is;

• Update the existing docs in a separate PR to show that examples using the key=value syntax
• Don't re-add support for specifying a dict, we've already ripped the bandaid off so there's no sense in us going through the pain once again if we planned on deprecating it
• Deprecate app_parameters saying it will be removed in Ansible 2.12 as supporting a key=value syntax for arguments is a bit silly
• Alias app_parameters_free_form to a new option called arguments of the type str

I know you said the name AppParameters is used internally in nssm but I think we should still move away from that and use something that is a bit more standard in Ansible. My reasoning for this are;

• The gui for editing an nssm service has the relevant field as Arguments
• The install command line docs at nssm show the value as <arguments>
• There is a section around add/change/querying individual parameters that align with the AppParameters name but Ansible is more a state driven engine than an imperative action on individual args, e.g. a definition for win_nssm is more about specifying all the args for the service and not removing a particular one
• The screenshot you sent through was for the registry so it's more an internal naming standard in nssm and not necessarily what they expose to end users

Hopefully this makes sense but ultimately it means we can get rid of the Hashtable handling code and cordon off the key=value until 2.12 where we can then remove it.

[ksubileau]

Ok, I understand your reasoning but indeed I made the opposite reasoning while doing this PR. For me, what is specified in the module's current documentation is what should work. So I fixed the code so that the YAML dict notation works, and I tried to make only minimal changes so that this PR could be backported easily.
In the other side, AFAIK the key=value syntax for this option has never been documented, so I think we can assume that very few users use it.

So, taking into account your previous comment, here is my proposal:

• In this bugfix PR, for the upcoming 2.7 and possibly backported in 2.6:

  • Restore support of the key=value format (broken with the current code of this PR).
  • Keep support for specifying a YAML dict. Broken since 2.3.0, but that's what's specified in the doc, the work is already done, and the code will be shared with the key=value format. If this PR is backported, it will work as specified in the doc for versions 2.6.x and 2.7
  • Eventually update the doc to show examples using the key=value syntax (here or in a separate PR)

• In a next feature PR, for 2.8 or another future release:

  • Deprecate app_parameters
  • Alias app_parameters_free_form to a new option called arguments
  • Add support of list form to arguments
  • Maybe also add check mode, add more NSSM options like AppDirectory, and more...

I've already started working on this next PR, and the first three points are already done (see #45693)

Let me know if you agree with this plan, I think this may be a good compromise ;)

[jborean93]

No worries, I totally understand your reasoning with adding back the dict support but because we have had 4 (soon to be 5) releases where the YAML dict format has not worked I don't see the point in adding it back in considering no playbook since 2.2 would have worked.

Are you on IRC, it would be great to talk to you in an easier fashion and we can sum up what to do in one post once we've come to an agreement? My nickname on there is jborean93 but we can also just chat on the #ansible-windows channel.

[jborean93]

We will still need to call this for backwards compatibility, would probably need a check to ensure the string is in this form so we can fire the warning.

[jborean93]

I'm not sure if separating by a space is better than a comma, a service name can definitely have a space and this break backwards compatibility.

Instead we should change the type of dependencies to be a list which allows people to define it like

dependencies
- adf
- tcpip

This means they user doesn't have to deal with escaping issues for complex service names as well as keeping backwards compatibilities as the code already splits by ,. This is all done in Get-AnsibleParam so no logic is required in win_nssm.

[ksubileau]

Neither space nor comma are good separators in fact, as a service name can also have a comma (and probably any printable ASCII character except slash and backslashes):

But again here is a simple and easily backportable fix to get current and previous versions to work almost well, because the current code just doesn't work if you use a comma as separator (the module doesn't splits by ,, it pass the string as is to NSSM and an error is thrown saying that the service "service1,service2" does not exist). In fact the doc does not reflect the current code (or the other way). So from my point of view this doesn't really break BC. Nevertheless I can restore comma easily, just a single char to change in code. Either we match the doc to the current code, or the opposite ;)

I also thought about the list form and I think it will be much better for the future, but this may be part of following PRs. I actually prepared the code in this PR for this purpose, as I already use arrays by splitting the string to be able to do an order insensitive comparison for idempotence check:

ansible/lib/ansible/modules/windows/win_nssm.ps1

Lines 434 to 437 in 51843a7

	$dependencies_array = @($dependencies.split(" ") | where { $_ -ne '' })
	$current_dependencies = @($nssm_result.stdout.split("`n`r") | where { $_ -ne '' })
	If (@(Compare-Object -ReferenceObject $current_dependencies -DifferenceObject $dependencies_array).Length -ne 0) {

[jborean93]

Yep agreed that services can have spaces or commas, this is why setting the type for this in the Get-AnsibleParam to list means someone can define a list in the playbook side and not have to worry about that. While it "may" not work today as a comma separated string, Legacy.psm1 has internal logic to split a string by , if type=list. Having it split by a space goes against this logic and really doesn't need to be done in win_nssm.

[ksubileau]

Ok, after a second look, I understand your point, and you're perfetly right. Do we need to change something in the doc to specify that a list is accepted ?

By the way, it makes me find another issue with this module: if you give a name containing a quote to your service, the module fails because the quote is not escaped in the commands run by the module...

- win_nssm:
    name: test"quote
    application: 'C:\Windows\System32\cmd.exe'

In a future PR, I think we need to consider refactoring the code more in depth so that all the command-line parameters go through the Escape-Argument/Argv-ToString functions, as other module parameters may be affected too.

[jborean93]

As stated in https://github.com/jborean93, let's add an explicit check and error our if the value is a Hashtable and not a string. We should also deprecate this option and remove in 2.12.

[jborean93]

Let's rename to arguments and have app_parameters_free_form as an alias for this. Up to you as to whether you want to add support for a list or arguments or not but my recommendation is to keep that for another PR.

[ksubileau]

As discussed throught IRC with @jborean93, I restored the support of key=value format and dropped the YAML dict format for the app_parameters option. An error is thrown if the value is not a string. I also updated the examples in documentation.
Perhaps we should also add a sentence in the module's documentation stating that the parameters order may not be preserved, as this misbehavior cannot be easily fixed ?
ready_for_review

[jborean93]

Awesome @ksubileau thanks for persisting with this, definitely getting on the right track to cleaning up this module.

[jborean93]

@ksubileau thanks once again, it would be great if you could create a backport PR for stable-2.7 and stable-2.6. Once in those branches they will appear in the next minor release for those versions.

[ksubileau]

Glad to contribute to this wonderful project!

Backport PRs
stable-2.6 #46306
stable-2.7 #46305

Next step for 2.8 : #45693