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