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

[Keycloak] Fix bugs on the docker image. #11761

Closed
wants to merge 1 commit into from
Closed

[Keycloak] Fix bugs on the docker image. #11761

wants to merge 1 commit into from

Conversation

monaka
Copy link
Member

@monaka monaka commented Oct 26, 2018

What does this PR do?

Fix the container image for Keycloak to use Postgres instead of H2.

What issues does this PR fix or reference?

Release Notes

Docs PR

@riuvshin
Copy link
Contributor

Can one of the admins verify this patch?

2 similar comments
@riuvshin
Copy link
Contributor

Can one of the admins verify this patch?

@riuvshin
Copy link
Contributor

Can one of the admins verify this patch?

@monaka monaka requested a review from benoitf as a code owner October 26, 2018 21:02
@monaka monaka requested a review from a user October 26, 2018 21:02
@benoitf benoitf added the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Oct 26, 2018
@monaka
Copy link
Member Author

monaka commented Oct 26, 2018

I've just tested on Helm. I guess there have the similar issue on another runtimes. But I didn't touch them as I'm not familiar with them.

@monaka
Copy link
Member Author

monaka commented Oct 27, 2018

I believe this patch itself is sane. But I encountered the another issue around authentication with Che.
I suspect there doesn't have json.erb files required by Che in the Keycloak image.
Which is better, a or b?

(a) I send the additional PR after closed this as they are separated issues.
(b) I push additional patches to his PR because the aim of this PR is to fix deployment failure.

@skabashnyuk
Copy link
Contributor

@monaka can you explain how changes you've made related to the topic of this pr? Especially commented lines in kc_realm_user.sh

@monaka
Copy link
Member Author

monaka commented Oct 27, 2018

@skabashnyuk The current image doesn't contain /scripts/*.json.erb. So kc_realm_user.sh generates empty /scripts/*.json.erb. Empty JSONs cause Keycloak with Postgres crash. This is a reason why I commented out.

But actually, it should contain /scripts/*.json.erb. Che can't use Keycloak as the realm for Che will be set up by them.

This is a point I commented above. This PR itself is sane. But some additional patches will be required for complete works.

@monaka
Copy link
Member Author

monaka commented Oct 28, 2018

Let me change the title and the aim for this PR. As I tweeted, I managed to run Che on AKS (managed vanilla Kubernetes). I'm going to add some more patches to here.

@monaka monaka changed the title [Keycloak] Use Postgres instead of H2. [Keycloak] Fix bugs on the docker image. Oct 28, 2018
@benoitf benoitf added the kind/bug Outline of a bug - must adhere to the bug report template. label Oct 28, 2018
@skabashnyuk
Copy link
Contributor

@monaka can you please update the description of this pr with such information:

  • What do you want to do?
  • On wich environment? Docker, OpenShift, Kubernetes?
  • Which version of infrastructure?
  • Which version of Che?
  • What issues do you have with current codebase? Traces, logs?

@monaka
Copy link
Member Author

monaka commented Oct 31, 2018

What do you want to do?

Enable to run Che on vanilla K8s clusters.

On wich environment? Docker, OpenShift, Kubernetes?

Kubernetes.
(Possibly Docker doesn't. Because it will use init container for bootstrapping.)

Which version of infrastructure?

Azure AKS 1.11.3.
But the issue will be reproduced all versions of K8s based infrastructure.

Which version of Che?

master branch, nightly builds.

What issues do you have with current codebase? Traces, logs?

  • I can't see any tables in keycloak database on Postgres container.
  • I can't see any tables in H2 for the che realm.

After fixing above,

  • There is no content for the custom login page.

@monaka
Copy link
Member Author

monaka commented Oct 31, 2018

Currently, /opt/jboss/keycloak/bin/standalone.sh is called directly on bootstrapping.
But it is required to use /opt/jboss/docker-entrypoint.sh if we want to use external DB.

@ghost
Copy link

ghost commented Oct 31, 2018

@monaka thanks for fixing it. However, I do not understand why you copied jsons? it's ok to use them from where they are

@monaka
Copy link
Member Author

monaka commented Oct 31, 2018

Indeed it's not good to put same files into different places. But I couldn't resolve how to access JSONs in init container from che-keycloak. I'm afraid my patches cause side effects to Openshift and Docker deployments, even though it might be resolved by some tricky patches to init.

@ghost
Copy link

ghost commented Nov 1, 2018

@monaka it is ok to reuse jsons from init.

Just changing start script in the entrypoint should be enough, and you did that already

@monaka
Copy link
Member Author

monaka commented Nov 3, 2018

@eivantsov Who setups the realm for Che?

@ghost
Copy link

ghost commented Nov 5, 2018

@monaka it happens in the entrypoint

@monaka
Copy link
Member Author

monaka commented Nov 6, 2018

Yes. In addition to patches here, we must add the patch to depoyment/keycloak like this.
I'm not sure I'm not familiar with Openshift. But I think this issue will be reproduced on it.

        - name: PROXY_ADDRESS_FORWARDING
          value: "true"
        - name: DB_VENDOR
          value: POSTGRES

@musienko-maxim
Copy link
Contributor

Can one of the admins verify this PR?

Signed-off-by: Masaki Muranaka <monaka@monami-ya.com>
@monaka
Copy link
Member Author

monaka commented Jul 4, 2019

I close this as it have not been updated.

@monaka monaka closed this Jul 4, 2019
@monaka monaka deleted the pr-use-postgres branch July 4, 2019 03:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Outline of a bug - must adhere to the bug report template. status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants