Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: add Linux capabilities config for pyroscope java #3599

Closed
wants to merge 2 commits into from

Conversation

marcsanmi
Copy link
Contributor

@marcsanmi marcsanmi requested review from a team as code owners October 1, 2024 15:52
@marcsanmi marcsanmi force-pushed the marcsanmi/update-java-examples-docs branch from b0f4205 to a83a4ee Compare October 2, 2024 09:12
@jdbaldry
Copy link
Member

jdbaldry commented Oct 2, 2024

Would the team consider backporting this to the latest release? I'd love to confirm that the backport workflow, most recently modified in #3585, is behaving as intended.

docs/sources/configure-client/grafana-agent/java/_index.md Outdated Show resolved Hide resolved
Comment on lines +84 to +95
```yaml
alloy:
securityContext:
runAsUser: 0
runAsNonRoot: false
capabilities:
add:
- PERFMON
- SYS_PTRACE
- SYS_RESOURCE
- SYS_ADMIN
```
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technical question: If we are setting runAsUser: 0 and runAsNonRoot: false, does the process not already have complete capabilities as the process is running as the root user?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For context, this comes from grafana/alloy#1616.

In Kubernetes, runAsUser: 0 and runAsNonRoot: false run the container as root. However, root privileges don't automatically grant all capabilities in containerized environments. The capabilities setting adds specific capabilities, but its exact behavior depends on the Container Runtime Interface (CRI) used.

AFAIK, in Docker, "The operator can whitelist specific capabilities, and any capabilities not whitelisted are dropped from the container." However, other CRIs may behave differently... 🤔

Maybe adding a comment such as:

# Note: Capability behavior depends on CRI settings. In Docker, non-whitelisted capabilities are dropped by default.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for explaining :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you choose to add the note, consider changing whitelisted to allowlisted instead.

Co-authored-by: Jack Baldry <jack.baldry@grafana.com>
@marcsanmi marcsanmi added type/docs Improvements for doc docs. Used by Docs team for project management backport labels Oct 3, 2024
Comment on lines +43 to +63
## Additional Configuration for Linux Capabilities

If your Kubernetes environment has Linux capabilities enabled, configure the following in your Helm values to ensure `pyroscope.java` functions properly:

```yaml
alloy:
securityContext:
runAsUser: 0
runAsNonRoot: false
capabilities:
add:
- PERFMON
- SYS_PTRACE
- SYS_RESOURCE
- SYS_ADMIN
```
These capabilities enable Alloy to access performance monitoring subsystems, trace processes, override resource limits, and perform necessary system administration tasks for profiling.
{{< admonition type="note" >}}
Adjust capabilities based on your specific security requirements and environment, following the principle of least privilege.
{{< /admonition >}}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if this section makes sense to be in this file. We don't use Helm in this example so we can't configure this. Having it in the public docs is definitely great.

On a separate note, the {{<admonition>}} macros wouldn't work in these files afaik.

Copy link
Contributor Author

@marcsanmi marcsanmi Oct 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, this can be confusing. While alloy is listed as a dependency in pyroscope chart, we're configuring it in our values.yaml. Even moving this configuration next to the pyroscope chart wouldn't make sense.

On a separate note, the {{}} macros wouldn't work in these files afaik.

Good catch! This was me copy-pasting from the other PR 😅

I'm closing it.

Copy link
Contributor

@knylander-grafana knylander-grafana Oct 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made a suggestion to change the admonition shortcode to one used by GitHub markdown.

Instead of including the duplicate contents in the README, you can also just link to the other docs that have the Helm info from the README.

@@ -78,6 +78,27 @@ see [profiler-options](https://github.com/async-profiler/async-profiler?tab=read
You must run the collector, either Grafana Alloy (recommended) or Agent (legacy), as root and inside host `pid` namespace for the `pyroscope.java`
and `discover.process` components to work.

### Additional configuration for Linux capabilities
Copy link
Contributor

@knylander-grafana knylander-grafana Oct 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
### Additional configuration for Linux capabilities
### Optional: Configure Linux capabilities

@@ -40,6 +40,27 @@ After the container is operational, the Grafana Agent profiles the Java applicat

You need root privileges to run the Grafana Agent for profiling. The Agent must be executed within the host's PID namespace.

## Additional Configuration for Linux Capabilities
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
## Additional Configuration for Linux Capabilities
## Optional: Configure Linux capabilities

Comment on lines +60 to +62
{{< admonition type="note" >}}
Adjust capabilities based on your specific security requirements and environment, following the principle of least privilege.
{{< /admonition >}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change Hugo shortcode to GitHub flavored Markdown admonition.

Suggested change
{{< admonition type="note" >}}
Adjust capabilities based on your specific security requirements and environment, following the principle of least privilege.
{{< /admonition >}}
> [!NOTE]
> Adjust capabilities based on your specific security requirements and environment, following the principle of least privilege.

Copy link
Contributor

@knylander-grafana knylander-grafana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving changes, pending your best opinions for where the content should be included.

Do we need an updated backport tag for 1.9?

@knylander-grafana knylander-grafana added the backport release/v1.8 This label will backport a merged PR to the release/v1.8 branch label Oct 4, 2024
@marcsanmi
Copy link
Contributor Author

As suggested, I'm closing it to avoid confusion :)

@marcsanmi marcsanmi closed this Oct 4, 2024
Copy link
Contributor

github-actions bot commented Oct 4, 2024

This PR must be merged before a backport PR will be created.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport release/v1.8 This label will backport a merged PR to the release/v1.8 branch backport type/docs Improvements for doc docs. Used by Docs team for project management
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants