Add bindings to website #20

shahargolshani · 2025-01-27T11:44:00Z

SUMMARY

Add bindings setup option to website module

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

plugins/modules/website.ps1
plugins/modules/website.yml
plugins/modules/website_info.ps1
plugins/modules/website_info.yml
tests/integration/targets/website/tasks/bindings.yml
tests/integration/targets/website/tasks/main.yml
tests/integration/targets/website/tasks/tests.yml
tests/integration/targets/website_info/tasks/tests.yml

ADDITIONAL INFORMATION

Add bindings with add/set/remove option

softwarefactory-project-zuul · 2025-01-27T11:54:51Z

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/469e899d83604069bbc141fda7614f21

✔️ ansible-galaxy-importer SUCCESS in 4m 38s
✔️ build-ansible-collection SUCCESS in 5m 14s

softwarefactory-project-zuul · 2025-01-27T14:55:06Z

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/09eed8c7985747a28fc8ecf0b4a73c5a

✔️ ansible-galaxy-importer SUCCESS in 4m 36s
✔️ build-ansible-collection SUCCESS in 5m 30s

softwarefactory-project-zuul · 2025-01-27T15:18:02Z

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/8f5075e1b9ed43e788c25fbf68c1dc98

✔️ ansible-galaxy-importer SUCCESS in 5m 44s
✔️ build-ansible-collection SUCCESS in 5m 24s

softwarefactory-project-zuul · 2025-01-27T16:09:50Z

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/944f137ecdc74008adbba50743947db2

✔️ ansible-galaxy-importer SUCCESS in 7m 53s
✔️ build-ansible-collection SUCCESS in 5m 40s

jborean93

Looks pretty good. I think we should adjust the ssl_flags side and look a bit deeper into how to edit existing bindings and wanting to change the SSL options like the flags or cert hashes.

jborean93 · 2025-01-27T20:03:22Z

plugins/modules/website.ps1

+        require_server_name_indication = @{ type = 'bool'; default = $false }
+        use_centrelized_certificate_store = @{ type = 'bool'; default = $false }
+        certificate_hash = @{ type = 'str' }
+        certificate_store_name = @{ type = 'str' }


I think we should have a default here, looks like win_iis_webbinding used my https://docs.ansible.com/ansible/latest/collections/community/windows/win_iis_webbinding_module.html#parameter-certificate_store_name. This means we don't need the required_together option below when we have a default.

I thought about removing it because in the community module this default was set in code and not in $params

#apply default for cert store name If (-Not $certificateStoreName) { $certificateStoreName = 'my' }

community $params specified -default ([string]::Empty) an empty string as the default and then used it to inform the user that this value cannot be set together with CCS

When the default is set in $spec I didn't find a way to know if the value came from the user or from the default value
Will it be ok to just ignore it if it was set by the user but not used ?

We can also use the same approach the community used, but I guess it is not the best practice to set a default outside the $spec and more error prone.

Will it be ok to just ignore it if it was set by the user but not used ?

I think so, having this as a default, especially to My would cover most scenarios allowing people to only need to set it if they have it stored elsewhere. I think for the CCS check we should only check if the certificate hash option is set and just ignore this value.

Default value "my" was added to certificate_store_name

jborean93 · 2025-01-27T20:14:51Z

plugins/modules/website.ps1

+        require_server_name_indication = @{ type = 'bool'; default = $false }
+        use_centrelized_certificate_store = @{ type = 'bool'; default = $false }


I think we should still keep this as ssl_flags but but set the choices to the more human friendly names

Suggested change

require_server_name_indication = @{ type = 'bool'; default = $false }

use_centrelized_certificate_store = @{ type = 'bool'; default = $false }

ssl_flags = @{

type = 'str'

choices = @('none', 'use_sni', 'use_centralized_store', 'use_centralized_store_and_sni')

default = 'none'

}

The reason why I went this direction is that we can easily map the string values to the raw integer one and we don't know what future values may be added. Using two separate options here could be problematic if MS ever add more options that aren't tied specifically to some combination of SNI and the centralised store setup whereas now with ssl_flags we just reflect the underlying value with a human friendly label.

https://learn.microsoft.com/en-us/iis/configuration/system.applicationhost/sites/site/bindings/binding#attributes

I agree this is applicable for ssl_flags 0-3
And we really need to think which approach we are taking here
if we go with pwsh:
New-WebBinding -SslFlags $ssl_flags
or with IIS GUI checkboxes

I got the idea of separating it from this community issue:
ansible-collections/community.windows#584

Apparently sslFlags can have values above 3 as a combination of the selected SSL options
SNI and CCS are not really tied to each other it is just set by MS in a single value
If in the future we decide to include more ssl_flags
having a single ssl_flags parameter will make the ssl_flags choices long and complicated because we'll have to include all combinations.

so if there are 3 ssl options we get to 8 ssl_flags choices if we include all 7 options we'll need 128 (2^7) choices
which is of course not reasonable. (In theory, In practice some combinations are not possible but still it's too many options)

I don't know why MS documents sslFlags as 0-3 and also limits the cli to those values i.e.
New-WebBinding : Parameter 'SslFlags' value is out of range: 0-3
While it can be set to values above 3

Regarding setting the ssl_flags int value out of a human friendly value it can be done
as MS set every option in the power of 2 so it can be easily map to integer value with
x = 1/2/4/8/16/32/64/128
ssl_flags += x

And derived in the info module with
[math]::Floor($ssl_flags / x)
and $ssl_flags % 2

Ah ok, that's a pity these extra flags are not documented on that page, it made it seem like they were enum values and not bit fields.

It makes more sense to have them as separate options like you've done here because of that, basically 0-3 can be calculated through a combination of these two options being true/false. I think maybe we should rename them to something a bit shorter like the below and just expand the acronyms in the description

use_sni

use_ccs

It does bring into question how does the hostname value work if use_sni is false. Can you have an empty hostname in either scenario. Does it need to be set if use_sni is true.

If we ever do support those extra flags in the future we could expose them either as a list/str under ssl_flags or their own bool option. The calculation internally would become

$ssl_flags = 0 if ($disable_http2) { $ssl_flags = $ssl_flags -bor 4 }

But we don't need to worry about that for this implementation., as long as it is possible to do so in the future and I think what you've done enables that here.

jborean93 · 2025-01-27T20:16:38Z

plugins/modules/website.ps1

@@ -130,6 +151,84 @@ Try {
                $module.Result.changed = $true
            }
        }
+        # Add Remove or Set bindings if needed
+        if ($bindings) {


In pwsh an empty hashtable is automatically true when checked like this. If we are wanting to see if add/remove/set is present we would need to do the following to check the entry count as a truthy value where 0 is $false.

Suggested change

if ($bindings) {

if ($bindings.Count) {

Yes, I see that even when set/add/remove are all null still
$bindings.Count is
3

So maybe go further here with

if ( $null -ne $bindings.set -or $bindings.add.Count -gt 0 -or $bindings.remove.Count -gt 0 )

Ah yes because the default module args would be to populate the inner dictionary with the add/set/remove defaults. I think what you've proposed makes sense to me.

plugins/modules/website.ps1

jborean93 · 2025-01-27T20:23:19Z

plugins/modules/website.yml

@@ -64,16 +181,26 @@ EXAMPLES: |
      physical_path: C:\inetpub\wwwroot
      state: started

-  - name: Create a website, set all values and start it
+  - name: Create a website with two bindings, set all values and starts it


We should look at adding a few more examples for things like

set: [] to remove all bindings

Some with add/remove to show how they can be specified.

More examples were added

jborean93 · 2025-01-27T20:24:19Z

plugins/modules/website_info.ps1

+            if ($certificate_hash) {
+                $psObject | Add-Member -MemberType NoteProperty -Name "certificate_hash" -Value $certificate_hash
+            }
+            if ($certificate_store_name) {
+                $psObject | Add-Member -MemberType NoteProperty -Name "certificate_store_name" -Value $certificate_store_name
+            }


We should always add these, just keep them $null if unset.

certificate_hash & certificate_store_name always returned

jborean93 · 2025-01-27T20:24:58Z

plugins/modules/website_info.ps1

    $WebsiteInfoDict = @{
        name = $site.Name
-        id = $site.ID
+        site_id = $site.ID


Any reason for this change, we would need to update the return documentation to show this new new.

The original community parameter is site_id
I believe we should return the same parameter name site_id in the info module and it was missed in the initial website module migration so I fixed it here.
I will fix the documentation accordingly

Makes sense, thanks for clarifying.

jborean93 · 2025-01-27T20:26:19Z

tests/integration/targets/website/tasks/bindings.yml

+      RequestType = Cert
+    dest: "{{ item.dest }}"
+  with_items:
+    - {name: test.com, dest: 'c:\windows\temp\certreq1.txt'}


There's only 1 entry here so the loops here and below are not really needed. We should also look at using remote_tmp_path instead of C:\windows\temp to ensure it is cleaned up after the test.

All loops with a single iteration were removed
remote_tmp_path is used now for the certificate files
cleanup task of removing the files was removed from main.yml as it is redundant now

jborean93 · 2025-01-27T20:29:19Z

tests/integration/targets/website/tasks/main.yml

+
+  - name: remove certs
+    raw: 'remove-item cert:\localmachine\my\{{ item }} -force -ea silentlycontinue'
+    with_items:


We will want a when on this to check that thumbprint1 is defined. This is just so that if a test failed this will also not fail and stop the cleanup/hide the original error.

Suggested change

with_items:

when: thumbprint1 is defined

with_items:

Added
when: thumbprint1 is defined

softwarefactory-project-zuul · 2025-01-30T19:44:55Z

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/30f2e2f3bcac4d2a82d411b52e768263

✔️ ansible-galaxy-importer SUCCESS in 3m 45s
✔️ build-ansible-collection SUCCESS in 5m 15s

softwarefactory-project-zuul · 2025-01-30T20:04:47Z

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/bd1346754f7c4056958d72faf8da7a4e

✔️ ansible-galaxy-importer SUCCESS in 3m 43s
✔️ build-ansible-collection SUCCESS in 5m 28s

jborean93 · 2025-01-31T04:42:08Z

plugins/modules/website.ps1

+        use_sni = @{ type = 'bool'; default = $false }
+        use_ccs = @{ type = 'bool'; default = $false }


I think the default should be unset here (ultimately $null). That way we can differentiate between keep existing value (when null) or set/unset, e.g.

if ($null -ne $use_sni) { if ($use_sni) { 'test SNI' } else { 'unset SNI' } }

If we are creating a binding we can then treat null as false.

This will most likely make your SSLFlag builder a bit more complicated but you can probably get it working by adding in a parameter that is -DefaultSNI and -DefaultCCS that is set to the SNI and CCS value of the existing binding or $false if no binding is present. That way you can still validate all the necessary info, don't modify if the user hasn't set a value, and still have a sane default when creating the binding from scratch.

That makes sense,
If so then what about protocol that has a default value of http
Does it also need to be treated the same way ? and keep the same value that exist on the binding if $null ?

Regarding the unset of default ($null) from use_sni & use_ccs
In case of a new binding (toAdd) both $false/$null will not get into
if ($BindingInfo.use_sni)
if ($BindingInfo.use_ccs)
So no change is needed

In case of an existing binding (toEdit)
We can set the user values use_sni and/or use_ccs to the site existing values and then run
the function Get-SSLFlagFromBinding over it.

Yep, I think what you've said makes sense. Essentially the module arg spec has $null as the default to help distinguish between "keep existing value", turn on, turn off. If we are going from a http to https binding I think $null should also mean $false.

plugins/modules/website.ps1

softwarefactory-project-zuul · 2025-02-02T13:46:21Z

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/03e442934c654d0db58e6ea5214ab9d2

✔️ ansible-galaxy-importer SUCCESS in 3m 56s
✔️ build-ansible-collection SUCCESS in 5m 17s

fix func/param naming conventions

softwarefactory-project-zuul · 2025-02-02T14:02:05Z

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/c4a3e7e03a364022aed39883d27d43d8

✔️ ansible-galaxy-importer SUCCESS in 4m 23s
✔️ build-ansible-collection SUCCESS in 5m 43s

shahargolshani added 2 commits January 24, 2025 07:39

Add bindings to website

227d354

Edit binding protocol and sslFlags

263cb23

shahargolshani force-pushed the feature/add-bindings-to-website branch from 964eabb to e1154d6 Compare January 27, 2025 14:44

shahargolshani force-pushed the feature/add-bindings-to-website branch from e1154d6 to a5fa173 Compare January 27, 2025 15:05

shahargolshani force-pushed the feature/add-bindings-to-website branch from a5fa173 to 227d354 Compare January 27, 2025 15:55

jborean93 requested changes Jan 27, 2025

View reviewed changes

shahargolshani force-pushed the feature/add-bindings-to-website branch 2 times, most recently from 48e5a4b to 263cb23 Compare January 30, 2025 19:54

shahargolshani requested a review from jborean93 January 30, 2025 20:08

jborean93 requested changes Jan 31, 2025

View reviewed changes

unset use_sni and use_ccs default value

4ba3963

fix func/param naming conventions

shahargolshani force-pushed the feature/add-bindings-to-website branch from fc7cb00 to 4ba3963 Compare February 2, 2025 13:51

shahargolshani requested a review from jborean93 February 2, 2025 14:33

jborean93 approved these changes Feb 2, 2025

View reviewed changes

jborean93 merged commit 1252f90 into ansible-collections:main Feb 2, 2025
23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add bindings to website #20

Add bindings to website #20

shahargolshani commented Jan 27, 2025

softwarefactory-project-zuul bot commented Jan 27, 2025

softwarefactory-project-zuul bot commented Jan 27, 2025

softwarefactory-project-zuul bot commented Jan 27, 2025

softwarefactory-project-zuul bot commented Jan 27, 2025

jborean93 left a comment

jborean93 Jan 27, 2025

shahargolshani Jan 28, 2025

jborean93 Jan 28, 2025

shahargolshani Jan 30, 2025

jborean93 Jan 27, 2025

shahargolshani Jan 28, 2025

jborean93 Jan 28, 2025 •

edited

Loading

jborean93 Jan 27, 2025

shahargolshani Jan 28, 2025

jborean93 Jan 28, 2025

jborean93 Jan 27, 2025

shahargolshani Jan 30, 2025

jborean93 Jan 27, 2025

shahargolshani Jan 30, 2025

jborean93 Jan 27, 2025

shahargolshani Jan 28, 2025

jborean93 Jan 28, 2025

jborean93 Jan 27, 2025

shahargolshani Jan 30, 2025

jborean93 Jan 27, 2025

shahargolshani Jan 30, 2025

softwarefactory-project-zuul bot commented Jan 30, 2025

softwarefactory-project-zuul bot commented Jan 30, 2025

jborean93 Jan 31, 2025

shahargolshani Feb 2, 2025

jborean93 Feb 2, 2025

softwarefactory-project-zuul bot commented Feb 2, 2025

softwarefactory-project-zuul bot commented Feb 2, 2025

		require_server_name_indication = @{ type = 'bool'; default = $false }
		use_centrelized_certificate_store = @{ type = 'bool'; default = $false }

		use_sni = @{ type = 'bool'; default = $false }
		use_ccs = @{ type = 'bool'; default = $false }

Add bindings to website #20

Add bindings to website #20

Conversation

shahargolshani commented Jan 27, 2025

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

softwarefactory-project-zuul bot commented Jan 27, 2025

softwarefactory-project-zuul bot commented Jan 27, 2025

softwarefactory-project-zuul bot commented Jan 27, 2025

softwarefactory-project-zuul bot commented Jan 27, 2025

jborean93 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jborean93 Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

softwarefactory-project-zuul bot commented Jan 30, 2025

softwarefactory-project-zuul bot commented Jan 30, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

softwarefactory-project-zuul bot commented Feb 2, 2025

softwarefactory-project-zuul bot commented Feb 2, 2025

jborean93 Jan 28, 2025 •

edited

Loading