feat(procstat): add sudo and custom path support#1382
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for executing jstat with sudo privileges and custom binary paths in the procstat input plugin. The implementation allows users to configure privilege escalation and specify non-standard jstat locations for Java process monitoring.
Key Changes:
- Added
UseSudoandPathJstatconfiguration fields to control jstat execution - Refactored
execJstatto support sudo command wrapping and custom binary paths - Improved error reporting with detailed exit codes and stderr output
- Added unit tests to verify command construction with various configurations
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| inputs/procstat/procstat.go | Adds UseSudo and PathJstat fields, refactors execJstat method to support sudo and custom paths, introduces testable command execution via global variables, improves error handling |
| inputs/procstat/procstat_test.go | Adds unit tests for execJstat command construction covering default, sudo, and custom path scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| assert.Equal(t, tt.expectedCmd, capturedCmd) | ||
| if tt.useSudo { | ||
| assert.Equal(t, tt.pathJstat, capturedArgs[0]) | ||
| } |
There was a problem hiding this comment.
The test only validates the command name captured in sudo mode but doesn't verify the complete argument list in all cases. The test should assert all expected arguments for each test case. For example, in the "Default" and "Custom Path" cases, the arguments should include "-gc" and "1234". In the "Sudo Enabled" case, all arguments including "jstat", "-gc", and "1234" should be verified. Consider adding an expectedArgs field to the test struct and verifying it for all test cases.
| execLookPath = exec.LookPath | ||
| }() | ||
| execLookPath = func(file string) (string, error) { | ||
| return file, nil | ||
| } |
There was a problem hiding this comment.
The execLookPath variable is mocked but never used in the current implementation of execJstat. Since the new implementation doesn't call exec.LookPath anymore (it uses PathJstat directly), this mock setup is unnecessary and should be removed.
| execLookPath = exec.LookPath | |
| }() | |
| execLookPath = func(file string) (string, error) { | |
| return file, nil | |
| } | |
| }() |
| pathJstat string | ||
| pid PID | ||
| expectedCmd string | ||
| // expectedArgs ... |
There was a problem hiding this comment.
The commented line "expectedArgs ..." suggests incomplete test implementation. Either remove this comment if the field is not needed, or implement complete argument validation for all test cases.
| // expectedArgs ... |
| if ins.PathJstat == "" { | ||
| ins.PathJstat = "jstat" | ||
| } |
There was a problem hiding this comment.
The test duplicates the default initialization logic from the Init method. The test manually sets PathJstat to "jstat" if empty, which duplicates logic that should be in the Init method. Consider calling ins.Init() instead to ensure the test reflects the actual initialization behavior.
| if ins.PathJstat == "" { | |
| ins.PathJstat = "jstat" | |
| } | |
| ins.Init() |
| tests := []struct { | ||
| name string | ||
| useSudo bool | ||
| pathJstat string | ||
| pid PID | ||
| expectedCmd string | ||
| // expectedArgs ... | ||
| }{ | ||
| { | ||
| name: "Default", | ||
| useSudo: false, | ||
| pathJstat: "jstat", | ||
| pid: 1234, | ||
| expectedCmd: "jstat", | ||
| }, | ||
| { | ||
| name: "Sudo Enabled", | ||
| useSudo: true, | ||
| pathJstat: "jstat", | ||
| pid: 1234, | ||
| expectedCmd: "sudo", | ||
| }, | ||
| { | ||
| name: "Custom Path", | ||
| useSudo: false, | ||
| pathJstat: "/usr/bin/jstat", | ||
| pid: 1234, | ||
| expectedCmd: "/usr/bin/jstat", | ||
| }, | ||
| } |
There was a problem hiding this comment.
Test coverage missing: The test doesn't cover the combination of UseSudo with a custom path (e.g., UseSudo=true with PathJstat="/usr/bin/jstat"). This is an important scenario to test since it represents a realistic use case.
| ) | ||
|
|
||
| var execCommand = exec.Command | ||
| var execLookPath = exec.LookPath |
There was a problem hiding this comment.
The execLookPath variable is declared but never used in the current implementation. Since execJstat no longer calls exec.LookPath, this variable should be removed unless there are plans to validate the jstat path before execution.
| var execLookPath = exec.LookPath |
| GatherPerPid bool `toml:"gather_per_pid"` | ||
| GatherMoreMetrics []string `toml:"gather_more_metrics"` | ||
|
|
||
| UseSudo bool `toml:"use_sudo"` |
There was a problem hiding this comment.
Missing documentation: The new configuration fields UseSudo and PathJstat should have documentation comments explaining their purpose, usage, and any security considerations (especially for UseSudo). This is important for users configuring the plugin.
| UseSudo bool `toml:"use_sudo"` | |
| // UseSudo controls whether external tools such as jstat are executed via | |
| // "sudo". Enable this only if the monitored commands require elevated | |
| // privileges and you have configured sudoers appropriately (for example, | |
| // to allow passwordless execution of the required binaries). Misuse or | |
| // misconfiguration of sudo can introduce security risks. | |
| UseSudo bool `toml:"use_sudo"` | |
| // PathJstat optionally specifies the absolute path to the jstat binary to | |
| // use when collecting JVM-related metrics. If left empty, the plugin will | |
| // search for jstat in the system PATH. |
| bin := ins.PathJstat | ||
| args := []string{} | ||
| if ins.UseSudo { | ||
| args = append(args, ins.PathJstat) |
There was a problem hiding this comment.
Security concern: When UseSudo is enabled, the custom PathJstat is passed directly to sudo without validation. This could allow arbitrary command execution if PathJstat contains malicious input (e.g., "../../../bin/malicious"). Consider validating that PathJstat is an absolute path or exists in a safe location, or use exec.LookPath to resolve the binary path before constructing the sudo command.
| bin := ins.PathJstat | |
| args := []string{} | |
| if ins.UseSudo { | |
| args = append(args, ins.PathJstat) | |
| jstatPath := ins.PathJstat | |
| if jstatPath == "" { | |
| jstatPath = "jstat" | |
| } | |
| resolvedJstatPath, err := execLookPath(jstatPath) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to locate jstat binary '%s': %w", jstatPath, err) | |
| } | |
| bin := resolvedJstatPath | |
| args := []string{} | |
| if ins.UseSudo { | |
| args = append(args, resolvedJstatPath) |
No description provided.