feat(deployment): make securityContext configurable for initContainers only #715

2024-10-11T08:11:42Z

dti-horvath commented

2024-10-11 08:11:42 +00:00

(Migrated from gitea.com)

Description of the change

Add a new parameter to set the securityContext for initContainers only.

Benefits

initContainers may need other rights for setting up Gitea. E.g.: special userid which is allowed to write to a specific volume or is allowed to connect to a specific network.

Additional information

This change rose from the need to set a specific userid for initContainers when running with Istio. Istio handles initContainers differently, so that it is needed to define security constraints to them.

Applicable issues

Fixes #671

Checklist

Parameters are documented in the values.yaml and added to the README.md using readme-generator-for-helm

### Description of the change Add a new parameter to set the `securityContext` for `initContainers` only. ### Benefits `initContainers` may need other rights for setting up Gitea. E.g.: special userid which is allowed to write to a specific volume or is allowed to connect to a specific network. ### Additional information This change rose from the need to set a specific userid for `initContainers` when running with Istio. Istio handles `initContainers` differently, so that it is needed to define security constraints to them. ### Applicable issues - Fixes #671 ### Checklist - [X] Parameters are documented in the `values.yaml` and added to the `README.md` using [readme-generator-for-helm](https://github.com/bitnami-labs/readme-generator-for-helm)

pat-s (Migrated from gitea.com) reviewed 2024-10-11 08:11:42 +00:00

justusbunsi (Migrated from gitea.com) reviewed 2024-10-18 13:57:59 +00:00

justusbunsi (Migrated from gitea.com) left a comment

Please add some unit tests to verify that combining initContainerSecurityContext and containerSecurityContext works as expected.

Please add some unit tests to verify that combining `initContainerSecurityContext` and `containerSecurityContext` works as expected.

justusbunsi commented

2024-11-10 15:31:06 +00:00

(Migrated from gitea.com)

First of all: Thanks for contributing to this repository. We appreciate your efforts.

Also thanks for adding the requested tests. Looking at the changes from Helm and Kubernetes point-of-view, they are valid and I was tempted to merge this Pull Request.
However, reviewing the added tests I doubted that the current solution would result in a working Gitea application. Trying to apply those values as a Helm release confirms my concerns:

Using following values lead to a crash-looping Gitea container, stating missing permissions to create '/data/gitea/git`

initContainerSecurityContext:
  runAsUser: 1001
signing:
  enabled: true
  existingSecret: "custom-gpg-key"

We have hard-coded chown 1000:1000 in our scripts¹. Most of them are executed in the first init container (init-directories) which requires that particular init container to run as root - without having a runAsUser at all.

Mixing different initContainerSecurityContext.runAsUser and containerSecurityContext.runAsUser lead to the same error, and make the hard-coded vs. default values even worse: The rootless Gitea image uses 1000:1000 as default uid:gid mapping, leading to a filesystem that is owned by e.g. 1001:1000. So Gitea is not able to start.

My first thought on fixing this was to just overwrite UID and GID envs for Gitea container and replace the hard-coded 1000 with placeholders for these envs. But this doesn't resolve the root-cause of doing too many different things in different contexts as init containers. This is not only an Istio-related issue².

A real long-term solution is moving the init container logic into Helm hook jobs prior to starting Gitea itself. That way, no init container exists and e.g. Istio doesn't handle them differently.

A solution for the current init container way might be to support separate securityContexts for every init container without hidden defaults in the templates or shell scripts. But we have to make the configuration resistent to user configuration errors, or we cause avoidable issues.

To be honest, I am not sure if we should directly long for the job-based solution or have the per-init-container-context solution first. Right now, your proposed solution is not ready to be merged. I welcome your (@dti-horvath) thoughts on this topic - and those of anyone else reading this.

First of all: Thanks for contributing to this repository. We appreciate your efforts. Also thanks for adding the requested tests. Looking at the changes from Helm and Kubernetes point-of-view, they are valid and I was tempted to merge this Pull Request. However, reviewing the added tests I doubted that the current solution would result in a working Gitea application. Trying to apply those values as a Helm release confirms my concerns: Using following values lead to a crash-looping Gitea container, stating missing permissions to create '/data/gitea/git` ```yaml initContainerSecurityContext: runAsUser: 1001 signing: enabled: true existingSecret: "custom-gpg-key" ``` We have hard-coded `chown 1000:1000` in our scripts[^1]. Most of them are executed in the first init container (`init-directories`) which requires that particular init container to run as root - without having a `runAsUser` at all. Mixing different `initContainerSecurityContext.runAsUser` and `containerSecurityContext.runAsUser` lead to the same error, and make the `hard-coded vs. default values` even worse: The rootless Gitea image uses `1000:1000` as default uid:gid mapping, leading to a filesystem that is owned by e.g. 1001:1000. So Gitea is not able to start. My first thought on fixing this was to just overwrite `UID` and `GID` envs for Gitea container and replace the hard-coded `1000` with placeholders for these envs. But this doesn't resolve the root-cause of doing too many different things in different contexts as init containers. This is not only an Istio-related issue[^2]. A real long-term solution is moving the init container logic into Helm hook jobs prior to starting Gitea itself. That way, no init container exists and e.g. Istio doesn't handle them differently. A solution for the current init container way might be to support separate `securityContext`s for every init container without hidden defaults in the templates or shell scripts. But we have to make the configuration resistent to user configuration errors, or we cause avoidable issues. To be honest, I am not sure if we should directly long for the job-based solution or have the per-init-container-context solution first. Right now, your proposed solution is not ready to be merged. I welcome your (@dti-horvath) thoughts on this topic - and those of anyone else reading this. [^1]: https://gitea.com/gitea/helm-chart/issues/338#issuecomment-756745 [^2]: https://gitea.com/gitea/helm-chart/issues?state=open&type=all&q=runAsUser (excluding 643)

This pull request can be merged automatically.

You are not authorized to merge this pull request.

View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin dti-horvath/feat/sec-context-for-init:dti-horvath/feat/sec-context-for-init

git checkout dti-horvath/feat/sec-context-for-init

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: lunny/helm-chart#715