Skip to content

WFSSL-120 Improve logging when Wildfly OpenSSL misconfigured#145

Open
RanabirChakraborty wants to merge 1 commit intowildfly-security:mainfrom
RanabirChakraborty:WFSSL-120
Open

WFSSL-120 Improve logging when Wildfly OpenSSL misconfigured#145
RanabirChakraborty wants to merge 1 commit intowildfly-security:mainfrom
RanabirChakraborty:WFSSL-120

Conversation

@RanabirChakraborty
Copy link
Contributor

Copy link
Contributor

@darranl darranl left a comment

Choose a reason for hiding this comment

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

Thank you for your PR.

FYI in additional to my comments we should look at getting these strings in the Messages.java class.

instance = new SSLImpl();
} catch (Throwable e) {
//Log the initial failure to provide a clear root cause.
logger.log(Level.WARNING, "Failed to load wfssl native library from system path. Will attempt to load from classpath.", e);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should log at WARN, I think Debug would be sufficient - logging an exception at WARN would lead to very verbose output that users will not be able to turn off - especially as this is only informational and the fallback mechanism may still work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, Fixed accordingly

Runtime.getRuntime().load(libPath);
instance = new SSLImpl();
} catch (Throwable t) {
throw new RuntimeException("Failed to load wfssl native library from specified path: " + libPath, t);
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are throwing this exception is there not a more appropriate type we can use?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, Fixed accordingly

Copy link
Contributor

@darranl darranl left a comment

Choose a reason for hiding this comment

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

Sorry we are still missing one further change, the message that is thrown in the exception should be obtained from Messages.java

@RanabirChakraborty
Copy link
Contributor Author

@darranl thanks, did the changes. Please review now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants