fateh288 commented on code in PR #584:
URL: https://github.com/apache/ranger/pull/584#discussion_r2205865345
##########
security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java:
##########
@@ -2860,6 +2856,45 @@ public void updateServiceAuditConfig(String
searchUsrGrpRoleName, REMOVE_REF_TYP
LOG.debug("<=== ServiceDBStore.updateServiceAuditConfig(
searchUsrGrpRoleName : {} removeRefType : {})", searchUsrGrpRoleName,
removeRefType);
}
+ public Map<String, String> getPluginsSpecialConfigsForNameTransformed() {
Review Comment:
We should move this and any property / configs related operations to
PropertiesUtil class instead of having it in ServiceDBStore
##########
security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java:
##########
@@ -2860,6 +2856,45 @@ public void updateServiceAuditConfig(String
searchUsrGrpRoleName, REMOVE_REF_TYP
LOG.debug("<=== ServiceDBStore.updateServiceAuditConfig(
searchUsrGrpRoleName : {} removeRefType : {})", searchUsrGrpRoleName,
removeRefType);
}
+ public Map<String, String> getPluginsSpecialConfigsForNameTransformed() {
+ Map<String, String> configs = new HashMap<String, String>();
+ configs.put(RangerCommonConstants.PLUGINS_MAPPING_SEPARATOR,
getRegexSeparator());
+
configs.putAll(getAllRegexPatternsConfig(RangerCommonConstants.PLUGINS_MAPPING_USERNAME));
+
configs.putAll(getAllRegexPatternsConfig(RangerCommonConstants.PLUGINS_MAPPING_GROUPNAME));
+ return configs;
+ }
+
+ public String getRegexSeparator() {
+ String ret = UgsyncCommonConstants.DEFAULT_MAPPING_SEPARATOR;
+ String val =
PropertiesUtil.getProperty(RangerCommonConstants.PLUGINS_MAPPING_SEPARATOR);
+ if (StringUtils.isNotEmpty(val)) {
+ if (val.length() == 1) {
+ ret = val;
+ } else {
+ LOG.warn("More than one character found in RegEx Separator,
using default RegEx Separator");
+ }
+ }
+ LOG.info(String.format("Using %s as the RegEx Separator", ret));
+ return ret;
+ }
+
+ public Map<String, String> getAllRegexPatternsConfig(String baseProperty) {
Review Comment:
This method can be private
##########
security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java:
##########
@@ -1919,6 +1899,22 @@ public Map<String, String>
getServiceConfigForPlugin(Long serviceId) {
}
}
}
+ Set<String> excludedKeys = new HashSet<>(
+ Arrays.asList(
+ RangerCommonConstants.PLUGINS_MAPPING_SEPARATOR,
+ RangerCommonConstants.PLUGINS_MAPPING_USERNAME,
+ RangerCommonConstants.PLUGINS_MAPPING_GROUPNAME
+ )
+ );
+ Map<String, String> rangerPluginsPrefixConfig =
PropertiesUtil.getConfigMapWithPrefixAndDefaultValue("ranger.plugins.",
excludedKeys);
Review Comment:
We should have a constant string defined for "ranger.plugins." similar to
RANGER_PLUGIN_CONFIG_PREFIX.
##########
security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java:
##########
@@ -1919,6 +1899,22 @@ public Map<String, String>
getServiceConfigForPlugin(Long serviceId) {
}
}
}
+ Set<String> excludedKeys = new HashSet<>(
Review Comment:
I think we should read these properties we want to exclude from the configs
as a separate config instead of hardcoding these properties in the code to
improve extensibility.
##########
security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java:
##########
@@ -1919,6 +1899,22 @@ public Map<String, String>
getServiceConfigForPlugin(Long serviceId) {
}
}
}
+ Set<String> excludedKeys = new HashSet<>(
+ Arrays.asList(
+ RangerCommonConstants.PLUGINS_MAPPING_SEPARATOR,
+ RangerCommonConstants.PLUGINS_MAPPING_USERNAME,
+ RangerCommonConstants.PLUGINS_MAPPING_GROUPNAME
+ )
+ );
+ Map<String, String> rangerPluginsPrefixConfig =
PropertiesUtil.getConfigMapWithPrefixAndDefaultValue("ranger.plugins.",
excludedKeys);
Review Comment:
Why are we naming this method as getConfigMapWithPrefixAndDefaultValue and
not getConfigMapWithPrefix() ?
I don't see any default value being passed to this function / being read
within the method
##########
security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java:
##########
@@ -2860,6 +2856,45 @@ public void updateServiceAuditConfig(String
searchUsrGrpRoleName, REMOVE_REF_TYP
LOG.debug("<=== ServiceDBStore.updateServiceAuditConfig(
searchUsrGrpRoleName : {} removeRefType : {})", searchUsrGrpRoleName,
removeRefType);
}
+ public Map<String, String> getPluginsSpecialConfigsForNameTransformed() {
Review Comment:
Also, these all seem to be static methods
##########
security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java:
##########
@@ -2860,6 +2856,45 @@ public void updateServiceAuditConfig(String
searchUsrGrpRoleName, REMOVE_REF_TYP
LOG.debug("<=== ServiceDBStore.updateServiceAuditConfig(
searchUsrGrpRoleName : {} removeRefType : {})", searchUsrGrpRoleName,
removeRefType);
}
+ public Map<String, String> getPluginsSpecialConfigsForNameTransformed() {
+ Map<String, String> configs = new HashMap<String, String>();
+ configs.put(RangerCommonConstants.PLUGINS_MAPPING_SEPARATOR,
getRegexSeparator());
+
configs.putAll(getAllRegexPatternsConfig(RangerCommonConstants.PLUGINS_MAPPING_USERNAME));
+
configs.putAll(getAllRegexPatternsConfig(RangerCommonConstants.PLUGINS_MAPPING_GROUPNAME));
+ return configs;
+ }
+
+ public String getRegexSeparator() {
Review Comment:
This method can be private
##########
security-admin/src/main/java/org/apache/ranger/common/PropertiesUtil.java:
##########
@@ -163,6 +164,36 @@ public static Properties getProps() {
return ret;
}
+ public static Map<String, String>
getConfigMapWithPrefixAndDefaultValue(String confPrefix) {
+ Map<String, String> configMap = new HashMap<>();
+ Map<String, String> propsMap = getPropertiesMap();
+ for (Map.Entry<String, String> entry : propsMap.entrySet()) {
+ String key = entry.getKey();
+ if (key.startsWith(confPrefix)) {
+ String value = StringUtils.isNotEmpty(entry.getValue()) ?
entry.getValue() : null;
+ if (value != null) {
+ configMap.put(key, value);
+ }
+ }
+ }
+ return configMap;
+ }
+
+ public static Map<String, String>
getConfigMapWithPrefixAndDefaultValue(String confPrefix, Set<String>
excludedKeys) {
Review Comment:
There is very high code duplication in this method with the above method
getConfigMapWithPrefixAndDefaultValue(String confPrefix).
I think it would be better to have a separate utility called
removeConfigsIfPresent() which remove the excluded keys from the hashmap.
So first we can call getConfigMapWithPrefixAndDefaultValue(String
confPrefix) and then call removeConfigsIfPresent(HashMap<String, String>
configs, Set<String> toRemoveConfigs) to achieve the same desired result
##########
security-admin/src/main/java/org/apache/ranger/biz/ServiceDBStore.java:
##########
@@ -1919,6 +1899,22 @@ public Map<String, String>
getServiceConfigForPlugin(Long serviceId) {
}
}
}
+ Set<String> excludedKeys = new HashSet<>(
Review Comment:
Another suggestion I have is - instead of removing these configs in the
code, can we consider naming these configs such that they don't start with
"ranger.plugins." ? This would keep the logic uniform - that any property
starting with "ranger.plugins." would go into all all plugins. Adding
exceptions to the rules adds code complexity and confusion
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
