Baseline for Keycloak deployment in operator #9628

vmuzikar · 2022-01-18T18:18:44Z

Limitations that will be addressed as a follow-up:

No clustering support (Clustering support in operator #9730)
Pod Templates are not implemented in the controller (Pod Templates in operator #9729)
No tests (Testsuite baseline #9174)

Resolves #9171, resolves #9552

andreaTP

First round of review finished!

I like the overall structure and especially the State management, although I think we should try to avoid introducing (so early!) complexities like WatchedResourcesStore and the custom "Vault" integration relying respectively on the capabilities of the Java SDK and the standard K8s API.

operator/src/main/java/org/keycloak/operator/OperatorManagedResource.java

andreaTP · 2022-01-18T18:36:22Z

operator/src/main/java/org/keycloak/operator/OperatorManagedResource.java

+        setDefaultLabels(resource);
+        setOwnerReferences(resource);
+
+        Log.debugf("Creating or updating resource: %s", resource);


Can we agree on one logging framework and stick with it? I don't really mind which as long as is one.

It is still the same logging framework coming from Quarkus. It's just using static methods and not CDI. I realized CDI approach is pretty stupid for logging as we don't have everything as beans and passing deps manually is a mess. This is an elegant way offered by Quarkus and we should stick with it everywhere.

Do we mind that, in this same codebase, the Keycloak Quarkus distribution takes a different approach?

this is still valid, but I don't have a super strong opinion against, feel free to mark this as Resolved if we don't care about consistency in this codebase.

Well, AFAIK rest of the KC can't fully utilize Quarkus features (due to WF), that includes the logging. In the operator we can use what Quarkus provides.

andreaTP · 2022-01-18T18:37:22Z

operator/src/main/java/org/keycloak/operator/OperatorManagedResource.java

+    }
+
+    protected void setOwnerReferences(HasMetadata resource) {
+        if (!cr.getMetadata().getNamespace().equals(resource.getMetadata().getNamespace())) {


why we have this if? IIRC UID spans over different namespaces

Cross-namespace owner references are disallowed by design. Namespaced dependents can specify cluster-scoped or namespaced owners.

https://kubernetes.io/docs/concepts/overview/working-with-objects/owners-dependents/

In this case I would throw an exception instead of silently ignoring.

this comment instead is still valid, can you, please, either remove the condition at all or throw an Exception instead of silently returning?

What if we have a managed resource in different namespace than the CR? We surely want to support those scenarios. Of course, the resources then would have to be "manually" removed by the Operator upon CR deletion.

correct, but the early return doesn't solve this either ...

So what do you suggest exactly? For sure throwing an exception is not a solution in context of this method. Of course, for true support for foreign resources located in other namespaces we'd need to implement additional logic.

In any case, I'm pretty sure this gets refactored sooner or later when the Dependent Resources are ready in the SDK.

I would completely remove this condition as it's misleading and not used ATM, but not anything blocking 👍

operator/src/main/java/org/keycloak/operator/WatchedResourcesStore.java

operator/src/main/java/org/keycloak/operator/v2alpha1/KeycloakDeployment.java

operator/src/main/resources/base-keycloak-deployment.yaml

operator/src/main/resources/example-keycloak.yml

operator/src/main/java/org/keycloak/operator/WatchedResourcesStore.java

vmuzikar · 2022-01-19T10:10:25Z

@andreaTP Thank you for your review! I hopefully answered all the comments. :)

vmuzikar · 2022-01-21T11:21:24Z

I removed the watched resources store (i.e. the Deployment won't be restarted on secret change). But I decided not to change the format for referring Secrets (${secret:...) for now as it would create even bigger difference between local configuration using conf files and configuration using CRs. Because we'd need to use name and values separately.

andreaTP

@vmuzikar Great update! 👏

A few more comments, but only one important regarding the(missing or I missed it 🙂 ) condition for recreating the Deployment, all the rest is totally in the right direction!

operator/src/main/java/org/keycloak/operator/v2alpha1/KeycloakController.java

operator/src/main/java/org/keycloak/operator/v2alpha1/KeycloakDeployment.java

operator/src/main/java/org/keycloak/operator/v2alpha1/crds/KeycloakSpec.java

operator/src/main/resources/postgres.yaml

operator/src/main/java/org/keycloak/operator/OperatorManagedResource.java

andreaTP · 2022-01-21T15:45:59Z

operator/src/main/java/org/keycloak/operator/OperatorManagedResource.java

+        setDefaultLabels(resource);
+        setOwnerReferences(resource);
+
+        Log.debugf("Creating or updating resource: %s", resource);


this is still valid, but I don't have a super strong opinion against, feel free to mark this as Resolved if we don't care about consistency in this codebase.

andreaTP · 2022-01-21T15:47:12Z

operator/src/main/java/org/keycloak/operator/OperatorManagedResource.java

+    }
+
+    protected void setOwnerReferences(HasMetadata resource) {
+        if (!cr.getMetadata().getNamespace().equals(resource.getMetadata().getNamespace())) {


this comment instead is still valid, can you, please, either remove the condition at all or throw an Exception instead of silently returning?

andreaTP

LGTM, totally good enough to start iterating! 💯

jonathanvila · 2022-01-24T14:38:45Z

operator/src/main/java/org/keycloak/operator/v2alpha1/KeycloakDeployment.java

    }

-    private Deployment baseDeployment;
+    @Override
+    protected HasMetadata getReconciledResource() {


Probably using other variables names could make this less confusing ? wdyt ?

Ok, unified the naming of the local variable with the field name.

jonathanvila · 2022-01-24T14:39:53Z

operator/src/main/java/org/keycloak/operator/v2alpha1/crds/KeycloakSpec.java

 public class KeycloakSpec {

    private int instances = 1;
+    private String image;
+    private Map<String, String> serverConfiguration;
+    private PodTemplate podTemplate;


I would suggest to move this out to another PR

jonathanvila · 2022-01-24T14:41:55Z

operator/src/main/resources/application.properties

@@ -2,3 +2,6 @@ quarkus.operator-sdk.crd.apply=true
 quarkus.operator-sdk.generate-csv=true
 quarkus.container-image.builder=jib
 quarkus.operator-sdk.crd.validate=false
+
+# Operator config
+operator.operand.image=quay.io/keycloak/keycloak-x:latest


I would suggest another naming, as every element created by the operator are operands .... so probably operator.operand.keycloakImage could be clearer ? wdyt ?

Renamed to operator.keycloak.image. WDYT?

jonathanvila · 2022-01-24T14:43:57Z

operator/src/main/resources/example-postgres.yaml

+apiVersion: apps/v1
+kind: StatefulSet
+metadata:
+  name: postgresql-db


should we always prefix the operands with the cr name ? or at least with keycloak ? In the same namespace the user could have another postgresql-db for other purposes. Wdyt ?

This is actually not an operand. ;) It's just a helper deployment for devs to easily deploy KC with external DB.

Yes you are right....I see what you mean.

vmuzikar · 2022-01-24T16:22:13Z

@jonathanvila Thanks for the review! I pushed requested changes and rebased on top of master. Will squash the commits once I have a final approval from you and @andreaTP.

andreaTP

LGTM

jonathanvila

LGTM 👍

vmuzikar · 2022-01-25T08:12:33Z

I squashed the commits.

AndrewFarley · 2022-03-24T00:49:23Z

Please forgive me if this is a silly question, but can I ask if/when this will make it into a release of Keycloak-X? I'd like to have a reasonable probe in Kubernetes instead of just a tcpProbe. I can't see this being functional on the latest docker image, 16.1.1, or perhaps the question should instead be when will the docker images be updated eg: to the 17.0.1 release from yesterday?

andreaTP · 2022-03-24T12:32:46Z

Hi @AndrewFarley this operator will be included as preview in the next Keycloak release:
https://www.keycloak.org/2022/03/releases

vmuzikar requested review from andreaTP and jonathanvila January 18, 2022 18:23

vmuzikar added area/operator Keycloak.X Operator keycloak.x kind/feature Categorizes a PR related to a new feature labels Jan 18, 2022

andreaTP suggested changes Jan 18, 2022

View reviewed changes

vmuzikar force-pushed the keycloak-deployment branch from fab4fe5 to 8426c40 Compare January 21, 2022 11:09

andreaTP suggested changes Jan 21, 2022

View reviewed changes

vmuzikar marked this pull request as ready for review January 21, 2022 16:28

andreaTP previously approved these changes Jan 21, 2022

View reviewed changes

jonathanvila reviewed Jan 24, 2022

View reviewed changes

vmuzikar dismissed andreaTP’s stale review via d8dc050 January 24, 2022 16:16

vmuzikar force-pushed the keycloak-deployment branch 2 times, most recently from d8dc050 to 0332c4c Compare January 24, 2022 16:18

vmuzikar requested review from andreaTP and jonathanvila January 24, 2022 16:22

andreaTP previously approved these changes Jan 24, 2022

View reviewed changes

jonathanvila previously approved these changes Jan 24, 2022

View reviewed changes

Baseline for Keycloak deployment in operator

ae47e70

vmuzikar dismissed stale reviews from jonathanvila and andreaTP via ae47e70 January 25, 2022 08:12

vmuzikar force-pushed the keycloak-deployment branch from 0332c4c to ae47e70 Compare January 25, 2022 08:12

pedroigor approved these changes Jan 25, 2022

View reviewed changes

pedroigor merged commit 6b485b8 into keycloak:main Jan 25, 2022

vmuzikar deleted the keycloak-deployment branch January 25, 2022 14:07

Baseline for Keycloak deployment in operator #9628

Baseline for Keycloak deployment in operator #9628

Conversation

vmuzikar commented Jan 18, 2022 • edited

andreaTP 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

vmuzikar commented Jan 19, 2022

vmuzikar commented Jan 21, 2022

andreaTP left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreaTP 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

vmuzikar commented Jan 24, 2022

andreaTP left a comment

Choose a reason for hiding this comment

jonathanvila left a comment

Choose a reason for hiding this comment

vmuzikar commented Jan 25, 2022

AndrewFarley commented Mar 24, 2022

andreaTP commented Mar 24, 2022

vmuzikar commented Jan 18, 2022 •

edited