Copilot commented on code in PR #899: URL: https://github.com/apache/ranger/pull/899#discussion_r3007289787
########## pdp/src/test/java/org/apache/ranger/pdp/RangerPdpStatusServletTest.java: ########## @@ -0,0 +1,112 @@ +/* + * 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. + */ + +package org.apache.ranger.pdp; + +import org.apache.ranger.pdp.config.RangerPdpConfig; +import org.apache.ranger.pdp.config.RangerPdpConstants; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +import java.io.File; +import java.io.PrintWriter; +import java.io.StringWriter; Review Comment: Unused import `java.io.File` will fail the build under the repository Checkstyle `UnusedImports` rule (see dev-support/checkstyle.xml). Remove the import (it doesn't appear to be used in this test). ########## pdp/src/test/java/org/apache/ranger/pdp/RangerPdpStatusServletTest.java: ########## @@ -0,0 +1,112 @@ +/* + * 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. + */ + +package org.apache.ranger.pdp; + +import org.apache.ranger.pdp.config.RangerPdpConfig; +import org.apache.ranger.pdp.config.RangerPdpConstants; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +import java.io.File; +import java.io.PrintWriter; +import java.io.StringWriter; +import java.lang.reflect.InvocationHandler; +import java.lang.reflect.Method; +import java.lang.reflect.Proxy; +import java.nio.file.Path; +import java.util.HashMap; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +public class RangerPdpStatusServletTest { + @AfterEach + public void clearOverrides() { + System.clearProperty(RangerPdpConstants.PROP_AUTHZ_POLICY_CACHE_DIR); + } + + @Test + public void testMetricsEndpointRendersCounters() throws Exception { + RangerPdpStats stats = new RangerPdpStats(); + + stats.recordRequestSuccess(5_000_000L); + stats.recordRequestError(5_000_000L); + stats.recordAuthFailure(5_000_000L); + + RangerPdpStatusServlet servlet = new RangerPdpStatusServlet(stats, new RangerPdpConfig(), RangerPdpStatusServlet.Mode.METRICS); + HttpServletRequest req = proxy(HttpServletRequest.class, (proxy, method, args) -> null); + ResponseCapture capture = new ResponseCapture(); + HttpServletResponse resp = capture.responseProxy(); + + servlet.doGet(req, resp); + + assertEquals(HttpServletResponse.SC_OK, capture.status); + assertTrue(capture.body.toString().contains("ranger_pdp_requests_total 3")); + assertTrue(capture.body.toString().contains("ranger_pdp_auth_failures_total 1")); + } + + @Test + public void testLoadedServicesCount() throws Exception { + RangerPdpStatusServlet servlet = new RangerPdpStatusServlet(new RangerPdpStats(), new RangerPdpConfig(), RangerPdpStatusServlet.Mode.READY); + HttpServletRequest req = proxy(HttpServletRequest.class, (proxy, method, args) -> null); + Method method = RangerPdpStatusServlet.class.getDeclaredMethod("getLoadedServicesCount", HttpServletRequest.class); + + method.setAccessible(true); + + Integer ageMs = (Integer) method.invoke(servlet, req); + + assertTrue(ageMs >= 0L); Review Comment: In `testLoadedServicesCount()`, variable name `ageMs` is misleading since the reflected method returns a loaded-services count. Rename to something like `loadedServicesCount` to avoid confusion while reading the test. ########## pdp/src/test/java/org/apache/ranger/pdp/RangerPdpStatusServletTest.java: ########## @@ -0,0 +1,112 @@ +/* + * 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. + */ + +package org.apache.ranger.pdp; + +import org.apache.ranger.pdp.config.RangerPdpConfig; +import org.apache.ranger.pdp.config.RangerPdpConstants; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +import java.io.File; +import java.io.PrintWriter; +import java.io.StringWriter; +import java.lang.reflect.InvocationHandler; +import java.lang.reflect.Method; +import java.lang.reflect.Proxy; +import java.nio.file.Path; +import java.util.HashMap; Review Comment: Unused import `java.nio.file.Path` will fail the build under the repository Checkstyle `UnusedImports` rule (see dev-support/checkstyle.xml). Remove the import (it doesn't appear to be used in this test). ########## pdp/src/test/java/org/apache/ranger/pdp/RangerPdpStatusServletTest.java: ########## @@ -0,0 +1,112 @@ +/* + * 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. + */ + +package org.apache.ranger.pdp; + +import org.apache.ranger.pdp.config.RangerPdpConfig; +import org.apache.ranger.pdp.config.RangerPdpConstants; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + Review Comment: Unused import `org.junit.jupiter.api.io.TempDir` will fail the build under the repository Checkstyle `UnusedImports` rule (see dev-support/checkstyle.xml). Remove the import or use it in the test. ########## pdp/conf.dist/README-k8s.md: ########## @@ -0,0 +1,103 @@ +<!--- +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. +--> +# Ranger PDP Kubernetes Notes + +This document captures Kubernetes-oriented runtime recommendations for `ranger-pdp`. + +## Health Probes + +Use: + +- Liveness: `GET /health/live` +- Readiness: `GET /health/ready` + +The readiness endpoint reports NOT_READY until: + +- server is started +- authorizer is initialized +- PDP is accepting requests + +## Metrics + +PDP exposes Prometheus-style text metrics at: + +- `GET /metrics` + +Current metrics include request counts, auth failures, average latency and policy-cache age. Review Comment: The metrics description mentions "policy-cache age", but `/metrics` currently exposes request counters, auth failures, average latency, and loaded services only. Please update this sentence to match the actual metrics or add the missing metric. ```suggestion Current metrics include request counts, auth failures, average latency, and loaded services. ``` ########## pdp/src/main/java/org/apache/ranger/pdp/RangerPdpStatusServlet.java: ########## @@ -0,0 +1,132 @@ +/* + * 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. + */ + +package org.apache.ranger.pdp; + +import com.fasterxml.jackson.databind.ObjectMapper; +import org.apache.ranger.authz.embedded.RangerEmbeddedAuthorizer; +import org.apache.ranger.pdp.config.RangerPdpConfig; +import org.apache.ranger.pdp.config.RangerPdpConstants; + +import javax.servlet.ServletContext; +import javax.servlet.http.HttpServlet; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import javax.ws.rs.core.MediaType; + +import java.io.IOException; +import java.util.LinkedHashMap; +import java.util.Map; +import java.util.Set; + +public class RangerPdpStatusServlet extends HttpServlet { + private static final ObjectMapper MAPPER = new ObjectMapper(); + + private final RangerPdpStats runtimeState; + private final RangerPdpConfig config; + private final Mode mode; + + public enum Mode { LIVE, READY, METRICS } + + public RangerPdpStatusServlet(RangerPdpStats runtimeState, RangerPdpConfig config, Mode mode) { + this.runtimeState = runtimeState; + this.config = config; + this.mode = mode; + } + + @Override + protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException { + switch (mode) { + case LIVE: + writeLive(resp); + break; + case READY: + writeReady(req, resp); + break; + case METRICS: + writeMetrics(req, resp); + break; + default: + resp.setStatus(HttpServletResponse.SC_NOT_FOUND); + } + } + + private void writeLive(HttpServletResponse resp) throws IOException { + Map<String, Object> payload = new LinkedHashMap<>(); + + payload.put("status", runtimeState.isServerStarted() ? "UP" : "DOWN"); + payload.put("service", "ranger-pdp"); + payload.put("live", runtimeState.isServerStarted()); + + resp.setStatus(runtimeState.isServerStarted() ? HttpServletResponse.SC_OK : HttpServletResponse.SC_SERVICE_UNAVAILABLE); + resp.setContentType(MediaType.APPLICATION_JSON); + + MAPPER.writeValue(resp.getOutputStream(), payload); + } + + private void writeReady(HttpServletRequest req, HttpServletResponse resp) throws IOException { + boolean ready = runtimeState.isServerStarted() && runtimeState.isAuthorizerInitialized() && runtimeState.isAcceptingRequests(); + + Map<String, Object> payload = new LinkedHashMap<>(); + + payload.put("status", ready ? "READY" : "NOT_READY"); + payload.put("service", "ranger-pdp"); + payload.put("ready", ready); + payload.put("authorizerInitialized", runtimeState.isAuthorizerInitialized()); + payload.put("acceptingRequests", runtimeState.isAcceptingRequests()); + payload.put("loadedServicesCount", getLoadedServicesCount(req)); + + resp.setStatus(ready ? HttpServletResponse.SC_OK : HttpServletResponse.SC_SERVICE_UNAVAILABLE); + resp.setContentType(MediaType.APPLICATION_JSON); + + MAPPER.writeValue(resp.getOutputStream(), payload); + } + + private void writeMetrics(HttpServletRequest req, HttpServletResponse resp) throws IOException { + StringBuilder sb = new StringBuilder(512); + + sb.append("# TYPE ranger_pdp_requests_total counter\n"); + sb.append("ranger_pdp_requests_total ").append(runtimeState.getTotalRequests()).append('\n'); + sb.append("# TYPE ranger_pdp_requests_success_total counter\n"); + sb.append("ranger_pdp_requests_success_total ").append(runtimeState.getTotalAuthzSuccess()).append('\n'); + sb.append("# TYPE ranger_pdp_requests_bad_request_total counter\n"); + sb.append("ranger_pdp_requests_bad_request_total ").append(runtimeState.getTotalAuthzBadRequest()).append('\n'); + sb.append("# TYPE ranger_pdp_requests_error_total counter\n"); + sb.append("ranger_pdp_requests_error_total ").append(runtimeState.getTotalAuthzErrors()).append('\n'); + sb.append("# TYPE ranger_pdp_auth_failures_total counter\n"); + sb.append("ranger_pdp_auth_failures_total ").append(runtimeState.getTotalAuthFailures()).append('\n'); + sb.append("# TYPE ranger_pdp_request_latency_avg_ms gauge\n"); + sb.append("ranger_pdp_request_latency_avg_ms ").append(runtimeState.getAverageLatencyMs()).append('\n'); + sb.append("# TYPE ranger_pdp_loaded_services_count counter\n"); Review Comment: `ranger_pdp_loaded_services_count` is an instantaneous value, so Prometheus metric type should be `gauge` rather than `counter` (counters must be monotonically increasing). Update the `# TYPE` line to avoid misleading dashboards/alerts. ```suggestion sb.append("# TYPE ranger_pdp_loaded_services_count gauge\n"); ``` -- 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]
