Add /enableMonitoring API for JMX Agent Configuration#1926
Open
shreyabiradar07 wants to merge 2 commits into
Open
Add /enableMonitoring API for JMX Agent Configuration#1926shreyabiradar07 wants to merge 2 commits into
shreyabiradar07 wants to merge 2 commits into
Conversation
Signed-off-by: Shreya Biradar <shbirada@ibm.com>
Contributor
Reviewer's GuideAdds a new /enableMonitoring REST endpoint that programmatically enables JMX exporter–based monitoring on Java workloads in Kubernetes by mutating Deployments, creating ConfigMaps/Services/ServiceMonitors, and wiring in the JMX agent via JAVA_OPTS, along with the necessary RBAC and API objects. Sequence diagram for /enableMonitoring JMX setup flowsequenceDiagram
actor User
participant EnableMonitoring
participant KubernetesServices
User->>EnableMonitoring: HTTP POST /enableMonitoring
EnableMonitoring->>EnableMonitoring: doPost
EnableMonitoring->>EnableMonitoring: processContainer
EnableMonitoring->>KubernetesServices: getDeploymentBy(namespace, workloadName)
alt deployment found and workload_type == deployment
EnableMonitoring->>EnableMonitoring: enableJMXMonitoring(kubernetesServices, deployment, namespace, workloadName, containerName)
EnableMonitoring->>EnableMonitoring: createJMXConfigMap(client, namespace, configMapName)
EnableMonitoring->>KubernetesServices: replaceDeployment(namespace, workloadName, deployment)
alt replaceDeployment successful
EnableMonitoring->>KubernetesServices: restartDeployment(namespace, workloadName)
EnableMonitoring->>EnableMonitoring: updateServiceWithJMXPort(client, namespace, workloadName)
EnableMonitoring->>EnableMonitoring: createServiceMonitor(client, namespace, workloadName)
else replaceDeployment failed
EnableMonitoring-->>EnableMonitoring: mark container failure
end
else invalid workload or deployment missing
EnableMonitoring-->>EnableMonitoring: mark container failure
end
EnableMonitoring-->>User: JSON response with successful/failed lists
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Signed-off-by: Shreya Biradar <shbirada@ibm.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- The
EnableMonitoringservlet has grown very large and mixes HTTP handling, JSON parsing, K8s orchestration, and Prometheus wiring in a single class; consider extracting the Kubernetes/JMX configuration logic into dedicated service/utility classes to keep the servlet focused on request/response handling. - The servlet currently downloads the JMX exporter JAR from Maven Central on request and embeds it as
binaryDatain a ConfigMap, which introduces network latency/failure modes and large ConfigMaps; it would be more robust to source the JAR from a pre-baked image, a configurable internal URL, or a pre-created ConfigMap/volume. - The new
EnableJMXAgentAPIObjectandEnableJMXAgentResponseAPIObjecttypes are not used by the/enableMonitoringendpoint, which still manually builds/parses JSON; consider either wiring these POJOs into the API layer for type safety and consistency or removing them to avoid dead code.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `EnableMonitoring` servlet has grown very large and mixes HTTP handling, JSON parsing, K8s orchestration, and Prometheus wiring in a single class; consider extracting the Kubernetes/JMX configuration logic into dedicated service/utility classes to keep the servlet focused on request/response handling.
- The servlet currently downloads the JMX exporter JAR from Maven Central on request and embeds it as `binaryData` in a ConfigMap, which introduces network latency/failure modes and large ConfigMaps; it would be more robust to source the JAR from a pre-baked image, a configurable internal URL, or a pre-created ConfigMap/volume.
- The new `EnableJMXAgentAPIObject` and `EnableJMXAgentResponseAPIObject` types are not used by the `/enableMonitoring` endpoint, which still manually builds/parses JSON; consider either wiring these POJOs into the API layer for type safety and consistency or removing them to avoid dead code.
## Individual Comments
### Comment 1
<location path="src/main/java/com/autotune/analyzer/services/EnableMonitoring.java" line_range="228" />
<code_context>
+ if (!initContainerExists) {
+ Container initContainer = new Container();
+ initContainer.setName("jmx-exporter-init");
+ initContainer.setImage("busybox:latest");
+ initContainer.setCommand(List.of("sh", "-c",
+ "cp /config/" + JMX_JAR_FILE + " /jmx-exporter/ && " +
</code_context>
<issue_to_address>
**🚨 issue (security):** Avoid using the `latest` tag for the init container image to ensure reproducibility and reduce supply-chain risk.
Using `busybox:latest` makes deployments non-deterministic and may pull unvetted images, which complicates security reviews and SBOM generation. Please pin to a specific trusted tag (for example, `busybox:1.36.1`) or an internal, controlled base image.
</issue_to_address>
### Comment 2
<location path="src/main/java/com/autotune/analyzer/services/EnableMonitoring.java" line_range="443-452" />
<code_context>
+ private boolean createJMXConfigMap(KubernetesClient client, String namespace, String configMapName) {
</code_context>
<issue_to_address>
**🚨 issue (security):** Downloading the JMX exporter JAR at runtime from Maven Central is fragile and poses security and operability concerns.
This downloads the agent JAR over the network on each enablement, which (1) breaks in air‑gapped/restricted clusters, (2) adds a hard runtime dependency on Maven Central, (3) increases startup latency, and (4) expands the supply‑chain attack surface. Prefer bundling the JAR into a known image (init/sidecar) or mounting it from a pre-provisioned ConfigMap/Secret created at install time, so the JAR isn’t fetched on each request and doesn’t depend on live external endpoints.
</issue_to_address>
### Comment 3
<location path="src/main/java/com/autotune/analyzer/services/EnableMonitoring.java" line_range="204" />
<code_context>
+ KubernetesClient client = null;
+ try {
+ // Create Kubernetes client
+ client = new DefaultKubernetesClient();
+
+ // Step 1: Create ConfigMap with JMX exporter configuration
</code_context>
<issue_to_address>
**suggestion (performance):** Creating a new `DefaultKubernetesClient` inside `enableJMXMonitoring` for each call is unnecessary overhead.
This creates and closes a new `DefaultKubernetesClient` for every container, despite an existing `KubernetesServices` instance at the servlet level. This adds avoidable connection setup/teardown and duplicates configuration. Please reuse a shared client (e.g., from `KubernetesServicesImpl`) or inject a client so a single long‑lived instance is used per servlet rather than per call.
Suggested implementation:
```java
private boolean enableJMXMonitoring(KubernetesServices kubernetesServices, Deployment deployment,
String namespace, String workloadName, String containerName) {
KubernetesClient client = kubernetesServices.getClient();
try {
// Step 1: Create ConfigMap with JMX exporter configuration
```
1. Ensure that the `KubernetesServices` interface (and its implementation, e.g. `KubernetesServicesImpl`) exposes a `getClient()` method returning the shared `KubernetesClient` instance.
2. Confirm that the shared `KubernetesClient` lifecycle (creation/closing) is managed at the servlet/application level, not inside `enableJMXMonitoring`, and that this method does not close the client.
3. If the existing accessor method on `KubernetesServices` uses a different name (e.g. `getKubernetesClient()`), adjust the call accordingly.
</issue_to_address>
### Comment 4
<location path="src/main/java/com/autotune/analyzer/services/EnableMonitoring.java" line_range="553" />
<code_context>
+ LOGGER.info("Found Service {} for workload {} in namespace {}", serviceName, workloadName, namespace);
+
+ // Check if JMX metrics port already exists
+ List<ServicePort> ports = service.getSpec().getPorts();
+ boolean jmxPortExists = ports.stream()
+ .anyMatch(p -> p.getName() != null && p.getName().equals(JMX_PROMETHEUS_PORT_NAME));
</code_context>
<issue_to_address>
**issue (bug_risk):** `service.getSpec().getPorts()` can be null, which would cause a NullPointerException in `updateServiceWithJMXPort`.
If the Service has no ports, `service.getSpec().getPorts()` can be null, so `ports.stream()` and later `ports.add(jmxPort)` will throw an NPE. Initialize `ports` when null (e.g. `ports = new ArrayList<>()` and set it back on the spec) before streaming or adding entries.
</issue_to_address>
### Comment 5
<location path="manifests/crc/default-db-included-installation/openshift/kruize-crc-openshift.yaml" line_range="88-93" />
<code_context>
- apiGroups: [ "" ]
resources: [ "namespaces" ]
verbs: [ "get", "list" ]
+ - apiGroups: [ "" ]
+ resources: [ "configmaps" ]
+ verbs: [ "create", "get", "list", "update", "patch", "delete" ]
+ - apiGroups: [ "" ]
+ resources: [ "services" ]
</code_context>
<issue_to_address>
**🚨 question (security):** The new RBAC rules grant broad permissions (including delete) on ConfigMaps and ServiceMonitors; consider tightening to least privilege.
Given this flow only needs to manage a small set of our own ConfigMaps/Services/ServiceMonitors, broad cluster-wide CRUD (including delete) significantly enlarges the impact of any compromise. Please narrow the permissions where possible (e.g., drop `delete` if not strictly needed, scope to specific namespaces, or use `resourceNames`) to better follow least-privilege.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if (!initContainerExists) { | ||
| Container initContainer = new Container(); | ||
| initContainer.setName("jmx-exporter-init"); | ||
| initContainer.setImage("busybox:latest"); |
Contributor
There was a problem hiding this comment.
🚨 issue (security): Avoid using the latest tag for the init container image to ensure reproducibility and reduce supply-chain risk.
Using busybox:latest makes deployments non-deterministic and may pull unvetted images, which complicates security reviews and SBOM generation. Please pin to a specific trusted tag (for example, busybox:1.36.1) or an internal, controlled base image.
Comment on lines
+443
to
+452
| private boolean createJMXConfigMap(KubernetesClient client, String namespace, String configMapName) { | ||
| try { | ||
| // Default JMX exporter configuration | ||
| String jmxConfig = "lowercaseOutputName: true\n" + | ||
| "lowercaseOutputLabelNames: true\n" + | ||
| "rules:\n" + | ||
| "- pattern: \".*\"\n"; | ||
|
|
||
| // Download JMX exporter JAR | ||
| LOGGER.info("Downloading JMX exporter JAR from {}", JMX_EXPORTER_DOWNLOAD_URL); |
Contributor
There was a problem hiding this comment.
🚨 issue (security): Downloading the JMX exporter JAR at runtime from Maven Central is fragile and poses security and operability concerns.
This downloads the agent JAR over the network on each enablement, which (1) breaks in air‑gapped/restricted clusters, (2) adds a hard runtime dependency on Maven Central, (3) increases startup latency, and (4) expands the supply‑chain attack surface. Prefer bundling the JAR into a known image (init/sidecar) or mounting it from a pre-provisioned ConfigMap/Secret created at install time, so the JAR isn’t fetched on each request and doesn’t depend on live external endpoints.
| KubernetesClient client = null; | ||
| try { | ||
| // Create Kubernetes client | ||
| client = new DefaultKubernetesClient(); |
Contributor
There was a problem hiding this comment.
suggestion (performance): Creating a new DefaultKubernetesClient inside enableJMXMonitoring for each call is unnecessary overhead.
This creates and closes a new DefaultKubernetesClient for every container, despite an existing KubernetesServices instance at the servlet level. This adds avoidable connection setup/teardown and duplicates configuration. Please reuse a shared client (e.g., from KubernetesServicesImpl) or inject a client so a single long‑lived instance is used per servlet rather than per call.
Suggested implementation:
private boolean enableJMXMonitoring(KubernetesServices kubernetesServices, Deployment deployment,
String namespace, String workloadName, String containerName) {
KubernetesClient client = kubernetesServices.getClient();
try {
// Step 1: Create ConfigMap with JMX exporter configuration- Ensure that the
KubernetesServicesinterface (and its implementation, e.g.KubernetesServicesImpl) exposes agetClient()method returning the sharedKubernetesClientinstance. - Confirm that the shared
KubernetesClientlifecycle (creation/closing) is managed at the servlet/application level, not insideenableJMXMonitoring, and that this method does not close the client. - If the existing accessor method on
KubernetesServicesuses a different name (e.g.getKubernetesClient()), adjust the call accordingly.
| LOGGER.info("Found Service {} for workload {} in namespace {}", serviceName, workloadName, namespace); | ||
|
|
||
| // Check if JMX metrics port already exists | ||
| List<ServicePort> ports = service.getSpec().getPorts(); |
Contributor
There was a problem hiding this comment.
issue (bug_risk): service.getSpec().getPorts() can be null, which would cause a NullPointerException in updateServiceWithJMXPort.
If the Service has no ports, service.getSpec().getPorts() can be null, so ports.stream() and later ports.add(jmxPort) will throw an NPE. Initialize ports when null (e.g. ports = new ArrayList<>() and set it back on the spec) before streaming or adding entries.
Comment on lines
88
to
+93
| - apiGroups: [ "" ] | ||
| resources: [ "namespaces" ] | ||
| verbs: [ "get", "list" ] | ||
| - apiGroups: [ "" ] | ||
| resources: [ "configmaps" ] | ||
| verbs: [ "create", "get", "list", "update", "patch", "delete" ] |
Contributor
There was a problem hiding this comment.
🚨 question (security): The new RBAC rules grant broad permissions (including delete) on ConfigMaps and ServiceMonitors; consider tightening to least privilege.
Given this flow only needs to manage a small set of our own ConfigMaps/Services/ServiceMonitors, broad cluster-wide CRUD (including delete) significantly enlarges the impact of any compromise. Please narrow the permissions where possible (e.g., drop delete if not strictly needed, scope to specific namespaces, or use resourceNames) to better follow least-privilege.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Introduces a new REST API endpoint to automatically configure JMX monitoring for Java workloads in Kubernetes.
Changes
/enableMonitoringPOST endpoint for automated JMX agent setupFixes # (issue)
Type of change
How has this been tested?
Please describe the tests that were run to verify your changes and steps to reproduce. Please specify any test configuration required.
Test Configuration
Checklist 🎯
Additional information
Include any additional information such as links, test results, screenshots here
Summary by Sourcery
Introduce an /enableMonitoring REST endpoint that configures JMX exporter-based monitoring for Java Kubernetes workloads, wiring it into the analyzer service and Kubernetes integration layer.
New Features:
Enhancements:
Deployment: