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

ipa_getkeytab: Create module #8938

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

abakanovskii
Copy link
Contributor

SUMMARY

ipa_getkeytab as a fancy wrapper around ipa-getkeytab command

ISSUE TYPE
  • New Module/Plugin Pull Request
COMPONENT NAME

ipa_getkeytab

ADDITIONAL INFORMATION

Pretty much a simple wrapper around a ipa-getkeytab using cmd_runner
I tested this module by my hands and it seems to work as intended
I don't know how to write unit tests because for integration test I would need FreeIPA server and client installed so i wrote something up using test_npm as a source

# Before this module you would have to do something like this
    - name: Create keytab
      ansible.builtin.shell: ipa-getkeytab -s <ipa_server> -p <some_principal> -k <some_path>

# Now you can do this like this
    - name: Create keytab
      community.general.ipa_getkeytab:
        path: <some_path>
        principal: <some_principal>
        ipa_server:  <ipa_server>

@ansibullbot
Copy link
Collaborator

@ansibullbot ansibullbot added module module new_plugin New plugin plugins plugin (any type) tests tests unit tests/unit labels Sep 26, 2024
@ansibullbot

This comment was marked as outdated.

@ansibullbot ansibullbot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Sep 26, 2024
@abakanovskii abakanovskii marked this pull request as draft September 26, 2024 12:27
@ansibullbot

This comment was marked as outdated.

@ansibullbot ansibullbot added the WIP Work in progress label Sep 26, 2024
@abakanovskii abakanovskii marked this pull request as ready for review September 26, 2024 12:35
@ansibullbot ansibullbot removed WIP Work in progress needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Sep 26, 2024
@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-9 Automatically create a backport for the stable-9 branch labels Sep 26, 2024
Comment on lines 169 to 171
def _exec(self, run_in_check_mode=False, check_rc=True):
if not self.module.check_mode or (self.module.check_mode and run_in_check_mode):
params = dict(self.module.params)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure these parameters make sense. They are never really used and module's code is not reused. Code is simpler without them.
Additionally, you might want to take a look at StateModuleHelper, see https://docs.ansible.com/ansible/latest/collections/community/general/docsite/guide_modulehelper.html for details on how to use it. There is a number of modules in this collection using that you can look up to for examples of actual usage.

description:
- The list of encryption types to use to generate keys.
- It will use local client defaults if not provided.
- Valid values depend on the Kerberos library version and configuration.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a minimum level for the Kerberos library? In any case, the library (with or without min version) should be cited in the requirements, for completeness.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well it comes with freeipa-client package, I used this sentence from man pages :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe add a link to those man pages (in one of those sites that publish them)? Or some other relevant reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems that there are not much trusted resources with man page of ipa-getkeytab, there are source file for that (https://github.com/freeipa/freeipa/blob/master/client/man/ipa-getkeytab.1) but other than that I found Ubuntu docs https://manpages.ubuntu.com/manpages/jammy/man1/ipa-getkeytab.1.html
What do you think? Should I add that link in the full description of a module?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yup, one of those (whichever you prefer). https://linux.die.net/man/1/ipa-getkeytab also appears out of google, just as an example.
Anyways, if you prefer not to add that to the description, maybe add it to the notes section.
Come to think of it, I will review all my modules (that call commands) to add links to the respective man pages. :-)

Copy link
Contributor Author

@abakanovskii abakanovskii Oct 4, 2024

Choose a reason for hiding this comment

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

Agreed :) I used ubuntu doc as linux.die.net don't have all options documented
I'll do the same thing for a kutils module then

Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Thanks!

plugins/modules/ipa_getkeytab.py Show resolved Hide resolved
.github/BOTMETA.yml Outdated Show resolved Hide resolved
plugins/modules/ipa_getkeytab.py Outdated Show resolved Hide resolved
plugins/modules/ipa_getkeytab.py Outdated Show resolved Hide resolved
plugins/modules/ipa_getkeytab.py Outdated Show resolved Hide resolved
plugins/modules/ipa_getkeytab.py Outdated Show resolved Hide resolved
@abakanovskii
Copy link
Contributor Author

Thanks for the review! @felixfontein

Copy link
Collaborator

@russoz russoz left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-9 Automatically create a backport for the stable-9 branch check-before-release PR will be looked at again shortly before release and merged if possible. module module new_plugin New plugin plugins plugin (any type) tests tests unit tests/unit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants