Skip to content
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

Add missing runtime permission to TikaImpl #28602

Merged
merged 1 commit into from Feb 9, 2018

Conversation

cbuescher
Copy link
Member

Tests on jdk10 were failing because of a change in java 10s ZipFile
implementation that now needs accessDeclaredMembers permissions.
Adding this to the policy file and TikaImpl for tests.

Closes #28568

Tests on jdk10 were failing because of a change in java 10s ZipFile
implementation that now needs `accessDeclaredMembers` permissions.
Adding this to the policy file and TikaImpl for tests.

Closes elastic#28568
@cbuescher cbuescher added >test Issues or PRs that are addressing/adding tests review v7.0.0 v6.3.0 jdk10 labels Feb 9, 2018
Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy we found the cause.

@cbuescher
Copy link
Member Author

@martijnvg thanks for the review, will merge to the affected branches then now.

@cbuescher cbuescher merged commit cc9cb53 into elastic:master Feb 9, 2018
cbuescher pushed a commit that referenced this pull request Feb 9, 2018
Tests on jdk10 were failing because of a change in its ZipFile implementation 
that now needs `accessDeclaredMembers` permissions. This change adds 
the missing permission to the plugins security policy and TikaImpl.

Closes #28568
@@ -161,6 +161,8 @@ static PermissionCollection getRestrictedPermissions() {
perms.add(new ReflectPermission("suppressAccessChecks"));
// xmlbeans, use by POI, needs to get the context classloader
perms.add(new RuntimePermission("getClassLoader"));
// ZipFile needs accessDeclaredMembers on Java 10
perms.add(new RuntimePermission("accessDeclaredMembers"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we should have left an assertion here that the Java version is not JDK 11 (I think we will be able to remove this for JDK 11). I also think that this code should have been guarded by an if block checking that we are on JDK 10 and otherwise not add this permission.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course. I can add the conditional check. Whats the best (in terms of least changing) system property used for this that is the same across different JVMs? Maybe "java.specification.version" or a prefix of "java.version"?
About adding an assertion, are we sure that this is going to change back in JDK 11? Would an issue to trask this be better?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened #28603.

jasontedor pushed a commit that referenced this pull request Feb 9, 2018
Tests on jdk10 were failing because of a change in its ZipFile implementation 
that now needs `accessDeclaredMembers` permissions. This change adds 
the missing permission to the plugins security policy and TikaImpl.

Closes #28568
@clintongormley clintongormley added :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP and removed :Plugin Ingest Attachment labels Feb 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP jdk10 >test Issues or PRs that are addressing/adding tests v6.3.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants