gitea/helm-chart
45
55
Fork 126
Code Issues 32 Pull Requests 8 Actions Releases 87 Activity

Fix rootless image usage with enhanced security-context #160

Merged
luhahn merged 1 commits from justusbunsi/helm-chart:master into master 2021-06-07 13:27:26 +00:00
Conversation 6 Commits 1 Files Changed 2 +5 -2
justusbunsi commented 2021-05-15 17:59:18 +00:00
Member
Copy Link

I've noticed that the commented securityContext is not really useable with the rootless image due to different directory structure compared to the default image.

Important for the readOnlyRootFilesystem is to declare the TMPDIR environment variable, so that the tmp directory (which is readonly in this case) won't be used. Instead, another writeable directory can be used.

Another thing is the explicit hint that all these security options cannot be used with the default (root-based) image, because of its design.

Although this PR would fix the referenced issue, I am not totally happy with the current implementation. It would be more straight forward to use the same mount points for both image variants. Unfortunately, this is not possible right now due to hard coded paths in the default (root) image startup scripts.

Anyone have suggestions on how this could be more simple?

Sum-up:
As mentioned in Discord, this PR tried to make too many changes. The necessary changes made in 1f331a7e6577fc798196a84a957330aca0d663cd will fix an error that occurs due to restricted access to the /tmp directory in a rootless image with all the securityContext options enabled.

I also updated the default image to 1.14.2.

Fixes: #158

I've noticed that the commented `securityContext` is not really useable with the rootless image due to different directory structure compared to the default image. Important for the `readOnlyRootFilesystem` is to declare the `TMPDIR` environment variable, so that the tmp directory (which is readonly in this case) won't be used. Instead, another writeable directory can be used. Another thing is the explicit hint that all these security options cannot be used with the default (root-based) image, because of its design. ~~Although this PR would fix the referenced issue, I am not totally happy with the current implementation. It would be more straight forward to use the same mount points for both image variants. Unfortunately, this is not possible right now due to hard coded paths in the default (root) image startup scripts.~~ ~~Anyone have suggestions on how this could be more simple?~~ ------- **Sum-up:** As mentioned in Discord, this PR tried to make too many changes. The necessary changes made in 1f331a7e6577fc798196a84a957330aca0d663cd will fix an error that occurs due to restricted access to the `/tmp` directory in a rootless image with all the `securityContext` options enabled. I also updated the default image to 1.14.2. Fixes: #158
justusbunsi changed title from WIP: Fix rootless image usage with enhanced security-context to Fix rootless image usage with enhanced security-context 2021-05-22 12:20:40 +00:00
luhahn commented 2021-05-25 06:41:35 +00:00
Member
Copy Link

Thanks for the PR.

Will look at your changes this week.

Thanks for the PR. Will look at your changes this week.
justusbunsi commented 2021-05-30 16:52:26 +00:00
Author
Member
Copy Link

Maybe I found a solution for my mentioned worries regarding the current fix. But it includes a patch for the rootful docker image in the Gitea project. Currently, this root based image has hard coded /data references in its setup scripts and templates. Making this part of the image configurable during runtime by setting the GITEA_WORK_DIR environment variable inside the image to /data allows us to handle both image variants the same way in this helm chart.

I am currently working on these modifications in Gitea and provide a PR; it's useful anyway to allow such configuration ?. After it's merged, my suggested changes for this helm chart will be less complex since both images can use the same paths configured from inside the chart.

What do you say @luhahn? Shall we wait for the necessary modification in Gitea before making any further actions in this PR?

EDIT: Without having it tested yet, this is how this PR could be simplified.

Maybe I found a solution for my mentioned worries regarding the current fix. But it includes a patch for the rootful docker image in the Gitea project. Currently, this root based image has hard coded `/data` references in its setup scripts and templates. Making this part of the image configurable during runtime by setting the `GITEA_WORK_DIR` environment variable inside the image to `/data` allows us to handle both image variants the same way in this helm chart. I am currently working on these modifications in Gitea and provide a PR; it's useful anyway to allow such configuration ?. After it's merged, my suggested changes for this helm chart will be less complex since both images can use the same paths configured from inside the chart. What do you say @luhahn? Shall we wait for the necessary modification in Gitea before making any further actions in this PR? EDIT: Without having it tested yet, [this is how this PR could be simplified](https://gitea.com/justusbunsi/helm-chart/commit/2c5493d0fcc176a3814f7805e7c9a5ad4715dd45).
luhahn commented 2021-06-02 09:45:57 +00:00
Member
Copy Link

sorry again, our cluster had some major issues, which caused me to have no time at all. I will try to get back to you this week!

sorry again, our cluster had some major issues, which caused me to have no time at all. I will try to get back to you this week!
justusbunsi force-pushed master from 648e168f3b to 1f331a7e65 2021-06-05 14:46:08 +00:00 Compare
luhahn reviewed 2021-06-07 07:37:44 +00:00
luhahn left a comment
Member
Copy Link

Hi, I finally got some time to spend on gitea :)

I've got some questions regarding your PR.

Hi, I finally got some time to spend on gitea :) I've got some questions regarding your PR.
templates/gitea/statefulset.yaml Outdated
@ -77,6 +77,8 @@ spec:
value: /data
- name: GITEA_TEMP
value: /tmp/gitea
- name: TMPDIR
luhahn commented 2021-06-07 07:36:39 +00:00
Member
Copy Link

Little bit confused here.
Why do we need a second env variable for a tmp directory?

- name: GITEA_TEMP
  value: /tmp/gitea

GITEA_TEMP is already existing and pointing to the exact same directory

Little bit confused here. Why do we need a second env variable for a tmp directory? ```yaml - name: GITEA_TEMP value: /tmp/gitea ``` GITEA_TEMP is already existing and pointing to the exact same directory
luhahn marked this conversation as resolved
templates/gitea/statefulset.yaml Outdated
@ -135,3 +137,3 @@
volumeMounts:
- name: temp
mountPath: /tmp/gitea
mountPath: /tmp
luhahn commented 2021-06-07 07:37:06 +00:00
Member
Copy Link

why storing all the tmp data directly into tmp and not into a seperate directory?

why storing all the tmp data directly into tmp and not into a seperate directory?
luhahn marked this conversation as resolved
luhahn commented 2021-06-07 07:51:39 +00:00
Member
Copy Link

sorry, should've read your explanation before :D

sorry, should've read your explanation before :D
luhahn approved these changes 2021-06-07 07:57:34 +00:00
luhahn left a comment
Member
Copy Link

Looks good to me, tested on my cluster

Looks good to me, tested on my cluster
6543 approved these changes 2021-06-07 11:52:23 +00:00
6543 commented 2021-06-07 11:53:09 +00:00
Owner
Copy Link

@justusbunsi please "update branch" se we can merge it

@justusbunsi please "update branch" se we can merge it
justusbunsi force-pushed master from 1f331a7e65 to 2d43ae9c3c 2021-06-07 12:33:46 +00:00 Compare
justusbunsi commented 2021-06-07 12:34:04 +00:00
Author
Member
Copy Link

@6543 Done.

@6543 Done.
luhahn merged commit 5ab596937a into master 2021-06-07 13:27:26 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
luhahn
6543
Labels
Clear labels   
has
backport

  
in progress

   
invalid
  
kind
breaking

  
kind
bug

  
kind
build

  
kind
dependency

  
kind
deployment

  
kind
docs

  
kind
enhancement

  
kind
feature

  
kind
lint

  
kind
proposal

  
kind
question

  
kind
refactor

  
kind
security

  
kind
testing

  
kind
translation

  
kind
ui

  
need
backport

  
priority
critical

  
priority
low

  
priority
maybe

  
priority
medium

  
reviewed
duplicate

  
reviewed
invalid

  
reviewed
wontfix

  
skip-changelog
  
status
blocked

  
status
needs-feedback

  
status
needs-reviews

  
status
wip

  
upstream
gitea

  
upstream
other

No Label
has
backport
in progress
invalid
kind
breaking
kind
bug
kind
build
kind
dependency
kind
deployment
kind
docs
kind
enhancement
kind
feature
kind
lint
kind
proposal
kind
question
kind
refactor
kind
security
kind
testing
kind
translation
kind
ui
need
backport
priority
critical
priority
low
priority
maybe
priority
medium
reviewed
duplicate
reviewed
invalid
reviewed
wontfix
skip-changelog
status
blocked
status
needs-feedback
status
needs-reviews
status
wip
upstream
gitea
upstream
other
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: gitea/helm-chart#160
No description provided.
Delete Branch "justusbunsi/helm-chart:master"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?