-
Notifications
You must be signed in to change notification settings - Fork 13.9k
[FLINK-33309] Add -Djava.security.manager=allow
#23547
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
zentol
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also needs to be added to env.java.opts.all in flink-conf.yaml
|
thanks,
UPD: Another idea is to add logic to [1] https://dev.azure.com/apache-flink/apache-flink/_build/results?buildId=53933&view=results |
|
@flinkbot run azure |
| 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
|
@flinkbot run azure |
| 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bc isn't available in our CI image. any reason this cant use plain bash arithmetic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
What is the purpose of the change
Since because of JEP411 [1] starting from java 18 default value of
java.security.managerwill be disallow the PR adds allow optio where it is neededVerifying 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:
@Public(Evolving): ( no)Documentation