Copilot commented on code in PR #847:
URL: https://github.com/apache/ranger/pull/847#discussion_r2888254631
##########
agents-common/src/test/java/org/apache/ranger/plugin/audit/TestRangerDefaultAuditHandler.java:
##########
@@ -286,15 +286,17 @@ public void test07_getDatasetsAndProjects_fromContext() {
public void test08_getAdditionalInfo_nullAndNonNull() {
RangerDefaultAuditHandler handler = new RangerDefaultAuditHandler();
RangerAccessRequestImpl req = new RangerAccessRequestImpl();
+ RangerAccessResult res = new RangerAccessResult(0, "svc", null,
req);
req.setRemoteIPAddress(null);
req.setForwardedAddresses(new ArrayList<>());
- Assertions.assertNull(handler.getAdditionalInfo(req));
+ Assertions.assertTrue(handler.getAdditionalInfo(req, res).isEmpty());
RangerAccessRequestImpl req2 = new RangerAccessRequestImpl();
+ RangerAccessResult res2 = new RangerAccessResult(0, "svc", null,
req);
req2.setRemoteIPAddress("10.1.1.1");
List<String> fwd = new ArrayList<>(Arrays.asList("1.1.1.1"));
req2.setForwardedAddresses(fwd);
- String info = handler.getAdditionalInfo(req2);
+ String info = handler.getAdditionalInfo(req2, res2);
Review Comment:
`res2` is constructed with `req` instead of `req2`, which means the test
isn't exercising the same request instance that is passed to
`getAdditionalInfo(req2, res2)`. Construct `res2` with `req2` to keep the
request/result pair consistent and avoid false positives.
##########
agents-common/src/main/java/org/apache/ranger/plugin/audit/RangerDefaultAuditHandler.java:
##########
@@ -230,16 +231,28 @@ public final Set<Long> getDatasetIds(RangerAccessRequest
request) {
return gdsResult != null ? gdsResult.getDatasetIds() : null;
}
- public String getAdditionalInfo(RangerAccessRequest request) {
- if (StringUtils.isBlank(request.getRemoteIPAddress()) &&
CollectionUtils.isEmpty(request.getForwardedAddresses())) {
- return null;
+ public String getAdditionalInfo(RangerAccessRequest request,
RangerAccessResult result) {
+ String ret =
org.apache.commons.lang3.StringUtils.EMPTY;
+ Map<String, String> addInfomap = new HashMap<>();
+
+ if (!CollectionUtils.isEmpty(request.getForwardedAddresses())) {
+ addInfomap.put("forwarded-ip-addresses", "[" +
org.apache.commons.lang3.StringUtils.join(request.getForwardedAddresses(), ",
") + "]");
}
- Map<String, String> addInfomap = new HashMap<>();
- addInfomap.put("forwarded-ip-addresses", "[" +
StringUtils.join(request.getForwardedAddresses(), ", ") + "]");
- addInfomap.put("remote-ip-address", request.getRemoteIPAddress());
+ if
(org.apache.commons.lang3.StringUtils.isNotEmpty(request.getRemoteIPAddress()))
{
+ addInfomap.put("remote-ip-address", request.getRemoteIPAddress());
+ }
- return JsonUtils.mapToJson(addInfomap);
+ String serviceType = result.getServiceTypeName();
+ if (org.apache.commons.lang3.StringUtils.isNotEmpty(serviceType)) {
+ addInfomap.put("serviceType", serviceType);
+ }
+
+ if (MapUtils.isNotEmpty(addInfomap)) {
+ ret = JsonUtils.mapToJson(addInfomap);
+ }
+
+ return ret;
Review Comment:
This changes the method contract from returning `null` when there is no
additional info to returning an empty string. That can be a behavioral/API
change for downstream code and storage pipelines (e.g., JSON serialization
differences, DB fields, filters checking for null). Consider preserving the
previous behavior by returning `null` when `addInfomap` is empty (or document
this as an intentional breaking change and ensure all consumers can handle
empty string as 'no data').
##########
agents-common/src/main/java/org/apache/ranger/plugin/service/RangerBasePlugin.java:
##########
@@ -400,7 +401,18 @@ public void init() {
if (!providerFactory.isInitDone()) {
if (pluginConfig.getProperties() != null) {
- providerFactory.init(pluginConfig.getProperties(), getAppId());
+ Properties auditProps = pluginConfig.getProperties();
+ String serviceType = getServiceType();
+ if (StringUtils.isNotEmpty(serviceType)) {
+ auditProps.setProperty("ranger.plugin.audit.service.type",
serviceType);
+ LOG.info("Added serviceType={} to audit properties for
audit destination", serviceType);
+ }
+ String appId = getAppId();
+ if (StringUtils.isNotEmpty(appId)) {
+ auditProps.setProperty("ranger.plugin.audit.app.id",
appId);
+ LOG.info("Added appId={} to audit properties for audit
destination", appId);
+ }
Review Comment:
`pluginConfig.getProperties()` is being mutated in-place. If those
properties are reused elsewhere (or shared across components), this introduces
side effects that are hard to reason about. Consider copying into a new
`Properties` instance (or otherwise ensuring `pluginConfig` properties are not
reused) before adding audit-specific keys, then pass the copy to
`providerFactory.init(...)`.
```suggestion
Properties baseProps = pluginConfig.getProperties();
Properties auditProps = new Properties();
auditProps.putAll(baseProps);
String serviceType = getServiceType();
if (StringUtils.isNotEmpty(serviceType)) {
auditProps.setProperty("ranger.plugin.audit.service.type", serviceType);
LOG.info("Added serviceType={} to audit properties for
audit destination", serviceType);
}
String appId = getAppId();
if (StringUtils.isNotEmpty(appId)) {
auditProps.setProperty("ranger.plugin.audit.app.id",
appId);
LOG.info("Added appId={} to audit properties for audit
destination", appId);
}
```
##########
agents-common/src/main/java/org/apache/ranger/plugin/audit/RangerDefaultAuditHandler.java:
##########
@@ -230,16 +231,28 @@ public final Set<Long> getDatasetIds(RangerAccessRequest
request) {
return gdsResult != null ? gdsResult.getDatasetIds() : null;
}
- public String getAdditionalInfo(RangerAccessRequest request) {
- if (StringUtils.isBlank(request.getRemoteIPAddress()) &&
CollectionUtils.isEmpty(request.getForwardedAddresses())) {
- return null;
+ public String getAdditionalInfo(RangerAccessRequest request,
RangerAccessResult result) {
+ String ret =
org.apache.commons.lang3.StringUtils.EMPTY;
Review Comment:
This changes the method contract from returning `null` when there is no
additional info to returning an empty string. That can be a behavioral/API
change for downstream code and storage pipelines (e.g., JSON serialization
differences, DB fields, filters checking for null). Consider preserving the
previous behavior by returning `null` when `addInfomap` is empty (or document
this as an intentional breaking change and ensure all consumers can handle
empty string as 'no data').
##########
dev-support/ranger-docker/Dockerfile.ranger-audit-server:
##########
@@ -0,0 +1,86 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+ARG RANGER_BASE_IMAGE
+ARG RANGER_BASE_VERSION
+FROM ${RANGER_BASE_IMAGE}:${RANGER_BASE_VERSION}
+
+# Declare ARG for use in this build stage
+ARG RANGER_VERSION
+ARG KERBEROS_ENABLED
+
+# Install required packages including Kerberos client
+RUN apt-get update && apt-get install -y \
+ python3 \
+ python3-pip \
+ curl \
+ vim \
+ net-tools \
+ krb5-user \
+ && rm -rf /var/lib/apt/lists/*
+
+# Volume for keytabs
+VOLUME /etc/keytabs
+
+# Set up working directory
+WORKDIR /opt/ranger-audit-server
+
+# Copy ranger-audit-server-service distribution
+COPY ./dist/ranger-${RANGER_VERSION}-ranger-audit-server-service.tar.gz /tmp/
+
+# Copy scripts and Kerberos configuration
+COPY ./scripts/audit-server/service-check-functions.sh ${RANGER_SCRIPTS}/
+COPY ./scripts/audit-server/ranger-audit-server.sh ${RANGER_SCRIPTS}/
+COPY ./scripts/kdc/krb5.conf /etc/krb5.conf
+
+# Extract and setup
+RUN tar xzf /tmp/ranger-${RANGER_VERSION}-ranger-audit-server-service.tar.gz
-C /opt/ranger-audit-server --strip-components=1 && \
+ rm -f /tmp/ranger-${RANGER_VERSION}-ranger-audit-server-service.tar.gz && \
+ mkdir -p /var/log/ranger/ranger-audit-server/audit/spool && \
+ mkdir -p /var/log/ranger/ranger-audit-server/audit/archive && \
+ mkdir -p /home/ranger/scripts && \
+ rm -rf /opt/ranger-audit-server/logs && \
+ ln -sf /var/log/ranger/ranger-audit-server /opt/ranger-audit-server/logs
&& \
+ chown -R ranger:ranger /opt/ranger-audit-server/ /var/log/ranger/
${RANGER_SCRIPTS} && \
+ chmod -R 755 /opt/ranger-audit-server && \
+ chmod 755 ${RANGER_SCRIPTS}/ranger-audit-server.sh
+
+# Set Java home
+ENV JAVA_HOME=/opt/java/openjdk
+ENV PATH=$JAVA_HOME/bin:$PATH
+
+# Environment variables for audit server
+ENV AUDIT_SERVER_HOME_DIR=/opt/ranger-audit-server
+ENV AUDIT_SERVER_CONF_DIR=/opt/ranger-audit-server/conf
+ENV AUDIT_SERVER_LOG_DIR=/var/log/ranger/ranger-audit-server
+ENV RANGER_USER=ranger
+
+# Expose ports (HTTP and HTTPS)
+EXPOSE 7081 7182
+
+# Health check - test the REST API endpoint
+HEALTHCHECK --interval=30s --timeout=10s --start-period=60s --retries=3 \
+ CMD curl -f -o /dev/null -s -w "%{http_code}"
http://localhost:7081/api/audit/health | grep -q -E "200|401" || exit 1
+
+# Environment variable for Kerberos support
+ENV KERBEROS_ENABLED=${KERBEROS_ENABLED:-false}
+
+# Switch to ranger user
+USER ranger
+
+# Start the audit server using the custom startup script
+WORKDIR /opt/ranger-audit-server
+ENTRYPOINT ["/home/ranger/scripts/ranger-audit-server.sh"]
Review Comment:
The Dockerfile copies scripts to `${RANGER_SCRIPTS}/` but the `ENTRYPOINT`
uses a hard-coded `/home/ranger/scripts/...` path. If `${RANGER_SCRIPTS}` is
not exactly `/home/ranger/scripts`, the container will fail to start. Use a
consistent path (either copy to `/home/ranger/scripts` explicitly, or set
`ENTRYPOINT` to `${RANGER_SCRIPTS}/ranger-audit-server.sh`).
```suggestion
ENTRYPOINT ${RANGER_SCRIPTS}/ranger-audit-server.sh
```
##########
dev-support/ranger-docker/Dockerfile.ranger-audit-server:
##########
@@ -0,0 +1,86 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+ARG RANGER_BASE_IMAGE
+ARG RANGER_BASE_VERSION
+FROM ${RANGER_BASE_IMAGE}:${RANGER_BASE_VERSION}
+
+# Declare ARG for use in this build stage
+ARG RANGER_VERSION
+ARG KERBEROS_ENABLED
+
+# Install required packages including Kerberos client
+RUN apt-get update && apt-get install -y \
+ python3 \
+ python3-pip \
+ curl \
+ vim \
+ net-tools \
+ krb5-user \
+ && rm -rf /var/lib/apt/lists/*
+
+# Volume for keytabs
+VOLUME /etc/keytabs
+
+# Set up working directory
+WORKDIR /opt/ranger-audit-server
+
+# Copy ranger-audit-server-service distribution
+COPY ./dist/ranger-${RANGER_VERSION}-ranger-audit-server-service.tar.gz /tmp/
+
+# Copy scripts and Kerberos configuration
+COPY ./scripts/audit-server/service-check-functions.sh ${RANGER_SCRIPTS}/
+COPY ./scripts/audit-server/ranger-audit-server.sh ${RANGER_SCRIPTS}/
Review Comment:
The Dockerfile copies scripts to `${RANGER_SCRIPTS}/` but the `ENTRYPOINT`
uses a hard-coded `/home/ranger/scripts/...` path. If `${RANGER_SCRIPTS}` is
not exactly `/home/ranger/scripts`, the container will fail to start. Use a
consistent path (either copy to `/home/ranger/scripts` explicitly, or set
`ENTRYPOINT` to `${RANGER_SCRIPTS}/ranger-audit-server.sh`).
--
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]
