keycloak_user_federation: add module argument that allows excluding bindCredential from update check

[fgruenbauer]

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]

cc @eikef @laurpaum @mattock @ndclt @thomasbach-dev
click here for bot help

[russoz]

Hi @fgruenbauer

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

Other than that, it does LGTM.

[russoz]

Thanks, LGTM

[felixfontein]

Thanks for your contribution!

[felixfontein]

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.

[fgruenbauer]

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)

[felixfontein]

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?

[fgruenbauer]

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.

[felixfontein]

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

[fgruenbauer]

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

[felixfontein]

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

[patchback]

Backport to stable-9: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-9/3b109abe18243aa21e2f5efa82fb005496a8235f/pr-8898

Backported as #8999

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

[felixfontein]

@fgruenbauer @russoz thanks!