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

Allow processing Keycloak subgroups as a tree #135

Closed
wants to merge 5 commits into from
Closed

Allow processing Keycloak subgroups as a tree #135

wants to merge 5 commits into from

Conversation

bastjan
Copy link

@bastjan bastjan commented Nov 1, 2021

This PR adds a second processing method for nested Keycloak groups. Fixes #129.

The current and method of flattening sub groups is supplemented by a method that preserves the hierarchy by adding the path to the group name.

The new behavior can be enabled by a new field in the CRD:

[...]
spec:
  providers:
  - name: keycloak
    keycloak:
      scope: sub
      subGroupProcessing: join
      subGroupSeparator: "+"

See the added tests for a more extended sample of the behaviour including how group members are handled.

I'm sorry for the huge change set, the added tests required some refactoring. 😅

Old, default behaviour:

Note the duplicated child group. The last one wins and is applied to the cluster.

* group-a
** child
* group-b
** child

becomes:

* group-a
* child
* group-b
* child

Added join behaviour with : as separator:

* group-a
** child
* group-b
** child

becomes:

* group-a
* group-a+child
* group-b
* group-b+child

@sabre1041
Copy link
Collaborator

@raffaelespazzoli any thoughts here as you were the most interested in a hierarchy structure previously

@raffaelespazzoli
Copy link
Contributor

this seems like a good idea to me as we were losing groups.
@bastjan there were annotations in place to make it possible to navigate the hierarchy, are they still being added to the groups created in OCP?
Also is : the best separator. usually group names end up being part of the names of something else in OCP which needs to comply with the name field constraint (valid fqdn no longer than 64). Ok now I see that it is set by the user and that this behavior is optional.
I think we must not lose information in the group sync, so the current behavior is flat out wrong. I would make this the default and only behavior.

@bastjan
Copy link
Author

bastjan commented Jan 3, 2022

@raffaelespazzoli

there were annotations in place to make it possible to navigate the hierarchy, are they still being added to the groups created in OCP?

Yes those annotations are still added.

Also is : the best separator?

No it's not. I breaks in some places. The group names don't have to comply to name field constraints as far as i did see. We created groups with names that consisted of 1000+ characters to test.

In the end we decided on using +. I'll update the PR description.

I would make this the default and only behavior.

I'd be glad to do that. I didn't because it could break use-cases for other users.

@appuio appuio closed this by deleting the head repository May 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Name collision when synchronizing sub groups from Keycloak
4 participants