search

ansible-collections / community.windows

Windows community collection for Ansible
https://galaxy.ansible.com/community/windows
GNU General Public License v3.0
199 stars 152 forks source link

win_dhcp_lease always changes a reservation #407

Open netsandbox opened 2 years ago

netsandbox commented 2 years ago
SUMMARY

If you call the win_dhcp_lease module for the same reservation multiple times with the same values, the reservation is always changed.

ISSUE TYPE
COMPONENT NAME

win_dhcp_lease

ANSIBLE VERSION
ansible [core 2.13.0]
  config file = /home/user/code/repo/dhcp-dns/ansible.cfg
  configured module search path = ['/home/user/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /home/user/code/repo/dhcp-dns/venv/lib/python3.8/site-packages/ansible
  ansible collection location = /home/user/code/repo/dhcp-dns/collections
  executable location = /home/user/code/repo/dhcp-dns/venv/bin/ansible
  python version = 3.8.10 (default, Mar 15 2022, 12:22:08) [GCC 9.4.0]
  jinja version = 3.1.2
  libyaml = True
COLLECTION VERSION
# /home/user/code/repo/dhcp-dns/collections/ansible_collections
Collection        Version
----------------- -------
community.windows 1.10.0
CONFIGURATION
COLLECTIONS_PATHS(/home/user/code/repo/dhcp-dns/ansible.cfg) = ['/home/user/code/repo/dhcp-dns/collections']
DEFAULT_HOST_LIST(/home/user/code/repo/dhcp-dns/ansible.cfg) = ['/home/user/code/repo/dhcp-dns/inventory.ini']
DISPLAY_SKIPPED_HOSTS(/home/user/code/repo/dhcp-dns/ansible.cfg) = False
HOST_KEY_CHECKING(/home/user/code/repo/dhcp-dns/ansible.cfg) = False
RETRY_FILES_ENABLED(/home/user/code/repo/dhcp-dns/ansible.cfg) = False
OS / ENVIRONMENT

Target OS: Microsoft Windows Server 2019 Standard 10.0.17763.0

STEPS TO REPRODUCE

Step 1: create reservation

$ ansible all -m community.windows.win_dhcp_lease -a 'type=reservation ip=10.20.0.100 scope_id=10.20.0.0 mac=00-50-56-8f-29-ea description=ansible-testing reservation_name=ansible-testing'
ad | CHANGED => {
    "changed": true,
    "lease": {
        "address_state": "InactiveReservation",
        "client_id": "00-50-56-8f-29-ea",
        "description": "ansible-testing",
        "ip_address": "10.20.0.100",
        "name": "",
        "scope_id": "10.20.0.0"
    }
}

Step 2: confirm reservation in AD

$ ansible all -m ansible.windows.win_shell -a "Get-DhcpServerv4Scope | Get-DhcpServerv4Lease | Where-Object IPAddress -eq 10.20.0.100 | Format-List"
ad | CHANGED | rc=0 >>

IPAddress       : 10.20.0.100
ScopeId         : 10.20.0.0
Description     : ansible-testing
ClientId        : 00-50-56-8f-29-ea
HostName        : 
ClientType      : None
AddressState    : InactiveReservation
DnsRR           : NoRegistration
DnsRegistration : NotApplicable
LeaseExpiryTime : 
NapCapable      : False
NapStatus       : FullAccess
ProbationEnds   : 
PolicyName      : 
ServerIP        : 255.255.255.255

Step 3: start client OS

Step 4: confirm reservation in AD

$ ansible all -m ansible.windows.win_shell -a "Get-DhcpServerv4Scope | Get-DhcpServerv4Lease | Where-Object IPAddress -eq 10.20.0.100 | Format-List"
ad | CHANGED | rc=0 >>

IPAddress       : 10.20.0.100
ScopeId         : 10.20.0.0
Description     : ansible-testing
ClientId        : 00-50-56-8f-29-ea
HostName        : ansible-testing
ClientType      : Dhcp
AddressState    : ActiveReservation
DnsRR           : NoRegistration
DnsRegistration : NotApplicable
LeaseExpiryTime : 
NapCapable      : False
NapStatus       : FullAccess
ProbationEnds   : 
PolicyName      : 
ServerIP        : 10.20.0.10

Compared to Step 2, the following fields changed in AD: (HostName), ClientType, AddressState and ServerIP

Step 5: run Ansible again with the same command as in Step 1

$ ansible all -m community.windows.win_dhcp_lease -a 'type=reservation ip=10.20.0.100 scope_id=10.20.0.0 mac=00-50-56-8f-29-ea description=ansible-testing reservation_name=ansible-testing'
ad | CHANGED => {
    "changed": true,
    "lease": {
        "address_state": "InactiveReservation",
        "client_id": "00-50-56-8f-29-ea",
        "description": "ansible-testing",
        "ip_address": "10.20.0.100",
        "name": null,
        "scope_id": "10.20.0.0"
    }
}

Step 6: confirm reservation in AD

$ ansible all -m ansible.windows.win_shell -a "Get-DhcpServerv4Scope | Get-DhcpServerv4Lease | Where-Object IPAddress -eq 10.20.0.100 | Format-List"
ad | CHANGED | rc=0 >>

IPAddress       : 10.20.0.100
ScopeId         : 10.20.0.0
Description     : ansible-testing
ClientId        : 00-50-56-8f-29-ea
HostName        : ansible-testing
ClientType      : None
AddressState    : InactiveReservation
DnsRR           : NoRegistration
DnsRegistration : NotApplicable
LeaseExpiryTime : 
NapCapable      : False
NapStatus       : FullAccess
ProbationEnds   : 
PolicyName      : 
ServerIP        : 255.255.255.255

Compared to Step 4, the following fields changed in AD: ClientType, AddressState and ServerIP

If the client now renew the DHCP lease, you will see AD changes as in Step 4, and if you then run again the Ansible command as in Step 5, you will see the same changes as in Step 6.

EXPECTED RESULTS

The Ansible command in Step 5 doesn't change the AD DHCP reservation.

ACTUAL RESULTS

The Ansible command in Step 5 change the AD DHCP reservation.

tigattack commented 2 years ago

+1. This happens because the reservation is not set correctly, per the output of step 2 (HostName is null). You can also see the name attribute passed to win_dhcp_lease.ps1 is null if you run ansible with more verbosity.
However, it is set correctly if you run step 1 twice, though weirdly it doesn't report as changed even though on the second run it actually sets the reservation name.

I think this is because win_dhcp_lease.ps1#L431 simply uses the data from the lease created earlier, which doesn't use $reservation_name. If you then run it again, you hit win_dhcp_lease.ps1#L342 which updates the lease with the reservation name. Without further investigation, I'm not sure why this second change doesn't actually report back to ansible as changed.

netsandbox commented 2 years ago

@tigattack I can confirm that running step 1 twice helps in the situation you described.

But as noted above, I see the most important problem with the change in step 5, because this will always change the reservation after the host has renewed the lease.

tigattack commented 2 years ago

Yes, agreed. I think this is most likely because the AddressState attribute of a reservation object is being compared (win_dhcp_lease.ps1#L77) when deciding whether Ansible needs to make a change or not.

I can think of little functional need for this, but I could be missing something. Hopefully the maintainer(s) of this module can provide some insight.

netsandbox commented 2 years ago

I did some testing today. The problem is that in https://github.com/ansible-collections/community.windows/blob/a45cc22674a1f4e16702622ab3b5d6b41c920396/plugins/modules/win_dhcp_lease.ps1#L342 the reservation is always updated. And Set-DhcpServerv4Reservation always update the reservation and set the AddressState to InactiveReservation. And as Set-DhcpServerv4Reservation don't have a AddressState attribute, you can't suppress the change.

The only solution I see here is to compare the $params values with the $current_lease values and update $current_lease only if there is a change in https://github.com/ansible-collections/community.windows/blob/a45cc22674a1f4e16702622ab3b5d6b41c920396/plugins/modules/win_dhcp_lease.ps1#L312-L342