feat(jans-pycloudlib): add support for connecting to cloudsql via cloud auth proxy #12788
Conversation
Signed-off-by: moabu <[email protected]>
📝 WalkthroughWalkthroughAdds Cloud SQL Python Connector docs and runtime support in pycloudlib persistence (CloudSqlConnectorMixin, connector-gated engine creation, SqlClient context/cleanup), updates dependencies, replaces per-field JDBC templates with a single connection URI, and adds Cloud SQL socket-factory build stages and JANS_SOURCE_VERSION updates across multiple Dockerfiles. (50 words) Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
moabu
changed the title
mo-auto
added
area-documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
jans-pycloudlib/docs/api/persistence/cloudsql_connector.md(1 hunks)jans-pycloudlib/jans/pycloudlib/persistence/sql.py(9 hunks)jans-pycloudlib/pyproject.toml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
jans-pycloudlib/jans/pycloudlib/persistence/sql.py (1)
jans-pycloudlib/jans/pycloudlib/utils.py (1)
as_boolean(47-66)
🪛 markdownlint-cli2 (0.18.1)
jans-pycloudlib/docs/api/persistence/cloudsql_connector.md
203-203: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
210-210: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🪛 Ruff (0.14.6)
jans-pycloudlib/jans/pycloudlib/persistence/sql.py
179-182: Avoid specifying long messages outside the exception class
(TRY003)
194-194: Dynamically typed expressions (typing.Any) are disallowed in getconn
(ANN401)
340-343: Avoid specifying long messages outside the exception class
(TRY003)
355-355: Dynamically typed expressions (typing.Any) are disallowed in getconn
(ANN401)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: sonar scan (jans-casa)
- GitHub Check: sonar scan (jans-keycloak-link)
- GitHub Check: sonar scan (jans-fido2)
- GitHub Check: sonar scan (agama)
- GitHub Check: sonar scan (jans-scim)
- GitHub Check: sonar scan (jans-core)
- GitHub Check: sonar scan (jans-config-api)
- GitHub Check: sonar scan (jans-orm)
- GitHub Check: Analyze (python)
🔇 Additional comments (11)
jans-pycloudlib/docs/api/persistence/cloudsql_connector.md (3)
1-31: LGTM! Documentation overview and installation section look solid, my friend!Yo, this is some quality documentation right here! The overview clearly explains the purpose, and the supported databases table is nice and clean. The installation instructions are straightforward.
One tiny thing - Line 30 mentions pymysql is already a base dependency, which aligns with the
pyproject.tomlwherepymysql>=1.0.2is listed. Good consistency!
32-112: Configuration examples are on point!Like a wise ol' pigeon says - these examples are clear as day! The environment variable table is comprehensive, and both PostgreSQL and MySQL code examples demonstrate the proper usage pattern matching the implementation in
sql.py.
217-251: Backward compatibility and troubleshooting sections are ready to fly!This is some solid documentation, my friend! The migration steps are clear, and the troubleshooting section covers the most likely issues users will encounter. Let's get this party rollin'!
jans-pycloudlib/jans/pycloudlib/persistence/sql.py (7)
38-45: Runtime gating pattern is solid!Like a wise ol' pigeon would say - this is the right way to handle optional dependencies! The try/except pattern with a flag is clean and allows graceful degradation when the connector isn't installed.
76-128: PostgresqlAdapter initialization and enabled check look great!Hey friend, this implementation is ready to party! The lazy initialization pattern for
_cloudsql_connectoris smart, and thecloudsql_connector_enabledproperty handles the edge case where the env var is set but the library isn't installed - that warning message will save folks some debugging time!
130-149: connect_args property handles both paths correctly!This is some smart logic right here! When Cloud SQL Connector is enabled, returning an empty dict is correct since the connector handles all connection parameters internally. The existing SSL config path is preserved for backward compatibility.
234-286: MysqlAdapter has same structure - looking good!The initialization and enabled check mirror the PostgreSQL implementation correctly. The driver-specific warning message (pymysql vs psycopg2) is a nice touch!
518-560: Engine property updates are well-structured!Let's get this party rollin' - this implementation is clean! The branching logic correctly handles both connector and non-connector paths. Good choices with
pool_pre_ping=Truefor connection health checks andhide_parameters=Truefor security. The MySQL-specific event listeners are still properly attached regardless of connection method.
562-568: Safe delegation pattern!Using
getattrwith a default ofFalseis a smart defensive pattern - it'll gracefully handle any future adapter that doesn't implementcloudsql_connector_enabled. Nice!
570-576: Clear docstring update!Good documentation noting that
engine_urlis not used when Cloud SQL Connector is enabled. This helps developers understand the flow!jans-pycloudlib/pyproject.toml (1)
77-81: Optional dependency group looks good — versions verified as current.The optional dependency setup is clean and follows best practices. Using
pip install 'jans-pycloudlib[cloudsql]'for opt-in installation is the right approach. Both version constraints are valid and permissive: cloud-sql-python-connector latest is 1.18.5 (as of October 2025) and pg8000 latest is 1.31.5 (as of September 2025), both well within the>=1.0.0and>=1.30.0ranges specified.
Signed-off-by: moabu <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
jans-pycloudlib/jans/pycloudlib/persistence/sql.py (1)
339-407: Consider reducing duplication between adapters, my friend!Like a wise ol' pigeon would say - these methods are nearly identical between
PostgresqlAdapterandMysqlAdapter. The only differences are the driver name ("pymysql"vs"pg8000") and some docstring text.Consider extracting the common logic into a mixin or base class:
class CloudSqlConnectorMixin: """Mixin providing Cloud SQL Connector functionality.""" _cloudsql_connector: _t.Any = None _cloudsql_driver: str # Set in subclass: "pg8000" or "pymysql" @property def cloudsql_connector_enabled(self) -> bool: # Common implementation... def get_cloudsql_connection_creator(self, manager: _t.Optional[Manager] = None) -> _t.Callable[[], _t.Any]: # Common implementation using self._cloudsql_driver... def close(self) -> None: # Common implementation...This would reduce ~150 lines of duplicated code.
Note: The same password caching consideration from the PostgreSQL adapter applies here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
jans-pycloudlib/jans/pycloudlib/persistence/sql.py(12 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
jans-pycloudlib/jans/pycloudlib/persistence/sql.py (2)
jans-pycloudlib/jans/pycloudlib/utils.py (1)
as_boolean(47-66)jans-pycloudlib/jans/pycloudlib/manager.py (2)
get(32-33)get(81-96)
🪛 Ruff (0.14.6)
jans-pycloudlib/jans/pycloudlib/persistence/sql.py
182-185: Avoid specifying long messages outside the exception class
(TRY003)
192-196: Avoid specifying long messages outside the exception class
(TRY003)
205-205: Dynamically typed expressions (typing.Any) are disallowed in getconn
(ANN401)
223-223: Dynamically typed expressions (typing.Any) are disallowed in upsert_query
(ANN401)
240-240: Do not catch blind exception: Exception
(BLE001)
241-241: Logging statement uses f-string
(G004)
368-371: Avoid specifying long messages outside the exception class
(TRY003)
378-382: Avoid specifying long messages outside the exception class
(TRY003)
391-391: Dynamically typed expressions (typing.Any) are disallowed in getconn
(ANN401)
409-409: Dynamically typed expressions (typing.Any) are disallowed in upsert_query
(ANN401)
423-423: Do not catch blind exception: Exception
(BLE001)
424-424: Logging statement uses f-string
(G004)
525-525: Remove quotes from type annotation
Remove quotes
(UP037)
537-537: Do not catch blind exception: Exception
(BLE001)
538-538: Logging statement uses f-string
(G004)
671-671: Do not catch blind exception: Exception
(BLE001)
672-672: Logging statement uses f-string
(G004)
681-681: Remove quotes from type annotation
Remove quotes
(UP037)
693-693: Dynamically typed expressions (typing.Any) are disallowed in exc_tb
(ANN401)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: sonar scan (jans-scim)
- GitHub Check: sonar scan (jans-casa)
- GitHub Check: sonar scan (jans-pycloudlib)
- GitHub Check: sonar scan (jans-orm)
- GitHub Check: sonar scan (jans-cli-tui)
- GitHub Check: sonar scan (jans-auth-server)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (6)
jans-pycloudlib/jans/pycloudlib/persistence/sql.py (6)
40-47: Nice work on the guarded import pattern, my friend!Like a wise ol' pigeon droppin' knowledge, I gotta say this is a solid approach for handling optional dependencies. Setting
ConnectorandIPTypestoNoneon import failure allows the rest of the code to check availability cleanly without crashing at import time.
78-130: Let's get this party rollin' - solid connector enablement logic!The
cloudsql_connector_enabledproperty handles the edge case where the user setsCN_SQL_CLOUDSQL_CONNECTOR_ENABLEDbut hasn't installed the optional dependency. The warning message with installation instructions is helpful for debugging.
229-243: LGTM - proper resource cleanup!The
close()method handles exceptions gracefully during cleanup (important for teardown scenarios) and usesfinallyto ensure_cloudsql_connectoris always reset. The broad exception catch is appropriate here to prevent cleanup errors from masking original exceptions.
562-579: LGTM on the SqlClient initialization updates!The
_closedflag for idempotent cleanup and registration in_sql_client_instancesfor atexit cleanup are well-integrated with the existing initialization flow.
601-643: Let's get this party rollin' - well-structured engine initialization!The engine property cleanly handles both Cloud SQL Connector and standard connection paths. Key observations:
pool_pre_ping=Trueensures stale connections are detected - crucial for cloud environmentshide_parameters=Trueprevents credential leakage in logs- MySQL event listeners are correctly attached regardless of connection method
704-718: Documentation update is helpful!The docstring clarification that
engine_urlis not used when Cloud SQL Connector is enabled helps future maintainers understand the code flow.
Signed-off-by: moabu <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
jans-pycloudlib/jans/pycloudlib/persistence/sql.py (1)
166-185: Hey friend, password caching in the closure - still worth considering!Listen up! The database password is captured once at line 168 when
get_cloudsql_connection_creatoris called and stored in the closure. For long-running services with credential rotation policies, new connections would use the stale password until the engine is recreated.If credential rotation is a requirement, consider reading the password inside
getconn():def getconn() -> _t.Any: """Create a connection to Cloud SQL using Private IP. Returns: A database connection object. """ + # Read password on each connection to support credential rotation + current_password = get_sql_password(manager) conn = self._cloudsql_connector.connect( instance_connection_name, driver, user=db_user, - password=db_password, + password=current_password, db=db_name, ip_type=IPTypes.PRIVATE, ) return conn
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
jans-pycloudlib/docs/api/persistence/cloudsql_connector.md(1 hunks)jans-pycloudlib/jans/pycloudlib/persistence/sql.py(12 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
jans-pycloudlib/jans/pycloudlib/persistence/sql.py (1)
jans-pycloudlib/jans/pycloudlib/utils.py (1)
as_boolean(47-66)
🪛 markdownlint-cli2 (0.18.1)
jans-pycloudlib/docs/api/persistence/cloudsql_connector.md
133-133: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
141-141: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
242-242: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
249-249: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🪛 Ruff (0.14.6)
jans-pycloudlib/jans/pycloudlib/persistence/sql.py
94-96: Logging statement uses f-string
(G004)
115-119: Avoid specifying long messages outside the exception class
(TRY003)
132-135: Avoid specifying long messages outside the exception class
(TRY003)
171-171: Dynamically typed expressions (typing.Any) are disallowed in getconn
(ANN401)
200-200: Do not catch blind exception: Exception
(BLE001)
201-201: Logging statement uses f-string
(G004)
466-466: Remove quotes from type annotation
Remove quotes
(UP037)
478-478: Do not catch blind exception: Exception
(BLE001)
479-479: Logging statement uses f-string
(G004)
612-612: Do not catch blind exception: Exception
(BLE001)
613-613: Logging statement uses f-string
(G004)
622-622: Remove quotes from type annotation
Remove quotes
(UP037)
634-634: Dynamically typed expressions (typing.Any) are disallowed in exc_tb
(ANN401)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: sonar scan (jans-lock)
- GitHub Check: sonar scan (jans-link)
- GitHub Check: sonar scan (jans-orm)
- GitHub Check: sonar scan (jans-pycloudlib)
- GitHub Check: sonar scan (jans-fido2)
- GitHub Check: sonar scan (jans-config-api)
- GitHub Check: sonar scan (jans-casa)
- GitHub Check: sonar scan (agama)
- GitHub Check: sonar scan (jans-keycloak-integration)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (14)
jans-pycloudlib/jans/pycloudlib/persistence/sql.py (10)
40-47: Solid guarded import pattern, my friend!Yo, like a wise ol' pigeon would say, this is the way to handle optional dependencies! The try/except with a fallback flag ensures graceful degradation when the Cloud SQL Connector isn't installed. Setting
Connector = NoneandIPTypes = Noneas fallbacks allows type hints to work while preventing import errors.
63-99: Nice mixin pattern - let's get this party rollin'!The
CloudSqlConnectorMixinelegantly centralizes Cloud SQL Connector logic and eliminates the duplication previously flagged between adapters. Thecloudsql_connector_enabledproperty correctly checks both the environment variable AND library availability, with a helpful fallback warning.
101-138: You better think twice? Nah, this validation is solid!The instance connection name validation (line 112) properly strips whitespace and provides a clear, actionable error message with the expected format. The lazy connector initialization in
_ensure_connectoris also well-implemented. This addresses the concerns from previous review cycles.
189-203: Proper cleanup - like a wise ol' pigeon knows, always clean your nest!The
closemethod correctly handles connector cleanup with appropriate exception handling. Thefinallyblock ensures_cloudsql_connectoris set toNoneregardless of success or failure, making subsequent calls idempotent.
206-279: PostgresqlAdapter lookin' good with that mixin inheritance!The adapter properly inherits from
CloudSqlConnectorMixinand sets the driver attributes. Theconnect_argsproperty correctly returns an empty dict when Cloud SQL Connector is enabled (since the connector handles connection security), otherwise falls back to the existing SSL configuration logic.
288-364: MysqlAdapter follows the same pattern - consistency is key, friend!Like its PostgreSQL sibling,
MysqlAdapterproperly inherits from the mixin and configures MySQL-specific drivers. Theconnect_argsproperty correctly short-circuits when Cloud SQL Connector is enabled.
466-482: Module-level cleanup with WeakSet - now that's some wise ol' pigeon thinking!The
WeakSetallows proper garbage collection of unreferencedSqlClientinstances while still enabling cleanup of live instances on shutdown. Thelist(_sql_client_instances)snapshot at line 475 prevents issues during iteration. Theatexitregistration ensures proper cleanup.Minor style note: Since the file uses
from __future__ import annotations, the quoted"SqlClient"at line 466 is redundant and could be unquoted, but this is purely cosmetic.
503-520: SqlClient initialization with lifecycle tracking - let's get this party rollin'!The
_closedflag and registration in_sql_client_instancesset up proper lifecycle management. The registration happens at the end of__init__, ensuring the instance is fully initialized before being tracked.
542-592: Engine property with Cloud SQL Connector branching - solid implementation!The engine creation logic correctly branches based on
_use_cloudsql_connector()and handles both PostgreSQL (pg8000) and MySQL (pymysql) adapters with appropriate connection URLs. Thegetattrwith defaultFalsein_use_cloudsql_connectoris a safe defensive pattern. Both paths maintain consistent settings (pool_pre_ping=True,hide_parameters=True).
594-643: Context manager with proper cleanup order - you better think twice? Nah, this is correct!The cleanup sequence is exactly right: dispose the engine first (releases connection pool), then close the adapter (stops token-refresh threads). The idempotent
_closedflag prevents double-cleanup issues. Standard context manager implementation that ensures resources are released.jans-pycloudlib/docs/api/persistence/cloudsql_connector.md (4)
1-31: Comprehensive documentation - let's get this party started with good docs!Excellent documentation covering the Cloud SQL Python Connector integration. The overview clearly explains the purpose, the supported databases table is helpful, and the installation instructions are straightforward.
32-127: Configuration and examples are on point, friend!The environment variable table is clear and comprehensive. Both PostgreSQL and MySQL configuration examples are provided with matching code samples demonstrating context manager usage. This aligns well with the implemented functionality.
154-234: Mandatory checklist - like a wise ol' pigeon checkin' all the boxes!This checklist is invaluable for users setting up Cloud SQL with Cloud Run. The gcloud commands for each step make it actionable. Covering IAM roles, VPC connectors, Cloud SQL instance configuration, and firewall rules ensures users don't miss critical setup steps.
256-290: Backward compatibility and troubleshooting - wrapping it up nicely!The documentation clearly states the opt-in nature of the Cloud SQL Connector and provides a straightforward migration path. The troubleshooting section covers the most likely issues users will encounter with actionable solutions. Well done!
…on, supporting runtime password rotation Signed-off-by: moabu <[email protected]>
Signed-off-by: moabu <[email protected]>
There was a problem hiding this comment.
Please adjust the examples in cloudsql_connector.md (refer to #12788 (comment) for details).
Signed-off-by: moabu <[email protected]>
mo-auto
added
comp-docker-jans-all-in-one
|
confirmed working with cloud auth proxy with ssl set to verify identity ConfiguratorPersistence |
Signed-off-by: moabu <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
jans-pycloudlib/jans/pycloudlib/persistence/sql.py(13 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
jans-pycloudlib/jans/pycloudlib/persistence/sql.py (2)
jans-pycloudlib/jans/pycloudlib/utils.py (2)
as_boolean(47-66)get_password_from_file(611-668)jans-pycloudlib/jans/pycloudlib/manager.py (3)
get(32-33)get(81-96)Manager(353-435)
🪛 Ruff (0.14.7)
jans-pycloudlib/jans/pycloudlib/persistence/sql.py
94-96: Logging statement uses f-string
(G004)
115-119: Avoid specifying long messages outside the exception class
(TRY003)
132-135: Avoid specifying long messages outside the exception class
(TRY003)
172-175: Avoid specifying long messages outside the exception class
(TRY003)
182-182: Dynamically typed expressions (typing.Any) are disallowed in getconn
(ANN401)
214-214: Do not catch blind exception: Exception
(BLE001)
215-215: Logging statement uses f-string
(G004)
492-492: Do not catch blind exception: Exception
(BLE001)
493-493: Logging statement uses f-string
(G004)
624-625: try-except-pass detected, consider logging the exception
(S110)
624-624: Do not catch blind exception: Exception
(BLE001)
631-631: Do not catch blind exception: Exception
(BLE001)
632-632: Logging statement uses f-string
(G004)
653-653: Dynamically typed expressions (typing.Any) are disallowed in exc_tb
(ANN401)
1143-1147: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (10)
jans-pycloudlib/jans/pycloudlib/persistence/sql.py (10)
40-99: Connector import gating and feature flagging look robust.Listen up, my friend! The guarded import plus
CLOUDSQL_CONNECTOR_AVAILABLEandcloudsql_connector_enabledgive a clear, deterministic switch for enabling the connector and produce a helpful warning when the library is missing. Behavior is predictable and fails safe, so I’ve got no concerns here.
101-201: CloudSqlConnectorMixin centralizes connector usage nicely and supports password rotation.You better think twice before changing this, because it’s in good shape: instance connection name is validated early with a clear error, the connector is lazily initialized once,
cloudsql_driveris required so subclasses can’t silently misconfigure, andget_cloudsql_connection_creator()fetches the password on each call viaget_sql_password(manager), which plays well with rotation. Overall this is a clean, reusable abstraction for both adapters.
203-218: Connector close() behavior is appropriate for shutdown paths.Listen up, my friend! The adapter-level
close()correctly tears down the underlying connector and nulls the reference, with a warning-only handler on failures. Given this is shutdown/cleanup code and SqlClient guards usage with_closed, this is a reasonable trade‑off between robustness and not crashing on teardown.
235-239: Adapters cleanly separate Cloud SQL vs. traditional SSL connection paths.You better think twice before complicating this further: when the connector is enabled,
connect_argsreturns{}, letting the Cloud SQL path own transport concerns; when it’s disabled, the existing SSL env‑driven behavior is preserved for both PostgreSQL and MySQL. Thecloudsql_driver/fallback_driver_namefields also make logging and configuration intent clear. Looks solid.Also applies to: 275-293, 317-322, 357-378
480-497: WeakSet tracking and atexit cleanup are a sensible safety net.Listen up, my friend! Tracking
SqlClientinstances in aWeakSetand running_cleanup_sql_clients()atatexitis a nice pattern: it doesn’t keep clients alive artificially and still ensures forgotten instances get closed on interpreter shutdown. The snapshot vialist(_sql_client_instances)avoids iterator races, so this is safe even if some clients are being garbage‑collected.
517-535: SqlClient initialization and instance registration align well with the new lifecycle.You better think twice before tweaking this: adapter selection remains backwards‑compatible,
_closedis initialized up front, SSL assets are bootstrapped only when requested, and each instance is registered in_sql_client_instancesfor eventual cleanup. This ties neatly into the new atexit handler and the explicitclose()method.
556-607: Engine creation cleanly switches between Cloud SQL and direct DB access.Listen up, my friend! The
_use_cloudsql_connector()gate, informative log message, and separatecreate_enginecalls for connector vs. traditional URL+connect_args make the behavior explicit and easy to reason about. Pool pre‑ping andhide_parameters=Trueare preserved in both paths, and dialect‑specific hooks (set_mysql_strict_mode,preconfigure_simple_json) still run. This is a tidy integration point.
641-663: Context manager support and idempotent close() are well‑designed.Listen up, my friend!
__enter__/__exit__simply wrapclose(), and_closedensures cleanup is idempotent. Combined with engine disposal, adapterclose(), metadata reset, and WeakSet removal, this gives you a clean, predictable lifecycle both forwith SqlClient(...)and long‑lived instances.
666-670: Engine URL docstring correctly clarifies it’s unused in Cloud SQL mode.You better think twice before removing that note: documenting that
engine_urlis ignored when the connector is enabled helps future readers understand why direct host/port/password URL construction still exists. It avoids confusion when both flows coexist.
1122-1149: get_sql_password now fails fast with a clear message when Manager is missing.Listen up, my friend! The new signature and logic are a nice hardening step: you first prefer the password file (preserving existing behavior), and only when it’s absent do you require a
Manager, raising a descriptiveRuntimeErrorinstead of leaking an AttributeError. This keeps existing call sites that always passedmanagerworking while making misuse much easier to diagnose.
Signed-off-by: moabu <[email protected]>
|
@misba7 can you please run aio installation using image |
|
Caught an error on javas side Add Cloud SQL JDBC Socket Factory JAR needs to be added to the images |
…ry support for Java services Signed-off-by: moabu <[email protected]>
…va services Signed-off-by: moabu <[email protected]>
Signed-off-by: moabu <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docker-jans-casa/Dockerfile (1)
61-73: Add ownership and permission fix for custom/libs directory.The custom/libs directory is created at line 216 but lacks the ownership and permission adjustments applied to other directories. Without this, the non-root jetty user (UID 1000) created at line 239 will fail to load the Cloud SQL Socket Factory JARs at runtime.
Add the following to the permission adjustment block (around line 251):
&& chown -R 1000:0 ${JETTY_BASE}/jans-casa/custom/libs \ && chmod -R g=u ${JETTY_BASE}/jans-casa/custom/libsAdditionally, the wget commands at lines 217-218 use the
-qflag, which silently hides download failures. Consider adding error handling or checksum verification for the downloaded JARs to ensure integrity and visibility into potential network issues.The Python Cloud SQL Connector integration via the pinned commit is correctly implemented with proper Manager parameter handling and connector lifecycle management.
♻️ Duplicate comments (7)
docker-jans-link/templates/jans-mysql.properties (1)
3-3: Verifyrdbm_connection_uriincludes all required JDBC parameters.Same pattern as other template files. Ensure TLS settings and other driver-specific options are included in
rdbm_connection_urifor both standard MySQL and Cloud SQL scenarios.docker-jans-casa/templates/jans-pgsql.properties (1)
3-3: Verifyrdbm_connection_uriincludes all PostgreSQL connection parameters.Same verification concern as other PostgreSQL templates. Ensure proper SSL/TLS configuration and Cloud SQL Connector compatibility.
docker-jans-casa/templates/jans-mysql.properties (1)
3-3: Verifyrdbm_connection_uriincludes TLS and other MySQL JDBC parameters.Same verification concern as other MySQL templates. Ensure
enabledTLSProtocols=TLSv1.2and other driver-specific options are preserved inrdbm_connection_uri.docker-jans-saml/templates/jans-pgsql.properties (1)
3-3: Verifyrdbm_connection_uriincludes all PostgreSQL connection parameters.Same verification concern as other PostgreSQL templates. Ensure proper SSL/TLS configuration and Cloud SQL Connector compatibility.
docker-jans-saml/templates/jans-mysql.properties (1)
3-3: Verifyrdbm_connection_uriincludes TLS and other MySQL JDBC parameters.Same verification concern as other MySQL templates. Ensure
enabledTLSProtocols=TLSv1.2and other driver-specific options are preserved inrdbm_connection_uri.docker-jans-scim/templates/jans-pgsql.properties (1)
3-3: Verifyrdbm_connection_uriincludes all PostgreSQL connection parameters.Same verification concern as other PostgreSQL templates. Ensure proper SSL/TLS configuration and Cloud SQL Connector compatibility.
docker-jans-config-api/templates/jans-mysql.properties (1)
3-3: TLS enforcement verification applies to this file as well.Identical MySQL template pattern to docker-jans-fido2/templates/jans-mysql.properties. The same TLSv1.2 enforcement verification is required here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (20)
docker-jans-auth-server/Dockerfile(1 hunks)docker-jans-auth-server/scripts/bootstrap.py(1 hunks)docker-jans-casa/Dockerfile(2 hunks)docker-jans-casa/templates/jans-mysql.properties(1 hunks)docker-jans-casa/templates/jans-pgsql.properties(1 hunks)docker-jans-config-api/Dockerfile(2 hunks)docker-jans-config-api/templates/jans-mysql.properties(1 hunks)docker-jans-config-api/templates/jans-pgsql.properties(1 hunks)docker-jans-fido2/Dockerfile(2 hunks)docker-jans-fido2/templates/jans-mysql.properties(1 hunks)docker-jans-fido2/templates/jans-pgsql.properties(1 hunks)docker-jans-link/Dockerfile(2 hunks)docker-jans-link/templates/jans-mysql.properties(1 hunks)docker-jans-link/templates/jans-pgsql.properties(1 hunks)docker-jans-saml/templates/jans-mysql.properties(1 hunks)docker-jans-saml/templates/jans-pgsql.properties(1 hunks)docker-jans-scim/Dockerfile(2 hunks)docker-jans-scim/templates/jans-mysql.properties(1 hunks)docker-jans-scim/templates/jans-pgsql.properties(1 hunks)jans-pycloudlib/docs/api/persistence/cloudsql_connector.md(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-10T17:38:39.610Z
Learnt from: iromli
Repo: JanssenProject/jans PR: 12679
File: docker-jans-config-api/Dockerfile:120-120
Timestamp: 2025-11-10T17:38:39.610Z
Learning: In docker-jans-config-api/Dockerfile, the sed command that replaces 'your-openid-provider.server' with '%(hostname)s' in the adminui-policy-store.json file is intentional. The %(hostname)s variable uses Python string interpolation format and is consumed by scripts executed during container runtime.
Applied to files:
docker-jans-casa/templates/jans-pgsql.propertiesdocker-jans-scim/templates/jans-pgsql.propertiesdocker-jans-config-api/templates/jans-pgsql.propertiesdocker-jans-fido2/templates/jans-mysql.propertiesdocker-jans-scim/templates/jans-mysql.propertiesdocker-jans-saml/templates/jans-mysql.propertiesdocker-jans-link/templates/jans-pgsql.propertiesdocker-jans-casa/Dockerfiledocker-jans-saml/templates/jans-pgsql.propertiesdocker-jans-fido2/templates/jans-pgsql.propertiesdocker-jans-config-api/Dockerfiledocker-jans-fido2/Dockerfiledocker-jans-casa/templates/jans-mysql.properties
📚 Learning: 2025-11-26T12:38:52.165Z
Learnt from: misba7
Repo: JanssenProject/jans PR: 12737
File: automation/mysql.yaml:22-59
Timestamp: 2025-11-26T12:38:52.165Z
Learning: In the Janssen project, Kubernetes manifests in the automation/ directory (such as mysql.yaml and pgsql.yaml) are for demo and testing purposes only. These manifests do not require production-level hardening (security contexts, resource limits, health checks, etc.) as they are intended for local development and demonstration scenarios, not production deployments.
Applied to files:
docker-jans-saml/templates/jans-mysql.propertiesdocker-jans-casa/Dockerfile
🪛 markdownlint-cli2 (0.18.1)
jans-pycloudlib/docs/api/persistence/cloudsql_connector.md
166-166: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
174-174: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
196-196: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
196-196: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
202-202: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
202-202: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
233-233: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
351-351: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
358-358: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: sonar scan (jans-core)
- GitHub Check: docker (configurator)
- GitHub Check: sonar scan (jans-orm)
- GitHub Check: sonar scan (jans-casa)
- GitHub Check: docker (monolith)
- GitHub Check: Analyze (python)
- GitHub Check: docker (auth-server)
- GitHub Check: docker (persistence-loader)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript)
- GitHub Check: wasm_tests
- GitHub Check: python_tests (3.10)
- GitHub Check: golang_tests
- GitHub Check: rust_tests
- GitHub Check: python_tests (3.11)
- GitHub Check: rust_benchmarks
🔇 Additional comments (11)
jans-pycloudlib/docs/api/persistence/cloudsql_connector.md (1)
207-216: Verify: Are Cloud SQL JDBC Socket Factory JARs actually included in Docker images?Lines 209–216 state that the Cloud SQL JDBC Socket Factory JARs "are included in the Janssen Docker images" and "are always available." However, the PR objectives note an action item: "Cloud SQL JDBC Socket Factory JAR needs to be added to the images."
This creates a contradiction—the documentation claims they're already included, but the PR work suggests they are still being added. Clarify whether these JARs are currently available or if the documentation is premature.
Once the JARs are confirmed to be included, update the documentation to note the version or commit hash when they were added, or adjust the language to reflect ongoing work if not yet complete.
docker-jans-fido2/Dockerfile (2)
67-67: Version bump: JANS_SOURCE_VERSION updated.Confirms the source version is pinned to the latest commit hash containing pycloudlib changes for Cloud SQL connector support.
219-221: Ensure custom/libs directory has appropriate ownership and permissions.The custom/libs directory is created on line 219 and populated with JARs, but does not appear in the explicit ownership/permission adjustments on lines 242–253 (e.g., line 248 handles
${JETTY_BASE}/common/libsbut not${JETTY_BASE}/jans-fido2/custom/libs). Confirm that the directory inherits sufficient permissions from the parent or is handled elsewhere, and verify the Jetty process (user 1000) can read these JARs at runtime.docker-jans-link/Dockerfile (1)
207-207: The Cloud SQL JDBC Socket Factory version 1.27.0 is valid and current, published on Maven Central as of October 2025. The version includes core artifacts (jdbc-socket-factory-core:1.27.0) and driver-specific variants (postgres-socket-factory:1.27.0) that match the download targets in this Dockerfile. No incompatibility issues found.docker-jans-scim/Dockerfile (2)
62-62: JANS_SOURCE_VERSION commit includes Cloud SQL Auth Proxy changes.The commit
5343272241b17a3e4efa7d998ae031f6ad512aa1includes the complete pycloudlib Cloud SQL Python Connector implementation with supporting documentation (docs/api/persistence/cloudsql_connector.md), test workflows, and configuration files.
210-210: Version 1.27.0 is current and appropriate; no changes needed.CLOUDSQL_SOCKET_FACTORY_VERSION=1.27.0 is the latest stable release for both the MySQL Connector/J 8 and PostgreSQL socket factory artifacts (released October 30, 2025). The use of the
-qflag onwgetcommands is consistent with the established pattern throughout this codebase across all Dockerfiles. Checksum verification is not a standard practice in this project's Docker builds, and the error handling via&&command chaining is sufficient—if either download fails, the subsequent commands in the RUN block will not execute, causing the build to fail at that step.docker-jans-config-api/Dockerfile (1)
74-74: Commit hash verified in repository.The JANS_SOURCE_VERSION commit (5343272) exists in the JanssenProject/jans repository. However, note that the commit message is "docs: fix get manager example" and does not indicate Cloud SQL Connector-related changes. Verify this is the intended commit for the configuration updates.
docker-jans-scim/templates/jans-mysql.properties (1)
3-3: The review comment should be removed. Therdbm_connection_uriis properly constructed in the Python persistence layer (jans-pycloudlib/jans/pycloudlib/persistence/sql.py, lines 1061-1107) with all required JDBC parameters:
- Standard MySQL: Includes
enabledTLSProtocols=TLSv1.2exactly as in the old template- Cloud SQL: Includes socketFactory (
com.google.cloud.sql.mysql.SocketFactory) which manages SSL/TLS connections- Server timezone: Included in the URI for Cloud SQL and as a separate driver property for standard MySQL
- Other driver properties (cachePrepStmts, cacheResultSetMetadata, etc.): Remain as separate driver properties in the template, as expected
The centralization of URI construction to the Python layer maintains all existing security and functionality requirements.
docker-jans-fido2/templates/jans-pgsql.properties (1)
3-3: Connection URI parameter change mirrors other template files.Consistent with the parallel changes in docker-jans-link and other template files. The verification and concerns raised for rdbm_connection_uri injection apply equally here.
docker-jans-fido2/templates/jans-mysql.properties (1)
3-3: Verify TLSv1.2 enforcement is preserved for all MySQL connection scenarios.The original MySQL connection URI explicitly enforced TLSv1.2 via
enabledTLSProtocols=TLSv1.2. The generatedrdbm_connection_uripreserves this for standard MySQL connections, but the Cloud SQL Connector path uses implicit TLS via the socketFactory without explicit version enforcement. Confirm that the Cloud SQL Connector's automatic TLS encryption meets the same security requirements as the original explicit TLSv1.2 enforcement, or add explicit TLS version parameters to the Cloud SQL connection URI.docker-jans-link/templates/jans-pgsql.properties (1)
3-3: Ensurerdbm_connection_uriis properly populated for all connection scenarios.This change delegates JDBC URI construction to a template variable. The
rdbm_connection_uriis correctly generated by_build_jdbc_connection_uri()in the Python persistence layer and handles all code paths:
- Standard database connections (MySQL and PostgreSQL)
- Cloud SQL Connector with proper instance connection name validation
- Private IP support for Cloud SQL
- Complete URIs with necessary parameters (TLS protocols, socket factories, timezones)
The implementation validates required configuration (Cloud SQL instance name) and raises errors for misconfiguration, preventing malformed URIs from being injected.
docker-jans-config-api/Dockerfile
Outdated
| ARG CLOUDSQL_SOCKET_FACTORY_VERSION=1.27.0 | ||
|
|
||
| # Download Cloud SQL JDBC Socket Factory JARs directly to custom/libs (always available) | ||
| RUN mkdir -p ${JETTY_BASE}/jans-config-api/custom/libs \ | ||
| && wget -q https://repo1.maven.org/maven2/com/google/cloud/sql/mysql-socket-factory-connector-j-8/${CLOUDSQL_SOCKET_FACTORY_VERSION}/mysql-socket-factory-connector-j-8-${CLOUDSQL_SOCKET_FACTORY_VERSION}.jar -P ${JETTY_BASE}/jans-config-api/custom/libs/ \ | ||
| && wget -q https://repo1.maven.org/maven2/com/google/cloud/sql/postgres-socket-factory/${CLOUDSQL_SOCKET_FACTORY_VERSION}/postgres-socket-factory-${CLOUDSQL_SOCKET_FACTORY_VERSION}.jar -P ${JETTY_BASE}/jans-config-api/custom/libs/ |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Cloud SQL JDBC Socket Factory version 1.27.0 Maven Central latest release
💡 Result:
Yes — Cloud SQL JDBC Socket Factory 1.27.0 is published on Maven Central (release date Oct 30, 2025).
Maven coordinates (parent/core & driver artifacts):
- com.google.cloud.sql:jdbc-socket-factory-parent:1.27.0. [1]
- com.google.cloud.sql:jdbc-socket-factory-core:1.27.0. [2]
- driver-specific artifacts (example): com.google.cloud.sql:postgres-socket-factory:1.27.0 (also available). [1]
Maven example:
com.google.cloud.sql
postgres-socket-factory
1.27.0
Gradle example:
implementation "com.google.cloud.sql:postgres-socket-factory:1.27.0"
Sources: Maven Central listings. [1][2]
🌐 Web query:
google-cloud-sql mysql-socket-factory-connector-j-8 latest version
💡 Result:
The latest released version on Maven Central is 1.25.0 (released Apr 28, 2025). [1][2]
Sources:
- mvnrepository.com listing for com.google.cloud.sql:mysql-socket-factory-connector-j-8 — version 1.25.0. [1]
- Maven Central (package artifact overview showing recent versions). [2]
References:
[1] (mvnrepository.com)
[2] (central.sonatype.com)
Citations:
- 1: https://mvnrepository.com/artifact/com.google.cloud.sql/mysql-socket-factory-connector-j-8/1.25.0?utm_source=openai
- 2: https://central.sonatype.com/artifact/com.google.cloud.sql/mysql-socket-factory-connector-j-8/1.21.2?utm_source=openai
🌐 Web query:
google-cloud-sql postgres-socket-factory latest version
💡 Result:
The latest released version of com.google.cloud.sql:postgres-socket-factory is 1.27.0 (released Oct 30, 2025). [1][2]
Sources:
- Artifact listing showing 1.27.0 as the newest release. [1]
- 1.26.1 release page for context (previous release). [2]
[1] https://mvnrepository.com/artifact/com.google.cloud.sql/postgres-socket-factory
[2] https://mvnrepository.com/artifact/com.google.cloud.sql/postgres-socket-factory/1.26.1
Update mysql-socket-factory-connector-j-8 to version 1.25.0, the latest stable release on Maven Central.
The postgres-socket-factory 1.27.0 is current (released Oct 30, 2025), but mysql-socket-factory-connector-j-8 latest available version is 1.25.0 (released Apr 28, 2025). Using mismatched versions across the two artifacts may cause compatibility issues. Consider adding checksum verification for downloaded JARs to strengthen supply chain security.
| db.schema.name=%(rdbm_schema)s | ||
|
|
||
| connection.uri=jdbc:postgresql://%(rdbm_host)s:%(rdbm_port)s/%(rdbm_db)s | ||
| connection.uri=%(rdbm_connection_uri)s |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find files related to database connection URI construction
find . -type f -name "*.py" -o -name "*.properties" | head -20Repository: JanssenProject/jans
Length of output: 1028
🏁 Script executed:
# Search for rdbm_connection_uri construction
rg "rdbm_connection_uri" --type-list | head -20Repository: JanssenProject/jans
Length of output: 452
🏁 Script executed:
# Search for where connection URIs are built
rg "connection\.uri" -A 3 -B 3 --type propertiesRepository: JanssenProject/jans
Length of output: 97
🏁 Script executed:
# Find references to pg8000 and cloud-sql-python-connector
rg "pg8000|cloud.sql.python.connector|cloud-sql-python-connector" -iRepository: JanssenProject/jans
Length of output: 2981
🏁 Script executed:
# Look for PostgreSQL connection configuration
rg "postgresql|postgres" -i --type properties --type python -B 2 -A 2 | head -50Repository: JanssenProject/jans
Length of output: 149
🏁 Script executed:
# Search for rdbm_connection_uri in all file types
rg "rdbm_connection_uri"Repository: JanssenProject/jans
Length of output: 1475
🏁 Script executed:
# Find jans-config-api related files
find . -path "*/docker-jans-config-api/*" -type f | grep -E "\.(py|sh|properties)$" | head -30Repository: JanssenProject/jans
Length of output: 590
🏁 Script executed:
# Look at jans-pgsql.properties and surrounding configuration
find . -name "jans-pgsql.properties" -o -name "jans-mysql.properties"Repository: JanssenProject/jans
Length of output: 928
🏁 Script executed:
# Search for how these template properties are used/interpolated
rg "rdbm_host|rdbm_port|rdbm_db" -A 2 -B 2 | head -80Repository: JanssenProject/jans
Length of output: 6568
🏁 Script executed:
# Look for entrypoint or configuration scripts in docker-jans-config-api
find ./docker-jans-config-api -type f \( -name "*.py" -o -name "*.sh" \) | head -20Repository: JanssenProject/jans
Length of output: 476
🏁 Script executed:
# Find the _build_jdbc_connection_uri function
rg "_build_jdbc_connection_uri" -A 20 -B 2Repository: JanssenProject/jans
Length of output: 4210
🏁 Script executed:
# Look at the entire sql.py file to understand the context
cat jans-pycloudlib/jans/pycloudlib/persistence/sql.py | head -200Repository: JanssenProject/jans
Length of output: 7552
🏁 Script executed:
# Get more of the sql.py file focusing on the URI building logic
rg "def _build_jdbc_connection_uri" -A 30Repository: JanssenProject/jans
Length of output: 3055
🏁 Script executed:
# Get the complete function implementation
rg "def _build_jdbc_connection_uri" -A 60Repository: JanssenProject/jans
Length of output: 5694
🏁 Script executed:
# Check if there are separate SSL properties for PostgreSQL in the templates
cat ./docker-jans-config-api/templates/jans-pgsql.propertiesRepository: JanssenProject/jans
Length of output: 1275
🏁 Script executed:
# Compare with old PostgreSQL templates to see how SSL was handled before
cat ./jans-linux-setup/jans_setup/templates/jans-pgsql.propertiesRepository: JanssenProject/jans
Length of output: 1472
Missing SSL/TLS configuration for standard PostgreSQL connections.
The template migration removed critical SSL/TLS driver properties (ssl, sslmode, sslfactory) that were present in the original configuration. While Cloud SQL connections use the socketFactory mechanism for SSL, standard PostgreSQL deployments now have no explicit TLS configuration.
The _build_jdbc_connection_uri() function returns jdbc:postgresql://host:port/dbname for non-Cloud SQL deployments, lacking the SSL parameters that should be included. MySQL connections compensate by including enabledTLSProtocols=TLSv1.2 in the URI, but PostgreSQL does not.
Ensure that SSL/TLS settings are either:
- Included in the URI via
sslmodeparameter (e.g.,?sslmode=require), or - Added back as driver properties in the template
🤖 Prompt for AI Agents
In docker-jans-config-api/templates/jans-pgsql.properties around line 3, the
generated JDBC URI for standard PostgreSQL lacks TLS options
(ssl/sslmode/sslfactory) so non-Cloud-SQL deployments no longer enable TLS;
update the template to either append SSL parameters to the connection.uri (for
example add a configurable query string like ?sslmode=require or
?ssl=true&sslmode=require&enabledTLSProtocols=TLSv1.2 using an injected template
variable) or reintroduce driver properties (ssl=true, sslmode=require and
optional sslfactory) in the properties file, while preserving the existing
socketFactory logic for Cloud SQL paths so those keep using the socket factory
instead of redundant SSL params.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
docker-jans-fido2/Dockerfile (1)
212-221: Add explicit ownership for custom/libs directory (unresolved from prior review).The custom/libs directory created at lines 216–221 and populated with Cloud SQL Socket Factory JARs is not explicitly assigned to user 1000 in the permission block (lines 242–253), unlike common/libs (line 248). This may cause runtime permission issues for the non-root Jetty process.
Apply this fix to ensure the Jetty user can access the JARs:
&& chown -R 1000:0 ${JETTY_BASE}/common/libs \ + && chown -R 1000:0 ${JETTY_BASE}/jans-fido2/custom/libs \ && chown -R 1000:0 /usr/share/java \docker-jans-casa/Dockerfile (1)
209-218: Add explicit ownership for the custom/libs directory.The custom/libs directory created at lines 216–218 is not explicitly owned by user 1000 in the permission block (line 251), unlike common/libs. This will cause permission issues for the non-root Jetty process at runtime.
Add the following line to the chown block:
&& chown -R 1000:0 ${JETTY_BASE}/common/libs \ + && chown -R 1000:0 ${JETTY_BASE}/jans-casa/custom/libs \ && chown -R 1000:0 /usr/share/java \docker-jans-link/Dockerfile (1)
210-212: Set ownership and permissions for custom/libs directory (flagged in prior review).As noted in the previous review, the custom/libs directory created at line 210 and populated with Cloud SQL Socket Factory JARs (lines 211–212) must have ownership and permissions set for the jetty user (UID 1000). The permission section (lines 234–245) does not include these settings, which will cause runtime failures when the Java application attempts to load the JARs.
Apply this diff to the permission section (after line 239):
&& chown -R 1000:0 ${JETTY_BASE}/common/libs \ + && chown -R 1000:0 ${JETTY_BASE}/jans-link/custom/libs \ + && chmod -R g=u ${JETTY_BASE}/jans-link/custom/libs \ && chown -R 1000:0 /usr/share/java \
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
docker-jans-all-in-one/Dockerfile(1 hunks)docker-jans-auth-server/Dockerfile(2 hunks)docker-jans-casa/Dockerfile(2 hunks)docker-jans-cloudtools/Dockerfile(1 hunks)docker-jans-config-api/Dockerfile(2 hunks)docker-jans-configurator/Dockerfile(1 hunks)docker-jans-fido2/Dockerfile(2 hunks)docker-jans-link/Dockerfile(2 hunks)docker-jans-monolith/Dockerfile(1 hunks)docker-jans-persistence-loader/Dockerfile(1 hunks)docker-jans-saml/Dockerfile(1 hunks)docker-jans-scim/Dockerfile(2 hunks)jans-cedarling/flask-sidecar/Dockerfile(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-10T17:38:39.610Z
Learnt from: iromli
Repo: JanssenProject/jans PR: 12679
File: docker-jans-config-api/Dockerfile:120-120
Timestamp: 2025-11-10T17:38:39.610Z
Learning: In docker-jans-config-api/Dockerfile, the sed command that replaces 'your-openid-provider.server' with '%(hostname)s' in the adminui-policy-store.json file is intentional. The %(hostname)s variable uses Python string interpolation format and is consumed by scripts executed during container runtime.
Applied to files:
jans-cedarling/flask-sidecar/Dockerfiledocker-jans-configurator/Dockerfiledocker-jans-saml/Dockerfiledocker-jans-casa/Dockerfiledocker-jans-persistence-loader/Dockerfiledocker-jans-fido2/Dockerfiledocker-jans-config-api/Dockerfiledocker-jans-auth-server/Dockerfile
📚 Learning: 2025-11-18T07:43:55.761Z
Learnt from: pujavs
Repo: JanssenProject/jans PR: 12704
File: jans-config-api/docs/jans-config-api-swagger.yaml:17540-17546
Timestamp: 2025-11-18T07:43:55.761Z
Learning: The file `jans-config-api/docs/jans-config-api-swagger.yaml` is auto-generated with dependent modules changes and metadata. The config API does not override it.
Applied to files:
docker-jans-configurator/Dockerfile
📚 Learning: 2025-11-26T12:38:52.165Z
Learnt from: misba7
Repo: JanssenProject/jans PR: 12737
File: automation/mysql.yaml:22-59
Timestamp: 2025-11-26T12:38:52.165Z
Learning: In the Janssen project, Kubernetes manifests in the automation/ directory (such as mysql.yaml and pgsql.yaml) are for demo and testing purposes only. These manifests do not require production-level hardening (security contexts, resource limits, health checks, etc.) as they are intended for local development and demonstration scenarios, not production deployments.
Applied to files:
docker-jans-casa/Dockerfile
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: docker (auth-server)
- GitHub Check: wasm_tests
- GitHub Check: golang_tests
- GitHub Check: rust_tests
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (12)
docker-jans-configurator/Dockerfile (1)
30-30: Commit hash is valid and implements Cloud SQL support.The commit
be95dc67a0f482b417aaab2e13ff9bde09f8d7adis legitimate and implements Cloud SQL JDBC Socket Factory support for Java services, as evidenced by the commit message and related Dockerfile updates across multiple services (auth-server, config-api, fido2, scim, etc.). The corresponding jans-pycloudlib documentation file was also updated, confirming the source version change pulls in the necessary cloud infrastructure enhancements.jans-cedarling/flask-sidecar/Dockerfile (1)
34-34: Commit hash verified—routine version update is valid.The JANS_SOURCE_VERSION has been updated to
be95dc67a0f482b417aaab2e13ff9bde09f8d7ad, which exists in the JanssenProject/jans repository. This is a routine version bump consistent with other Dockerfile updates in this PR.docker-jans-monolith/Dockerfile (1)
44-44: Straightforward source version update for asset alignment.JANS_SOURCE_VERSION update aligns the asset snapshot with the Cloud SQL Connector changes introduced elsewhere in the PR.
docker-jans-saml/Dockerfile (1)
42-42: Straightforward source version update for asset alignment.JANS_SOURCE_VERSION update aligns the asset snapshot with other images in the PR.
docker-jans-fido2/Dockerfile (1)
67-67: Straightforward source version update.JANS_SOURCE_VERSION update aligns the asset snapshot with Cloud SQL Connector changes.
docker-jans-cloudtools/Dockerfile (1)
47-47: Straightforward source version update for asset alignment.JANS_SOURCE_VERSION update aligns the asset snapshot across the deployment.
docker-jans-persistence-loader/Dockerfile (1)
21-21: Straightforward source version update for asset alignment.JANS_SOURCE_VERSION update aligns the asset snapshot with Python library changes for Cloud SQL Connector support.
docker-jans-all-in-one/Dockerfile (1)
62-62: Straightforward source version update for asset alignment.JANS_SOURCE_VERSION update aligns the asset snapshot; Cloud SQL Socket Factory JARs are inherited via COPY --from base images.
docker-jans-casa/Dockerfile (1)
61-61: Straightforward source version update.JANS_SOURCE_VERSION update aligns the asset snapshot with Cloud SQL Connector changes.
docker-jans-config-api/Dockerfile (1)
74-74: Straightforward source version update.JANS_SOURCE_VERSION update aligns the asset snapshot with Cloud SQL Connector changes.
docker-jans-auth-server/Dockerfile (2)
67-83: Custom/libs directory structure and Cloud SQL JAR downloads look correct.The
custom/libsdirectory is created at line 68, populated with Cloud SQL Socket Factory JARs for both MySQL and Postgres at lines 76–79, and later combined with additional custom libs from the for loop (lines 80–83). Permissions are handled via the recursivechownat line 296 andchmodat line 289 on the parentcustomdirectory, which is compliant.Note: The for loops (lines 80–83, 85–88) don't include explicit failure handling (e.g.,
set -eor|| exit 1), but this is consistent with the existing Dockerfile pattern and was noted in previous reviews as acceptable if some jars are optional.
101-101: JANS_SOURCE_VERSION update is consistent across the PR.The version
be95dc67a0f482b417aaab2e13ff9bde09f8d7adis updated in all three Dockerfiles and the git sparse-checkout includesjans-pycloudlib(line 113), ensuring assets and pycloudlib changes are synchronized.
Signed-off-by: moabu <[email protected]>
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
docker-jans-scim/Dockerfile (1)
5-5: Same cloudsql-builder image issue as docker-jans-fido2.The pipeline failure here is identical to the one in
docker-jans-fido2/Dockerfile. Address the root cause investigation there to fix this occurrence as well.docker-jans-config-api/Dockerfile (1)
5-5: Same cloudsql-builder image issue.This is the same pipeline failure occurring in other Dockerfiles. Fix the root cause to resolve this instance.
docker-jans-auth-server/Dockerfile (1)
5-5: Same cloudsql-builder image issue.This is the same pipeline failure. Address the root cause to fix all occurrences.
docker-jans-link/Dockerfile (1)
5-5: Same cloudsql-builder image issue.Duplicate of the pipeline failure in other Dockerfiles.
jans-pycloudlib/jans/pycloudlib/persistence/sql.py (1)
583-637: Comprehensive SqlClient lifecycle with proper cleanup order and idempotency.The implementation correctly:
- Uses
_closedflag for idempotency (lines 592-595)- Removes from tracker first (lines 597-600)
- Disposes engine before closing adapter (lines 602-612), ensuring connection pool is released before stopping token-refresh threads
- Provides context manager support for automatic cleanup (lines 616-637)
- Logs cleanup errors without propagating them, allowing orderly shutdown
One minor cleanup: remove the quoted type annotation at line 616 since
from __future__ import annotationsis enabled:- def __enter__(self) -> "SqlClient": + def __enter__(self) -> SqlClient:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
docker-jans-auth-server/Dockerfile(3 hunks)docker-jans-auth-server/scripts/bootstrap.py(1 hunks)docker-jans-casa/Dockerfile(3 hunks)docker-jans-config-api/Dockerfile(4 hunks)docker-jans-fido2/Dockerfile(3 hunks)docker-jans-link/Dockerfile(3 hunks)docker-jans-monolith/scripts/entrypoint.sh(7 hunks)docker-jans-scim/Dockerfile(3 hunks)jans-pycloudlib/docs/api/persistence/cloudsql_connector.md(1 hunks)jans-pycloudlib/jans/pycloudlib/persistence/sql.py(16 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-10T17:38:39.610Z
Learnt from: iromli
Repo: JanssenProject/jans PR: 12679
File: docker-jans-config-api/Dockerfile:120-120
Timestamp: 2025-11-10T17:38:39.610Z
Learning: In docker-jans-config-api/Dockerfile, the sed command that replaces 'your-openid-provider.server' with '%(hostname)s' in the adminui-policy-store.json file is intentional. The %(hostname)s variable uses Python string interpolation format and is consumed by scripts executed during container runtime.
Applied to files:
docker-jans-casa/Dockerfiledocker-jans-config-api/Dockerfile
🪛 Checkov (3.2.334)
docker-jans-auth-server/Dockerfile
[low] 1-342: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
docker-jans-casa/Dockerfile
[low] 1-285: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
docker-jans-scim/Dockerfile
[low] 1-275: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
docker-jans-config-api/Dockerfile
[low] 1-320: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
docker-jans-fido2/Dockerfile
[low] 1-282: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
docker-jans-link/Dockerfile
[low] 1-274: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
🪛 GitHub Actions: Build and Publish Releases to Hub
docker-jans-fido2/Dockerfile
[error] 5-5: FROM maven:3.9-eclipse-temurin-17-alpine AS cloudsql-builder
🪛 Hadolint (2.14.0)
docker-jans-auth-server/Dockerfile
[warning] 9-9: Pin versions in apk add. Instead of apk add <package> use apk add <package>=<version>
(DL3018)
docker-jans-casa/Dockerfile
[warning] 9-9: Pin versions in apk add. Instead of apk add <package> use apk add <package>=<version>
(DL3018)
docker-jans-scim/Dockerfile
[warning] 9-9: Pin versions in apk add. Instead of apk add <package> use apk add <package>=<version>
(DL3018)
docker-jans-config-api/Dockerfile
[warning] 9-9: Pin versions in apk add. Instead of apk add <package> use apk add <package>=<version>
(DL3018)
docker-jans-fido2/Dockerfile
[warning] 9-9: Pin versions in apk add. Instead of apk add <package> use apk add <package>=<version>
(DL3018)
docker-jans-link/Dockerfile
[warning] 9-9: Pin versions in apk add. Instead of apk add <package> use apk add <package>=<version>
(DL3018)
🪛 markdownlint-cli2 (0.18.1)
jans-pycloudlib/docs/api/persistence/cloudsql_connector.md
362-362: Ordered list item prefix
Expected: 1; Actual: 4; Style: 1/1/1
(MD029, ol-prefix)
🪛 Ruff (0.14.7)
docker-jans-auth-server/scripts/bootstrap.py
187-187: Logging statement uses f-string
(G004)
jans-pycloudlib/jans/pycloudlib/persistence/sql.py
100-104: Avoid specifying long messages outside the exception class
(TRY003)
147-150: Avoid specifying long messages outside the exception class
(TRY003)
157-157: Dynamically typed expressions (typing.Any) are disallowed in getconn
(ANN401)
189-189: Do not catch blind exception: Exception
(BLE001)
190-190: Logging statement uses f-string
(G004)
467-467: Do not catch blind exception: Exception
(BLE001)
468-468: Logging statement uses f-string
(G004)
599-599: Do not catch blind exception: Exception
(BLE001)
600-600: Logging statement uses f-string
(G004)
606-606: Do not catch blind exception: Exception
(BLE001)
607-607: Logging statement uses f-string
(G004)
616-616: Remove quotes from type annotation
Remove quotes
(UP037)
628-628: Dynamically typed expressions (typing.Any) are disallowed in exc_tb
(ANN401)
1055-1059: Avoid specifying long messages outside the exception class
(TRY003)
1168-1172: Avoid specifying long messages outside the exception class
(TRY003)
🪛 Shellcheck (0.11.0)
docker-jans-monolith/scripts/entrypoint.sh
[info] 50-50: Double quote to prevent globbing and word splitting.
(SC2086)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: python_tests (3.10)
- GitHub Check: golang_tests
- GitHub Check: wasm_tests
- GitHub Check: rust_benchmarks
- GitHub Check: sonar scan (jans-orm)
- GitHub Check: sonar scan (agama)
- GitHub Check: sonar scan (jans-pycloudlib)
- GitHub Check: sonar scan (jans-link)
- GitHub Check: sonar scan (jans-keycloak-link)
- GitHub Check: sonar scan (jans-lock)
- GitHub Check: sonar scan (jans-linux-setup)
- GitHub Check: sonar scan (jans-config-api)
- GitHub Check: sonar scan (jans-scim)
- GitHub Check: sonar scan (jans-auth-server)
- GitHub Check: sonar scan (jans-casa)
- GitHub Check: sonar scan (jans-fido2)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (python)
🔇 Additional comments (17)
docker-jans-monolith/scripts/entrypoint.sh (4)
3-3: Enablingpipefailimproves pipeline error detection.Ensuring that pipeline failures propagate correctly prevents silent failures when intermediate commands fail.
100-118: Robust error handling for install script execution.The addition of exit code capture (line 108) and detailed error messaging (lines 110–116) with a clear path to troubleshooting (
/setup_log) significantly improves debuggability. The-Lflag on curl (line 103) correctly follows redirects.
125-127: Clean conditional integration of RDBMS wait.The call to
wait_for_rdbmsis properly gated behind a check for RDBMS configuration, avoiding unnecessary delays when no database is involved.
241-243: Simplified completion messaging aligns with containerized deployment model.Removing the log-tail routine and replacing it with status messages and guidance on accessing logs via
docker-composeis appropriate for container entrypoints, where services are typically managed by external orchestrators (e.g., Systemd, Docker Compose).docker-jans-casa/Dockerfile (3)
237-238: Multistage COPY correctly pulls Cloud SQL Socket Factory JARs.The COPY --from pattern properly references the cloudsql-builder stage and preserves fat JAR artifacts with all dependencies bundled. The preceding mkdir ensures the target directory exists.
Note: These two RUN commands could optionally be consolidated with adjacent RUN operations (e.g., the block starting at line 240) to reduce layer count, but the current separation is clear and maintainable if each step serves a distinct logical purpose.
272-273: Ownership and permissions for custom/libs are now properly configured.Lines 272–273 correctly address the prior concern by setting ownership to user 1000 and applying group-write permissions (
g=u) to the custom/libs directory, matching the pattern applied to other Jetty asset directories.The non-root user (uid 1000) will now have full access to the Cloud SQL Socket Factory JARs at runtime.
1-19: Git version pinning in apk add is not enforced in this project.The Dockerfile consistently does not pin
apkpackage versions across all commands (lines 9, 32, 33), allowing automatic use of the latest available patch versions from Alpine repositories. This is an intentional project pattern. While Hadolint's DL3018 warning can be suppressed if pinning is not desired, the current approach is aligned with the project's existing practices.docker-jans-auth-server/scripts/bootstrap.py (1)
176-193: LGTM! Targeted library copying with appropriate safeguards.The refined logic correctly:
- Gates execution on
CN_LOCK_ENABLEDwith an early return (lines 179-180)- Uses a specific glob pattern
jans-lock*.jarto avoid copying unintended files (line 183)- Warns when lock is enabled but no matching JARs are found, aiding troubleshooting (lines 185-189)
- Copies only the filtered lock JARs to
custom/libs(lines 191-193)The warning message provides clear diagnostic information without blocking the build, which is the right balance for this scenario.
docker-jans-scim/Dockerfile (1)
262-263: Ownership and permissions correctly configured for custom/libs.The explicit ownership assignment
chown -R 1000:0and permission settingchmod -R g=uensure the Jetty user (UID 1000) can read the Cloud SQL Socket Factory JARs at runtime. This resolves the concern raised in past reviews.docker-jans-config-api/Dockerfile (1)
306-307: Explicit ownership for custom/libs correctly applied.The ownership and permissions are properly configured for the Cloud SQL Socket Factory JARs, addressing the past review concern. The Jetty user (UID 1000) will have read access at runtime.
docker-jans-auth-server/Dockerfile (2)
102-116: Excellent error handling withset -eand explicit exit.The use of
set -eat the start of each RUN block (lines 102, 112) combined with|| exit 1in the loops ensures that any failedwgetwill abort the build immediately. This prevents partial library populations and makes failures explicit, addressing the concern from past reviews.
328-329: Ownership and permissions correctly configured.The explicit
chown -R 1000:0andchmod -R g=uensure proper access for the Jetty user at runtime.docker-jans-link/Dockerfile (1)
260-261: Ownership and permissions correctly applied.The explicit ownership and permission settings ensure the Jetty user can access the Cloud SQL Socket Factory JARs, resolving the past review concern.
jans-pycloudlib/jans/pycloudlib/persistence/sql.py (3)
57-193: Excellent CloudSqlConnectorMixin design with comprehensive lifecycle management.The mixin implementation is well-structured:
- Clear separation of concerns (connector lifecycle, validation, connection creation)
- Proper lazy initialization with
_ensure_connector()(line 113)- Strict validation of instance connection name with helpful error messages (lines 95-106)
- Driver abstraction via
cloudsql_driverclass attribute (lines 146-150)- Fresh password fetch per connection to support rotation (line 170)
- Explicit cleanup with
close()method (lines 178-192)This design eliminates duplication between PostgreSQL and MySQL adapters while maintaining flexibility.
455-471: Solid global cleanup strategy with WeakSet and atexit.The module-level tracking with
weakref.WeakSet(line 455) andatexitregistration (line 471) ensures that Cloud SQL Connectors and SQLAlchemy engines are properly disposed on interpreter shutdown, even if callers forget to close clients explicitly. The WeakSet prevents keeping clients alive artificially.The defensive iteration over
list(_sql_client_instances)in_cleanup_sql_clients(line 464) prevents modification-during-iteration issues.
1036-1082: Well-structured JDBC URI builder with clear Cloud SQL connector handling.The
_build_jdbc_connection_urihelper cleanly separates Cloud SQL Connector URI generation from standard JDBC URIs:
- Validates instance connection name when connector is enabled (lines 1053-1059)
- Generates correct Cloud SQL Connector URIs with
socketFactoryparameters (lines 1061-1073)- Falls back to standard host:port URIs when connector is disabled (lines 1075-1081)
- Handles dialect-specific differences (MySQL serverTimezone, PostgreSQL format)
This centralization ensures consistency between Python and Java components.
docker-jans-fido2/Dockerfile (1)
5-5: No action required—the maven:3.9-eclipse-temurin-17-alpine base image is legitimate and available.The image exists on Docker Hub and is actively maintained as part of the official Maven Docker repository. The GitHub Actions workflow shows no failures related to this base image. The Dockerfile syntax is correct, and multiple other services in this repository successfully use similar maven base images (auth-server, casa, config-api, link, scim). No evidence of network access issues, digest changes, or image availability problems exists.
Likely an incorrect or invalid review comment.
| wait_for_rdbms() { | ||
| echo "Checking RDBMS connection..." | ||
| # Check if RDBMS_HOST is set | ||
| if [ -z "${RDBMS_HOST}" ]; then | ||
| echo "Error: RDBMS_HOST is not set. Cannot wait for database." | ||
| exit 1 | ||
| fi | ||
|
|
||
| local port="" | ||
| local db_type="" | ||
|
|
||
| if [[ "${CN_INSTALL_MYSQL}" == "true" ]]; then | ||
| port=3306 | ||
| db_type="MySQL" | ||
| elif [[ "${CN_INSTALL_PGSQL}" == "true" ]]; then | ||
| port=5432 | ||
| db_type="PostgreSQL" | ||
| else | ||
| echo "No RDBMS specified in CN_INSTALL_MYSQL or CN_INSTALL_PGSQL. Skipping wait." | ||
| return 0 | ||
| fi | ||
|
|
||
| echo "Waiting for ${db_type} at ${RDBMS_HOST}:${port}..." | ||
| # Use curl to check the port. We install curl in the Dockerfile, so it's available. | ||
| # Loop until curl can successfully connect to the RDBMS port | ||
| until curl ${RDBMS_HOST}:${port} > /dev/null 2>&1; do | ||
| echo "${db_type} is unavailable - sleeping for 2 seconds..." | ||
| sleep 2 | ||
| done | ||
| echo "${db_type} is up and running!" | ||
| } |
There was a problem hiding this comment.
Fix curl-based port check: incompatible with MySQL/PostgreSQL and missing variable quoting.
Two critical issues:
-
Unquoted variable (line 50):
curl ${RDBMS_HOST}:${port}lacks quotes around${RDBMS_HOST}:${port}, risking word-splitting if the hostname contains spaces or special characters (shellcheck SC2086). -
Wrong tool for TCP connectivity check: curl is designed for HTTP/HTTPS and will fail to establish connections to MySQL (port 3306) or PostgreSQL (port 5432) ports, which don't speak HTTP. The until loop will not exit successfully even if the database port is open, defeating the function's purpose.
Recommended fix using bash builtin /dev/tcp (portable across Linux containers):
- echo "Waiting for ${db_type} at ${RDBMS_HOST}:${port}..."
- # Use curl to check the port. We install curl in the Dockerfile, so it's available.
- # Loop until curl can successfully connect to the RDBMS port
- until curl ${RDBMS_HOST}:${port} > /dev/null 2>&1; do
+ echo "Waiting for ${db_type} at ${RDBMS_HOST}:${port}..."
+ # Use bash builtin /dev/tcp to verify TCP connectivity
+ until timeout 1 bash -c "</dev/tcp/${RDBMS_HOST}/${port}" > /dev/null 2>&1; do
echo "${db_type} is unavailable - sleeping for 2 seconds..."
sleep 2
doneAlternatively, if netcat (nc) is available in the Dockerfile, use: until nc -z "${RDBMS_HOST}" "${port}" > /dev/null 2>&1; do
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| wait_for_rdbms() { | |
| echo "Checking RDBMS connection..." | |
| # Check if RDBMS_HOST is set | |
| if [ -z "${RDBMS_HOST}" ]; then | |
| echo "Error: RDBMS_HOST is not set. Cannot wait for database." | |
| exit 1 | |
| fi | |
| local port="" | |
| local db_type="" | |
| if [[ "${CN_INSTALL_MYSQL}" == "true" ]]; then | |
| port=3306 | |
| db_type="MySQL" | |
| elif [[ "${CN_INSTALL_PGSQL}" == "true" ]]; then | |
| port=5432 | |
| db_type="PostgreSQL" | |
| else | |
| echo "No RDBMS specified in CN_INSTALL_MYSQL or CN_INSTALL_PGSQL. Skipping wait." | |
| return 0 | |
| fi | |
| echo "Waiting for ${db_type} at ${RDBMS_HOST}:${port}..." | |
| # Use curl to check the port. We install curl in the Dockerfile, so it's available. | |
| # Loop until curl can successfully connect to the RDBMS port | |
| until curl ${RDBMS_HOST}:${port} > /dev/null 2>&1; do | |
| echo "${db_type} is unavailable - sleeping for 2 seconds..." | |
| sleep 2 | |
| done | |
| echo "${db_type} is up and running!" | |
| } | |
| wait_for_rdbms() { | |
| echo "Checking RDBMS connection..." | |
| # Check if RDBMS_HOST is set | |
| if [ -z "${RDBMS_HOST}" ]; then | |
| echo "Error: RDBMS_HOST is not set. Cannot wait for database." | |
| exit 1 | |
| fi | |
| local port="" | |
| local db_type="" | |
| if [[ "${CN_INSTALL_MYSQL}" == "true" ]]; then | |
| port=3306 | |
| db_type="MySQL" | |
| elif [[ "${CN_INSTALL_PGSQL}" == "true" ]]; then | |
| port=5432 | |
| db_type="PostgreSQL" | |
| else | |
| echo "No RDBMS specified in CN_INSTALL_MYSQL or CN_INSTALL_PGSQL. Skipping wait." | |
| return 0 | |
| fi | |
| echo "Waiting for ${db_type} at ${RDBMS_HOST}:${port}..." | |
| # Use bash builtin /dev/tcp to verify TCP connectivity | |
| until timeout 1 bash -c "</dev/tcp/${RDBMS_HOST}/${port}" > /dev/null 2>&1; do | |
| echo "${db_type} is unavailable - sleeping for 2 seconds..." | |
| sleep 2 | |
| done | |
| echo "${db_type} is up and running!" | |
| } |
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 50-50: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 Prompt for AI Agents
In docker-jans-monolith/scripts/entrypoint.sh around lines 25 to 55, the
port-check uses an unquoted curl call which is both vulnerable to word-splitting
and the wrong tool for testing raw TCP ports (MySQL/Postgres do not speak HTTP);
replace the curl call with a TCP check and quote variables: either use bash
/dev/tcp by testing redirection to /dev/tcp/"${RDBMS_HOST}"/"${port}" in the
until loop (ensuring the script runs under bash), or use netcat if available
with until nc -z "${RDBMS_HOST}" "${port}" > /dev/null 2>&1; do; keep the
existing sleep/retry logic and ensure all variable references are quoted.
| --filter="bindings.role:roles/cloudsql.client" | ||
| ``` | ||
|
|
||
| 4. **Verify VPC Connector Status**: |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Optional: Fix ordered list numbering for consistency.
Static analysis flagged the list item numbering at line 362. While previous markdown formatting suggestions were marked as ignored, correcting the numbering to sequential values would improve consistency:
3. **Verify IAM Bindings**:
```bash
gcloud projects get-iam-policy PROJECT_ID \
--flatten="bindings[].members" \
--filter="bindings.role:roles/cloudsql.client"
```
-4. **Verify VPC Connector Status**:
+4. **Verify VPC Connector Status**:Feel free to skip if the team prefers the current formatting.
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
362-362: Ordered list item prefix
Expected: 1; Actual: 4; Style: 1/1/1
(MD029, ol-prefix)
🤖 Prompt for AI Agents
In jans-pycloudlib/docs/api/persistence/cloudsql_connector.md around line 362,
fix the ordered list item formatting by replacing the malformed "-4. **Verify
VPC Connector Status**:" token with a proper ordered-list entry "4. **Verify VPC
Connector Status**:" (remove the stray leading dash and ensure the numeric
ordering matches the surrounding list).
Signed-off-by: moabu <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docker-jans-all-in-one/Dockerfile (1)
58-72: The referenced commit hash 9489a96 cannot be verified in the janssenproject/jans repository.This commit is not accessible through standard repository queries or public search results. The Dockerfile will fail during the build if this commit does not exist or is not yet pushed to the repository. Verify that the correct commit hash has been pushed and is accessible on the main branch, or confirm that the correct commit SHA has been used before merging.
♻️ Duplicate comments (5)
docker-jans-casa/Dockerfile (1)
1-19: ****Same cloudsql-builder platform compatibility issue as docker-jans-auth-server. The
maven:3.9-eclipse-temurin-17-alpinebase image fails to resolve for target platforms in the build pipeline.docker-jans-scim/Dockerfile (1)
1-19: ****Cloudsql-builder platform compatibility issue (maven:3.9-eclipse-temurin-17-alpine does not support target architectures). This issue affects all Dockerfiles in this PR that use this stage.
docker-jans-config-api/Dockerfile (1)
1-19: ****Cloudsql-builder platform compatibility issue affects this Dockerfile as well (maven:3.9-eclipse-temurin-17-alpine).
docker-jans-fido2/Dockerfile (1)
1-19: ****Cloudsql-builder platform compatibility issue (maven:3.9-eclipse-temurin-17-alpine) is present in this Dockerfile as well.
docker-jans-link/Dockerfile (1)
1-19: ****Same cloudsql-builder platform compatibility issue across all Dockerfiles in this PR (maven:3.9-eclipse-temurin-17-alpine does not support linux/amd64, linux/arm64).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
docker-jans-all-in-one/Dockerfile(1 hunks)docker-jans-auth-server/Dockerfile(4 hunks)docker-jans-casa/Dockerfile(4 hunks)docker-jans-config-api/Dockerfile(4 hunks)docker-jans-configurator/Dockerfile(1 hunks)docker-jans-fido2/Dockerfile(4 hunks)docker-jans-link/Dockerfile(4 hunks)docker-jans-monolith/Dockerfile(1 hunks)docker-jans-monolith/scripts/entrypoint.sh(7 hunks)docker-jans-persistence-loader/Dockerfile(1 hunks)docker-jans-scim/Dockerfile(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-10T17:38:39.610Z
Learnt from: iromli
Repo: JanssenProject/jans PR: 12679
File: docker-jans-config-api/Dockerfile:120-120
Timestamp: 2025-11-10T17:38:39.610Z
Learning: In docker-jans-config-api/Dockerfile, the sed command that replaces 'your-openid-provider.server' with '%(hostname)s' in the adminui-policy-store.json file is intentional. The %(hostname)s variable uses Python string interpolation format and is consumed by scripts executed during container runtime.
Applied to files:
docker-jans-persistence-loader/Dockerfiledocker-jans-config-api/Dockerfiledocker-jans-auth-server/Dockerfiledocker-jans-casa/Dockerfiledocker-jans-configurator/Dockerfiledocker-jans-fido2/Dockerfile
📚 Learning: 2025-11-26T12:38:52.165Z
Learnt from: misba7
Repo: JanssenProject/jans PR: 12737
File: automation/mysql.yaml:22-59
Timestamp: 2025-11-26T12:38:52.165Z
Learning: In the Janssen project, Kubernetes manifests in the automation/ directory (such as mysql.yaml and pgsql.yaml) are for demo and testing purposes only. These manifests do not require production-level hardening (security contexts, resource limits, health checks, etc.) as they are intended for local development and demonstration scenarios, not production deployments.
Applied to files:
docker-jans-casa/Dockerfile
🪛 Checkov (3.2.334)
docker-jans-config-api/Dockerfile
[low] 1-320: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
docker-jans-auth-server/Dockerfile
[low] 1-342: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
docker-jans-scim/Dockerfile
[low] 1-275: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
docker-jans-casa/Dockerfile
[low] 1-285: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
docker-jans-fido2/Dockerfile
[low] 1-282: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
docker-jans-link/Dockerfile
[low] 1-274: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
🪛 GitHub Actions: Build and Publish Releases to Hub
docker-jans-monolith/Dockerfile
[warning] WARNING: No output specified with docker-container driver. Build result will only remain in the build cache.
docker-jans-persistence-loader/Dockerfile
[warning] WARNING: No output specified with docker-container driver. Build result will only remain in the build cache.
docker-jans-all-in-one/Dockerfile
[warning] WARNING: No output specified with docker-container driver. Build result will only remain in the build cache.
docker-jans-config-api/Dockerfile
[warning] WARNING: No output specified with docker-container driver. Build result will only remain in the build cache.
docker-jans-auth-server/Dockerfile
[error] 5-5: Build failed: no match for platform in manifest for maven:3.9-eclipse-temurin-17-alpine. Ensure the base image supports the target platform (linux/amd64, linux/arm64).
[error] 5-5: FROM maven:3.9-eclipse-temurin-17-alpine failed to resolve source metadata for the target platform.
[warning] WARNING: No output specified with docker-container driver. Build result will only remain in the build cache.
docker-jans-scim/Dockerfile
[warning] WARNING: No output specified with docker-container driver. Build result will only remain in the build cache.
docker-jans-casa/Dockerfile
[warning] WARNING: No output specified with docker-container driver. Build result will only remain in the build cache.
docker-jans-configurator/Dockerfile
[warning] WARNING: No output specified with docker-container driver. Build result will only remain in the build cache.
docker-jans-fido2/Dockerfile
[warning] WARNING: No output specified with docker-container driver. Build result will only remain in the build cache.
docker-jans-link/Dockerfile
[warning] WARNING: No output specified with docker-container driver. Build result will only remain in the build cache.
🪛 Hadolint (2.14.0)
docker-jans-config-api/Dockerfile
[warning] 9-9: Pin versions in apk add. Instead of apk add <package> use apk add <package>=<version>
(DL3018)
docker-jans-auth-server/Dockerfile
[warning] 9-9: Pin versions in apk add. Instead of apk add <package> use apk add <package>=<version>
(DL3018)
docker-jans-scim/Dockerfile
[warning] 9-9: Pin versions in apk add. Instead of apk add <package> use apk add <package>=<version>
(DL3018)
docker-jans-casa/Dockerfile
[warning] 9-9: Pin versions in apk add. Instead of apk add <package> use apk add <package>=<version>
(DL3018)
docker-jans-fido2/Dockerfile
[warning] 9-9: Pin versions in apk add. Instead of apk add <package> use apk add <package>=<version>
(DL3018)
docker-jans-link/Dockerfile
[warning] 9-9: Pin versions in apk add. Instead of apk add <package> use apk add <package>=<version>
(DL3018)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: sonar scan (jans-link)
- GitHub Check: sonar scan (jans-scim)
- GitHub Check: sonar scan (jans-cli-tui)
- GitHub Check: sonar scan (jans-pycloudlib)
- GitHub Check: sonar scan (jans-keycloak-link)
- GitHub Check: sonar scan (jans-linux-setup)
- GitHub Check: sonar scan (jans-auth-server)
- GitHub Check: sonar scan (jans-config-api)
- GitHub Check: sonar scan (jans-core)
- GitHub Check: golang_tests
- GitHub Check: python_tests (3.11)
- GitHub Check: python_tests (3.10)
- GitHub Check: rust_benchmarks
- GitHub Check: wasm_tests
- GitHub Check: rust_tests
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (go)
🔇 Additional comments (16)
docker-jans-monolith/Dockerfile (1)
44-44: Approve version update aligned with cloud auth proxy support.The JANS_SOURCE_VERSION update to
9489a963cde645364b18127548244925f6c7fb43is consistent with widespread version bumps across multiple Dockerfiles in this PR and corresponds to the cloud auth proxy feature additions.docker-jans-monolith/scripts/entrypoint.sh (5)
3-3: Excellent addition ofset -o pipefail.This complements the existing
set -eand ensures pipe failures are properly caught during script execution, improving error detection in compound commands.
25-50: Past critical issue resolved: TCP port check now uses/dev/tcpcorrectly.The implementation of
wait_for_rdbms()addresses the previously flagged critical issues:✓ Variable quoting:
${RDBMS_HOST}and${port}are properly expanded in the outer shell context before being passed tobash -c.
✓ Correct tool for TCP connectivity: Uses bash's/dev/tcpfile descriptor mechanism instead of curl, which is appropriate for raw TCP ports (MySQL, PostgreSQL).
✓ Exit handling: Returns 0 when no RDBMS is configured, allowing graceful fallthrough.
✓ Timeout safeguard: Each attempt is bounded to 1 second, preventing indefinite hangs on unresponsive hosts.
✓ Defensive validation: Checks thatRDBMS_HOSTis set before attempting connection (lines 28–31).The logic correctly detects the database type and port based on
CN_INSTALL_MYSQLandCN_INSTALL_PGSQLenvironment variables.
102-111: Robust error handling forinstall.pyexecution.The addition of explicit exit code capture and detailed error reporting significantly improves debuggability:
- Captures the exit code before shell options intervene (line 103).
- Provides actionable error message directing users to
/setup_logfor details (line 108).- Preserves the original exit code when re-exiting (line 110), maintaining proper error semantics.
This prevents silent failures and makes troubleshooting container initialization issues much easier.
120-122: Correct conditional integration of RDBMS readiness check.Calling
wait_for_rdbmsonly when RDBMS is configured (lines 120–122) matches the internal logic of thewait_for_rdbms()function itself (lines 34–43), ensuring consistency and preventing unnecessary execution.
236-238: Improved service lifecycle messaging.Replacing the blocking
tail -Fwith explicit completion messages and guidance for accessing logs viadocker-composeis more appropriate for containerized environments and provides clearer handoff to systemd-managed services.docker-jans-all-in-one/Dockerfile (1)
62-62: Approve: Asset version bump to pull Cloud Auth Proxy support.The version bump correctly targets the new commit that includes Cloud Auth Proxy support in jans-pycloudlib. The sparse-checkout scope (line 71) is appropriately narrowed to pull only the pycloudlib assets, reducing surface area and build time.
docker-jans-persistence-loader/Dockerfile (2)
21-21: Note: Verify Cloud SQL Socket Factory JAR addition in build stages.Per PR objectives, the Java-side connectivity requires adding the Cloud SQL JDBC Socket Factory JAR to images. Confirm this Dockerfile (or related build stages) includes the necessary steps to fetch and embed this JAR, as it is not visible in the current version update.
21-21: Verify the commit hash contains the expected Cloud SQL Connector changes.The JANS_SOURCE_VERSION update uses a valid 40-character commit hash format. However, confirm that commit
9489a963cde645364b18127548244925f6c7fb43includes the CloudSqlConnectorMixin and Cloud SQL Python Connector support additions mentioned in the PR objectives, as this cannot be independently verified from public sources.docker-jans-auth-server/Dockerfile (2)
90-116: ✅ Solid error handling and intentional lib separation.The use of
set -efor strict error handling, explicit|| exit 1on wget loops, and separation ofcustom/libs(third-party and extension JARs) from_libs(jans-lock JARs) follows best practices. The COPY from cloudsql-builder will fail the build if no JARs are produced, which is the correct behavior.
128-335: ✅ Version update and permissions align with PR pattern.The JANS_SOURCE_VERSION bump to 9489a96 is consistent across all modified Dockerfiles, and ownership/permissions for custom/libs (lines 328–329) correctly ensure the non-root jetty user can access the Cloud SQL JARs at runtime.
docker-jans-casa/Dockerfile (1)
84-238: ✅ Consistent with PR pattern across services.The JANS_SOURCE_VERSION update and custom/libs handling (mkdir, COPY from builder, ownership/permissions) follow the same pattern as other services and correctly prepare the Cloud SQL JARs for the non-root jetty user.
docker-jans-scim/Dockerfile (1)
85-263: ✅ Consistent custom/libs pattern and version alignment.JANS_SOURCE_VERSION update and custom/libs creation, copy, and permissions follow the established pattern across all services in the PR.
docker-jans-config-api/Dockerfile (1)
97-307: ✅ Pattern consistency maintained.Version update and custom/libs handling (lines 263–264, 306–307) align with the established pattern across all modified Dockerfiles in the PR.
docker-jans-fido2/Dockerfile (1)
90-270: ✅ Consistent with PR-wide pattern.JANS_SOURCE_VERSION update to 9489a96 and custom/libs handling (mkdir, COPY, chown, chmod) follow the same pattern as other services.
docker-jans-link/Dockerfile (1)
86-267: ✅ Pattern consistency and version alignment confirmed.JANS_SOURCE_VERSION and custom/libs handling (mkdir, COPY from cloudsql-builder, ownership/permissions) are consistent across all six modified Dockerfiles in the PR.
| # ================================= | ||
| # Cloud SQL JDBC Socket Factory Builder | ||
| # ================================= | ||
| # Builds fat JARs with all dependencies for Cloud SQL connectivity | ||
| FROM maven:3.9-eclipse-temurin-17-alpine AS cloudsql-builder | ||
|
|
||
| ARG CLOUDSQL_SOCKET_FACTORY_VERSION=1.27.0 | ||
|
|
||
| RUN apk add --no-cache git \ | ||
| && git clone --depth 1 --branch v${CLOUDSQL_SOCKET_FACTORY_VERSION} \ | ||
| https://github.com/GoogleCloudPlatform/cloud-sql-jdbc-socket-factory.git /build | ||
|
|
||
| WORKDIR /build | ||
|
|
||
| # Build fat JARs with all dependencies bundled | ||
| RUN mvn -P jar-with-dependencies clean package -DskipTests -q \ | ||
| && mkdir -p /jars \ | ||
| && cp jdbc/mysql-j-8/target/mysql-socket-factory-connector-j-8-*-jar-with-dependencies.jar /jars/ \ | ||
| && cp jdbc/postgres/target/postgres-socket-factory-*-jar-with-dependencies.jar /jars/ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check Docker Hub API for maven:3.9-eclipse-temurin-17-alpine availability
curl -s https://registry.hub.docker.com/v2/library/maven/manifests/3.9-eclipse-temurin-17-alpine 2>&1 | head -100Repository: JanssenProject/jans
Length of output: 218
🏁 Script executed:
# Also check the simpler maven:3.9-alpine variant
curl -s https://registry.hub.docker.com/v2/library/maven/manifests/3.9-alpine 2>&1 | head -100Repository: JanssenProject/jans
Length of output: 218
🌐 Web query:
maven docker image 3.9-eclipse-temurin-17-alpine supported platforms architectures
💡 Result:
The tag maven:3.9-eclipse-temurin-17-alpine is published for linux/amd64 only. [1][2]
Sources:
- Docker image listing showing OS/platform: linux/amd64. [1]
- Official docker-maven repository (image variants / Alpine-based images note). [2]
🌐 Web query:
maven:3.9-alpine docker image supported architectures linux/amd64 arm64
💡 Result:
Short answer: Not reliably — the official maven:3.9-alpine family is primarily amd64 by default; some specific alpine-based tags have multi‑arch manifests (including linux/arm64) but many alpine tags are amd64-only. Check the tag manifest for the exact platforms before using it.
How to verify yourself:
- Inspect the tag’s manifest (shows platforms): docker manifest inspect --verbose maven:3.9-alpine
- Or with buildx imagetools: docker buildx imagetools inspect maven:3.9-alpine
Sources: Docker Hub tag metadata (example tag showing Architecture: amd64) and the docker-maven GitHub / community notes about platform availability. [1][2].
🌐 Web query:
eclipse-temurin:17-alpine docker image supported architectures linux/amd64 arm64
💡 Result:
Short answer: The eclipse-temurin:17-alpine images are provided for linux/amd64 only — they do not include linux/arm64 in the published manifest. [1][2][3]
Sources:
- example mplatform/mquery output / explanation showing the -alpine tag only lists linux/amd64. [1]
- StackOverflow answer noting the alpine temurin tags are x86-only and recommending non‑alpine temurin images for arm64. [2]
- Adoptium/containers GitHub issue discussing missing arm64 manifest for -alpine tags. [3]
🌐 Web query:
maven:3.9 official docker image multi-architecture support arm64 amd64 alternatives
💡 Result:
Short answer: the official library/maven 3.9 tags are available as multi‑arch images (amd64 + arm64) in practice; if you need other variants or stricter small/minimal images, use one of the community multi‑arch alternatives or build your own with docker buildx.
Sources / options:
- Official Docker Maven sources / image repo (carlossg/docker-maven) — official image builds and multi‑platform workflow. [1]
- Chainguard-maintained maven multi‑arch images (amd64 + arm64) — small, security-focused alternative. [2]
- Docker docs on building/inspecting multi‑platform images (docker buildx, QEMU emulation) — how to build or verify multi‑arch images. [3]
Practical alternatives:
- ghcr.io/carlossg/maven or docker.io/library/maven (official) — official builds/tags (check tag manifest with docker manifest inspect or docker buildx imagetools inspect). [1]
- cgr.dev/chainguard/maven — minimal/secure multi‑arch images. [2]
- alpine‑maven / woahbase/alpine-maven — smaller Alpine-based multi‑arch community images. [4]
- Build your own multi‑arch maven image with docker buildx --platform linux/amd64,linux/arm64 (use QEMU or native nodes). [3]
How to verify on your host:
- docker buildx imagetools inspect maven:3.9
- docker manifest inspect maven:3.9
(Use the above to see the exact platforms/manifests for the specific 3.9 tag you plan to use.)
Sources:
[1] GitHub carlossg/docker-maven (official image repo).
[2] Chainguard maven image directory (shows amd64 + arm64 variants).
[3] Docker docs — Multi-platform builds (buildx / QEMU / verification).
[4] alpine-maven (community Alpine-based multi‑arch image).
⛔ Critical: cloudsql-builder base image does not support linux/arm64.
The maven:3.9-eclipse-temurin-17-alpine image is available for linux/amd64 only and lacks linux/arm64 support. This blocks multi-architecture builds targeting arm64. The suggested alternative maven:3.9-alpine has the same limitation.
Use the official multi-architecture Maven image instead:
- FROM maven:3.9-eclipse-temurin-17-alpine AS cloudsql-builder
+ FROM maven:3.9 AS cloudsql-builderThe maven:3.9 tag supports both linux/amd64 and linux/arm64. If a smaller image is required, evaluate cgr.dev/chainguard/maven:latest or build a custom multi-arch image with docker buildx.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Checkov (3.2.334)
[low] 1-342: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
🪛 GitHub Actions: Build and Publish Releases to Hub
[error] 5-5: Build failed: no match for platform in manifest for maven:3.9-eclipse-temurin-17-alpine. Ensure the base image supports the target platform (linux/amd64, linux/arm64).
[error] 5-5: FROM maven:3.9-eclipse-temurin-17-alpine failed to resolve source metadata for the target platform.
[warning] WARNING: No output specified with docker-container driver. Build result will only remain in the build cache.
🪛 Hadolint (2.14.0)
[warning] 9-9: Pin versions in apk add. Instead of apk add <package> use apk add <package>=<version>
(DL3018)
🤖 Prompt for AI Agents
In docker-jans-auth-server/Dockerfile around lines 1 to 19 the cloudsql-builder
base image maven:3.9-eclipse-temurin-17-alpine is amd64-only and breaks
multi-arch (arm64) builds; replace it with a multi-architecture image such as
maven:3.9 (official multi-arch) or a minimal multi-arch alternative like
cgr.dev/chainguard/maven:latest, update the FROM line accordingly, and verify
the multi-arch build using docker buildx (or document the chosen image rationale
if size/security tradeoffs require a custom multi-arch image).
| # =========== | ||
|
|
||
| ENV JANS_SOURCE_VERSION=04d297c046dd9d8ef54c0544e6187153b7ed87cf | ||
| ENV JANS_SOURCE_VERSION=9489a963cde645364b18127548244925f6c7fb43 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the JANS_SOURCE_VERSION commit hash
COMMIT_HASH="9489a963cde645364b18127548244925f6c7fb43"
REPO_URL="https://github.com/janssenproject/jans"
# Check if commit exists using git ls-remote (doesn't require cloning)
echo "=== Checking for commit hash in jans repository ==="
git ls-remote "$REPO_URL" | grep -i "$COMMIT_HASH" || echo "Not found in refs"
# Try to fetch more information about the commit
echo ""
echo "=== Attempting to fetch commit info ==="
# Use git fetch to get commit information without full clone
git fetch --depth=50 "$REPO_URL" "$COMMIT_HASH" 2>&1 | head -20 || echo "Could not fetch directly"
# Alternative: check if this is a valid SHA-1 format (40 hex chars)
echo ""
echo "=== Validating hash format ==="
if [[ $COMMIT_HASH =~ ^[0-9a-f]{40}$ ]]; then
echo "✓ Valid SHA-1 format: $COMMIT_HASH"
else
echo "✗ Invalid SHA-1 format"
fiRepository: JanssenProject/jans
Length of output: 390
Commit hash verified; consider adding an explanatory comment for future maintainers.
The JANS_SOURCE_VERSION commit hash 9489a963cde645364b18127548244925f6c7fb43 is valid and exists in the jans repository. To improve clarity for future maintainers, consider adding an inline comment explaining the reason for this version bump (e.g., # Updated to include Cloud SQL Connector support).
🧰 Tools
🪛 GitHub Actions: Build and Publish Releases to Hub
[warning] WARNING: No output specified with docker-container driver. Build result will only remain in the build cache.
🤖 Prompt for AI Agents
In docker-jans-configurator/Dockerfile around line 30, the ENV
JANS_SOURCE_VERSION is set to a specific commit hash but lacks context; add a
concise inline comment immediately above or at end of that ENV line explaining
why this particular commit was chosen (for example: "# Updated to include Cloud
SQL Connector support" or the actual changelog/issue reference and date/PR
number), so future maintainers understand the reason for the version bump.
Signed-off-by: moabu <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
docker-jans-config-api/Dockerfile (1)
1-20: ⛔ Critical: cloudsql-builder base image does not support linux/arm64 architecture.Same issue as docker-jans-fido2:
maven:3.9-eclipse-temurin-17is amd64-only. Replace withmaven:3.9to enable multi-architecture builds targeting linux/arm64.[request_verification, raise_critical_issue]
docker-jans-scim/Dockerfile (1)
1-20: ⛔ Critical: cloudsql-builder base image does not support linux/arm64 architecture.Same multi-arch blocker:
maven:3.9-eclipse-temurin-17is amd64-only. Replace withmaven:3.9for linux/arm64 support.[request_verification, raise_critical_issue]
docker-jans-auth-server/Dockerfile (1)
1-20: ⛔ Critical: cloudsql-builder base image does not support linux/arm64 architecture.Same multi-arch blocker across all Dockerfiles:
maven:3.9-eclipse-temurin-17is amd64-only. Replace withmaven:3.9to support multi-platform builds including linux/arm64.[request_verification, raise_critical_issue]
docker-jans-link/Dockerfile (1)
1-20: ⛔ Critical: cloudsql-builder base image does not support linux/arm64 architecture.Same multi-arch limitation:
maven:3.9-eclipse-temurin-17is amd64-only. Replace withmaven:3.9to enable linux/arm64 builds.[request_verification, raise_critical_issue]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
docker-jans-auth-server/Dockerfile(4 hunks)docker-jans-casa/Dockerfile(4 hunks)docker-jans-config-api/Dockerfile(4 hunks)docker-jans-fido2/Dockerfile(4 hunks)docker-jans-link/Dockerfile(4 hunks)docker-jans-scim/Dockerfile(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-10T17:38:39.610Z
Learnt from: iromli
Repo: JanssenProject/jans PR: 12679
File: docker-jans-config-api/Dockerfile:120-120
Timestamp: 2025-11-10T17:38:39.610Z
Learning: In docker-jans-config-api/Dockerfile, the sed command that replaces 'your-openid-provider.server' with '%(hostname)s' in the adminui-policy-store.json file is intentional. The %(hostname)s variable uses Python string interpolation format and is consumed by scripts executed during container runtime.
Applied to files:
docker-jans-casa/Dockerfiledocker-jans-config-api/Dockerfiledocker-jans-fido2/Dockerfile
📚 Learning: 2025-11-26T12:38:52.165Z
Learnt from: misba7
Repo: JanssenProject/jans PR: 12737
File: automation/mysql.yaml:22-59
Timestamp: 2025-11-26T12:38:52.165Z
Learning: In the Janssen project, Kubernetes manifests in the automation/ directory (such as mysql.yaml and pgsql.yaml) are for demo and testing purposes only. These manifests do not require production-level hardening (security contexts, resource limits, health checks, etc.) as they are intended for local development and demonstration scenarios, not production deployments.
Applied to files:
docker-jans-casa/Dockerfile
🪛 Checkov (3.2.334)
docker-jans-casa/Dockerfile
[low] 1-286: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
docker-jans-scim/Dockerfile
[low] 1-276: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
docker-jans-link/Dockerfile
[low] 1-275: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
docker-jans-auth-server/Dockerfile
[low] 1-343: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
docker-jans-config-api/Dockerfile
[low] 1-321: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
docker-jans-fido2/Dockerfile
[low] 1-283: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
🪛 Hadolint (2.14.0)
docker-jans-casa/Dockerfile
[warning] 9-9: Pin versions in apt get install. Instead of apt-get install <package> use apt-get install <package>=<version>
(DL3008)
docker-jans-scim/Dockerfile
[warning] 9-9: Pin versions in apt get install. Instead of apt-get install <package> use apt-get install <package>=<version>
(DL3008)
docker-jans-link/Dockerfile
[warning] 9-9: Pin versions in apt get install. Instead of apt-get install <package> use apt-get install <package>=<version>
(DL3008)
docker-jans-auth-server/Dockerfile
[warning] 9-9: Pin versions in apt get install. Instead of apt-get install <package> use apt-get install <package>=<version>
(DL3008)
docker-jans-config-api/Dockerfile
[warning] 9-9: Pin versions in apt get install. Instead of apt-get install <package> use apt-get install <package>=<version>
(DL3008)
docker-jans-fido2/Dockerfile
[warning] 9-9: Pin versions in apt get install. Instead of apt-get install <package> use apt-get install <package>=<version>
(DL3008)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: docker (auth-server)
- GitHub Check: sonar scan (jans-keycloak-link)
- GitHub Check: sonar scan (jans-config-api)
- GitHub Check: golang_tests
- GitHub Check: docker (scim)
- GitHub Check: docker (fido2)
- GitHub Check: rust_benchmarks
- GitHub Check: python_tests (3.11)
- GitHub Check: docker (monolith)
- GitHub Check: python_tests (3.10)
- GitHub Check: docker (persistence-loader)
- GitHub Check: docker (configurator)
- GitHub Check: docker (config-api)
- GitHub Check: wasm_tests
- GitHub Check: docker (cloudtools)
- GitHub Check: rust_tests
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (go)
🔇 Additional comments (14)
docker-jans-fido2/Dockerfile (3)
1-20: ⛔ Critical: cloudsql-builder base image does not support linux/arm64 architecture.The
maven:3.9-eclipse-temurin-17image is available for linux/amd64 only and lacks linux/arm64 support. This blocks multi-architecture Docker builds. The same issue was flagged in previous reviews and remains unresolved across all five Dockerfiles in this PR.To fix, replace the base image with a multi-architecture alternative:
- FROM maven:3.9-eclipse-temurin-17 AS cloudsql-builder + FROM maven:3.9 AS cloudsql-builderThe
maven:3.9tag supports both linux/amd64 and linux/arm64. If a smaller image is essential, considercgr.dev/chainguard/maven:latest(multi-arch, security-focused) as an alternative. Verify the selected image withdocker buildx imagetools inspect <image>:<tag>to confirm platform coverage.[request_verification, raise_critical_issue]
236-242: Custom/libs directory integration looks good.The custom/libs directory is properly created, populated with Cloud SQL fat JARs via COPY from the builder stage, and given appropriate permissions in the subsequent chown/chmod block (lines 270–271). This integration is correct and complete.
91-91: JANS_SOURCE_VERSION update is correct.The asset snapshot has been properly updated to
9489a963cde645364b18127548244925f6c7fb43in line with the Cloud SQL Connector support. This is consistent across all modified images.docker-jans-config-api/Dockerfile (2)
259-265: Custom/libs directory integration is complete and properly secured.The directory is created (line 264), populated with Cloud SQL JARs via COPY (line 265), and ownership/permissions are correctly set (lines 307–308). This matches the pattern across all other services and is properly implemented.
98-98: JANS_SOURCE_VERSION correctly updated.The snapshot reference aligns with the Cloud SQL Connector support introduced in this PR.
docker-jans-scim/Dockerfile (2)
230-236: Custom/libs directory handling is correct and consistent.The directory is properly created, populated with Cloud SQL fat JARs, and ownership/permissions are set correctly (lines 263–264). This aligns with the implementation across all services.
86-86: JANS_SOURCE_VERSION correctly updated.Snapshot reference is in sync with other services for Cloud SQL Connector support.
docker-jans-auth-server/Dockerfile (3)
88-117: Error handling for JAR downloads is now robust.The addition of
set -eand|| exit 1(lines 103, 108–109, 113, 116) ensures that any failed wget aborts the build immediately. This prevents the image from being built with partially populatedcustom/libsor_libsdirectories. This is a solid improvement over the prior pattern.
329-330: Custom/libs directory ownership and permissions are correctly set.Lines 329–330 properly assign ownership and group-user permissions for the custom/libs directory, matching the pattern applied to
common/libs(line 328). The related_libsdirectory also receives proper ownership (line 335). This is consistent and complete.
130-130: JANS_SOURCE_VERSION correctly updated for Cloud SQL Connector support.Snapshot reference is aligned with other services.
docker-jans-link/Dockerfile (2)
227-233: Custom/libs directory is properly integrated and secured.Directory creation (line 232), JAR population via COPY (line 233), and ownership/permissions (lines 261–262) are all correctly implemented. This follows the established pattern across all services.
87-87: JANS_SOURCE_VERSION correctly updated.Snapshot reference is in sync with Cloud SQL Connector support across all services.
docker-jans-casa/Dockerfile (2)
233-239: Cloud SQL JAR integration is correctly implemented.The custom/libs directory is properly created, populated with Cloud SQL socket factory JARs from the builder stage, and permissions are correctly applied (lines 273–274). This follows the pattern established in other Dockerfiles per the PR changes and aligns with the Jetty configuration documented in mod_context.py.
273-274: Ownership and permissions for custom/libs are correctly applied.The non-root user (uid 1000) now has appropriate access to the custom/libs directory via
chown -R 1000:0andchmod -R g=u, mirroring the pattern used for other library directories and addressing prior permission concerns.
| # ================================= | ||
| # Cloud SQL JDBC Socket Factory Builder | ||
| # ================================= | ||
| # Builds fat JARs with all dependencies for Cloud SQL connectivity | ||
| FROM maven:3.9-eclipse-temurin-17 AS cloudsql-builder | ||
|
|
||
| ARG CLOUDSQL_SOCKET_FACTORY_VERSION=1.27.0 | ||
|
|
||
| RUN apt-get update && apt-get install -y --no-install-recommends git \ | ||
| && rm -rf /var/lib/apt/lists/* \ | ||
| && git clone --depth 1 --branch v${CLOUDSQL_SOCKET_FACTORY_VERSION} \ | ||
| https://github.com/GoogleCloudPlatform/cloud-sql-jdbc-socket-factory.git /build | ||
|
|
||
| WORKDIR /build | ||
|
|
||
| # Build fat JARs with all dependencies bundled | ||
| RUN mvn -P jar-with-dependencies clean package -DskipTests -q \ | ||
| && mkdir -p /jars \ | ||
| && cp jdbc/mysql-j-8/target/mysql-socket-factory-connector-j-8-*-jar-with-dependencies.jar /jars/ \ | ||
| && cp jdbc/postgres/target/postgres-socket-factory-*-jar-with-dependencies.jar /jars/ |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
find . -name "Dockerfile" -path "*/docker-jans-casa/*" | head -20Repository: JanssenProject/jans
Length of output: 92
🏁 Script executed:
head -30 docker-jans-casa/DockerfileRepository: JanssenProject/jans
Length of output: 1215
🌐 Web query:
Docker multi-stage build digest pinning best practices reproducibility
💡 Result:
Concise best-practices for digest-pinning in Docker multi-stage builds to maximise reproducibility and supply-chain safety:
-
Pin every external FROM (and COPY --from=imagename) to an exact digest (image@sha256:...), not just a tag. This guarantees identical fetched content across builds. [1][2][6]
-
Also digest-pin any external stage images used via COPY --from=
(not only stages defined earlier in the same Dockerfile). [2][6]
-
Use immutable, specific tags when digests aren’t practical, but prefer digests for production images. Avoid floating tags like :latest or broad minor tags. [1][4]
-
Automate safe updates: use CI tools that detect new digests and open PRs (or use dependabot-like flows) so you intentionally roll forward pinned digests rather than leaving builds drifting. [1][6]
-
Reproducible-build settings: set SOURCE_DATE_EPOCH and stable build metadata (fixed labels, deterministic timestamps) in CI to make image contents/digests reproducible across runs. Follow reproducible-build guidance in your pipeline. [5]
-
Maintain a controlled image source: mirror critical base images in an internal registry or use a private cache/mirror to avoid external tag re-writes and to ensure availability. [4]
-
Verify integrity at pull time: combine digest pinning with image signing/verifying (Cosign/Docker Content Trust/OCI signatures) and enforce via CI/admission rules. [3][4]
-
Record provenance and generate SBOMs and attestations for each pinned digest (store image digest, source, build commit, builder versions) to enable audits and rollbacks. [5][1]
-
Enforce policy: add linters/CI checks or admission controls to reject unpinned images in production manifests (Dockerfiles, Compose, Kubernetes, Helm). [3][6]
-
Handle multi-arch carefully: pin manifest list digests if you need a specific arch or rely on platform= selection in BuildKit and record the resolved manifest digest used in the build. [2][1]
References
- Dockerfile best practices / multi-stage builds (pin by digest). [1][2]
- Snyk: prefer fixed tags / immutability recommendations. [4]
- Supply-chain risk from unpinned tags and policy/automation guidance. [3][6]
- Reproducible-build CI techniques (SOURCE_DATE_EPOCH, deterministic metadata). [5]
Pin the builder stage image to a digest for consistency and reproducibility.
The main image (line 25) uses a pinned digest, but the builder stage (line 5) does not. This inconsistency means the builder may produce different JAR artifacts if the maven:3.9-eclipse-temurin-17 image is updated. Pin the builder stage to match:
-FROM maven:3.9-eclipse-temurin-17 AS cloudsql-builder
+FROM maven:3.9-eclipse-temurin-17@sha256:<digest> AS cloudsql-builderCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Checkov (3.2.334)
[low] 1-286: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
🪛 Hadolint (2.14.0)
[warning] 9-9: Pin versions in apt get install. Instead of apt-get install <package> use apt-get install <package>=<version>
(DL3008)
🤖 Prompt for AI Agents
In docker-jans-casa/Dockerfile around lines 1 to 20, the builder stage FROM
maven:3.9-eclipse-temurin-17 is not pinned to a digest which can cause
non-reproducible builds; replace the tag-only reference with the image pinned to
a specific digest (maven:3.9-eclipse-temurin-17@sha256:<digest>) by fetching the
canonical digest (e.g., docker pull maven:3.9-eclipse-temurin-17 && docker
inspect --format='{{index .RepoDigests 0}}' maven:3.9-eclipse-temurin-17 or use
`docker manifest inspect`), then update the Dockerfile to use that digest so the
builder stage is immutable and consistent with the main image.
|
|
|
Signed-off-by: moabu <[email protected]>
Signed-off-by: moabu <[email protected]>
Prepare
Description
Target issue
closes #12787
Implementation Details
Test and Document the changes
Please check the below before submitting your PR. The PR will not be merged if there are no commits that start with
docs:to indicate documentation changes or if the below checklist is not selected.Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.