Skip to content

Conversation

@rurod
Copy link
Contributor

@rurod rurod commented May 13, 2025

What this PR does / why we need it:
This PR updates the SecurityContextConstraints helm template. It allows Loki to use HostPath volumes in an OpenShift Environment.

Which issue(s) this PR fixes:
Fixes #17679

Special notes for your reviewer:
Feel free to suggest any change!

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Title matches the required conventional commits format, see here
    • Note that Promtail is considered to be feature complete, and future development for logs collection will be in Grafana Alloy. As such, feat PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
  • If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR

@rurod rurod requested a review from a team as a code owner May 13, 2025 09:34
@jkroepke
Copy link
Contributor

@rurod What did you think about leave the default to false and make the values configurable through the values.yaml?

@rurod
Copy link
Contributor Author

rurod commented Jun 30, 2025

Hi, thanks for having a look.

The issue here is the fact that hostPath is listed in the volumes while the allowHostDirVolumePlugin parameter defaults to false.

Thus OpenShift loops on the SCC and removes hostPath from the volumes list.

Then ArgoCD patches the SCC again to add hostPath to the volumes list.

Finally, the two controllers end up being in concurrence and they loop on it forever.

I see two solutions :

  1. Having a boolean that sets the value of allowHostDirVolumePlugin to true and append hostPath value to the list.
  2. Having the allowHostDirVolumePlugin set to true and the hostPath value in the volumes list by default.

What do you think @jkroepke ?

@jkroepke
Copy link
Contributor

Thanks for clarify that. I have not much OpenShift knowledge. But I understand the config drift leads to unstable ArgoCD syncs.

I also not understand the point of the SecurityContextConstraints. Normally, the service accounts of the deployment should be linked under users.

First, I had the feeling that ˋhostPath` should not be allowed by default, but I'm fine that administrators can enable hostPath if they wish.

I also found grafana/k8s-monitoring-helm#487 that introduces a toggle, like the mentioned option 2.

@jkroepke
Copy link
Contributor

jkroepke commented Jul 1, 2025

Hey @rurod

just to double check that. is that expected that your commits flagged as unverified?

@rurod
Copy link
Contributor Author

rurod commented Jul 1, 2025

Hi @jkroepke,

In a nutshell, SecurityContextConstraints is an interface for configuring multiple security tools/options like seccomp, SElinux and many others. If you have any interest in this topic, here is a quick overview : About Security Context Constraints.

I updated the PR to add the rbac.sccAllowHostDirVolumePlugin parameter so admins can decide whether or not they want to allow the use of hostPath volumes.

Let me know what you think.

@rurod
Copy link
Contributor Author

rurod commented Jul 1, 2025

Hey @rurod

just to double check that. is that expected that your commits flagged as unverified?

The signed commits are from the GitHub web ui, the unsigned ones are from my laptop as I am only using an Authentication Key and no Signing Key at the moment. Let me correct this.

@rurod rurod force-pushed the fix/hostpath-securitycontextconstraints branch from c9db550 to 7c99dce Compare July 1, 2025 19:36
@rurod
Copy link
Contributor Author

rurod commented Jul 1, 2025

Tried to sign the very first commit, result was not what I expected.
Yet, last ones are signed and verified and the mess should be hidden by squashing the commits while merging.

If you need me to update something, feel free to ping me @jkroepke, I'll go over the failed checks in the meantime.

@pull-request-size pull-request-size bot added size/S and removed size/XS labels Jul 1, 2025
Copy link
Contributor

@jkroepke jkroepke left a comment

Choose a reason for hiding this comment

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

LGTM

@Jayclifford345 Jayclifford345 enabled auto-merge (squash) July 29, 2025 09:26
@Jayclifford345 Jayclifford345 merged commit 8e61a5b into grafana:main Jul 29, 2025
75 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cannot use HostPath volumes when using SecurityContextConstraints

4 participants