-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
ipa_host: Fix enabled and disabled states #8920
ipa_host: Fix enabled and disabled states #8920
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution!
plugins/modules/ipa_host.py
Outdated
) | ||
changed = False | ||
if state in ['present', 'enabled', 'disabled']: | ||
if state in ['present', 'enabled']: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change looks wrong. disabled
should (apparently) be a special case of present
, so the host exists, but is disabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, but if there are no host found It would create one which seems to be weird because enabled
and disabled
are states for managing already existing hosts so I created an additional if statement for that case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's weird. But I think there should also be another state, like disabled-or-absent
. Depending on what you want to achieve disabled
or disabled-or-absent
is the right state to use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added force_creation
parameter for that case
plugins/modules/ipa_host.py
Outdated
description: State to ensure. | ||
description: | ||
- State to ensure. | ||
- V("present") and V("enabled") give the same results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks wrong to me. A present
host can both be enabled or disabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, but there are no such thing as 'enabling' host, because host is considered as 'enabled' if it has a keytab so to 'enable' a host you would need to do something completely different https://freeipa.readthedocs.io/en/latest/api/host_disable.html
https://docs.redhat.com/en/documentation/red_hat_enterprise_linux/6/html/identity_management_guide/host-disable#reenabling-hosts
and this module should not create keytab files on its own so I don't see a reason for having 'enabled' state other than to be an alias for 'present'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know FreeIPA at all, I only know about Ansible semantics, and what I wrote is what enabled/disabled usually means, and seems to be what the module also means (or better: intended do mean) when looking at the current implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, in that case I will keep it as it was before my changes
Should |
I think there has been a (not sure how long) discussion some years ago of whether the IPA modules should be moved there / whether forces should be combined with that collection, but I'm not sure what happened to it. (I can't find the discussion anymore, unfortunately...) |
Now I found #1147 (comment) and some IRC logs from that time. In the IRC logs we kind of agreed that we should ask the FreeIPA community, but I'm not sure this ever happened. (Right now there's also https://forum.ansible.com/t/5738 which complicates things a bit...) |
Thanks for the info! Then I will fix this issue for now but will try to not focus on freeipa modules not to reinvent the wheel |
This comment was marked as outdated.
This comment was marked as outdated.
I cannot judge the IPA parts, but it looks OK to me. I'll merge this in ~a week if nobody objects. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Backport to stable-9: 💚 backport PR created✅ Backport PR branch: Backported as #8998 🤖 @patchback |
@abakanovskii @russoz thanks! |
* Fix ipa_host * PR Fixes * PR Fixes * PR Doc fixes * PR Doc fixes 2 * Fix default value (cherry picked from commit cc80096)
…bled states (#8998) ipa_host: Fix enabled and disabled states (#8920) * Fix ipa_host * PR Fixes * PR Fixes * PR Doc fixes * PR Doc fixes 2 * Fix default value (cherry picked from commit cc80096) Co-authored-by: alexander <[email protected]>
SUMMARY
Fix enabled and disabled states
Fixes #1094
ISSUE TYPE
COMPONENT NAME
ipa_host
ADDITIONAL INFORMATION
Im not sure why it is how it is on 267 line of code and what exactly
random_password
is used for but for better reverse compatibility I kept itAnd looking at the ansible-freeipa collection their module is way ahead of this one (if not all of them) so I don't know if these should be maintained in community.general