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_user_federation: add module argument that allows excluding bindCredential from update check #8898

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

Conversation

fgruenbauer
Copy link
Contributor

@fgruenbauer fgruenbauer commented Sep 22, 2024

SUMMARY

Keycloak redacts the value of the parameter bindCredential (ldap admin password) in its responses, which makes diff and change detection difficult. The desired and the before/existing value can never really be equal unless bindCredential is set to ********** or both are not set at all. This leads to the module always making an update if a value is set for bindCredential. The module might still return changed = false, because after an update the module compares before and after state. In check mode changed is always true but the diff empty, because the parameter is overwritten in the sanitize function.

The added module argument would allow users to exclude the bindCredential parameter when comparing before and desired state. This would prevent unnecessary updates and check mode always having changed = true.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

keycloak_user_federation

ADDITIONAL INFORMATION
  1. create ldap user federation and set bindCredential value
  2. run module in check mode

@ansibullbot
Copy link
Collaborator

@ansibullbot ansibullbot added WIP Work in progress bug This issue/PR relates to a bug module module plugins plugin (any type) labels Sep 22, 2024
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

Thank you for the contribution! Please add a changelog fragment.

Other than that, it does LGTM.

@fgruenbauer fgruenbauer marked this pull request as ready for review September 23, 2024 11:07
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.

Thanks, LGTM

@ansibullbot ansibullbot removed the WIP Work in progress label Sep 23, 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 23, 2024
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!

- Set to V(true) to exclude O(config.bindCredential) when comparing the before state with the desired state to
determine wether an update is required.
type: bool
default: false
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, how about using a string state with two states (choices) instead of a boolean? Then it is possible to later add more options, if for example the API is changed to allow other ways to handle this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, sounds good. I'm not sure about the naming though.

Maybe

bind_credential_handling:
    description:
        - The value of the config parameter O(config.bindCredential) is redacted in the Keycloak responses. There is currently no way to get the actual value. Comparing the redacted value with the desired value always evaluates to not equal. This also means the before and desired states are never equal if the parameter is set.
        - Set to V(exclude_from_update_check) to exclude O(config.bindCredential) when comparing the before state with the desired state to determine whether an update is required.
    type: str
    default: include_in_update_check
    choices:
        - include_in_update_check
        - exclude_from_update_check
        (- get_cleartext_value)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would probably adjust the option name a bit - maybe bind_credential_update_mode? - and simplify the choices: never (instead of exclude), always (instead of update). WDYT?

Copy link
Contributor Author

@fgruenbauer fgruenbauer Sep 24, 2024

Choose a reason for hiding this comment

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

I like the option name, but i'm not sure about never. So far the PR only removes the parameter from the update check not the actual update. If there are other changes the parameter would still get updated, so never seems kind of misleading i think. We could adjust the PR accordingly though. I think this would also make a bindCredential update more predictable, either do an update or not. Comparing the redacted value is kind of meaningless. If there is a way to get the actual value later we could then add on_change.

Or maybe skip_check to exclude from the update check, but still leave it in the update.

Copy link
Collaborator

Choose a reason for hiding this comment

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

True, never isn't the best name. How about only_indirect? I think it's a bit easier to understand than skip_check. (I could also be wrong/biased though :) )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I think we got a winner. I will adjust the PR.

@felixfontein
Copy link
Collaborator

I'll merge the beginning of next week if nobody objects.

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 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