You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "snuyanzin (via GitHub)" <gi...@apache.org> on 2023/10/18 22:13:02 UTC
[PR] [FLINK-33309] Add `-Djava.security.manager=allow` [flink]
snuyanzin opened a new pull request, #23547:
URL: https://github.com/apache/flink/pull/23547
## What is the purpose of the change
Since because of JEP411 [1] starting from java 18 default value of `java.security.manager` will be disallow the PR adds allow optio where it is needed
## Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): ( no)
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: ( no)
- The serializers: ( no)
- The runtime per-record code paths (performance sensitive): ( no )
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: ( no )
- The S3 file system connector: ( no )
## Documentation
- Does this pull request introduce a new feature? ( no)
- If yes, how is the feature documented? (not applicable )
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33309] Add `-Djava.security.manager=allow` [flink]
Posted by "flinkbot (via GitHub)" <gi...@apache.org>.
flinkbot commented on PR #23547:
URL: https://github.com/apache/flink/pull/23547#issuecomment-1769481551
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "8dc5eda4eefe7102af4b4c546795ee24ec3ddea1",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "8dc5eda4eefe7102af4b4c546795ee24ec3ddea1",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 8dc5eda4eefe7102af4b4c546795ee24ec3ddea1 UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run azure` re-run the last Azure build
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33309] Add `-Djava.security.manager=allow` [flink]
Posted by "zentol (via GitHub)" <gi...@apache.org>.
zentol commented on code in PR #23547:
URL: https://github.com/apache/flink/pull/23547#discussion_r1380568055
##########
flink-dist/src/main/flink-bin/bin/config.sh:
##########
@@ -334,6 +334,12 @@ if [ -z "${FLINK_ENV_JAVA_OPTS}" ]; then
# Remove leading and ending double quotes (if present) of value
FLINK_ENV_JAVA_OPTS="-XX:+IgnoreUnrecognizedVMOptions $( echo "${FLINK_ENV_JAVA_OPTS}" | sed -e 's/^"//' -e 's/"$//' )"
+
+ JAVA_SPEC_VERSION=`"${JAVA_RUN}" -XshowSettings:properties 2>&1 | grep "java.specification.version" | cut -d "=" -f 2 | tr -d '[:space:]'`
+ if [[ $(echo "$JAVA_SPEC_VERSION > 17" | bc ) == "1" ]]; then
Review Comment:
bc isn't available in our CI image. any reason this cant use plain bash arithmetic?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33309] Add `-Djava.security.manager=allow` [flink]
Posted by "zentol (via GitHub)" <gi...@apache.org>.
zentol commented on code in PR #23547:
URL: https://github.com/apache/flink/pull/23547#discussion_r1370228773
##########
flink-core/pom.xml:
##########
@@ -34,7 +34,14 @@ under the License.
<packaging>jar</packaging>
<properties>
- <surefire.module.config> <!--
+ <!--
+ Two properties below should be merged once support for jdk8 and jdk11 is dropped.
+ The reason of extraction into a separate property with profile is that
+ specifying -Djava.security.manager=allow within ${surefire.module.config} leads to
+ hanging of surefire for jdk8 and jdk11
Review Comment:
Would actually be interesting to know what is going on, because according to the JDK docs null/allow should be equivalent on JDK17-.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33309] Add `-Djava.security.manager=allow` [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #23547:
URL: https://github.com/apache/flink/pull/23547#discussion_r1372361525
##########
flink-dist/src/main/flink-bin/bin/config.sh:
##########
@@ -334,6 +334,12 @@ if [ -z "${FLINK_ENV_JAVA_OPTS}" ]; then
# Remove leading and ending double quotes (if present) of value
FLINK_ENV_JAVA_OPTS="-XX:+IgnoreUnrecognizedVMOptions $( echo "${FLINK_ENV_JAVA_OPTS}" | sed -e 's/^"//' -e 's/"$//' )"
+
+ JAVA_SPEC_VERSION=`"${JAVA_RUN}" -XshowSettings:properties 2>&1 | grep "java.specification.version" | cut -d "=" -f 2 | tr -d '[:space:]'`
+ if [ "$JAVA_SPEC_VERSION" == "21" ]; then
+ # set security manager property to allow calls to System.setSecurityManager() at runtime
+ FLINK_ENV_JAVA_OPTS="$FLINK_ENV_JAVA_OPTS -Djava.security.manager=allow"
+ fi
Review Comment:
true, added condition if version > 17
i check across vendors
for adoptium it's present from th ebeginning https://github.com/adoptium/jdk/blob/d96f38b80c1606b54b9f3dbfe9717ab9653a0605/src/java.base/share/classes/java/lang/System.java#L745
same for Oracle https://docs.oracle.com/javase/8/docs/api/java/lang/System.html
also Corretto https://github.com/corretto/corretto-8/blob/8ffa94d858593e085f7729c65786da976b3c5985/jdk/src/share/classes/java/lang/System.java#L576
JetBrains https://github.com/JetBrains/JetBrainsRuntime/blob/d85d45d4c3a2aa91ca0a96ac7c51fed108aea784/src/java.base/share/classes/java/lang/System.java#L745
GraallVM https://github.com/oracle/graal/blob/b7efba369c9be8c76942495d731a4103c52ef4de/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/SystemPropertiesSupport.java#L71
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33309] Add `-Djava.security.manager=allow` [flink]
Posted by "zentol (via GitHub)" <gi...@apache.org>.
zentol commented on code in PR #23547:
URL: https://github.com/apache/flink/pull/23547#discussion_r1380568055
##########
flink-dist/src/main/flink-bin/bin/config.sh:
##########
@@ -334,6 +334,12 @@ if [ -z "${FLINK_ENV_JAVA_OPTS}" ]; then
# Remove leading and ending double quotes (if present) of value
FLINK_ENV_JAVA_OPTS="-XX:+IgnoreUnrecognizedVMOptions $( echo "${FLINK_ENV_JAVA_OPTS}" | sed -e 's/^"//' -e 's/"$//' )"
+
+ JAVA_SPEC_VERSION=`"${JAVA_RUN}" -XshowSettings:properties 2>&1 | grep "java.specification.version" | cut -d "=" -f 2 | tr -d '[:space:]'`
+ if [[ $(echo "$JAVA_SPEC_VERSION > 17" | bc ) == "1" ]]; then
Review Comment:
bc isn't available in our CI image. any reason this cant use bash arithmetic?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33309] Add `-Djava.security.manager=allow` [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on PR #23547:
URL: https://github.com/apache/flink/pull/23547#issuecomment-1776061785
@flinkbot run azure
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33309] Add `-Djava.security.manager=allow` [flink]
Posted by "zentol (via GitHub)" <gi...@apache.org>.
zentol commented on code in PR #23547:
URL: https://github.com/apache/flink/pull/23547#discussion_r1380565691
##########
flink-core/pom.xml:
##########
@@ -34,7 +34,14 @@ under the License.
<packaging>jar</packaging>
<properties>
- <surefire.module.config> <!--
+ <!--
+ Two properties below should be merged once support for jdk8 and jdk11 is dropped.
+ The reason of extraction into a separate property with profile is that
+ specifying -Djava.security.manager=allow within ${surefire.module.config} leads to
+ hanging of surefire for jdk8 and jdk11
Review Comment:
Well that's just stupid. 😮💨
##########
flink-core/pom.xml:
##########
@@ -34,7 +34,14 @@ under the License.
<packaging>jar</packaging>
<properties>
- <surefire.module.config> <!--
+ <!--
+ Two properties below should be merged once support for jdk8 and jdk11 is dropped.
+ The reason of extraction into a separate property with profile is that
+ specifying -Djava.security.manager=allow within ${surefire.module.config} leads to
+ hanging of surefire for jdk8 and jdk11
Review Comment:
Well that's just stupid. 😮💨
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33309] Add `-Djava.security.manager=allow` [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on PR #23547:
URL: https://github.com/apache/flink/pull/23547#issuecomment-1781460690
@flinkbot run azure
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33309] Add `-Djava.security.manager=allow` [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on PR #23547:
URL: https://github.com/apache/flink/pull/23547#issuecomment-1775042476
thanks, done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33309] Add `-Djava.security.manager=allow` [flink]
Posted by "zentol (via GitHub)" <gi...@apache.org>.
zentol merged PR #23547:
URL: https://github.com/apache/flink/pull/23547
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33309] Add `-Djava.security.manager=allow` [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #23547:
URL: https://github.com/apache/flink/pull/23547#discussion_r1380841887
##########
flink-dist/src/main/flink-bin/bin/config.sh:
##########
@@ -334,6 +334,12 @@ if [ -z "${FLINK_ENV_JAVA_OPTS}" ]; then
# Remove leading and ending double quotes (if present) of value
FLINK_ENV_JAVA_OPTS="-XX:+IgnoreUnrecognizedVMOptions $( echo "${FLINK_ENV_JAVA_OPTS}" | sed -e 's/^"//' -e 's/"$//' )"
+
+ JAVA_SPEC_VERSION=`"${JAVA_RUN}" -XshowSettings:properties 2>&1 | grep "java.specification.version" | cut -d "=" -f 2 | tr -d '[:space:]'`
+ if [[ $(echo "$JAVA_SPEC_VERSION > 17" | bc ) == "1" ]]; then
Review Comment:
yes, I should have been checking that failure
the issue was with jdk 8 for which it returns `1.8`
finally i replaced `bc` with usage of substring from latest dot till end
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33309] Add `-Djava.security.manager=allow` [flink]
Posted by "zentol (via GitHub)" <gi...@apache.org>.
zentol commented on code in PR #23547:
URL: https://github.com/apache/flink/pull/23547#discussion_r1370226659
##########
flink-dist/src/main/flink-bin/bin/config.sh:
##########
@@ -334,6 +334,12 @@ if [ -z "${FLINK_ENV_JAVA_OPTS}" ]; then
# Remove leading and ending double quotes (if present) of value
FLINK_ENV_JAVA_OPTS="-XX:+IgnoreUnrecognizedVMOptions $( echo "${FLINK_ENV_JAVA_OPTS}" | sed -e 's/^"//' -e 's/"$//' )"
+
+ JAVA_SPEC_VERSION=`"${JAVA_RUN}" -XshowSettings:properties 2>&1 | grep "java.specification.version" | cut -d "=" -f 2 | tr -d '[:space:]'`
+ if [ "$JAVA_SPEC_VERSION" == "21" ]; then
+ # set security manager property to allow calls to System.setSecurityManager() at runtime
+ FLINK_ENV_JAVA_OPTS="$FLINK_ENV_JAVA_OPTS -Djava.security.manager=allow"
+ fi
Review Comment:
How compatible is this across JDK vendors?
This is gonna be annoying maintenance-wise because java 18,19,20 and 22 will all fail out-of-the-box.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33309] Add `-Djava.security.manager=allow` [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #23547:
URL: https://github.com/apache/flink/pull/23547#discussion_r1372332320
##########
flink-core/pom.xml:
##########
@@ -34,7 +34,14 @@ under the License.
<packaging>jar</packaging>
<properties>
- <surefire.module.config> <!--
+ <!--
+ Two properties below should be merged once support for jdk8 and jdk11 is dropped.
+ The reason of extraction into a separate property with profile is that
+ specifying -Djava.security.manager=allow within ${surefire.module.config} leads to
+ hanging of surefire for jdk8 and jdk11
Review Comment:
It seems current surefire plugin (3.0.0-M5) swallows the error, i bumped locally to 3.0.0-M6 and received a bit more info
```
Error occurred during initialization of VM
java.lang.Error: Could not create SecurityManager
at java.lang.System.initPhase3(java.base@11.0.20.1/System.java:2078)
Caused by: java.lang.ClassNotFoundException: allow
at jdk.internal.loader.BuiltinClassLoader.loadClass(java.base@11.0.20.1/BuiltinClassLoader.java:581)
at jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(java.base@11.0.20.1/ClassLoaders.java:178)
at java.lang.ClassLoader.loadClass(java.base@11.0.20.1/ClassLoader.java:527)
at java.lang.Class.forName0(java.base@11.0.20.1/Native Method)
at java.lang.Class.forName(java.base@11.0.20.1/Class.java:398)
at java.lang.System.initPhase3(java.base@11.0.20.1/System.java:2063)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33309] Add `-Djava.security.manager=allow` [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #23547:
URL: https://github.com/apache/flink/pull/23547#discussion_r1372332320
##########
flink-core/pom.xml:
##########
@@ -34,7 +34,14 @@ under the License.
<packaging>jar</packaging>
<properties>
- <surefire.module.config> <!--
+ <!--
+ Two properties below should be merged once support for jdk8 and jdk11 is dropped.
+ The reason of extraction into a separate property with profile is that
+ specifying -Djava.security.manager=allow within ${surefire.module.config} leads to
+ hanging of surefire for jdk8 and jdk11
Review Comment:
It seems current surefire plugin (3.0.0-M5) swallows the error, i bumped locally to 3.0.0-M6 and received a bit more info
```
Error occurred during initialization of VM
java.lang.Error: Could not create SecurityManager
at java.lang.System.initPhase3(java.base@11.0.20.1/System.java:2078)
Caused by: java.lang.ClassNotFoundException: allow
at jdk.internal.loader.BuiltinClassLoader.loadClass(java.base@11.0.20.1/BuiltinClassLoader.java:581)
at jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(java.base@11.0.20.1/ClassLoaders.java:178)
at java.lang.ClassLoader.loadClass(java.base@11.0.20.1/ClassLoader.java:527)
at java.lang.Class.forName0(java.base@11.0.20.1/Native Method)
at java.lang.Class.forName(java.base@11.0.20.1/Class.java:398)
at java.lang.System.initPhase3(java.base@11.0.20.1/System.java:2063)
```
and based on the source for _java.lang.System#initPhase3_
```java
/*
* Invoked by VM. Phase 3 is the final system initialization:
* 1. set security manager
* 2. set system class loader
* 3. set TCCL
*
* This method must be called after the module system initialization.
* The security manager and system class loader may be custom class from
* the application classpath or modulepath.
*/
private static void initPhase3() {
// set security manager
String cn = System.getProperty("java.security.manager");
if (cn != null) {
if (cn.isEmpty() || "default".equals(cn)) {
System.setSecurityManager(new SecurityManager());
} else {
try {
Class<?> c = Class.forName(cn, false, ClassLoader.getBuiltinAppClassLoader());
Constructor<?> ctor = c.getConstructor();
// Must be a public subclass of SecurityManager with
// a public no-arg constructor
if (!SecurityManager.class.isAssignableFrom(c) ||
!Modifier.isPublic(c.getModifiers()) ||
!Modifier.isPublic(ctor.getModifiers())) {
throw new Error("Could not create SecurityManager: " + ctor.toString());
}
// custom security manager implementation may be in unnamed module
// or a named module but non-exported package
ctor.setAccessible(true);
SecurityManager sm = (SecurityManager) ctor.newInstance();
System.setSecurityManager(sm);
} catch (Exception e) {
throw new Error("Could not create SecurityManager", e);
}
}
}
...
}
```
if it's non empty and not `default` it tries to load it using as a classname which is not present in jdk8, jdk11...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org