MaikuMori / helm-charts

Maiku's Helm charts
https://artifacthub.io/packages/search?user=MaikuMori&sort=relevance
MIT License
18 stars 11 forks source link

Add `securityContext` compatibility with OpenShift platform #33

Closed jonasgeiler closed 3 months ago

jonasgeiler commented 4 months ago

This pull request makes the Gotenberg helm chart compatible with the OpenShift platform, which unfortunately requires specific securityContext settings to work. I adapted these changes from the bitnami helm chart repo where they include this OpenShift compatibility in all of their helm charts (see here and here). Basically I added a little helper which removes things like runAsUser, runAsGroup and fsGroup from the security context. I also added another helper which detects if the helm chart is being installed on an OpenShift cluster, which is used to automatically apply these compatibility changes. You can also force or disable this OpenShift detection using the new compatibility.openshift.adaptSecurityContext value.

Let me know if you have any further questions! We have tested this helm chart on our OpenShift cluster and it should not break anything for existing deployments.

Tasks:

MaikuMori commented 4 months ago

While I don't mind adding this, I have concerns about licensing.

The provided code is almost perfect copy of Bitnami code which is licensed by Broadcom, Inc. under Apache 2.0. MIT and Apache are compatible, but from my limited understanding this would effectively mean I need to include the whole license from Bitnami repository and state that the code has been modified slightly. I'm not a fan of complicating the licensing.

Alternatively, we could have our own implementation where the default value for security context check if it's running in OpenShift and removes the problematic fields. I personally prefer this since it doesn't make "magical" changes to security context when the user has specifically defined it. I feel like if someone manually sets security context then it should be honored as is and that also puts the responsibility of the actual value on the user.

Thoughts?

jonasgeiler commented 4 months ago

While I don't mind adding this, I have concerns about licensing.

The provided code is almost perfect copy of Bitnami code which is licensed by Broadcom, Inc. under Apache 2.0. MIT and Apache are compatible, but from my limited understanding this would effectively mean I need to include the whole license from Bitnami repository and state that the code has been modified slightly. I'm not a fan of complicating the licensing.

Hmmm I definitely see the problem... Although, I have found some other repos using the exact same snippet:

Maybe we could at least include the _compatibility.tpl header somehow:

{{/*
Copyright Broadcom, Inc. All Rights Reserved.
SPDX-License-Identifier: APACHE-2.0
*/}}

Or give credits in some other way.

We might also be able to ping the original author here: @javsalgar Would be great to hear his opinion on this! 🥺


Alternatively, we could have our own implementation where the default value for security context check if it's running in OpenShift and removes the problematic fields. I personally prefer this since it doesn't make "magical" changes to security context when the user has specifically defined it. I feel like if someone manually sets security context then it should be honored as is and that also puts the responsibility of the actual value on the user.

Unfortunately I'm not a 100% sure what you mean by this and what exactly you prefer about this solution 🤔 Could you give me a quick example?

MaikuMori commented 4 months ago

mean by this

From what I understand, currently, if you deploy this chart to OpenShift as is with default configuration, it will not work. What I want to do is to basically not add default security context if it's running on OpenShift.

I can provide code in a bit

jonasgeiler commented 4 months ago

@MaikuMori wrote: From what I understand, currently, if you deploy this chart to OpenShift as is with default configuration, it will not work.

That's not the case. The utility I have added automatically detects OpenShift and modifies the default securityContext to work with OpenShift (basically just removes runAsUser from securityContext in values.yaml). At our company we got it working with the default configuration - no problems there.

MaikuMori commented 4 months ago

Hi, I can't push to this PR for some reason. I've created a pr which implements what I meant: #34.

Do you think that's sufficient for the chart to work by default on OpenShift? If a user overwrites the default values, it's their responsibility to make sure it works on their specific setup or in general.

jonasgeiler commented 3 months ago

@MaikuMori wrote: Hi, I can't push to this PR for some reason. I've created a pr which implements what I meant: #34.

Do you think that's sufficient for the chart to work by default on OpenShift? If a user overwrites the default values, it's their responsibility to make sure it works on their specific setup or in general.

Not sure why you can't edit my PR but probably because I used the master branch instead of a new branch 😬 Anyway I added some comments in the PR, but it looks okay and works for me!

MaikuMori commented 3 months ago

Closed in favor of #34

jonasgeiler commented 3 months ago

Closed in favor of #34

Awesome! Thanks!