DRILL-7625: Add options for SslContextFactory #2012
Conversation
vvysotskyi
left a comment
vvysotskyi
left a comment
There was a problem hiding this comment.
@ihuzenko, thanks for adding so many SSL config properties. I have added several minor comments, please address them.
| String value = hasPath(path) ? getString(path) : null; | ||
| if (value == null || (value = value.trim()).isEmpty()) { | ||
| return defaultValue; | ||
| } | ||
| return value; |
There was a problem hiding this comment.
Nit: May be used StringUtils.defaultIfBlank) method to simplify this code.
There was a problem hiding this comment.
StringUtils.defaultIfBlank changes method logic, since in an original method was:
private String getConfigParamWithDefault(String name, String defaultValue) {
String value = "";
if (config.hasPath(name)) {
value = config.getString(name);
}
if (value.isEmpty()) {
value = defaultValue;
}
value = value.trim();
return value;
}Although I don't think that trimming was actually necessary, so I'll try your suggestion.
There was a problem hiding this comment.
There is no need for this method at all, default values can be set directly in drill-module.conf.
| .config(config).mode(SSLConfig.Mode.SERVER) | ||
| .initializeSSLContext(false).validateKeyStore(true) |
There was a problem hiding this comment.
| .config(config).mode(SSLConfig.Mode.SERVER) | |
| .initializeSSLContext(false).validateKeyStore(true) | |
| .config(config) | |
| .mode(SSLConfig.Mode.SERVER) | |
| .initializeSSLContext(false) | |
| .validateKeyStore(true) |
| # location of the OCSP Responder | ||
| ocspResponderURL: "", | ||
| # javax.net.ssl.SSLContext provider class name | ||
| provider: "fully.qualified.class.Name", |
There was a problem hiding this comment.
It looks like an invalid config. Have you checked whether with these default configs Drill works fine when SSL is enabled? If it works ok, does this config affects anything?
There was a problem hiding this comment.
I don't know which option to set here, it seems that the option allows using a custom implementation of java.security.Provider for SSLContext. As mentioned in the comment above sslContextFactory: all the options are optional and those who will configure them are expected to know what they're doing.
There was a problem hiding this comment.
My objection connected with setting non-null value for this option is because it may break configuration for the case when the default context provider is used. Here is a code from SslContextFactory:
context = _sslProvider == null ? SSLContext.getInstance(_sslProtocol) : SSLContext.getInstance(_sslProtocol, _sslProvider);
So user may want to specify sslProtocol only, but with non-null value it would fail.
There was a problem hiding this comment.
This file is far from Drill defaults, I'm convinced that if you'll try to actually use the file for overriding conf, your Drillbit will fail in numerous different ways. The file is here just to show a user which options are available in override conf. And handled badly, since there are a lot of options not present in the file.
There was a problem hiding this comment.
Oh, sorry, I thought it was drill-module.conf file. In this case, it is ok to leave as it is. Sorry for the confusion.
common/src/main/java/org/apache/drill/common/config/NestedConfig.java
Outdated
Show resolved
Hide resolved
...-exec/src/main/java/org/apache/drill/exec/server/rest/ssl/SslContextFactoryConfigurator.java
Show resolved
Hide resolved
vvysotskyi
left a comment
vvysotskyi
left a comment
There was a problem hiding this comment.
@ihuzenko, thanks for making changes.
LGTM, +1
|
Could we make a note to include this in the web documentation for Drill once this is committed? |
| * @param defaultValue default value | ||
| * @return option or default value | ||
| */ | ||
| public String getString(String path, String defaultValue) { |
There was a problem hiding this comment.
There is no need for this method at all, default values can be set directly in drill-module.conf.
|
Thank you, +1. |
4 participants
DRILL-7625: Add options for SslContextFactory
Description
Added ability to set more options on Jetty's SslContextFactory object and fixed application of drill.exec.ssl.protocol setting for Web UI client.
Documentation
Users now can provide more granular configuration for Jetty https connector. All additional options are listed in drill-override-example.conf in this pull request.
Testing
Added unit test.