[keycloak] Rename environment variables. #13791

monaka · 2019-07-08T09:56:14Z

Signed-off-by: Masaki Muranaka [email protected]

I noticed when I deployed eclipse/che-keycloak:nightly NOT by using Helm nor Openshift.
So this patch is not tested well. But I believe multiuser deployments will be failed without this patch.

I can't determine if this is critical or not. As this is only critical for users who want to the multiuser env.

What does this PR do?

Renames environment variables applying to the che-keycloak instance.

PORTGRES_PORT_5432_TCP_ADDR to POSTGRES_ADDR .
POSTGRES_PORT_5432_TCP_PORT to POSTGRES_PORT .

What issues does this PR fix or reference?

refs #13625

che-bot · 2019-07-08T09:56:15Z

Can one of the admins verify this patch?

che-bot · 2019-07-08T09:56:15Z

Can one of the admins verify this patch?

l0rd · 2019-07-08T10:06:03Z

@davidfestal any idea about that?

davidfestal · 2019-07-08T10:35:18Z

Yes, in the latest versions of the Keycloak docker image, the environment variable names have changed.
I had to do a similar change in the operator code after upstream Che had switched to the latest Keycloak version.
However in the operator case, in order to still be backward compatible with previous versions of keycloak (and also to reduce risks of regression when used with previous versions of RHSSO), I only added the missing POSTGRES_PORT environment variable while also keeping the old ones.

musienko-maxim · 2019-07-08T10:39:55Z

Can one of the admins verify this PR?

l0rd · 2019-07-08T11:21:45Z

@davidfestal ok but it doesn't look like che deployment (with helm or deploy_che.sh) are affected by this problem. Is it because these deployments are using an older keycloak image? cc @skabashnyuk

monaka · 2019-07-08T11:46:22Z

Just by my rough inspection, the vanilla K8s controller will apply POSTGRES_PORT with unexpected value to the Keycloak instance. Because Helm charts deploys svc/postgres. So at least POSTGRES_PORT should be care, IMO.

skabashnyuk · 2019-07-08T14:23:16Z

@monaka I belive that eclipse/che-keycloak:nightly is used by Openshift https://github.com/eclipse/che/blob/master/deploy/openshift/deploy_che.sh#L169-L172 and that is something that we are testing on CI same for k8s https://github.com/eclipse/che/blob/master/deploy/kubernetes/helm/che/custom-charts/che-keycloak/values.yaml#L14. Can you describe what kind of issue you have and how to reproduce it?

monaka · 2019-07-08T15:06:16Z

@skabashnyuk I'm not sure how scripts in deploy/{kubernetes,openshift} are tested. And I'm not an expert of Openshift. So just a guess but.

Looks both PORTGRES_PORT_5432_TCP_ADDR and POSTGRES_PORT_5432_TCP_PORT are not used anywhere.
So POSTGRES_ADDR and POSTGRES_PORT won't applied as expected.

Even if the issue above, POSTGRES_ADDR might be a potential issue since it looks the default value of POSTGRES_ADDR will be postgres. I believe this will be a critical issue if the user uses the external Postgres. But we can ignore this issue as there is no way to setup external Postgres with the official scripts. (should be fixed after 7.0.0 released)

The pain point I guess is POSTGRES_PORT. K8s will applies unexpected POSTGRES_PORT to Keycloak as svc/postgres is defined and not set POSTGRES_PORT expressly.

... After I wrote above. I found the issue what I want to say. helm/charts#9561
I got similar issue like that and fixed on my fork. I believe it will be reproduced on the official deployment scripts.

monaka · 2019-07-12T03:11:33Z

I updated the patch in this PR as I found the issue #13821.
It will work on Openshift even though I've not checked on it.

skabashnyuk · 2019-07-12T06:53:59Z

ci-test

che-bot · 2019-07-12T10:07:05Z

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:13791
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

l0rd · 2019-07-22T11:50:13Z

@monaka this PR fixes #13919 right? It doesn't solve the migration part for users that currently use H2 but at least fix the problem for fresh deployments. I am removing the do-not-merge label since #13919 is a 7.0.0 issue.

l0rd

We need to test that but +1 to merge this PR as soon as @monaka is ready and @eclipse/eclipse-che-qa is ok.

mkuznyetsov · 2019-07-22T11:59:13Z

@monaka
I have troubles verifying these changes - can't start the Keycloak Pod
(running on Minikube, installed Che through chectl via Helm)
As I see it, Postgres DB is attempted to be used but there is some error related to Drivers

Pod logs:
logs-from-keycloak-in-keycloak-58656dbd4d-hgf52.txt

monaka · 2019-07-22T22:04:21Z

@mkuznyetsov Thank you for your checking and report. It may be required to apply some more patches to Dockerfile for che-keycloak. I'll inspect there today.

POSTGRES_* to DB_*. Signed-off-by: Masaki Muranaka <[email protected]>

Signed-off-by: Masaki Muranaka <[email protected]>

DB_VENDOR is parsed in `docker-entrypoint.sh`. `standalone.sh` will be called the tail of `docker-entrypoint.sh.` Signed-off-by: Masaki Muranaka <[email protected]>

monaka · 2019-07-23T05:05:16Z

@mkuznyetsov I pushed the additional patch here. (And I pushed the patched docker image into https://hub.docker.com/r/monaka/che-keycloak for saving time and effort.)

I checked my deployment by Helm boot up with no error on my AKS cluster.

skabashnyuk · 2019-07-23T07:25:18Z

ci-test

mkuznyetsov

@monaka thank you, it works now.

che-bot · 2019-07-23T10:39:07Z

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:13791
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

skabashnyuk · 2019-07-23T11:32:00Z

@eclipse/eclipse-che-qa do you see any issues in test results?

skabashnyuk · 2019-07-23T13:38:13Z

ci-build

skabashnyuk · 2019-07-23T14:19:21Z

@monaka if you don't mind I want to merge it to make it part of RC4

nickboldt · 2019-07-23T16:32:38Z

dockerfiles/keycloak/kc_realm_user.sh

-                                      -Dkeycloak.migration.strategy=IGNORE_EXISTING \
-                                      -Dkeycloak.migration.dir=/scripts/ \
-                                      -Djboss.bind.address=0.0.0.0
+exec /opt/jboss/docker-entrypoint.sh -Dkeycloak.migration.action=import \


why the change of script here? what does docker-entrypoint.sh do that standalone.sh doesn't? (Also, aside, "container-entrypoint" would be better. :) )

Please read docker-entrypoint.sh that stored in the base image.
Some important environment variables are parsed in there. standalone.sh is called by docker-entrypoint.sh after parsing.

It's best to ask maintainers of the jboss-dockerfiles organization (not me) if you don't like the name docker-entrypoint.sh.

monaka · 2019-07-24T03:24:11Z

@skabashnyuk I see. I pressed merge button now. And it may be better to back-port to Che 6. WDYT?

monaka added the kind/bug Outline of a bug - must adhere to the bug report template. label Jul 8, 2019

monaka requested review from l0rd, nickboldt, rhopp and sleshchenko as code owners July 8, 2019 09:56

monaka changed the title ~~Rename environment variables.~~ [keycloak] Rename environment variables. Jul 8, 2019

l0rd requested a review from davidfestal July 8, 2019 10:06

l0rd added do-not-merge/hold status/analyzing An issue has been proposed and it is currently being analyzed for effort and implementation approach and removed kind/bug Outline of a bug - must adhere to the bug report template. labels Jul 8, 2019

monaka mentioned this pull request Jul 12, 2019

[Helm] warnings on booting keycloak #13821

Closed

mkuznyetsov mentioned this pull request Jul 19, 2019

Keycloak uses H2 database in Che on Helm Kubernetes deployment #13919

Closed

23 tasks

l0rd removed do-not-merge/hold status/analyzing An issue has been proposed and it is currently being analyzed for effort and implementation approach labels Jul 22, 2019

l0rd approved these changes Jul 22, 2019

View reviewed changes

monaka added 3 commits July 23, 2019 12:05

Rename environment variables. (eclipse#13821)

2718fe6

POSTGRES_* to DB_*. Signed-off-by: Masaki Muranaka <[email protected]>

Add DB_VENDOR to env. (eclipse#13919)

bb7eac2

Signed-off-by: Masaki Muranaka <[email protected]>

Fix to use DB_VENDOR.

7aa8141

DB_VENDOR is parsed in `docker-entrypoint.sh`. `standalone.sh` will be called the tail of `docker-entrypoint.sh.` Signed-off-by: Masaki Muranaka <[email protected]>

monaka requested a review from benoitf as a code owner July 23, 2019 04:55

mkuznyetsov approved these changes Jul 23, 2019

View reviewed changes

nickboldt reviewed Jul 23, 2019

View reviewed changes

monaka merged commit 3d8ea83 into eclipse-che:master Jul 24, 2019

monaka deleted the pr-fix-env-var-for-keycloak branch July 24, 2019 03:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[keycloak] Rename environment variables. #13791

[keycloak] Rename environment variables. #13791

monaka commented Jul 8, 2019 •

edited

Loading

che-bot commented Jul 8, 2019

che-bot commented Jul 8, 2019

l0rd commented Jul 8, 2019

davidfestal commented Jul 8, 2019

musienko-maxim commented Jul 8, 2019

l0rd commented Jul 8, 2019

monaka commented Jul 8, 2019

skabashnyuk commented Jul 8, 2019

monaka commented Jul 8, 2019 •

edited

Loading

monaka commented Jul 12, 2019

skabashnyuk commented Jul 12, 2019

che-bot commented Jul 12, 2019

l0rd commented Jul 22, 2019

l0rd left a comment

mkuznyetsov commented Jul 22, 2019

monaka commented Jul 22, 2019

monaka commented Jul 23, 2019

skabashnyuk commented Jul 23, 2019

mkuznyetsov left a comment

che-bot commented Jul 23, 2019

skabashnyuk commented Jul 23, 2019

skabashnyuk commented Jul 23, 2019

skabashnyuk commented Jul 23, 2019

nickboldt Jul 23, 2019

monaka Jul 24, 2019

monaka commented Jul 24, 2019

[keycloak] Rename environment variables. #13791

[keycloak] Rename environment variables. #13791

Conversation

monaka commented Jul 8, 2019 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

che-bot commented Jul 8, 2019

che-bot commented Jul 8, 2019

l0rd commented Jul 8, 2019

davidfestal commented Jul 8, 2019

musienko-maxim commented Jul 8, 2019

l0rd commented Jul 8, 2019

monaka commented Jul 8, 2019

skabashnyuk commented Jul 8, 2019

monaka commented Jul 8, 2019 • edited Loading

monaka commented Jul 12, 2019

skabashnyuk commented Jul 12, 2019

che-bot commented Jul 12, 2019

l0rd commented Jul 22, 2019

l0rd left a comment

Choose a reason for hiding this comment

mkuznyetsov commented Jul 22, 2019

monaka commented Jul 22, 2019

monaka commented Jul 23, 2019

skabashnyuk commented Jul 23, 2019

mkuznyetsov left a comment

Choose a reason for hiding this comment

che-bot commented Jul 23, 2019

skabashnyuk commented Jul 23, 2019

skabashnyuk commented Jul 23, 2019

skabashnyuk commented Jul 23, 2019

nickboldt Jul 23, 2019

Choose a reason for hiding this comment

monaka Jul 24, 2019

Choose a reason for hiding this comment

monaka commented Jul 24, 2019

monaka commented Jul 8, 2019 •

edited

Loading

monaka commented Jul 8, 2019 •

edited

Loading