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

keycloak_realm: remove realm id requirement #9768

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

Conversation

fgruenbauer
Copy link
Contributor

SUMMARY

The keycloak_realm module requires an id when creating a new realm. I think it is better to let keycloak generate the id, like when creating a realm in the GUI.

Using the module with older keycloak versions that require an id would still be possible, but it would also allow users to create realms with uuids generated by keycloak.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

keycloak_realm

@ansibullbot
Copy link
Collaborator

@ansibullbot ansibullbot added bug This issue/PR relates to a bug module module plugins plugin (any type) labels Feb 17, 2025
@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-10 Automatically create a backport for the stable-10 branch labels Feb 17, 2025
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 for your contribution!

@@ -767,9 +766,6 @@ def main():
# Process a creation
result['changed'] = True

if 'id' not in desired_realm:
module.fail_json(msg='id needs to be specified when creating a new realm')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Right now you can run the module with neither id nor realm specified. That doesn't sound useful.

(Also it doesn't seem like the module actually works well if realm isn't specified?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

realm is the unique name that is required to create a realm. I just didn't want to add it as a requirement because i thought it might break older ansible code that didn't specify it. But i checked older keycloak versions again and it seems they all require realm, so even though the module did not require it, keycloak did. So i think we could make realm a requirement, because older code that didn't specify it, should not have worked anyway.

It seems to me the proper way is to let keycloak generate the id. The module can still allow specifying it, but it shouldn't be a requirement. In the older versions i checked, the web frontend set both variables, idand realm, to the provided realm(-name). That might be where this is coming from. None of the keycloak versions i tested required the id.

I checked keycloak version 12, 15, 17 and 20. The gui in 17 was the last one that set both id and realm.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm fine with making realm required. Since the module tries to determine whether a realm exists by looking for it, I don't think it really works (or ever worked) without realm anyway.

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.

Hi @fgruenbauer

Thanks for your contribution! It's probably nothing, but I have this one question about one of the changes there.

@@ -778,11 +774,11 @@ def main():

# create it
kc.create_realm(desired_realm)
after_realm = kc.get_realm_by_id(desired_realm['id'])
after_realm = kc.get_realm_by_id(desired_realm['realm'])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Without checking the code for kc.get_realm_by_id(), it begets the question: will that method accept a name instead of an ID? Is there a kc.get_realm_by_name() that we should be using instead?

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

Successfully merging this pull request may close these issues.

4 participants