DRILL-5664: Enable security for Drill HiveStoragePlugin based on a co… #870
Conversation
…nfig parameter
Change to enable/disable HiveStoragePlugin security configuration based on Drill's "security.storage_plugin.enabled" configuration. This will help to open secure channel between Drill's HiveClient and HiveMetastore
|
More details in JIRA DRILL-5664. @parthchandra / @ppadma - Can you please help to review this change ? |
|
I have a question. The user can change the storage plugin configuration itself instead of enabling/disabling this option. what is the use of this option ? |
|
@ppadma -
|
| @@ -33,6 +33,8 @@ | |||
|
|
|||
| public static final String NAME = "hive"; | |||
|
|
|||
| public static final String HIVE_SECURITY_CONFIG = "hive.metastore.sasl.enabled"; | |||
There was a problem hiding this comment.
Fine to define this config option here. But, since this is Hive, and Hive has its own drill-module.conf file, please provide a default option value in that file. (The config system will throw an exception if the requested option is undefined.)
Then, if you expect users to change this, please provide an example and description in drill-override-example.conf.
| * @return true - if modified | ||
| * false - if not modified | ||
| */ | ||
| public boolean compareAndUpdateSecurityConfig(String newValue) { |
There was a problem hiding this comment.
The compare part is an implementation detail. Maybe just call it updateSecurity().
Then, passing in strings seems strange. Maybe pass in a boolean. Then, in this method, do Boolean.toString() to get the string value.
Finally, why even bother to check fi the value exists? Why not just put the new value and return it (since it will be a boolean)?
| @@ -155,6 +155,10 @@ drill.exec: { | |||
| enabled: false, | |||
| max_chained_user_hops: 3 | |||
| }, | |||
|
|
|||
| // Setting to control security for storage_plugins. For now this is only for Hive | |||
There was a problem hiding this comment.
Better:
security: {
user.auth.enabled: false,
storage_plugin_enabled: false
}
That way, it will be clear that the two properties go together. Move your comment inside the brackets.
Now, it is a hassle that some previous properties used dots as word separators: they are actually group separators...
Then, if users must set this option, please add this property as an example in drill-override-example.conf. Provide a user-understandable instruction for what to do. Also, please mark this JIRA as doc-impacting if it is not already.
| final String name = entry.getKey(); | ||
| final StoragePluginConfig config = entry.getValue(); | ||
|
|
||
| // Update the security setting inside StoragePluginConfig based on secureStoragePlugin flag |
There was a problem hiding this comment.
This doesn't seem right at all... We are updating in ZK the value of a plugin property based on something defined in config. All this can ever be is wrong an out-of-date. If the user changes the plugin after start, do we honor the boot option or the storage plugin option? Will they understand when we change the option back on next restart? A big mess...
Better: add a new configure(PluginContext context) method to the base plugin class. One hopes that there is an abstract base class that can provide the default implementation.
Then, define the PluginContext class with information of interest to the plugins. This might include the DrillConfig and your security setting.
Plugins are then obligated to use the provided value; not one stored in the plugin config.
But, this has a separate problem. What if I have two Hive servers: one which is secure, and one which is open? The same rule must be applied to both. So, this argument says that security of a Hive server is not a Drill boot-time config setting, it is instead and attribute of the remote system and should be part of the storage plugin config, and should not be altered by Drill.
| @@ -260,6 +289,10 @@ public StoragePlugin createOrUpdate(String name, StoragePluginConfig config, boo | |||
|
|
|||
| if (done) { | |||
| if (persist) { | |||
| if (!config.isValidSecurityConfig(secureStoragePlugin)) { | |||
| logger.warn("Security setting for {} plugin is not recommended one. It should be set to: {}", | |||
There was a problem hiding this comment.
This will cause confusion if we let the user update the storage plugin config ignoring the drill security config. I agree with Paul this can be messy, with things getting out of sync. We should discuss about this.
|
What is the status of this PR? Appears that there was substantial discussion about whether the approach here is appropriate for Apache Drill. Had the PR been updated with an alternative? Or, should this PR be closed pending a revised solution? |
|
closing this PR pending the revised version. |
…nfig parameter