Copilot commented on code in PR #926:
URL: https://github.com/apache/ranger/pull/926#discussion_r3122174811
##########
authz-embedded/src/main/java/org/apache/ranger/authz/embedded/RangerAuthzConfig.java:
##########
@@ -58,12 +60,19 @@ public Properties getAuditProperties() {
if (propName.startsWith(PROP_PREFIX_AUDIT)) {
String propValue = properties.getProperty(propName);
String propSuffix =
propName.substring(PROP_PREFIX_AUDIT.length());
- String pluginPropName = "xasecure.audit." + propSuffix;
+ String pluginPropName = PROP_PREFIX_XASECURE_AUDIT +
propSuffix;
ret.setProperty(pluginPropName, propValue);
}
}
+ // add legacy audit configurations
+ for (String propName : properties.stringPropertyNames()) {
+ if (propName.startsWith(PROP_PREFIX_XASECURE_AUDIT)) {
Review Comment:
In getAuditProperties(), legacy xasecure.audit.* entries are added after
mapping ranger.authz.audit.* -> xasecure.audit.*. If both styles are present
for the same key, the legacy loop will overwrite the value set from
ranger.authz.audit.* with no explicit precedence rule. Consider either (a) only
setting legacy properties when the key is not already present, or (b) clearly
documenting/enforcing which source should win to avoid surprising overrides
when configs are merged.
```suggestion
// add legacy audit configurations only when an equivalent property
has not
// already been derived from ranger.authz.audit.* settings
for (String propName : properties.stringPropertyNames()) {
if (propName.startsWith(PROP_PREFIX_XASECURE_AUDIT) &&
!ret.containsKey(propName)) {
```
##########
authz-embedded/src/main/java/org/apache/ranger/authz/embedded/RangerAuthzConfig.java:
##########
@@ -115,6 +124,20 @@ public Properties getServiceProperties(String serviceName,
String serviceType) {
}
}
+ // add legacy configurations that start with pluginPropPrefix
("ranger.plugin." + serviceType + ".")
+ for (String propName : properties.stringPropertyNames()) {
+ if (propName.startsWith(pluginPropPrefix)) {
+ ret.setProperty(propName, properties.getProperty(propName));
+ }
+ }
+
+ // add legacy configurations that start with "xasecure."
+ for (String propName : properties.stringPropertyNames()) {
+ if (propName.startsWith(PROP_PREFIX_XASECURE) &&
!propName.startsWith(PROP_PREFIX_XASECURE_AUDIT)) {
+ ret.setProperty(propName, properties.getProperty(propName));
+ }
+ }
+
return ret;
}
Review Comment:
In getServiceProperties(), the added legacy property passes
(ranger.plugin.<serviceType>.* and xasecure.*) run after the ranger.authz.* ->
ranger.plugin.<serviceType>.* mapping and can overwrite previously set values
for the same keys. If the intent is “fallback for legacy configs”, consider
only populating legacy keys when absent, or otherwise codify the intended
precedence so mixed configurations behave predictably.
```suggestion
// only use them as fallback so they don't override values derived
from ranger.authz.* properties
for (String propName : properties.stringPropertyNames()) {
if (propName.startsWith(pluginPropPrefix)) {
setPropertyIfAbsent(ret, propName,
properties.getProperty(propName));
}
}
// add legacy configurations that start with "xasecure."
// only use them as fallback so mixed modern/legacy configurations
have predictable precedence
for (String propName : properties.stringPropertyNames()) {
if (propName.startsWith(PROP_PREFIX_XASECURE) &&
!propName.startsWith(PROP_PREFIX_XASECURE_AUDIT)) {
setPropertyIfAbsent(ret, propName,
properties.getProperty(propName));
}
}
return ret;
}
private void setPropertyIfAbsent(Properties target, String propName,
String propValue) {
if (!target.containsKey(propName)) {
target.setProperty(propName, propValue);
}
}
```
##########
authz-embedded/src/main/java/org/apache/ranger/authz/embedded/RangerAuthzConfig.java:
##########
@@ -115,6 +124,20 @@ public Properties getServiceProperties(String serviceName,
String serviceType) {
}
}
+ // add legacy configurations that start with pluginPropPrefix
("ranger.plugin." + serviceType + ".")
+ for (String propName : properties.stringPropertyNames()) {
+ if (propName.startsWith(pluginPropPrefix)) {
+ ret.setProperty(propName, properties.getProperty(propName));
+ }
+ }
+
+ // add legacy configurations that start with "xasecure."
+ for (String propName : properties.stringPropertyNames()) {
+ if (propName.startsWith(PROP_PREFIX_XASECURE) &&
!propName.startsWith(PROP_PREFIX_XASECURE_AUDIT)) {
+ ret.setProperty(propName, properties.getProperty(propName));
+ }
+ }
Review Comment:
New behavior adds support for legacy plugin configuration properties
(ranger.plugin.<serviceType>.* and xasecure.*) in getServiceProperties(), but
existing unit tests only cover the ranger.authz.* mapping path. Please
add/extend tests to assert that these legacy keys are included (and that
precedence is correct when both legacy and ranger.authz.* variants are present).
##########
authz-embedded/src/main/java/org/apache/ranger/authz/embedded/RangerAuthzConfig.java:
##########
@@ -58,12 +60,19 @@ public Properties getAuditProperties() {
if (propName.startsWith(PROP_PREFIX_AUDIT)) {
String propValue = properties.getProperty(propName);
String propSuffix =
propName.substring(PROP_PREFIX_AUDIT.length());
- String pluginPropName = "xasecure.audit." + propSuffix;
+ String pluginPropName = PROP_PREFIX_XASECURE_AUDIT +
propSuffix;
ret.setProperty(pluginPropName, propValue);
}
}
+ // add legacy audit configurations
+ for (String propName : properties.stringPropertyNames()) {
+ if (propName.startsWith(PROP_PREFIX_XASECURE_AUDIT)) {
Review Comment:
Both getAuditProperties() and getServiceProperties() iterate over
properties.stringPropertyNames() multiple times to collect different prefixes.
This is correct but makes the method harder to maintain and adds avoidable
overhead for large property sets. Consider consolidating these into a single
pass over the property names (handling all relevant prefixes within one loop).
```suggestion
} else if (propName.startsWith(PROP_PREFIX_XASECURE_AUDIT)) {
// add legacy audit configurations
```
--
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]
