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

win_iis_webapplication: add authentication parameters #56033

Merged
merged 10 commits into from Nov 15, 2019

Conversation

mhunsber
Copy link
Contributor

@mhunsber mhunsber commented May 2, 2019

SUMMARY

When setting up an application in IIS, you can specify the authentication type as either Application User (pass-through authentication) or specific user with a username and password. This just adds the parameters to support that.

I also added tests since it didn't seem like this module had any integration tests yet.

Added the following parameters to win_iis_webapplication: connect_as, username, password.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

win_iis_webapplication

ADDITIONAL INFORMATION

you can do something like this

win_iis_webapplication:
  state: present
  site: ACME
  name: api
  connect_as: specific_user
  username: acme_user
  password: secret

@ansibot
Copy link
Contributor

ansibot commented May 2, 2019

@ansibot ansibot added affects_2.9 This issue/PR affects Ansible v2.9 core_review In order to be merged, this PR must follow the core review workflow. feature This issue/PR relates to a feature request. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. windows Windows community needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels May 2, 2019
added version added to new options.
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels May 2, 2019
@@ -13,6 +13,9 @@ $site = Get-AnsibleParam -obj $params -name "site" -type "str" -failifempty $tru
$state = Get-AnsibleParam -obj $params -name "state" -type "str" -default "present" -validateset "absent","present"
$physical_path = Get-AnsibleParam -obj $params -name "physical_path" -type "str" -aliases "path"
$application_pool = Get-AnsibleParam -obj $params -name "application_pool" -type "str"
$connect_as = Get-AnsibleParam -obj $params -name 'connect_as' -type 'str' -validateset 'specific_user', 'pass_through'
Copy link
Contributor

Choose a reason for hiding this comment

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

You should add a default option of pass_through

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this were a new module, I would make that the default option, but there could be tasks out there using this module where they have changed the authentication to specific user a different way. Making a default value on this option would break that configuration.
Setting a default also wouldn't be consistent with the behavior of application_pool and physical_path, where they are only set if the user passes them as parameters.

Copy link
Contributor

Choose a reason for hiding this comment

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

you are absolutely right, missed that

@ShachafGoldstein
Copy link
Contributor

Should add a changelog fragment and update the .py file for documentation

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label May 13, 2019
@samdoran samdoran removed the needs_triage Needs a first human triage before being processed. label May 14, 2019
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. core_review In order to be merged, this PR must follow the core review workflow. labels Jun 6, 2019
@mhunsber
Copy link
Contributor Author

mhunsber commented Jun 6, 2019

@ShachafGoldstein I added the changelog fragment. The .py documentation was already updated to show the new options.
I don't really know what happened during the integration test. Looks like it failed on the first task of the tests, which uses the win_iis_website module. Complaining that "Configuration file is not well-formed XML", but the tests don't do anything to change the configuration file before that, unless win_copy somehow changed it when placing a backup configuration in the remote temporary directory. This only happened on the Windows 2019 server, which I think is new since the last CI.

@ShachafGoldstein
Copy link
Contributor

close and open the PR to make it run the tests again

@mhunsber mhunsber closed this Jun 6, 2019
@mhunsber mhunsber reopened this Jun 6, 2019
@mhunsber
Copy link
Contributor Author

mhunsber commented Jun 6, 2019

Failed on 2012-R2 this time, same error message as before.

@ShachafGoldstein
Copy link
Contributor

Only thing I can think of is that the copy somehow damages the file.
try coping to a different path or skip the copy just to check it maybe?

Micah Hunsberger added 3 commits June 7, 2019 11:01
remove unused iis version check
test checksum of iis configuration after backup
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jun 7, 2019
@mhunsber
Copy link
Contributor Author

mhunsber commented Jun 7, 2019

Adding some extra cleanup tasks before restoring the previous IIS configuration seemed to work. After looking closer I realized it was failing on the restore copy because IIS was still using the configuration file. Then the tests were run again in verbose mode, which is where we were seeing the failure in the website module.

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jun 15, 2019
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Nov 5, 2019
@mhunsber
Copy link
Contributor Author

mhunsber commented Nov 5, 2019

No idea why Server 2008R2 doesn't like when application_pool is null, I'll just skip it for now and run the tests on 2012 and higher. I'm guessing that it is something to do with the PowerShell version or web administration module version on the host. Maybe someone in the future will decide to make it work on 2008R2.

My guess is that it's either because of the splatting of parameters done here: https://github.com/ansible/ansible/pull/56033/files#diff-bd862e2bbcb44c9c332df7b6cacf6adaR55
or somehow the if($application_pool) is returning true and adding it as a parameter for the splat.

@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Nov 5, 2019
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Nov 13, 2019
@ansibot ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Nov 15, 2019
@jborean93
Copy link
Contributor

Thanks for the contribution and adding the tests as well. Apologies for the delay and thanks for sticking with it.

@jborean93 jborean93 merged commit 8ff6e4c into ansible:devel Nov 15, 2019
anshulbehl pushed a commit to anshulbehl/ansible that referenced this pull request Dec 10, 2019
* add connect_as, username, password parameters
add tests

* fixed reference to undefined variable.
added version added to new options.

* add changelog fragment

* fix line endings

* use ansible facts to determine os version
remove unused iis version check
test checksum of iis configuration after backup

* correct assertion

* added more cleanup tasks.

* version added is now 2.10

* skip server 2008 r2 for now

* run tests on server 2012 and higher
@ansible ansible locked and limited conversation to collaborators Dec 17, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.9 This issue/PR affects Ansible v2.9 core_review In order to be merged, this PR must follow the core review workflow. feature This issue/PR relates to a feature request. has_issue module This issue/PR relates to a module. support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. windows Windows community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants