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

Move IfConfig.logIfNecessary call into bootstrap #22455

Merged
merged 1 commit into from
Jan 6, 2017

Conversation

Tim-Brooks
Copy link
Contributor

This is related to #22116. A logIfNecessary() call makes a call to
NetworkInterface.getInterfaceAddresses() which requires SocketPermission connect
privileges. By moving this to bootstrap the logging call can be made before
installing the SecurityManager.

As a note:

IFConfig and its method need to be made public in order for this to work. Another option would be to move the class into a different package - but that would cause issues as it depends on package private methods in NetworkUtils.

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.

I left one comment about catching an exception that I think is no longer needed.


private static final Logger logger = Loggers.getLogger(IfConfig.class);
private static final String INDENT = " ";

/** log interface configuration at debug level, if its enabled */
static void logIfNecessary() {
public static void logIfNecessary() {
if (logger.isDebugEnabled()) {
try {
Copy link
Member

Choose a reason for hiding this comment

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

I was curious about this try block and I note that it currently catches SecurityException which should not be needed anymore with this change. Can we remove it now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed catching SecurityException. I think we still need the try and catch for IOException.

Copy link
Member

Choose a reason for hiding this comment

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

I agree on the IOException.

This is related to elastic#22116. A logIfNecessary() call makes a call to
NetworkInterface.getInterfaceAddresses() requiring SocketPermission
connect privileges. By moving this to bootstrap the logging call can be
made before installing the SecurityManager.
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.

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

@Tim-Brooks Tim-Brooks merged commit b9c2c2f into elastic:master Jan 6, 2017
@Tim-Brooks Tim-Brooks deleted the move_ifconfig branch January 6, 2017 17:10
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 >enhancement v6.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants