ivangfr / keycloak-clustered

Keycloak-Clustered extends quay.io/keycloak/keycloak official Keycloak Docker image by adding JDBC_PING discovery protocol.
162 stars 57 forks source link

JDBC_PING: Add Configuration for SQL-Server #8

Closed chrisbra closed 2 years ago

chrisbra commented 2 years ago

I just happened to configure keycloak in ha mode for SQL Server.

I must admit, I haven't used JDBC_PING.cli, but directly configured it using standalone-ha.xml. But since this is just for the properties 'initialize_sql', 'insert_sql', 'delete_sql' and 'select_all_pingdata' I copied my statements from the configuration file back to the JDBC_PING.cli.

Note: I have added the bind_addr as property into the table, because I think this is actually quite useful.

Also Note: I have dropped the part about creating the schema¹. Because from my understanding, if the Schema isn't there, Keycloak wouldn't start at all at least if the JGROUPSPING table is located in the same schema as all the keycloak tables.

The provided SQL Statements are working for me with SQL Server 2017 tested with 2 keycloaks 13.0.1.

Apparently, it looks like the JDBC_PING.cli is the same for all different keycloak servers, so I just changed one and copied it over all other copies :)

¹) For reference the following SQL could be used to create the schema as well:

IF NOT EXISTS
(SELECT 1 FROM INFORMATION_SCHEMA.SCHEMATA WHERE SCHEMA_NAME=${env.DB_SCHEMA:public})
EXEC ('CREATE SCHEMA ${env.DB_SCHEMA:public}');

But, when adding this before all the other initialize_sqls, keycloak didn't create anything at all. Most likely, SQL Server didn't like this statement and the other ones after each other and since I didn't notice any error message, I didn't investigate further.

ivangfr commented 2 years ago

Thanks @chrisbra for the PR! I will have a look whenever I have some free time. Best regards!

ivangfr commented 2 years ago

@chrisbra btw, do you know how to run SQL Server as a Docker container locally and initialize it?

So far, I have

docker network create keycloak-net

docker run --rm --name mssql -p 1433:1433 \
-e ACCEPT_EULA=Y \
-e SA_PASSWORD=my_Password \
--network keycloak-net \
mcr.microsoft.com/mssql/server:2019-CU14-ubuntu-20.04

Then, in order to initialize the database and user, I am trying

docker exec -i mssql /opt/mssql-tools/bin/sqlcmd -S localhost -U SA -P my_Password \
-Q 'CREATE DATABASE keycloak; CREATE LOGIN keycloak WITH PASSWORD = "my_Password"; CREATE USER keycloak FOR LOGIN keycloak; GRANT SELECT, INSERT, UPDATE, DELETE ON keycloak TO keycloak'

However, I am getting

Msg 15151, Level 16, State 1, Server a56e7b9e6a16, Line 1
Cannot find the object 'keycloak', because it does not exist or you do not have permission.

Thanks!

chrisbra commented 2 years ago

yes, you need to initialize the database using mcr.microsoft.com/mssql-tools. I am using this docker-compose script:

version: '3'

networks:
  cb_bridge:
    driver: bridge

services:
  mssql_server_2017:
    container_name: mssql_server
    image: mcr.microsoft.com/mssql/server:2017-latest
    hostname: mssql
    domainname: ibi.local
    networks:
      - cb_bridge
    ports:
      - 1433:1433
    environment:
      ACCEPT_EULA: Y
      SA_PASSWORD: yourStrong(!)Password

  mssqlscripts:
      image: mcr.microsoft.com/mssql-tools
      domainname: ibi.local
      networks:
        - cb_bridge
      depends_on:
          - mssql_server_2017
      command: /bin/bash -c 'until /opt/mssql-tools/bin/sqlcmd -S mssql -U sa -P "yourStrong(!)Password" -Q "create database Keycloak"; do sleep 5; done'

  keycloak:
    container_name: keycloak-node1
    image: chrisbra/keycloak:mssql-ha_v1
    hostname: keycloak-node1
    domainname: ibi.local
    networks:
      - cb_bridge
    ports:
      - 8080:8080
    depends_on: [ "mssql_server_2017", "mssqlscripts" ]
      #command: [ "sleep", "infinity" ]
    command: [ "/opt/keycloak/bin/start_keycloak.sh" ]

  keycloak2:
    container_name: keycloak-node2
    image: chrisbra/keycloak:mssql-ha_v1
    hostname: keycloak-node2
    domainname: ibi.local
    networks:
      - cb_bridge
    ports:
      - 8081:8080
    depends_on: [ "mssql_server_2017", "mssqlscripts" ]
      # command: [ "sleep", "infinity" ]
    command: [ "/opt/keycloak/bin/start_keycloak.sh" ]
ivangfr commented 2 years ago

Thanks @chrisbra ! Now, I am able to start MSSQL and I am testing your PR.

Here are my experiments:

I am not familiar with MSSQL. I know that in MySQL/MariaDB: SCHEMA and DATABASE are the same; Meanwhile, in Postgres, they have different definitions and usages.

We need to make this DB_SCHEMA to work in JDBC_PING.cli when using MSSQL; keep without it.

chrisbra commented 2 years ago

I believe public is not a valid schema name. I have been using this in the configuration:

vi /opt/keycloak/keycloak.conf
# The configuration you want to run
export WILDFLY_CONFIG=standalone-ha.xml

# The mode you want to run
export WILDFLY_MODE=standalone

# The address to bind to, change it to actual IP address (see, e.g. `ip a s` output)
export WILDFLY_BIND=$(hostname -i)

# IP address of this host, please make sure this IP can be accessed by the other Keycloak instances
export JGROUPS_DISCOVERY_EXTERNAL_IP=$WILDFLY_BIND

# protocol
export JGROUPS_DISCOVERY_PROTOCOL=JDBC_PING

# additional java properties
export JAVA_OPTS="-Djava.net.preferIPv4Stack=true -Djgroups.tcp.address=$WILDFLY_BIND"

# IP and Port of all host is stored in DB
export JGROUPS_DISCOVERY_PROPERTIES=datasource_jndi_name=java:jboss/datasources/KeycloakDS
export DB_SCHEMA=keycloak.dbo
chrisbra commented 2 years ago

you just need to make sure to properly configure DB_SCHEMA and dbo is the usual public schema for MS-SQL

ivangfr commented 2 years ago

Ok, got it. I think, if the user inform a new schema, we should create it, as it's done when handling postgres.

If I use a schema that already exists in mssql, for instance dbo, it will work.

chrisbra commented 2 years ago

Yes, I tried creating the schema initially (and mentioned it in the original statement) using:

IF NOT EXISTS
(SELECT 1 FROM INFORMATION_SCHEMA.SCHEMATA WHERE SCHEMA_NAME=${env.DB_SCHEMA:public})
EXEC ('CREATE SCHEMA ${env.DB_SCHEMA:public}');

But this failed, at least when I included this with the other statements and I did not see an error message. I guess MS-SQL Server did not like this for some reason, but I did not investigate it.

ivangfr commented 2 years ago

I believe I've found a way.

So, the commands to create the schema is

IF NOT EXISTS (SELECT 1 FROM sys.schemas WHERE name = '${env.DB_SCHEMA:dbo}') BEGIN EXEC ('CREATE SCHEMA [${env.DB_SCHEMA:dbo}] AUTHORIZATION [dbo]') END

Also, we need to set the default DB_SCHEMA as dbo

In short, the commands for MSSQL in JDBC_PING.cli would be

...
if (outcome == success && result == sqlserver) of /subsystem=datasources/data-source=KeycloakDS:read-attribute(name=driver-name)
    /subsystem=jgroups/stack=tcp/protocol=JDBC_PING/property=initialize_sql:add(value="IF NOT EXISTS (SELECT 1 FROM sys.schemas WHERE name = '${env.DB_SCHEMA:dbo}') BEGIN EXEC ('CREATE SCHEMA [${env.DB_SCHEMA:dbo}] AUTHORIZATION [dbo]') END; CREATE TABLE ${env.DB_SCHEMA:dbo}.JGROUPSPING (own_addr varchar(200) NOT NULL, cluster_name varchar(200) NOT NULL, bind_addr varchar(200) NOT NULL, updated datetime2 default getdate(), ping_data varbinary(5000), constraint PK_JGROUPSPING PRIMARY KEY (own_addr, cluster_name))")
    /subsystem=jgroups/stack=tcp/protocol=JDBC_PING/property=insert_single_sql:add(value="INSERT INTO ${env.DB_SCHEMA:dbo}.JGROUPSPING (own_addr, cluster_name, bind_addr, updated, ping_data) values (?, ?, '${jgroups.tcp.address:127.0.0.1}', GETDATE(), ?)")
    /subsystem=jgroups/stack=tcp/protocol=JDBC_PING/property=delete_single_sql:add(value="DELETE FROM ${env.DB_SCHEMA:dbo}.JGROUPSPING WHERE own_addr=? AND cluster_name=?")
    /subsystem=jgroups/stack=tcp/protocol=JDBC_PING/property=select_all_pingdata_sql:add(value="SELECT ping_data, own_addr, cluster_name FROM ${env.DB_SCHEMA:dbo}.JGROUPSPING WHERE cluster_name=?")
end-if
...

This way, if we inform "myschema" to DB_SCHEMA, in order to see the records in JGROUPSPING, we need to run select * from keycloak.myschema.JGROUPSPING

Example in a terminal

~ docker exec -it mssql /opt/mssql-tools/bin/sqlcmd -S localhost -U SA -P my_Password
1> select own_addr from keycloak.myschema.JGROUPSPING
2> go
own_addr
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
4d6d2195-d56a-07d2-256a-e51b30ed10f6
b5ebea5c-8ccf-b92e-c2d6-c002c840de75

(2 rows affected)
chrisbra commented 2 years ago

okay, I'll update the PR with your changes then

chrisbra commented 2 years ago

I have updated it with your changes. Let me know if I should squash it for your convenience.

However, you dropped the Check whether the table exists, so keycloak will always try to create that table. I am not sure this is correct and could causes some error messages.

Unfortunately, I cannot test it currently, to busy with other work :(

Can you please verify that this doesn't cause regressions?

ivangfr commented 2 years ago

You are right, I've removed the check whether the table exists while trying to make the schema to work.

Btw, looks like it's not needed. I've restarted several times the Keycloak instances and there is no ERROR log.

I've also tried to remove the IF NOT EXISTS in front of the Postgres CREATE TABLE command and, the same, no ERROR logs and the cluster is created successfully.

Later, I will merge your PR! Thanks for the contribution!

chrisbra commented 2 years ago

cool. Just as a final check, does it also work if you do it with an empty keycloak schema where just the keycloak.myschema.JGROUPSPING table already exists? So keycloak is trying to initialize everything.

ivangfr commented 2 years ago

Good point!

Btw, here is how you checked the table exists before creating it

IF NOT EXISTS(SELECT 1 FROM INFORMATION_SCHEMA.TABLES  WHERE TABLE_TYPE='BASE TABLE' AND TABLE_NAME='JGROUPSPING') CREATE TABLE ...

Do you believe that would be enough now considering the schema?

chrisbra commented 2 years ago

I would think so yes.