[stable/graylog] Fix issue #13328, adjusting journal directory permissions

[juliohm1978]

This should fix #13328, by allowing the initContainer to correctly adjust permissions on the journal directory before the main graylog container starts.

[k8s-ci-robot]

Hi @juliohm1978. Thanks for your PR.

I'm waiting for a helm member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[jlegrone]

/ok-to-test

[jlegrone]

I believe it is a pretty common convention to have something like this in values.yaml:

securityContext:
  enabled: false
  fsGroup: 1100
  runAsUser: 1100

Then in your pod template:

{{- if .Values.securityContext.enabled }}
securityContext:
  fsGroup: {{ .Values.securityContext.fsGroup }}
  runAsUser: {{ .Values.securityContext.runAsUser }}
{{- end }}

Any reason not to set securityContext on the statefulset? It seems like this would avoid drift between expected and actual user/group IDs.

[juliohm1978]

securityContext changes the user that runs inside the container. But that's not the cause of problem.

The graylog container itself already runs with 1100:1100, and that's why it fails to access a journal directory owned by another uid:gid. If the initContainer from this chart gets the same uid:gid, it won't be able to chown the journal directory. It won't be able to do that with the same user as the graylog container.

The chown operation is not always possible, or easily accomplished from outside the cluster in some cases.

I suppose the names used in values.yaml deserve consideration, but my intention was to make it clear that we are not running the initContainer with a different uid:gid, but rather using those values to fix permissions.

[jlegrone]

securityContext changes the user that runs inside the container. But that's not the cause of problem.

Right, what I'm wondering though is whether there would be a reason not to also make uid/gid configurable for the graylog container as part of this PR. If the graylog image switches to a different uid/gid (or if we wanted to make this configurable), then following convention now would prevent breaking changes in the future.

[juliohm1978]

That is a good point.

However, the graylog image creates its user at build (Dockerfile). I doubt this can be changed at runtime without side effects. That is a more complicated PR on their side.

This is not a critical issue, and it's particular to some use cases only. It can be troubling to workaround, but it's not impossible.

Maybe better comments on values.yaml would avoid confusion. But I'm open to putting this PR on hold until we find a better way to implement this.

[juliohm1978]

Cross relating this concern with the Graylog repo, issue #76 on their side.

I'm still not sure about the proper way to resolve this.

[juliohm1978]

Sounds good to me. I should be able to commit this later tonight.

Thank you, Jacob!

[juliohm1978]

If PR #78 gets approved on their side, this one becomes unecessary. Let's see how that goes.

[juliohm1978]

@jlegrone

Would it make sense to chmod 777 /usr/share/graylog/data/journal? That would make the journal directory writable to anyone, and there would no be a need to hardcode the uid:gid in the chart template. Keep in mind that would chmod only the root journal directory, and not its data and subdirectories.

[colegatron]

@juliohm1978 's chmod 777 might sound wrong from a security perspective, but since no one aside the system admins has access to the journal volume and also thinking the journal data is ephemeral I think it is a good solution.

[juliohm1978]

I was able to rebase the PR with the chmod solution. Let me know if anything else is required.

[juliohm1978]

/test pull-charts-e2e

[juliohm1978]

/retest

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: juliohm1978
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: kongz

If they are not already assigned, you can assign the PR to them by writing /assign @kongz in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• stable/graylog/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[jlegrone]

Hold until Graylog2/graylog-docker#78 is reviewed by graylog folks.

/hold

[juliohm1978]

I'm closing this PR because of the conflict created by #12983. Apparently, that PR already pushed changes that include chown -R 1100:1100 ${GRAYLOG_HOME}/data/, which is the reason this one was created in the first place.

Thank you for the support!
Best regards.