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

Remove accept SocketPermissions from core #22622

Merged
merged 6 commits into from
Jan 20, 2017

Conversation

Tim-Brooks
Copy link
Contributor

@Tim-Brooks Tim-Brooks commented Jan 13, 2017

This is related to #22116. Core no longer needs SocketPermission
accept. This permission is relegated to the transport-netty4 module
and (for tests) to the mocksocket jar.

@Tim-Brooks Tim-Brooks added :Core/Infra/Core Core issues without another label :Distributed Coordination/Network Http and internode communication implementations >enhancement review v6.0.0-alpha1 labels Jan 13, 2017
@Tim-Brooks
Copy link
Contributor Author

On the reindex module needing accept for tests issue:

One option would be to add the following permissions to the test-framework.policy file:

grant codeBase "${codebase.netty-transport-4.1.7.Final.jar}" {
   permission java.net.SocketPermission "*", "accept";
};

grant codeBase "${codebase.transport-netty4-6.0.0-alpha1-SNAPSHOT.jar}" {
  permission java.net.SocketPermission "*", "accept";
};

Unfortunately, that does not work with IDE junit test runners. As I mentioned, I had worked on adding support for parsing the directory code paths. The downside of that is you would need to duplicate permissions for the jar version of transport-netty4 (gradle runner) and the directory version of transport-netty4 (idea/probably eclipse runner).

The second option, is that the doPrivileged blocks move down to the netty level (netty/netty#6146). Then transport-netty4 module will not need permissions.

Or maybe someone else has a better idea on this?

@Tim-Brooks
Copy link
Contributor Author

Also I see that this broke the integration tests. So I'll need to resolve that before it is merged.

@Tim-Brooks
Copy link
Contributor Author

I opened PR #22646 which I believe will be necessary for the integration tests to pass in relation to this PR.

@Tim-Brooks
Copy link
Contributor Author

Alright @jasontedor and @s1monw - with the build updates to master this should be ready for review.

The build changes also removed the need for reindex to have SocketPermission.

Copy link
Contributor

@s1monw s1monw left a comment

Choose a reason for hiding this comment

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

LGTM hell yeah

Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

LGTM.

@Tim-Brooks Tim-Brooks merged commit bc16162 into elastic:master Jan 20, 2017
@Tim-Brooks Tim-Brooks deleted the remove_accept_from_core branch January 20, 2017 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Core Core issues without another label :Distributed Coordination/Network Http and internode communication implementations >enhancement v6.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants