Bugfix: keycloak_identity_provider does not handle mapper changes properly #7418

danekja · 2023-10-22T09:44:35Z

SUMMARY

keycloak_identity_provider mapper processing contained a bug which resulted in incorrect behaviour in the following cases:

deleting an existing mapper - nothing happened
adding a new mapper to a non-empty list - the new mapper was added, the pre-existing mappers were removed
repeated calls without modified configuration were not idempotent if the mappers were not ordered by name in task specification

Fixes #6002.

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

keycloak_identity_provider

ADDITIONAL INFORMATION

Issue was caused by dict.update() function being used to merge old and new states, without considering it does not recursively process nested lists and dictionaries.

Regression tests for all 3 cases have been added to tests/integration/targets/keycloak_identity_provider/tasks/main.yml

- wrong identityProviderAlias in mapper configuration

* test for removing an existing mapper * test for adding a new mapper when others already exist * test for module idempotency when mappers not ordered by name in ascending order

* removing an existing mapper * adding a new mapper when others already exist * module idempotency when mappers not ordered by name in ascending order

ansibullbot · 2023-10-22T09:50:42Z

cc @eikef @laurpaum @mattock @ndclt
click here for bot help

felixfontein

Thanks for your contribution! I've added some first comments.

changelogs/fragments/7418-kc_identity_provider-mapper-reconfiguration-fixes.yml

…uration-fixes.yml Co-authored-by: Felix Fontein <felix@fontein.de>

danekja · 2023-10-30T10:03:41Z

@felixfontein Are these checks stable normally, should I look into them failing after they had passed prior to changelog updates? Those could not have broken anything.

felixfontein · 2023-10-30T20:22:35Z

The unit test failure with Python 2.7 has been introduced by this PR; you are calling .copy() for a list object, which does not work with Python 2.7. Use list(foo) instead of foo.copy() for a list foo. These likely already failed before, but it looks like the GHA tests didn't run before so it wasn't possible to see these failures.

The AZP errors seem to be some misinformation, if you check out the results on AZP you can see that all tests passed.

felixfontein

I got interrupted when starting to review this, I at least want to send what I commented on so far.

changelogs/fragments/7418-kc_identity_provider-mapper-reconfiguration-fixes.yml

plugins/modules/keycloak_identity_provider.py

…uration-fixes.yml Co-authored-by: Felix Fontein <felix@fontein.de>

Co-authored-by: Felix Fontein <felix@fontein.de>

felixfontein

Sorry, I didn't manage to properly look at this before. I'm not sure whether sorting by name (which isn't always present) is a good idea. While trying to figure out what happens I noticed another problem (see my comments below), fixing that one might already solve the issues here.

plugins/modules/keycloak_identity_provider.py

danekja · 2023-12-13T08:05:30Z

@felixfontein can we help in any way to get this accepted?

felixfontein · 2023-12-14T19:16:23Z

@danekja my main problem with this is that I don't know whether the change is correct. That simply sorting the list fixes the main problem seems weird to me - it can be true, but it would surprise me. I have no way to test this since I don't have access to a Keycloak instance.

It would be great if someone else (for example the module maintainers) could independently check this and make sure this doesn't break their use-cases of this module.

danekja · 2023-12-15T13:16:25Z

@felixfontein sorting does not fix the main issue. Sorting only ensures module does not return "changed" when there are no changes. As I stated in the comment above - sorting is used by original author in the API call result parser. I have only ensured the playbook-provided mappers are sorted as well to ensure line 601 if desired_idp == before_idp: returns correct result in cases where mappers are not sorted in module parameters.

All the other changes fix the main issue (#6002).

What to do here? We have a schedule to keep, probably more fixes to do to these modules to make them viable and soon I will have to fall back to maintaining our own fork of these module if we are unable to proceed here.

I have done my best to keep implementation consistent with the original code, provided a regression test case and ensured the existing test suite is passing. I'm willing to make any changes necessary as I believe fixing the modules and contributing back to the community are the best course of action, but I can't afford to wait months.

felixfontein · 2023-12-23T13:22:15Z

It would help a lot of the module maintainers would review PRs for these modules. I don't use or know these modules. I'm going to merge this once the remaining problem is fixed.

…t potential failures in case name was not specified in playbook Co-authored-by: Felix Fontein <felix@fontein.de>

felixfontein

Looks good to me, as far as I can judge it.

patchback · 2023-12-28T08:50:05Z

Backport to stable-7: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-7/fd0d05d6f2c97b271b3bb6f13de75e4a50bf6093/pr-7418

Backported as #7774

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

…perly (#7418) * fix keycloak_identity_provider ITs to actually pass - wrong identityProviderAlias in mapper configuration * kc_identity_provider: add mapper reconfiguration regression tests * test for removing an existing mapper * test for adding a new mapper when others already exist * test for module idempotency when mappers not ordered by name in ascending order * kc_identity_provider: add bugfixes for mapper reconfigurations * removing an existing mapper * adding a new mapper when others already exist * module idempotency when mappers not ordered by name in ascending order * add changelog fragment * prevent unnecessary update_mapper calls when there is no change * Update changelogs/fragments/7418-kc_identity_provider-mapper-reconfiguration-fixes.yml Co-authored-by: Felix Fontein <felix@fontein.de> * Update changelogs/fragments/7418-kc_identity_provider-mapper-reconfiguration-fixes.yml Co-authored-by: Felix Fontein <felix@fontein.de> * Update changelogs/fragments/7418-kc_identity_provider-mapper-reconfiguration-fixes.yml Co-authored-by: Felix Fontein <felix@fontein.de> * Update plugins/modules/keycloak_identity_provider.py Co-authored-by: Felix Fontein <felix@fontein.de> * kc_identity_provider: sort changeset mappers via name OR id to prevent potential failures in case name was not specified in playbook Co-authored-by: Felix Fontein <felix@fontein.de> --------- Co-authored-by: Felix Fontein <felix@fontein.de> (cherry picked from commit fd0d05d)

patchback · 2023-12-28T08:50:13Z

Backport to stable-8: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-8/fd0d05d6f2c97b271b3bb6f13de75e4a50bf6093/pr-7418

Backported as #7775

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

felixfontein · 2023-12-28T08:50:19Z

@danekja thanks for your contribution, and sorry it took so long!

…perly (#7418) * fix keycloak_identity_provider ITs to actually pass - wrong identityProviderAlias in mapper configuration * kc_identity_provider: add mapper reconfiguration regression tests * test for removing an existing mapper * test for adding a new mapper when others already exist * test for module idempotency when mappers not ordered by name in ascending order * kc_identity_provider: add bugfixes for mapper reconfigurations * removing an existing mapper * adding a new mapper when others already exist * module idempotency when mappers not ordered by name in ascending order * add changelog fragment * prevent unnecessary update_mapper calls when there is no change * Update changelogs/fragments/7418-kc_identity_provider-mapper-reconfiguration-fixes.yml Co-authored-by: Felix Fontein <felix@fontein.de> * Update changelogs/fragments/7418-kc_identity_provider-mapper-reconfiguration-fixes.yml Co-authored-by: Felix Fontein <felix@fontein.de> * Update changelogs/fragments/7418-kc_identity_provider-mapper-reconfiguration-fixes.yml Co-authored-by: Felix Fontein <felix@fontein.de> * Update plugins/modules/keycloak_identity_provider.py Co-authored-by: Felix Fontein <felix@fontein.de> * kc_identity_provider: sort changeset mappers via name OR id to prevent potential failures in case name was not specified in playbook Co-authored-by: Felix Fontein <felix@fontein.de> --------- Co-authored-by: Felix Fontein <felix@fontein.de> (cherry picked from commit fd0d05d)

…ider does not handle mapper changes properly (#7774) Bugfix: keycloak_identity_provider does not handle mapper changes properly (#7418) * fix keycloak_identity_provider ITs to actually pass - wrong identityProviderAlias in mapper configuration * kc_identity_provider: add mapper reconfiguration regression tests * test for removing an existing mapper * test for adding a new mapper when others already exist * test for module idempotency when mappers not ordered by name in ascending order * kc_identity_provider: add bugfixes for mapper reconfigurations * removing an existing mapper * adding a new mapper when others already exist * module idempotency when mappers not ordered by name in ascending order * add changelog fragment * prevent unnecessary update_mapper calls when there is no change * Update changelogs/fragments/7418-kc_identity_provider-mapper-reconfiguration-fixes.yml Co-authored-by: Felix Fontein <felix@fontein.de> * Update changelogs/fragments/7418-kc_identity_provider-mapper-reconfiguration-fixes.yml Co-authored-by: Felix Fontein <felix@fontein.de> * Update changelogs/fragments/7418-kc_identity_provider-mapper-reconfiguration-fixes.yml Co-authored-by: Felix Fontein <felix@fontein.de> * Update plugins/modules/keycloak_identity_provider.py Co-authored-by: Felix Fontein <felix@fontein.de> * kc_identity_provider: sort changeset mappers via name OR id to prevent potential failures in case name was not specified in playbook Co-authored-by: Felix Fontein <felix@fontein.de> --------- Co-authored-by: Felix Fontein <felix@fontein.de> (cherry picked from commit fd0d05d) Co-authored-by: Jakub Danek <danekja@users.noreply.github.com>

…ider does not handle mapper changes properly (#7775) Bugfix: keycloak_identity_provider does not handle mapper changes properly (#7418) * fix keycloak_identity_provider ITs to actually pass - wrong identityProviderAlias in mapper configuration * kc_identity_provider: add mapper reconfiguration regression tests * test for removing an existing mapper * test for adding a new mapper when others already exist * test for module idempotency when mappers not ordered by name in ascending order * kc_identity_provider: add bugfixes for mapper reconfigurations * removing an existing mapper * adding a new mapper when others already exist * module idempotency when mappers not ordered by name in ascending order * add changelog fragment * prevent unnecessary update_mapper calls when there is no change * Update changelogs/fragments/7418-kc_identity_provider-mapper-reconfiguration-fixes.yml Co-authored-by: Felix Fontein <felix@fontein.de> * Update changelogs/fragments/7418-kc_identity_provider-mapper-reconfiguration-fixes.yml Co-authored-by: Felix Fontein <felix@fontein.de> * Update changelogs/fragments/7418-kc_identity_provider-mapper-reconfiguration-fixes.yml Co-authored-by: Felix Fontein <felix@fontein.de> * Update plugins/modules/keycloak_identity_provider.py Co-authored-by: Felix Fontein <felix@fontein.de> * kc_identity_provider: sort changeset mappers via name OR id to prevent potential failures in case name was not specified in playbook Co-authored-by: Felix Fontein <felix@fontein.de> --------- Co-authored-by: Felix Fontein <felix@fontein.de> (cherry picked from commit fd0d05d) Co-authored-by: Jakub Danek <danekja@users.noreply.github.com>

danekja added 4 commits October 22, 2023 10:43

fix keycloak_identity_provider ITs to actually pass

666d802

- wrong identityProviderAlias in mapper configuration

kc_identity_provider: add mapper reconfiguration regression tests

a97f64c

* test for removing an existing mapper * test for adding a new mapper when others already exist * test for module idempotency when mappers not ordered by name in ascending order

kc_identity_provider: add bugfixes for mapper reconfigurations

8a51802

* removing an existing mapper * adding a new mapper when others already exist * module idempotency when mappers not ordered by name in ascending order

add changelog fragment

93480a5

felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-6 labels Oct 22, 2023

prevent unnecessary update_mapper calls when there is no change

6e19540

ansibullbot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Oct 22, 2023

felixfontein reviewed Oct 22, 2023

View reviewed changes

changelogs/fragments/7418-kc_identity_provider-mapper-reconfiguration-fixes.yml Outdated Show resolved Hide resolved

changelogs/fragments/7418-kc_identity_provider-mapper-reconfiguration-fixes.yml Outdated Show resolved Hide resolved

danekja and others added 2 commits October 22, 2023 19:12

Update changelogs/fragments/7418-kc_identity_provider-mapper-reconfig…

64fbcef

…uration-fixes.yml Co-authored-by: Felix Fontein <felix@fontein.de>

Update changelogs/fragments/7418-kc_identity_provider-mapper-reconfig…

fe82d50

…uration-fixes.yml Co-authored-by: Felix Fontein <felix@fontein.de>

felixfontein reviewed Oct 30, 2023

View reviewed changes

changelogs/fragments/7418-kc_identity_provider-mapper-reconfiguration-fixes.yml Outdated Show resolved Hide resolved

plugins/modules/keycloak_identity_provider.py Outdated Show resolved Hide resolved

felixfontein mentioned this pull request Oct 30, 2023

CI: make sure that Python 2.7 unit tests also run with AZP #7450

Merged

ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Oct 30, 2023

patchback bot mentioned this pull request Oct 30, 2023

[PR #7450/f5c544d9 backport][stable-7] CI: make sure that Python 2.7 unit tests also run with AZP #7451

Merged

danekja and others added 2 commits October 31, 2023 10:04

Update changelogs/fragments/7418-kc_identity_provider-mapper-reconfig…

8c63ea0

…uration-fixes.yml Co-authored-by: Felix Fontein <felix@fontein.de>

Update plugins/modules/keycloak_identity_provider.py

69a092f

Co-authored-by: Felix Fontein <felix@fontein.de>

ansibullbot removed the stale_ci CI is older than 7 days, rerun before merging label Oct 31, 2023

felixfontein removed the backport-6 label Nov 1, 2023

felixfontein added the backport-8 Automatically create a backport for the stable-8 branch label Nov 1, 2023

ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Nov 9, 2023

felixfontein reviewed Nov 12, 2023

View reviewed changes

plugins/modules/keycloak_identity_provider.py Outdated Show resolved Hide resolved

plugins/modules/keycloak_identity_provider.py Show resolved Hide resolved

felixfontein mentioned this pull request Nov 19, 2023

[Keycloak] Identity Provider unchanged mappers are deleted #6002

Closed

1 task

ansibullbot added the has_issue label Nov 19, 2023

kc_identity_provider: sort changeset mappers via name OR id to preven…

1e8d963

…t potential failures in case name was not specified in playbook Co-authored-by: Felix Fontein <felix@fontein.de>

ansibullbot removed the stale_ci CI is older than 7 days, rerun before merging label Dec 28, 2023

felixfontein approved these changes Dec 28, 2023

View reviewed changes

felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Dec 28, 2023

felixfontein merged commit fd0d05d into ansible-collections:main Dec 28, 2023

patchback bot mentioned this pull request Dec 28, 2023

[PR #7418/fd0d05d6 backport][stable-7] Bugfix: keycloak_identity_provider does not handle mapper changes properly #7774

Merged

patchback bot mentioned this pull request Dec 28, 2023

[PR #7418/fd0d05d6 backport][stable-8] Bugfix: keycloak_identity_provider does not handle mapper changes properly #7775

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix: keycloak_identity_provider does not handle mapper changes properly #7418

Bugfix: keycloak_identity_provider does not handle mapper changes properly #7418

danekja commented Oct 22, 2023 •

edited by felixfontein

Loading

ansibullbot commented Oct 22, 2023

felixfontein left a comment

danekja commented Oct 30, 2023

felixfontein commented Oct 30, 2023

felixfontein left a comment

felixfontein left a comment

danekja commented Dec 13, 2023

felixfontein commented Dec 14, 2023

danekja commented Dec 15, 2023

felixfontein commented Dec 23, 2023

felixfontein left a comment

patchback bot commented Dec 28, 2023 •

edited

Loading

patchback bot commented Dec 28, 2023 •

edited

Loading

felixfontein commented Dec 28, 2023

Bugfix: keycloak_identity_provider does not handle mapper changes properly #7418

Bugfix: keycloak_identity_provider does not handle mapper changes properly #7418

Conversation

danekja commented Oct 22, 2023 • edited by felixfontein Loading

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

ansibullbot commented Oct 22, 2023

felixfontein left a comment

Choose a reason for hiding this comment

danekja commented Oct 30, 2023

felixfontein commented Oct 30, 2023

felixfontein left a comment

Choose a reason for hiding this comment

felixfontein left a comment

Choose a reason for hiding this comment

danekja commented Dec 13, 2023

felixfontein commented Dec 14, 2023

danekja commented Dec 15, 2023

felixfontein commented Dec 23, 2023

felixfontein left a comment

Choose a reason for hiding this comment

patchback bot commented Dec 28, 2023 • edited Loading

Backport to stable-7: 💚 backport PR created

patchback bot commented Dec 28, 2023 • edited Loading

Backport to stable-8: 💚 backport PR created

felixfontein commented Dec 28, 2023

danekja commented Oct 22, 2023 •

edited by felixfontein

Loading

patchback bot commented Dec 28, 2023 •

edited

Loading

patchback bot commented Dec 28, 2023 •

edited

Loading