Support for client identifiers other than just simple mac addresses

[arw-thomasm]

SUMMARY

Added support for client identifiers other than a simple mac address

ISSUE TYPE

• Feature Pull Request

COMPONENT NAME

win_dhcp_lease.ps1

ADDITIONAL INFORMATION

As stated in RFC2131, "the 'client identifier' is an opaque key, not to be interpreted by the server; for example, the 'client identifier' may contain a hardware address, identical to the contents of the 'chaddr' field, or it may contain another type of identifier, such as a DNS name. The 'client identifier' chosen by a DHCP client MUST be unique to that client within the subnet to which the client is attached."

A Windows DHCP server supports client identifiers with at least 2 characters (shorter identifiers will be prefixed with a 0) and allows a maximum length of 510 characters (this seems to be a bug in at least Windows Server 2016 Standard, as the server becomes unstable and crashes, when adding reservations with identifiers longer than 510 characters (without dashes)). Also the number of characters of an identifier has to be an even number, as otherwise Windows truncates the string automatically.

Windows only supports characters from a-f, A-F and 0-9, which imho conflicts with RFC2131, which makes no restrictions regarding the character set.

Currently, the module only supports client identifiers which are 12 or 17 characters long and there is not charset validation implemented. I added support for client identifiers from 2-510 characters long and with a validated charset a-f, A-F and 0-9. I also sanitize strings like MAC addresse (::...) and notations like (--...). I convert any strings into the format Windows uses (--*...).

[jborean93]

Hi

Thanks for your contribution, I just wanted to let you know that I am taking some time off and won't be able to review this PR until I get back in a few weeks. Please don't take the silence as I am ignoring your work, just that I won't be around to look at it for a while.

In the meantime it would be great if you could resolve the sanity issues and integration test problems and have the PR in a merge-able condition while you wait. You can ignore the 20s failures with unable to install the Galaxy deps, the problematic ones are the sanity and actual test failures.

If you have any questions please direct them to #ansible-windows on Libera chat, details on this can be found at https://docs.ansible.com/ansible/latest/community/communication.html#ansible-community-on-irc.

Thanks

[arw-thomasm]

Hi,

I fixed the sanity issues and also the integration test issue. Sorry for that, this is my first contribution to an open source project :-) and I used a new IDE where I forgot to configure automatic removing of trailing spaces.

What I do not understand is, why the sanity checks for Ansible-Devel and Ansible-2.13 fail with the message:

00:25 ERROR: Found 1 pslint issue(s) which need to be resolved:
00:25 ERROR: plugins/modules/win_dhcp_lease.ps1:142:5: PSPlaceCloseBrace: Close brace does not follow a new line. (100%)

The corresponding lines look like this:

[...]
# MacAddress was specified
if ($mac) {

    $mac = Convert-MacAddress -mac $mac

    if ($mac -eq $false) {
        $module.FailJson("The MAC Address is not properly formatted")
    } else {
        $current_lease = Get-DhcpServerv4Scope | Get-DhcpServerv4Lease | Where-Object ClientId -eq $mac
    }

}

# Did we find a lease/reservation
[...]

So there is a new line after the closing brace of the if/else statement...

[jborean93]

The problem is the } else { should be the follow due to the PSScriptAnalyzer rules we have enabled

}
else {

If you use Visual Studio Code the settings for this project has enabled formatOnSave as per https://github.com/ansible-collections/community.windows/blob/main/.vscode/settings.json that will automatically reformat your code into the style required by the project. This allows you write it in whatever style you wish but it will auto fix up some problems like the one you've seen. The integration test failures aren't a problem with your code but a separate thing entirely. You can ignore that for now.

[jborean93]

Please also keep in mind there is a separate PR that is touching the same code as this one. Whatever one gets merged first will most likely require a rebase. Finally adding a changelog fragment as per https://docs.ansible.com/ansible/latest/community/development_process.html#creating-a-changelog-fragment will help document what has changed in the upcoming CHANGELOG when this is released.

[jborean93]

There still seems to be some santiy problems this time with some whitespace after some lines like https://github.com/ansible-collections/community.windows/pull/430/files#diff-ce522e74d063082e388298a19ece018063ce5a7e4ad6e17033f5ed7f6dccfd94R236.

[jborean93]

Looks like there are some conflicts with existing changes to the file, these will have to be resolved before we can look further into the PR.

[github-actions[bot]]

This pull request is stale because it has been open for 4 weeks with no activity. Remove stale label or comment or this will be closed in 2 weeks.