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-7582 Discovery protocol support #151

Merged
merged 1 commit into from Sep 24, 2018

Conversation

@slaskawi
Copy link
Collaborator

commented Sep 18, 2018

https://issues.jboss.org/browse/KEYCLOAK-7582

This PR adds clustering support for the Keycloak Docker image. As the matter of fact, it makes allows to use any discovery protocol with custom configuration.

Here's how it works:

  • The configuration scripts pick up JGROUPS_DISCOVERY_PROTOCOL and JGROUPS_DISCOVERY_PROPERTIES variables.
  • In the next step, the main entry point script passes those two configuration variables to jgroups.sh script.
  • The script adjusts the variables and invokes cli/jgroups/discovery/default.cli (or a cli file with an exact protocol name if it exists; but in most of the cases, the default.cli should be enough).
  • The CLI script modifies standalone-ha.xml. It removed the default discovery protocol and replaces it with the one specified above.
  • The final piece of the puzzle is adjusting JGroups bind options. By default they operate on the private interface. We need rewire it to the external, public port. By default we use hostname -i for that.

I also uploaded a test image, so that you can play with it. Just replace the image tag in the template with slaskawi/keycloak-jgroups-discovery.

@@ -184,7 +191,8 @@
"containers": [
{
"name": "${APPLICATION_NAME}",
"image": "jboss/keycloak-openshift",
"image": "slaskawi/keycloak-jgroups-discovery",

This comment has been minimized.

Copy link
@pedroigor

pedroigor Sep 18, 2018

Contributor

Do we need to change the image here?

This comment has been minimized.

Copy link
@slaskawi

slaskawi Sep 19, 2018

Author Collaborator

No! definitely not.... that one slipped in by an accident... fixing.

@pedroigor

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2018

LGTM

@slaskawi slaskawi force-pushed the slaskawi:KEYCLOAK-7582 branch from 735bf15 to a380c10 Sep 19, 2018

BIND=$(hostname -i)
fi
if [ -z "$BIND_OPTS" ]; then
BIND_OPTS="-Djboss.bind.address.management=0.0.0.0 -Djgroups.join_timeout=1000 -Djgroups.bind_addr=$BIND -Djboss.bind.address=$BIND"

This comment has been minimized.

Copy link
@rhusar

rhusar Sep 19, 2018

Contributor

Why open management ports to any address?

This comment has been minimized.

Copy link
@rhusar

rhusar Sep 19, 2018

Contributor

Also, what's the reasoning behind GMS.join_timeout of 1 second? (Seems quite low to me for a cloud environment and I was thinking we may actually increase it from the default number for cloud images)

This comment has been minimized.

Copy link
@slaskawi

slaskawi Sep 20, 2018

Author Collaborator

Sorry, I just copied it from Infinispan template without putting much thought into it. Let me adjust it to our needs. Thanks a lot for spotting this Rado!

fi



This comment has been minimized.

Copy link
@rhusar

rhusar Sep 19, 2018

Contributor

To nitpick, unnecessary extra new lines here.

This comment has been minimized.

Copy link
@slaskawi

slaskawi Sep 20, 2018

Author Collaborator

Done.

@deitch

This comment has been minimized.

Copy link

commented Sep 19, 2018

I am coming to this from #96. Once merged, what would be the "official" way of running this in Kubernetes? Would we just use DNS_PING and set up a headless service with ClusterIP=None, thus resolving all of the affiliated IPs?

@stianst

This comment has been minimized.

Copy link
Collaborator

commented Sep 19, 2018

Haven't reviewed this yet, but realized we need a check to make sure clustering isn't enabled with embedded H2 dB as those two together makes no sense

BIND=$(hostname -i)
fi
if [ -z "$BIND_OPTS" ]; then
BIND_OPTS="-Djboss.bind.address.management=0.0.0.0 -Djgroups.join_timeout=1000 -Djgroups.bind_addr=$BIND -Djboss.bind.address=$BIND"

This comment has been minimized.

Copy link
@rhusar

rhusar Sep 19, 2018

Contributor

The jgroups.bind_addr has no effect in WildFly-based software using jgroups subsystem, which I assume is the case here.

This comment has been minimized.

Copy link
@slaskawi

slaskawi Sep 20, 2018

Author Collaborator

Thanks! Fixed.

This comment has been minimized.

Copy link
@slaskawi

slaskawi Sep 20, 2018

Author Collaborator

BTW, it's a bit mistery to me why JGroups bind to jboss.bind.address.private by default. Should they bind to the public interface instead? The only reason why not to do it is security aspect. But then we end up in assigning a public/external address to the private interface, which looks awkward....

@stianst
Copy link
Collaborator

left a comment

As far as I can see we don't automatically enable clustering on OpenShift/Kubernetes in this PR. That's OK and we can handle something automatically in a separate PR, but that would have to check that the configured DB is also up to clustering.

I'm less keen on the amount of things that seems to have to be set in the template.

"displayName": "Namespace used for DNS discovery",
"description": "This namespace is a part of DNS query sent to Kubernetes API. This query allows the DNS_PING protocol to extract cluster members. This parameter might be removed once https://issues.jboss.org/browse/JGRP-2292 is implemented.",
"name": "NAMESPACE",
"value": "myproject",

This comment has been minimized.

Copy link
@stianst

stianst Sep 20, 2018

Collaborator

Can the namespace somehow be templated so this doesn't have to be changed? Surely OpenShift knows what namespace it's being deployed to?

This comment has been minimized.

Copy link
@slaskawi

slaskawi Sep 20, 2018

Author Collaborator

Unfortunately no. And as the parameter description mentions, JGRP-2292 prevents us from using a Service name (since it appends svc.cluster.local suffix). See the JGRP-2292 ticket for details.

It isn't perfect, I know. But we'll improve it once the JGroups patch is in.

This comment has been minimized.

Copy link
@stianst

stianst Sep 21, 2018

Collaborator

OK, as it's very likely that myproject won't be the right namespace I would leave it unspecified so users are required to set a value. As it is now there's a chance that users forget to set it and it's left as the default myproject value

This comment has been minimized.

Copy link
@slaskawi

slaskawi Sep 21, 2018

Author Collaborator

This parameter will go away very shortly. We need two JGroups tickets to fix it (JGRP-2295 and JGRP-2292). Rob already sent a pull request for both. So if we are lucky, this parameter will disappear in a couple of weeks.

So what do you think, having the above in mind, should we still enforce this parameter from users? Even though it will be gone shortly?

This comment has been minimized.

Copy link
@slaskawi

slaskawi Sep 21, 2018

Author Collaborator

After giving it a little more thought... we can always remove that parameter once all changes are in. Give me a sec to introduce a fix.

@@ -185,6 +192,7 @@
{
"name": "${APPLICATION_NAME}",
"image": "jboss/keycloak-openshift",
"args": ["-c", "standalone-ha.xml"],

This comment has been minimized.

Copy link
@stianst

stianst Sep 20, 2018

Collaborator

Shouldn't changing to standalone-ha.xml be handled by the image itself rather than having to change the args here?

This comment has been minimized.

Copy link
@slaskawi

slaskawi Sep 20, 2018

Author Collaborator

Ok, so I had a pretty long chat with Infinispan folks. In short - the replicated caches (used in standalone-ha.xml) will start slower (since they try to do discovery) but the runtime performance should be roughly the same as local ones.

So it makes sense to always start the HA profile. I will add necessary switches.

This comment has been minimized.

Copy link
@slaskawi

slaskawi Sep 20, 2018

Author Collaborator

Done.

Here's an example that adds `DNS_PING` that queries `A` records from DNS with the following query `keycloak.myproject.svc.cluster.local`:

docker run \
-e JGROUPS_DISCOVERY_PROTOCOL=DNS_PING -e \

This comment has been minimized.

Copy link
@petr-k

petr-k Sep 20, 2018

Shouldn't the protocol name be dns.DNS_PING?

This comment has been minimized.

Copy link
@slaskawi

slaskawi Sep 20, 2018

Author Collaborator

Good point! Thanks!

@slaskawi

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 20, 2018

I am coming to this from #96. Once merged, what would be the "official" way of running this in Kubernetes? Would we just use DNS_PING and set up a headless service with ClusterIP=None, thus resolving all of the affiliated IPs?

@deitch The DNS_PING is the suggested way of doing that. The biggest pain of KUBE_PING is that it needs a view permissions, which are not granted by default in OpenShift. Apart from that, feel free to use either DNS_PING or KUBE_PING (or even PING/MPING if your environment supports IP Multicasting; this feature needs to be explicitly turned on in the OpenShift project if that's the environment you're using).

@slaskawi slaskawi force-pushed the slaskawi:KEYCLOAK-7582 branch from a380c10 to fe26731 Sep 20, 2018

@slaskawi

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 20, 2018

Haven't reviewed this yet, but realized we need a check to make sure clustering isn't enabled with embedded H2 dB as those two together makes no sense

This one obviously requires a bit more work. I created https://issues.jboss.org/browse/KEYCLOAK-8331 to track this.

@slaskawi slaskawi force-pushed the slaskawi:KEYCLOAK-7582 branch from fe26731 to 770ea7c Sep 20, 2018

@slaskawi

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 20, 2018

Dear all, I addressed all the comments and retested the image. Ready for the next round of review!

/subsystem=jgroups/stack=udp/protocol=PING:remove()
/subsystem=jgroups/stack=udp/protocol=$keycloak_jgroups_discovery_protocol:add(add-index=0, properties=$keycloak_jgroups_discovery_protocol_properties)

/subsystem=jgroups/stack=tcp/protocol=MPING:remove()

This comment has been minimized.

Copy link
@petr-k

petr-k Sep 20, 2018

If I wanted to use this in Kubernetes with dns.DNS_PING protocol, setting the dns_query appropriately, is this all there is to it? I think this will leave the discovery channel defaulting to UDP, but I am not sure if that's an issue. For example, a very similar PR (which aimed to use KUBE_PING) sets the discovery stack to TCP.. see https://github.com/jboss-dockerfiles/keycloak/pull/96/files#diff-ac0654459af759478556ff1471ebf32dR1

This comment has been minimized.

Copy link
@slaskawi

slaskawi Sep 21, 2018

Author Collaborator

I think we confuse things here... TCP or UDP is transport. KUBE_PING, PING, MPING or dns.DNS_PING are discovery protocols. You can not use more than one transport at a time (even if you create a fork channel). So I think there is no need to allow setting the discovery protocol separately per individual stacks.

Does this answer your question?

This comment has been minimized.

Copy link
@petr-k

petr-k Sep 21, 2018

I'm not referring to the TCP/UDP protocols, but to JGroups named stacks. If you inspect the standalone-ha.xml file, there are two stacks named tcp and udp - and they use TCP and UDP protocols, respectively. JGroups defaults to the udp stack. At least that's my understanding. Or does it handle TCP and UDP at the same time?

My line of thought is this: if the configuration results in a UDP-based communication, then in Kubernetes you'd need to have a headless Service over UDP as well, which is somewhat unexpected and is not the default.

This comment has been minimized.

Copy link
@petr-k

petr-k Sep 21, 2018

Nevermind. Since the headless service is used for DNS discovery only, it should not matter what protocol is used.

@deitch

This comment has been minimized.

Copy link

commented Sep 20, 2018

@slaskawi wrote:

The DNS_PING is the suggested way of doing that. The biggest pain of KUBE_PING is that it needs a view permissions, which are not granted by default in OpenShift.

I think this is great. I don't want to add additional permissions/roles to the service account under which keycloak runs - which, in any case, is an app, not a kube system-level service, so shouldn't get special kube rights to run - but more importantly it makes it generic. If I change my DNS, or service mesh, or whatever, DNS will "just work".

Thanks @slaskawi

"displayName": "Namespace used for DNS discovery",
"description": "This namespace is a part of DNS query sent to Kubernetes API. This query allows the DNS_PING protocol to extract cluster members. This parameter might be removed once https://issues.jboss.org/browse/JGRP-2292 is implemented.",
"name": "NAMESPACE",
"value": "myproject",

This comment has been minimized.

Copy link
@stianst

stianst Sep 21, 2018

Collaborator

OK, as it's very likely that myproject won't be the right namespace I would leave it unspecified so users are required to set a value. As it is now there's a chance that users forget to set it and it's left as the default myproject value

Here's an example that adds `DNS_PING` that queries `A` records from DNS with the following query `keycloak.myproject.svc.cluster.local`:

docker run \
-e JGROUPS_DISCOVERY_PROTOCOL=dns.DNS_PING -e \

This comment has been minimized.

Copy link
@stianst

stianst Sep 21, 2018

Collaborator

I'd change this section to match the DB section better. Add a section clustering. With a section for DNS_PING. I'd use DNS_PING instead of dns.DNS_PING as the value for JGROUPS_DISCOVER_PROTOCOL. I'm also not sure if I like the use if JGROUPS_DISCOVER_PROPERTIES and I'd probably rather have separate variables like DNS_PING_QUERY. Joining multiple parameters into one like this is awkward.

This comment has been minimized.

Copy link
@slaskawi

slaskawi Sep 21, 2018

Author Collaborator

Added proper manual entries. I hope it will look good now.

@@ -0,0 +1,19 @@
#!/bin/bash

This comment has been minimized.

Copy link
@stianst

stianst Sep 21, 2018

Collaborator

I don't think it's needed to have this mechanism that looks for files in an extended image. I'd rather have it look for JGROUPS_DISCOVERY_PROTOCOL=DNS_PING, and if it finds it then it configures DNS_PING. default.cli should be renamed to dnsping.cli or something like that.

If people want to extend the image with their own custom config they don't need to use these variables at all. They should just leave JGROUPS_DISCOVERY_PROTOCOL and make sure their custom config is added separately.

It should however be easy for people to contribute additional protocols to our image directly. For example someone should be able to contribute JDBC_PING (or whatever) by simply updating the README, adding a cli script, etc.

This comment has been minimized.

Copy link
@slaskawi

slaskawi Sep 21, 2018

Author Collaborator

I don't agree here.

I can imagine use cases where you will need multiple protocols for discovery (as we discussed a few days ago). Or maybe you will need to adjust FD_* protocol parameters with discovery timeouts (this is often the case when using KUBE_PING that polls Kubernetes API in a loop). In such case you will need to provide your own cli file that does it. As it is, it's super easy, just extend our image and drop a file into /opt/jboss/tools/cli/jgroups/discovery. If we removed this mechanism, you would need to drop a cli file and then copy-paste the jgroups.sh file. Now if you copy that file, you will need to maintain it manually by syncing manually changes we made in it with your own copy. That's a maintenance pain that is very easy to avoid with my proposed approach.

However, you might have a good point that this behavior should be described in the README. I'll add it.

This comment has been minimized.

Copy link
@slaskawi

slaskawi Sep 21, 2018

Author Collaborator

Added necessary manual entries.

@slaskawi slaskawi force-pushed the slaskawi:KEYCLOAK-7582 branch from 770ea7c to e7d42d9 Sep 21, 2018

@slaskawi

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 21, 2018

@deitch

I think this is great. I don't want to add additional permissions/roles to the service account under which keycloak runs - which, in any case, is an app, not a kube system-level service, so shouldn't get special kube rights to run - but more importantly it makes it generic. If I change my DNS, or service mesh, or whatever, DNS will "just work".

You're welcome! Just remember about creating a governing service and/or specifying proper namespace when running the template.

},
{
"name": "JGROUPS_DISCOVERY_PROPERTIES",
"value": "${APPLICATION_NAME}.${NAMESPACE}.svc.cluster.local"

This comment has been minimized.

Copy link
@petr-k

petr-k Sep 21, 2018

Shouldn't the value be "dns_query=${APPLICATION_NAME}.${NAMESPACE}.svc.cluster.local"?

This comment has been minimized.

Copy link
@slaskawi

slaskawi Sep 25, 2018

Author Collaborator

At the moment, no. The svc.cluster.local is appended automatically. However, we consider this as a mistake and there's already a JIRA to remove it.

This comment has been minimized.

Copy link
@rhusar

rhusar Oct 18, 2018

Contributor

This seems wrong, it should be "value": "dns_query=${APPLICATION_NAME}.${NAMESPACE}.svc.cluster.local"

@stianst stianst merged commit 42e8158 into jboss-dockerfiles:master Sep 24, 2018

@deitch

This comment has been minimized.

Copy link

commented Sep 24, 2018

Really nice to see this merged in, thank you!

What release will this hit? And when will that be available at https://hub.docker.com/r/jboss/keycloak/ ?

@stianst

This comment has been minimized.

Copy link
Collaborator

commented Sep 24, 2018

@deitch

This comment has been minimized.

Copy link

commented Sep 24, 2018

Many thanks @stianst . I will inform my colleagues who depend on keycloak. :-)

@unguiculus unguiculus referenced this pull request Oct 5, 2018
3 of 3 tasks complete
},
{
"name": "JGROUPS_DISCOVERY_PROPERTIES",
"value": "${APPLICATION_NAME}.${NAMESPACE}.svc.cluster.local"

This comment has been minimized.

Copy link
@rhusar

rhusar Oct 18, 2018

Contributor

This seems wrong, it should be "value": "dns_query=${APPLICATION_NAME}.${NAMESPACE}.svc.cluster.local"

unguiculus added a commit to unguiculus/helm-charts that referenced this pull request Jul 10, 2019

[keycloak] Fix bind address handling
Drops setting the system property in favor of setting the
BIND environment variable which was added in
jboss-dockerfiles/keycloak#151.

This does away with a duplicate entry of the system property
'-Djboss.bind.address' in the commandline.

Signed-off-by: Reinhard Naegele <unguiculus@gmail.com>

unguiculus added a commit to unguiculus/helm-charts that referenced this pull request Jul 10, 2019

[keycloak] Fix bind address handling
Drops setting the system property in favor of setting the
BIND environment variable which was added in
jboss-dockerfiles/keycloak#151.

This does away with a duplicate entry of the system property
'-Djboss.bind.address' in the commandline.

Signed-off-by: Reinhard Naegele <unguiculus@gmail.com>

unguiculus added a commit to unguiculus/helm-charts that referenced this pull request Jul 11, 2019

[keycloak] Fix bind address handling
Drops setting the system property in favor of setting the
BIND environment variable which was added in
jboss-dockerfiles/keycloak#151.

This does away with a duplicate entry of the system property
'-Djboss.bind.address' in the commandline.

Signed-off-by: Reinhard Naegele <unguiculus@gmail.com>

unguiculus added a commit to codecentric/helm-charts that referenced this pull request Jul 15, 2019

[keycloak] Fix bind address handling (#72)
Drops setting the system property in favor of setting the
BIND environment variable which was added in
jboss-dockerfiles/keycloak#151.

This does away with a duplicate entry of the system property
'-Djboss.bind.address' in the commandline.

Signed-off-by: Reinhard Naegele <unguiculus@gmail.com>

unguiculus added a commit to unguiculus/helm-charts that referenced this pull request Jul 22, 2019

[keycloak] Fix bind address handling (codecentric#72)
Drops setting the system property in favor of setting the
BIND environment variable which was added in
jboss-dockerfiles/keycloak#151.

This does away with a duplicate entry of the system property
'-Djboss.bind.address' in the commandline.

Signed-off-by: Reinhard Naegele <unguiculus@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.