feat(core): DSPX-2126 simplify docker compose UX
#2958
Conversation
Summary of ChangesHello @b-long, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the developer experience for setting up the OpenTDF platform locally by consolidating numerous manual configuration and provisioning steps into a single Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Docker builds with ease, Keys and configs now flow free, Setup, swift and clean. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves the developer experience by automating the platform setup process within docker compose. The changes are well-structured, introducing several new services for key generation, configuration downloading, and provisioning. My review focuses on enhancing the security, robustness, and maintainability of this new automated setup. The main suggestions involve addressing insecure file permissions, removing hardcoded secrets, improving the efficiency of initialization services, and making the documentation more reliable.
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request significantly improves the user experience for running the platform locally by introducing a self-contained docker-compose.consumer.yaml that automates the entire setup process. The refactoring of the developer's docker-compose.yaml to use a new docker-compose.base.yaml is also a good step towards reducing configuration duplication. However, I've identified several areas for improvement, primarily concerning security and maintainability. There are numerous instances of hardcoded credentials across the new compose files, which should be externalized. The new docker-compose.consumer.yaml duplicates a lot of configuration, creating a maintenance challenge. Additionally, the developer docker-compose.yaml appears to be broken by the refactoring, and some of the setup scripts in the consumer file are fragile. My detailed comments provide specific suggestions to address these issues.
| openssl x509 -req -in /tmp/sampleuser.req -CA /keys/keycloak-ca.pem -CAkey /keys/keycloak-ca-private.pem -CAcreateserial -out /keys/sampleuser.crt -days 3650 | ||
| # Convert to PKCS12 | ||
| openssl pkcs12 -export -in /keys/keycloak-ca.pem -inkey /keys/keycloak-ca-private.pem -out /keys/ca.p12 -nodes -passout pass:password |
There was a problem hiding this comment.
A password is hardcoded here for openssl pkcs12. This is a security risk. Please use an environment variable. You'll need to add KEY_PASSWORD to the environment section for this service.
openssl pkcs12 -export -in /keys/keycloak-ca.pem -inkey /keys/keycloak-ca-private.pem -out /keys/ca.p12 -nodes -passout env:KEY_PASSWORD| environment: | ||
| POSTGRES_DB: ers_test | ||
| POSTGRES_USER: ers_test_user | ||
| POSTGRES_PASSWORD: ers_test_pass |
There was a problem hiding this comment.
Hardcoded credentials should be avoided. Please use environment variables loaded from a .env file to manage secrets like database passwords.
POSTGRES_PASSWORD: ${ERS_TEST_POSTGRES_PASSWORD:-ers_test_pass}| LDAP_ADMIN_PASSWORD: "admin_password" | ||
| LDAP_CONFIG_PASSWORD: "config_password" | ||
| LDAP_READONLY_USER: "true" | ||
| LDAP_READONLY_USER_USERNAME: "readonly" | ||
| LDAP_READONLY_USER_PASSWORD: "readonly_password" |
There was a problem hiding this comment.
Hardcoded credentials for LDAP should be avoided. Please use environment variables loaded from a .env file to manage these secrets.
LDAP_ADMIN_PASSWORD: ${LDAP_ADMIN_PASSWORD:-admin_password}
LDAP_CONFIG_PASSWORD: ${LDAP_CONFIG_PASSWORD:-config_password}
LDAP_READONLY_USER: "true"
LDAP_READONLY_USER_USERNAME: "readonly"
LDAP_READONLY_USER_PASSWORD: ${LDAP_READONLY_USER_PASSWORD:-readonly_password}| - ers_ldap_config:/etc/ldap/slapd.d | ||
| - ./service/entityresolution/integration/ldap_test_data:/container/service/slapd/assets/config/bootstrap/ldif/custom | ||
| healthcheck: | ||
| test: ["CMD-SHELL", "ldapsearch -x -H ldap://localhost:389 -b dc=opentdf,dc=test -D cn=admin,dc=opentdf,dc=test -w admin_password '(objectclass=*)' dn"] |
There was a problem hiding this comment.
The healthcheck command contains a hardcoded password. This should also be replaced with an environment variable to avoid exposing secrets. Note the $$ is required to properly escape the variable for the shell.
test: ["CMD-SHELL", "ldapsearch -x -H ldap://localhost:389 -b dc=opentdf,dc=test -D cn=admin,dc=opentdf,dc=test -w $${LDAP_ADMIN_PASSWORD:-admin_password} '(objectclass=*)' dn"]| KC_DB_URL_PORT: 5432 | ||
| KC_DB_URL_DATABASE: keycloak | ||
| KC_DB_USERNAME: keycloak | ||
| KC_DB_PASSWORD: changeme |
There was a problem hiding this comment.
Hardcoded database password for Keycloak. Please use an environment variable.
KC_DB_PASSWORD: ${KC_DB_PASSWORD:-changeme}| - -c | ||
| - | | ||
| apk add --no-cache sed | ||
| sed -i 's|http://keycloak:8888|https://keycloak.opentdf.local:9443|g' /configs/opentdf.yaml |
There was a problem hiding this comment.
Using sed to patch a YAML file is fragile. If the structure or spacing of opentdf-example.yaml changes, this sed command could fail or have unintended side effects. Consider using a tool that understands YAML structure, like yq, for a more robust solution.
| command: | ||
| - -c | ||
| - | | ||
| apk add --no-cache openssl openjdk11-jre bash | ||
| cd /keys | ||
| # Generate KAS RSA private key | ||
| openssl genpkey -algorithm RSA -out /keys/kas-private.pem -pkeyopt rsa_keygen_bits:2048 | ||
| openssl rsa -in /keys/kas-private.pem -pubout -out /keys/kas-cert.pem | ||
| # Generate ECC Key | ||
| openssl ecparam -name prime256v1 > /tmp/ecparams.tmp | ||
| openssl req -x509 -nodes -newkey ec:/tmp/ecparams.tmp -subj "/CN=kas" -keyout /keys/kas-ec-private.pem -out /keys/kas-ec-cert.pem -days 365 | ||
| # Generate CA | ||
| openssl req -x509 -nodes -newkey RSA:2048 -subj "/CN=ca" -keyout /keys/keycloak-ca-private.pem -out /keys/keycloak-ca.pem -days 365 | ||
| # Generate localhost certificate | ||
| printf "subjectAltName=DNS:localhost,IP:127.0.0.1" > /tmp/sanX509.conf | ||
| printf "[req]\ndistinguished_name=req_distinguished_name\n[req_distinguished_name]\n[alt_names]\nDNS.1=localhost\nIP.1=127.0.0.1" > /tmp/req.conf | ||
| openssl req -new -nodes -newkey rsa:2048 -keyout /keys/localhost.key -out /tmp/localhost.req -batch -subj "/CN=localhost" -config /tmp/req.conf | ||
| openssl x509 -req -in /tmp/localhost.req -CA /keys/keycloak-ca.pem -CAkey /keys/keycloak-ca-private.pem -CAcreateserial -out /keys/localhost.crt -days 3650 -sha256 -extfile /tmp/sanX509.conf | ||
| # Generate sample user certificate | ||
| openssl req -new -nodes -newkey rsa:2048 -keyout /keys/sampleuser.key -out /tmp/sampleuser.req -batch -subj "/CN=sampleuser" | ||
| openssl x509 -req -in /tmp/sampleuser.req -CA /keys/keycloak-ca.pem -CAkey /keys/keycloak-ca-private.pem -CAcreateserial -out /keys/sampleuser.crt -days 3650 | ||
| # Convert to PKCS12 | ||
| openssl pkcs12 -export -in /keys/keycloak-ca.pem -inkey /keys/keycloak-ca-private.pem -out /keys/ca.p12 -nodes -passout pass:password | ||
| # Convert PKCS12 to JKS using keytool (no Docker needed) | ||
| keytool -importkeystore \ | ||
| -srckeystore /keys/ca.p12 \ | ||
| -srcstoretype PKCS12 \ | ||
| -destkeystore /keys/ca.jks \ | ||
| -deststoretype JKS \ | ||
| -srcstorepass "password" \ | ||
| -deststorepass "password" \ | ||
| -noprompt | ||
| echo "Keys generated successfully" |
There was a problem hiding this comment.
This large inline shell script for key generation is difficult to read, maintain, and test. It would be better to move this script to a separate .sh file that is downloaded by download-init-script and then executed by this service. This would significantly improve maintainability.
|
|
||
| # Download platform configuration file | ||
| download-platform-config: | ||
| image: curlimages/curl:latest |
There was a problem hiding this comment.
Using the latest tag for Docker images is not recommended for distributable configurations as it can lead to unexpected behavior when the image is updated. It's better to pin to a specific version (e.g., curlimages/curl:8.8.0) to ensure reproducibility. This applies to all ...:latest images used for setup services in this file.
| chmod 777 /configs /keys | ||
| mkdir -p /configs/service/internal/fixtures | ||
| chmod -R 777 /configs |
There was a problem hiding this comment.
Using chmod 777 is overly permissive and a potential security risk, as it gives read, write, and execute permissions to everyone. Please use more restrictive permissions. The fix-keys-permissions service later tightens these, but it's best practice to start with least privilege.
| test: | ||
| - CMD-SHELL | ||
| - | | ||
| [ -f /tmp/HealthCheck.java ] || echo "public class HealthCheck { | ||
| public static void main(String[] args) throws java.lang.Throwable { | ||
| javax.net.ssl.HttpsURLConnection.setDefaultHostnameVerifier((hostname, session) -> true); | ||
| javax.net.ssl.SSLContext sc = javax.net.ssl.SSLContext.getInstance(\"SSL\"); | ||
| sc.init(null, new javax.net.ssl.TrustManager[]{ | ||
| new javax.net.ssl.X509TrustManager() { | ||
| public java.security.cert.X509Certificate[] getAcceptedIssuers() { return null; } | ||
| public void checkClientTrusted(java.security.cert.X509Certificate[] certs, String authType) {} | ||
| public void checkServerTrusted(java.security.cert.X509Certificate[] certs, String authType) {} | ||
| } | ||
| }, new java.security.SecureRandom()); | ||
| javax.net.ssl.HttpsURLConnection.setDefaultSSLSocketFactory(sc.getSocketFactory()); | ||
| java.net.HttpURLConnection conn = (java.net.HttpURLConnection)new java.net.URL(args[0]).openConnection(); | ||
| System.exit(java.net.HttpURLConnection.HTTP_OK == conn.getResponseCode() ? 0 : 1); | ||
| } | ||
| }" > /tmp/HealthCheck.java && java ${JAVA_OPTS_APPEND} /tmp/HealthCheck.java https://localhost:9001/auth/health/live | ||
| timeout: 10s | ||
| retries: 3 | ||
| start_period: 2m |
There was a problem hiding this comment.
This inline Java class for the healthcheck is complex and hard to maintain within a YAML file. Consider moving this logic to a separate script file (e.g., healthcheck.sh) and mounting it into the container. This would improve readability and make it easier to test and update the healthcheck logic.
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
📘 Proposed Changes
Read the (new / rendered) getting started section here:
♻️ Rebuild "Getting Started"
If this PR (or something similar) is approved, we can use it as the basis to rebuild the getting started page:
Getting started will provide a single docker compose file (just like today), by rendering the file:
At first, I made the Github Actions test fail, because I modified the
docker-compose.yamlin this repo. On ourmainbranch, that file has two different goals:Later on, I split the
docker-compose.yamlfile into three parts:docker-compose.yaml, for the development use case.docker-compose.consumer.yamlfile for the consumer use case.docker-compose.base.yamlfile that is used by both developers and consumers.You'll notice the difference between
docker-compose.yamlanddocker-compose.consumer.yamlis fairly small.🤖 Testing Instructions
Follow the rendered instructions (linked above). Once the platform is running, you should be able to interact with it normally.
For instance:
You can also access Keycloak, via http://localhost:8888/auth/ .