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.

Sign up for GitHub

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

Jump to bottom

Operator-related fix and openshift v4 support #13554

Merged
merged 4 commits into from
Jun 17, 2019
Merged

Operator-related fix and openshift v4 support #13554

merged 4 commits into from
Jun 17, 2019
+6 −0

Conversation

davidfestal
Copy link
Contributor

What does this PR do?

This PR provides fixes to the che-keycloak image in order to allow the community Che operator
to successfully install a Che server integrated with the Openshift v4 OAuth authentication.

More precisely this PR:

  • Restores the initial user that is set in the base Keycloak image (1000 => jboss user), instead of keeping the ROOT user as the docker image user, which brings problems in both Kubernetes or Openshift contexts in the Che operator use-cases.
  • Fixes an incompatibility with the Che operator introduced by PR Used latest keycloak 6.0.1 #13429 : the reason is that the last Keycloak image has moved its entry-point. Since the Che operator overrides the entry-point and then delegates to the original one, Che operator was broken by this change. In order to provide compatibility of the Che operator with as many Che versions as possible, this PR add a link between the new and the old entry-point paths, so that the Che operator will be able to work with both.
  • Deploys the new Openshift-v4 identity provider implemented in the context of issues https://issues.jboss.org/browse/CRW-202 and https://issues.jboss.org/browse/KEYCLOAK-10169.
    When this new provider will be integrated into the Keycloak product itself in a future Keycloak release, we'll be able to remove this last change.

What issues does this PR fix or reference?

This PR is involved in fixing issue: redhat-developer/rh-che#1454,
and is also a followup of PR https://github.com/eclipse/che/pull/13429/files which had introduced a breaking change for the community Che operator.

@davidfestal
Restore initial image user
fbdbfa9 
Signed-off-by: David Festal <dfestal@redhat.com>
@davidfestal
Accomodate with the entrypoint move
daf7f61 
Signed-off-by: David Festal <dfestal@redhat.com>
@davidfestal
Add the new Openshift-v4 provider
e1ade5d 
This is provisional, until the new provider is
integrated into a future Keycloak release.

Signed-off-by: David Festal <dfestal@redhat.com>
@davidfestal davidfestal requested a review from l0rd June 17, 2019 09:53
@davidfestal davidfestal self-assigned this Jun 17, 2019
benoitf
benoitf reviewed Jun 17, 2019
View reviewed changes
dockerfiles/keycloak/Dockerfile Outdated
@@ -10,7 +10,14 @@ FROM jboss/keycloak:6.0.1
ADD che /opt/jboss/keycloak/themes/che
ADD . /scripts/
ADD cli /scripts/cli
RUN ln -s /opt/jboss/tools/docker-entrypoint.sh
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, can we merge all run instructions into a single one ?

benoitf
benoitf reviewed Jun 17, 2019
View reviewed changes
dockerfiles/keycloak/Dockerfile Outdated
@@ -10,7 +10,14 @@ FROM jboss/keycloak:6.0.1
ADD che /opt/jboss/keycloak/themes/che
ADD . /scripts/
ADD cli /scripts/cli
RUN ln -s /opt/jboss/tools/docker-entrypoint.sh
RUN curl --location https://github.com/davidfestal/KEYCLOAK-10169-OpenShift4-User-Provider/releases/download/6.0.1-openshift-v4-provider/openshift4-extension-6.0.1.jar -o /opt/jboss/keycloak/standalone/deployments/openshift4-extension-6.0.1.jar
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't have jar on something that is not belonging to a given user (davidfestal ? )

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe use a multi-staged build ?

Copy link
Contributor

@nickboldt nickboldt Jun 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eventually the fix will be in Keycloak upstream (and RH-SSO downstream) but for now it's a custom jar from https://github.com/slaskawi/KEYCLOAK-10169-OpenShift4-User-Provider (or David's fork). It's not yet in the Keycloak/SSO codebase. See KEYCLOAK-10169 for discussion/details.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to @benoitf suggestion. And consider using curl -sSL flags to follow redirections and avoid having the % for the download in the logs.

@davidfestal davidfestal mentioned this pull request Jun 17, 2019
Merged
@davidfestal
Fixes according to comments
a4a3d07 
Signed-off-by: David Festal <dfestal@redhat.com>
@davidfestal
Copy link
Contributor Author

@benoitf @l0rd I fixed your comments. Is it better like that ?

@davidfestal davidfestal merged commit 93a4926 into master Jun 17, 2019
@davidfestal davidfestal mentioned this pull request Jun 19, 2019
Closed
@che-bot che-bot added this to the 7.0.0 milestone Jun 27, 2019
@che-bot che-bot added the kind/bug Outline of a bug - must adhere to the bug report template. label Jun 27, 2019
@Katka92 Katka92 deleted the operator-fix-and-openshift-v4-support branch February 18, 2020 09:58
@Katka92 Katka92 restored the operator-fix-and-openshift-v4-support branch February 18, 2020 09:59
@davidfestal davidfestal deleted the operator-fix-and-openshift-v4-support branch February 18, 2020 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickboldt nickboldt nickboldt left review comments

@benoitf benoitf benoitf approved these changes

@l0rd l0rd l0rd approved these changes

Assignees

@davidfestal davidfestal

Labels
kind/bug Outline of a bug - must adhere to the bug report template.
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@davidfestal @nickboldt @benoitf @l0rd @che-bot