Conversation
There was a problem hiding this comment.
Pull request overview
This PR focuses on improving Java 17 build/runtime compatibility and tightening JMX defaults/security-related configuration.
Changes:
- Update build tooling plugin versions to run on JDK 17 (Felix bundle plugin, Maven WAR plugin).
- Adjust distribution/runtime artifacts for JDK 17 builds (launcher change; AMQP transport excluded from distribution build).
- Add support for configuring a remote JMX deserialization filter pattern and disable JMX by default in the shipped properties.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| repository/conf/synapse.properties | Disables JMX by default and introduces a configurable JMX deserialization filter pattern property. |
| pom.xml | Bumps build plugins to versions compatible with running Maven on JDK 17. |
| modules/distribution/src/main/bin/synapse.sh | Avoids passing the removed -Djava.endorsed.dirs option on newer JVMs by making it conditional. |
| modules/distribution/pom.xml | Comments out AMQP transport dependency to allow packaging when AMQP module is skipped under JDK 17. |
| modules/core/src/test/java/org/apache/synapse/JmxAdapterContextMapTest.java | Adds tests asserting the serial filter pattern is not placed into the JMX connector environment map. |
| modules/core/src/main/java/org/apache/synapse/jmx/JmxSerializationFilterSupport.java | Adds a helper to install a JVM-wide serial filter factory intended to constrain JMX/RMI deserialization. |
| modules/core/src/main/java/org/apache/synapse/JmxAdapter.java | Wires the serial filter configuration into JMX adapter startup. |
| modules/commons/src/test/java/org/apache/synapse/commons/jmx/JmxInformationFactoryTest.java | Adds tests for parsing the new serial filter pattern property and default JMX disabled behavior. |
| modules/commons/src/main/java/org/apache/synapse/commons/jmx/JmxInformationFactory.java | Parses synapse.jmx.remote.serial.filter.pattern into JmxInformation. |
| modules/commons/src/main/java/org/apache/synapse/commons/jmx/JmxInformation.java | Adds storage/accessors for the new serial filter pattern configuration. |
| modules/commons/src/main/java/org/apache/synapse/commons/jmx/JmxConfigurationConstants.java | Adds constants for the new property and the standard JMX env key name. |
| docs/JDK17-build-report.md | Documents iterative steps and workarounds used to produce a JDK 17 build. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import java.io.ObjectInputFilter; | ||
| import java.util.function.BinaryOperator; |
There was a problem hiding this comment.
This class imports and uses java.io.ObjectInputFilter, which is only available starting in Java 9. The build is currently configured for Java 8 (pom.xml sets <java.version>1.8</java.version>), so compiling/running on Java 8 will fail. Either raise the project bytecode/runtime baseline to 9+ (and update docs/scripts accordingly) or gate this code behind reflection / multi-release JAR / separate module so Java 8 builds remain compatible.
| BinaryOperator<ObjectInputFilter> factory = (curr, next) -> { | ||
| ObjectInputFilter base = next != null ? next : curr; | ||
| if (base == null) { | ||
| base = ObjectInputFilter.Config.getSerialFilter(); | ||
| } | ||
| if (!isJmxRemoteDeserializationStack()) { | ||
| return base; | ||
| } | ||
| return ObjectInputFilter.merge(jmxFilter, base); | ||
| }; | ||
| ObjectInputFilter.Config.setSerialFilterFactory(factory); |
There was a problem hiding this comment.
ObjectInputFilter.Config.setSerialFilterFactory(...) installs a JVM-wide factory; the implementation calls Thread.currentThread().getStackTrace() on every deserialization to decide whether to merge filters. That can add significant overhead to all deserialization in the process when this feature is enabled. Consider switching to StackWalker for a lighter check and/or narrowing the condition so the expensive stack inspection is avoided unless there’s strong evidence the call is coming from RMI/JMX.
| // not via jmx.remote.rmi.server.serial.filter.pattern in the connector env. | ||
| JmxSerializationFilterSupport.configureForJmxPattern(remoteSerialFilterPattern, log); | ||
| if (log.isDebugEnabled()) { | ||
| log.debug("Configured JEP 290 deserialization filter for remote JMX: " |
There was a problem hiding this comment.
The debug log message says "Configured JEP 290 deserialization filter" but the surrounding comments and JmxSerializationFilterSupport are explicitly using the JEP 415 serial filter factory mechanism. Please align the wording (e.g., reference JEP 415, or "JEP 290/415") to avoid confusion when debugging.
| log.debug("Configured JEP 290 deserialization filter for remote JMX: " | |
| log.debug("Configured JEP 415 deserialization filter factory for remote JMX: " |
| <!-- Skipped in JDK 17 full build: synapse-amqp-transport is Java 6 source/target; see docs/JDK17-build-report.md --> | ||
| <!--dependency> | ||
| <groupId>org.apache.synapse</groupId> | ||
| <artifactId>synapse-amqp-transport</artifactId> | ||
| </dependency> | ||
| </dependency--> |
There was a problem hiding this comment.
This dependency is now commented out unconditionally, which means the produced distribution will no longer include AMQP transport even for builds that do compile it. If the intent is only to support JDK 17 builds that exclude the module, consider gating this via a Maven profile/property (e.g., -Pjdk17 or -DskipAmqpTransport) instead of permanently removing it from the distribution POM.
| **Result:** Progressed past securevault; **FAILURE** at `synapse-amqp-transport`. | ||
|
|
There was a problem hiding this comment.
The report states "Synapse targets Java 17+", but the repository build configuration still targets Java 8 (pom.xml has <java.version>1.8</java.version>) and synapse.sh warns it’s tested up to Java 8. Please correct the report to match the actual supported baseline, or update the build baseline consistently across the project if Java 17+ is now the goal.
No description provided.