CRD generation from RealmRepresentation #9759

andreaTP · 2022-01-24T14:58:33Z

~~This is the first step for #9172~~

Resolves #9787

those are the changes to the model needed successfully generate a CRD for the operator.

andreaTP · 2022-01-24T15:02:09Z

core/src/main/java/org/keycloak/representations/overrides/NoSubGroupsGroupRepresentation.java

+    protected List<String> realmRoles;
+    protected Map<String, List<String>> clientRoles;
+    // TODO: eventually generate code for Nth levels of depth
+    // protected List<GroupRepresentation> subGroups;


Due to limitations in the CRD schema we should limit the recursive subGroups to a finite number.
What would be a reasonable number of allowed, nested, subGroups?
cc. @stianst @pedroigor

andreaTP · 2022-01-24T15:02:36Z

...ava/org/keycloak/representations/overrides/NoSubcomponentsComponentExportRepresentation.java

+    private String providerId;
+    private String subType;
+    // TODO: eventually generate code for Nth levels of depth
+    // private MultivaluedHashMap<String, ComponentExportRepresentation> subComponents = new MultivaluedHashMap<>();


Due to limitations in the CRD schema we should limit the recursive subComponents to a finite number.
What would be a reasonable number of allowed, nested, subComponents?
cc. @stianst @pedroigor

andreaTP · 2022-01-24T15:03:25Z

pom.xml

@@ -81,7 +81,7 @@
        <infinispan.version>12.1.7.Final</infinispan.version>
        <infinispan.protostream.processor.version>4.4.1.Final</infinispan.protostream.processor.version>
        <javax.annotation-api.version>1.3.2</javax.annotation-api.version>
-        <jackson.version>2.12.1</jackson.version>
+        <jackson.version>2.13.1</jackson.version>


Transitive update from the kubernetes-client (the old version breaks)

Jackson version needs to honor quarkus / wildfly versions and cannot be changed for the sake of an added library local to a part of the project. See e.g. quarkus/pom.xml on how to override library versions should this be necessary.

andreaTP · 2022-01-24T15:04:46Z

...ain/java/org/keycloak/representations/idm/AbstractAuthenticationExecutionRepresentation.java

@@ -67,6 +67,9 @@ public void setPriority(int priority) {
     *
     * @return
     */
+    @Deprecated
+    private boolean autheticatorFlow;


Instead of adding this unused field I would be happy to remove the following accessors, unfortunately, the latter is a breaking change for the import/export functionality.
cc. @stianst @pedroigor

I'm probably missing something, but why do you need to remove a simple boolean field?

Here we are adding back the field, that remains unused, since it's emitted by the Json serialization.
If we don't add it, you would need to remove all the relevant fields autheticatorFlow (WITH the typo) from the exported RealmRepresentation before importing it as a CR.

@stianst this is the only change in the core I'm leaving, as far as I can tell it should be ok, but let me know if I have to duplicate even this file.

...src/main/java/org/keycloak/representations/overrides/NoSubGroupsGroupRepresentationList.java

jonathanvila · 2022-01-25T11:07:11Z

I'm a bit lost here. I see files in the test folder, but I haven't found any proper Test checking the changes. Can you clarify ?

andreaTP · 2022-01-25T11:21:23Z

I'm a bit lost here. I see files in the test folder, but I haven't found any proper Test checking the changes. Can you clarify ?

Sure, the files in the test folder will make the compilation fail if the CRD can't be generated.

jonathanvila

LGTM

vmuzikar

@andreaTP Thanks, nice job! ;)

@iankko @pskopek Could you please double check that the dependencies changes (potentially affecting the whole project) are ok?

vmuzikar · 2022-01-25T14:59:35Z

core/pom.xml

+        <dependency>
+            <groupId>io.fabric8</groupId>
+            <artifactId>crd-generator-apt</artifactId>
+            <scope>test</scope>
+        </dependency>


Where is this dep used? We don't have any tests.

This is generating a CRD in the test scope.

vmuzikar · 2022-01-25T15:05:44Z

...ain/java/org/keycloak/representations/idm/AbstractAuthenticationExecutionRepresentation.java

@@ -67,6 +67,9 @@ public void setPriority(int priority) {
     *
     * @return
     */
+    @Deprecated
+    private boolean autheticatorFlow;


I'm probably missing something, but why do you need to remove a simple boolean field?

vmuzikar · 2022-01-25T15:13:59Z

core/src/main/java/org/keycloak/representations/idm/RealmRepresentation.java

@@ -42,6 +44,7 @@
    private static final Logger logger = Logger.getLogger(RealmRepresentation.class);

    protected String id;
+    @NotNull


I know that eventually realm really is not null but I'm not sure if in some cases it really cannot be null in the process.

Happy to move the NotNull to the id if it's needed.
Just waiting for a nudge from someone 🙂

vmuzikar · 2022-01-25T15:16:19Z

core/src/main/java/org/keycloak/representations/idm/authorization/ScopeRepresentation.java

@@ -33,7 +34,9 @@
    private String id;
    private String name;
    private String iconUri;
+    @SchemaFrom(type = java.lang.Void.class)


Does it mean we won't support policies here?

Correct, according to @pedroigor it's not needed.

vmuzikar · 2022-01-25T15:16:41Z

core/src/main/java/org/keycloak/representations/idm/authorization/ScopeRepresentation.java

    private List<PolicyRepresentation> policies;
+    @SchemaFrom(type = java.lang.Void.class)


Doe it meant we won't support resources here?

Correct, according to @pedroigor it's not needed.

vmuzikar · 2022-01-25T15:29:21Z

core/src/main/java/org/keycloak/representations/overrides/NoSubGroupsGroupRepresentation.java

+import java.util.List;
+import java.util.Map;
+
+public class NoSubGroupsGroupRepresentation {


I hate that we need to have another Group representation just without sub-groups. It feels very error prone as changes in the main class could be easily forgot to be ported here. But I guess we don't have many options, right?

But I guess we don't have many options, right?

In other parts of this repository we are generating those things using annotation processors, that would be a cleaner option but will require a little time investment.

vmuzikar · 2022-01-25T15:31:34Z

core/src/test/java/org/keycloak/crd/ExampleRealmCRD.java

+// Result is available at: target/test-classes/META-INF/fabric8/examplerealmcrds.keycloak.org-v1.yml
+@Group("keycloak.org")
+@Version("v1alpha1")
+public class ExampleRealmCRD extends CustomResource<ExampleRealmCRDSpec, Void> implements Namespaced {


This should not be part of the final PR. This belongs to the operator.

This is in the test scope, and guarantees that the model can generate a CRD without throwing exceptions at compile time. It's actually a "compile-time" regression test.

vmuzikar · 2022-01-25T15:31:53Z

core/src/test/java/org/keycloak/crd/ExampleRealmCRDSpec.java

+
+import javax.validation.constraints.NotNull;
+
+public class ExampleRealmCRDSpec {


This should not be part of the final PR. This belongs to the operator.

As above, I do believe that checking the CRD generation in this module is extremely good for debugging purposes.

vmuzikar · 2022-01-25T16:06:12Z

@andreaTP One more thing. Could you please create a separate GH Issue for this?

andreaTP · 2022-01-25T18:45:51Z

@vmuzikar ready for next round 👍

hmlnarik

Thank you for the PR.

I like the idea of generating CRDs from representations. I don't think this approach is valid though for the following reasons:

The approach adds new dependencies to keycloak core, meaning that all distributed artifacts (including adapters) would need another set of libraries
The approach changes Keycloak representations for the sake of single technology (operator) in a way that is bringing in additional complexity for every other component (REST endpoints, storage).
This also affects future storage ability to generate representations automatically, which is one of the future goals of storage work

Can the CRD generation be kept local to the operator module?

andreaTP · 2022-01-26T09:21:20Z

Hi @hmlnarik ! Thanks a lot for sharing your valuable POV!

Let me iterate on your observations:

The approach adds new dependencies to keycloak core, meaning that all distributed artifacts (including adapters) would need another set of libraries

This is not correct, in this PR, the only change of transitive dependencies is the bump of Jackson.
The other two libraries added are marked as provided so that they are needed only at compile time of this module but not included in the transitive dependencies.

The approach changes Keycloak representations for the sake of single technology (operator) in a way that is bringing in additional complexity for every other component (REST endpoints, storage).

All the changes in this PR are strictly NOT affecting the current representation that remains forward and backward compatible. The additions are also not affecting in any sense the usage of the representation in other modules (as demonstrated by the complete source compatibility).
Please note that the underlying reason for those changes is to break recursive data structures solely for the seek of CRD generation (they are not representable in OpenAPI v3).
I would argue that having a model with no recursive data structures is actually a simplification in a good direction (e.g. compatibility with OpenAPI v3) and we might want to tend to it in the future (removing the necessity for the additional annotations and alternative classes).

This also affects future storage ability to generate representations automatically, which is one of the future goals of storage work

I fail to see how, can you please elaborate how adding opt-in annotations for generating a correct CRD is breaking other workflows?

Can the CRD generation be kept local to the operator module?

Only with major hacks/tooling, having the ability to do those kind of changes is the main reason for having the operator in this repository.

stianst

Core module is used both for the server and the adapters, and can't have these additional dependencies added I'm afraid.

core/pom.xml

andreaTP · 2022-01-26T13:10:00Z

@stianst @hmlnarik @vmuzikar
I think this is ready for next round.

Changed the approach to include keycloak-core from sources instead of depending on it, with some Maven juggling.
This imply having to duplicate all the Java source files that need to be "patched", I hope we can revisit this approach in the future.

vmuzikar

@andreaTP Thanks for the update. Even though it's a nasty workaround, it is good enough for the operator prototype. We for sure need more permanent solution (to "annotate" the fields in a different way) before releasing the operator.

pom.xml

andreaTP · 2022-01-26T17:57:40Z

@vmuzikar here you have an alternative approach using sed in a bash script:
https://github.com/andreaTP/keycloak/pull/3/files

there are pro/cons in respect to the current approach ... check out which one is preferred.

vmuzikar · 2022-01-26T18:52:57Z

@andreaTP +1
The sed approach seems a little bit cleaner to me. More than good enough for the prototype.

Patching keycloak-core with sed

vmuzikar

@andreaTP Changes LGTM now. But I'm not able to test them. Running mvn package as usually doesn't generate the new CRD. Am I doing something wrong?

andreaTP · 2022-01-27T09:24:50Z

@vmuzikar running from the top folder of the repo:

mvn clean compile -f operator/pom.xml

generates the CRDs in e.g. operator/target/classes/META-INF/fabric8/realms.keycloak.org-v1.yml, the Realm CRD is not copied to the kubernetes folder since it doesn't have an associated Controller (just yet) reference.

Enabling CRD generation from RealmRepresentation Closes keycloak#9759

andreaTP requested review from jonathanvila and vmuzikar January 24, 2022 14:58

andreaTP commented Jan 24, 2022

View reviewed changes

andreaTP requested a review from pedroigor January 24, 2022 15:04

jonathanvila reviewed Jan 25, 2022

View reviewed changes

...src/main/java/org/keycloak/representations/overrides/NoSubGroupsGroupRepresentationList.java Outdated Show resolved Hide resolved

vmuzikar requested a review from stianst January 25, 2022 14:47

jonathanvila previously approved these changes Jan 25, 2022

View reviewed changes

vmuzikar reviewed Jan 25, 2022

View reviewed changes

vmuzikar requested review from pskopek and iankko January 25, 2022 16:01

hmlnarik requested changes Jan 26, 2022

View reviewed changes

hmlnarik requested review from mposolda and mhajas January 26, 2022 07:47

stianst requested changes Jan 26, 2022

View reviewed changes

stianst reviewed Jan 26, 2022

View reviewed changes

core/pom.xml Outdated Show resolved Hide resolved

stianst reviewed Jan 26, 2022

View reviewed changes

core/pom.xml Outdated Show resolved Hide resolved

andreaTP added 2 commits January 26, 2022 11:57

Enabling CRD generation from RealmRepresentation

1495dbf

move the changes to the operator module

8487032

andreaTP dismissed jonathanvila’s stale review via 8487032 January 26, 2022 12:58

andreaTP force-pushed the realm-crd branch from 3e9984c to 8487032 Compare January 26, 2022 12:58

cleanup

4e9d8c5

andreaTP changed the title ~~Enabling CRD generation from RealmRepresentation~~ CRD generation from RealmRepresentation Jan 26, 2022

vmuzikar requested changes Jan 26, 2022

View reviewed changes

pom.xml Outdated Show resolved Hide resolved

pom.xml Outdated Show resolved Hide resolved

kubernetes-client version in operator/pom.xml

14dd74e

patching keycloak-core with sed

fceaa95

Merge pull request #3 from andreaTP/sed-patching

3dc12b9

Patching keycloak-core with sed

vmuzikar reviewed Jan 27, 2022

View reviewed changes

vmuzikar approved these changes Jan 27, 2022

View reviewed changes

stianst approved these changes Jan 27, 2022

View reviewed changes

stianst merged commit 24d6f75 into keycloak:main Jan 27, 2022

dgozalo pushed a commit to dgozalo/keycloak that referenced this pull request Jan 31, 2022

CRD generation from RealmRepresentation (keycloak#9759)

e315864

Enabling CRD generation from RealmRepresentation Closes keycloak#9759

andreaTP mentioned this pull request Feb 3, 2022

[crd-generator] Implement SchemaSwap fabric8io/kubernetes-client#3797

Merged

11 tasks

xRodney mentioned this pull request Aug 24, 2022

@SchemaSwap can only be used once fabric8io/kubernetes-client#4350

Closed

		private List<PolicyRepresentation> policies;
		@SchemaFrom(type = java.lang.Void.class)


		import javax.validation.constraints.NotNull;

		public class ExampleRealmCRDSpec {

CRD generation from RealmRepresentation #9759

CRD generation from RealmRepresentation #9759

Conversation

andreaTP commented Jan 24, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanvila commented Jan 25, 2022

andreaTP commented Jan 25, 2022

jonathanvila left a comment

Choose a reason for hiding this comment

vmuzikar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vmuzikar commented Jan 25, 2022

andreaTP commented Jan 25, 2022

hmlnarik left a comment

Choose a reason for hiding this comment

andreaTP commented Jan 26, 2022

stianst left a comment

Choose a reason for hiding this comment

andreaTP commented Jan 26, 2022

vmuzikar left a comment

Choose a reason for hiding this comment

andreaTP commented Jan 26, 2022 • edited

vmuzikar commented Jan 26, 2022

vmuzikar left a comment

Choose a reason for hiding this comment

andreaTP commented Jan 27, 2022

andreaTP commented Jan 24, 2022 •

edited

andreaTP commented Jan 26, 2022 •

edited