Copilot commented on code in PR #911:
URL: https://github.com/apache/ranger/pull/911#discussion_r3060386021
##########
agents-common/src/test/java/org/apache/ranger/plugin/audit/TestRangerDefaultAuditHandler.java:
##########
@@ -170,7 +168,7 @@ public void
test02_getAuthzEvents_populatesFieldsAndDefaults() {
Assertions.assertEquals("clusterA", event.getClusterName());
Assertions.assertEquals("zone-1", event.getZoneName());
Assertions.assertEquals(Long.valueOf(7L), event.getPolicyVersion());
- Assertions.assertEquals("my-host", event.getAgentHostname());
+
Assertions.assertTrue(StringUtils.isNotBlank(event.getAgentHostname()));
Assertions.assertNotNull(event.getEventId());
Review Comment:
The test now only asserts `agentHostname` is non-blank, which doesn’t
validate the intended wiring (that `RangerDefaultAuditHandler` uses
`RangerRESTUtils.getAgentHostname()` for the event). Consider asserting
equality with `new RangerRESTUtils().getAgentHostname()` (or
`RangerRESTUtils.hostname` if it remains accessible) to keep the test
meaningful while still being environment-agnostic.
##########
agents-common/src/main/java/org/apache/ranger/plugin/util/RangerRESTUtils.java:
##########
@@ -73,7 +72,7 @@ public class RangerRESTUtils {
private static final Logger LOG
= LoggerFactory.getLogger(RangerRESTUtils.class);
private static final int MAX_PLUGIN_ID_LEN
= 255;
- public static String hostname;
+ public static final String hostname = getHostname();
Review Comment:
`hostname` is now `public static final`, but its name doesn’t follow the
established constant style in this class (e.g., `REST_PARAM_*`,
`MAX_PLUGIN_ID_LEN`). Consider either (a) renaming it to an uppercase constant
name, or (b) making it `private` and exposing it only through
`getAgentHostname()` (since that’s already the public API).
```suggestion
private static final String hostname = getHostname();
```
##########
agents-common/src/main/java/org/apache/ranger/plugin/util/RangerRESTUtils.java:
##########
@@ -143,12 +142,22 @@ public String getAppIdFromPluginId(String pluginId) {
return ret;
}
- static {
- try {
- hostname = Hostname.getHostname();
- } catch (Exception e) {
- LOG.error("ERROR: Unable to find hostname for the agent ", e);
- hostname = "unknownHost";
+ private static String getHostname() {
+ String hostname = System.getenv("HOSTNAME");
+
+ if (StringUtils.isBlank(hostname)) {
+ hostname = System.getenv("COMPUTERNAME");
+
+ if (StringUtils.isBlank(hostname)) {
+ try {
+ hostname = InetAddress.getLocalHost().getHostName();
+ } catch (Exception e) {
+ LOG.error("ERROR: Unable to find hostname for the agent ",
e);
+ hostname = "unknownHost";
+ }
+ }
}
+
+ return hostname.trim();
Review Comment:
`System.getenv()` can throw `SecurityException` in restricted runtimes (JDK8
security manager / hardened containers). Since this method is executed during
class initialization (`public static final ... = getHostname()`), an uncaught
`SecurityException` would fail class loading. Wrap the env-var lookups (or the
whole method) in a try/catch (at least for `SecurityException`) and fall back
to `InetAddress`/`unknownHost` like the existing error handling does.
```suggestion
String hostname = null;
try {
hostname = System.getenv("HOSTNAME");
if (StringUtils.isBlank(hostname)) {
hostname = System.getenv("COMPUTERNAME");
}
} catch (SecurityException e) {
LOG.error("ERROR: Unable to access environment variables for the
agent hostname ", e);
}
if (StringUtils.isBlank(hostname)) {
try {
hostname = InetAddress.getLocalHost().getHostName();
} catch (Exception e) {
LOG.error("ERROR: Unable to find hostname for the agent ",
e);
hostname = "unknownHost";
}
}
return StringUtils.isBlank(hostname) ? "unknownHost" :
hostname.trim();
```
--
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]
