Unable to set securityContext for controller containers

[gacopl]

Description

Observed Behavior:
This #4278 removed capability to set custom security contexts

It should allow to set them and just have defaults instead of removing whole capability

Unable to install on hardened clusters which require containers to have set contexts

Expected Behavior:
Ability to set own podSecurityContext containerSecurityContext and controllerSecurityContext
Reproduction Steps (Please include YAML):
Try to install on any cluster with gatekeeper that requires fsGroups on container or supplementalGroups
Versions:

• Chart Version: 0.31 and up
• Kubernetes Version (kubectl version):

• Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
• Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritize the request
• If you are interested in working on this issue or have submitted a pull request, please leave a comment

[jonathan-innis]

Unable to install on hardened clusters which require containers to have set contexts

Which part of the securityContext do you need to be able to set and doesn't work in the default configuration?

[gacopl]

for starters here runAs fsGroup etc is not settable, i don't understand the apporach, you should make defaults but allow to override them, first time i see this on any k8s addon i use

[jonathan-innis]

fsGroup is in the podSecurityContext and is settable in the latest versions of Karpenter. I think the general thinking was: Why do you need to set the security context of the container? Can we pick a sensible default that works for everyone?

[gacopl]

the sensible default is ability to override you will not cover all the cases anyway,
at the same time i cannot disclose what gatekeeper rules we use, just revert that PR and put the defaults in values.yaml
for container security context imagine any operator injecting init container that will use different group id

i don't wanna argue about this but all other projects just allow override

[jonathan-innis]

any operator injecting init container that will use different group id

If you inject an init container, can't you set the security context specifically for that?

at the same time i cannot disclose what gatekeeper rules we use, just revert that PR

I'll look around at some other charts, but it's hard for me to justify reverting a PR without having a concrete reason to do so.

[gacopl]

The reason is to have more flexibility and coverage of use cases :) also charting best practices.

I don't want to convince you it's not like I ask to change defaults just to have possibilities

[jonathan-innis]

The reason is to have more flexibility and coverage of use cases

I'll take a guess -- because I think I've seen valid use-cases for this (as I was refreshing my memory on this, I realized that I had this conversation with someone else before): #5794. Given I can see cases where you would want to be able to inject a different value per-container for this kind of thing, I can see an argument to opening this up. To your point, we don't lose a lot from doing it.

An even bigger question -- because I'm curious -- are there specific parts of the security context that you need us to open up or just something like the runAsUser. This is really the only value that I can imagine that you would need to/want to change and.

also charting best practices

Do you mind pointing me to a few examples? I did a quick search-around and I saw some differences here and there. Some seem to just allow runAsUser while others seem to open up the whole thing.

cc: @stevehipwell

[gacopl]

the stuff i used was opening the whole thing and in my opinion that's what karpenter should do too, considering its possible future adoption.

also i've seen some projects trying to be to detailed in their charts and they ended up with chart and values hell imo giving sane defaults for sections and opening them up to override its best road. otherwise instead of maintaining code you will end up maintaining charts :)

[jonathan-innis]

otherwise instead of maintaining code you will end up maintaining charts

Makes sense. If this was something that you wanted to propose to add, we'd be receptive to it. I'd personally go for a securityContext block under both the controller and webhook sections in the values.yaml file to handle both of the container contexts.

[gacopl]

yes under both sections plus bring back podSecurityContext

[stevehipwell]

@jonathan-innis @gacopl I can't see an actual issue in the current chart logic, although I can think of a couple of optimisations.

The podSecurityContext value is currently fully configurable; but I think we should probably provide some better defaults than just setting fsGroup.

Locking the Karpenter container security context down is IMHO absolutely the right thing to do for a first party Helm chart. Why would the user have any need to change most of these settings when they are explicitly tied to the logic and OCI image? That said I can potentially see a use for exposing seccompProfile & seLinuxOptions as these are the only options which are potentially end user driven.

Any additional containers added can have their security context set to whatever the end user desires, and as the podSecurityContext is open there are no fixed limitations on that.

[gacopl]

I have to fully disagree with @stevehipwell, first party Helm chart wannabe should absolutely give ability to control how the product is being run on user cluster, providing sane defaults with ability to override is the way to go many other projects successfully did. And if not override, add your defaults but allow to add whatever else user might need if you want to be strict. But this is not standalone product its plugin so you should allow some freedom for advanced users if you want adoption

Honestly I am surprised we have to discuss this it seemed like a no-brainer...

[gacopl]

I mean just look at how cluster-autoscaler is handlling it that's it

[stevehipwell]

@gacopl Karpenter (karpenter-aws-provider to be exact) is a first party Helm chart, where the service is targeting a single cloud vendor (also first party), with a direct SemVer relationship between the binary and Helm chart; this all means that the valid use pattern is as consistent as possible. I can't think of anything other than seccompProfile & seLinuxOptions that you could have a valid reason to modify on the pod security context.

I mean just look at how cluster-autoscaler is handlling it that's it

I'm pretty familiar with that project/chart having rewritten the chart before handing it back to be maintained. The difference there is the context; the primary maintainers aren't focused on Helm and the service itself is very much multi-cloud. This means that the potential use patterns are very different and the maintainers can't or don't want to handle the overhead of having fixed values.

Let's just acknowledge that the "perfect" Helm chart would be one with no input variables that still managed to meet each individual end-user's requirements. The closest we can get to this is either the operator pattern (notice how locked down operator pod specs tend to be) or a Helm chart well aligned to the use case (such as Karpenter).

To take this further please could you provide some use cases which you're currently blocked from achieving due to the Karpenter Helm chart?

[gacopl]

supplemental groups, seccomp, se..

unless you want to set defaults for all these which probably will not be good for everyone - override makes most sense. I don't understand why you don't want to agree to at least have extension possibility to add stuff besides whatever you specify.

i won't spend time arguing as i did - back in the day of EKS preview, if you don't plan to be more open we will just call it typical aws non-sesne and fork the chart - you will come around eventually

[stevehipwell]

supplemental groups, seccomp, se..

@gacopl of those 3 I'd already mentioned 2. I think customisable supplemental groups could be something the user might want to modify but I'm not sure of any use cases relevant to Karpenter? You've still not actually provided any use cases yet though, could you please share the ones which led to this issue?

FYI the beauty of OSS is that you can take something and modify it to fit your needs, although you need to want it enough to put the resources in to maintain the fork. In this case you don't even have to fork the chart, you could just use a post render function like kustomize to overlay a custom patch.

I'm not AWS BTW, just an OSS contributor.

[gacopl]

Steve like stated above I won't put in what exactly rules we use in gatekeeper that require us to modify the context from your default values, trying to find default values that will work for everyone is just vain

This was to open up the possibilities and shed light on this imo bad practice

There are multiple ways to workaround this kustomize that's what we usually do in more complex cases, nothing new to us but simply prefer to not play circus tricks over something that should be set right from the start. Fix the root

Anyways, If you don't intend changing approach feel free to close issue I won't play convincing game as I don't have time for it :)

Someone already brought it before me someone will bring it again eventually.

As for the OSS part I even more cannot understand the approach as the approach should be open too if it's OSS.
People run clusters in many ways, trying to enforce one right way by default non customizable chart values in some beta project is surprising.
Begs a question why even bother running it through helm if you want to have such strict approach idk

Godspeed