-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix docker image permissions of /pulsar for OpenShift #8242
Conversation
…the pulsar docker image
5791541
to
6d54827
Compare
The change seems to fix a very narrow problem. Does this approach work on other platforms? |
@sijie it is indeed a narrow problem, but it prevents us from deploying on openshift as it has stricter security requirements by default than vanilla kubernetes. On the other hand it is a change with very limited impact. Not tested on other platforms. Default group for a pod is the root group though. |
move to 2.8.0 first. |
I just opened #8751 to request that the pulsar I am not familiar with OpenShift, so I cannot speak to a solution for this specific problem, but I do think it is a security best practice to give applications the least necessary permission to run. As such, I do not think this PR should be merged. |
@michaeljmarshall The PR only gives the root group the necessary permissions to access the /pulsar folder. This is because on an openshift cluster, containers are run using a random user ID. An extract from the openshift documentation, see section "Support arbitrary user ids" (https://docs.openshift.com/container-platform/4.5/openshift_images/create-images.html)
In conclusion, I think this PR allows for better security by supporting running pulsar by an arbitrary user ID. |
@fransguelinckx - thanks for the added context. I am not familiar with OpenShift's container platform, so the context is helpful. It seems to me that the challenge here comes from competing paradigms between how OpenShift runs containers by default and how EKS (and other platforms) recommend running containers. OpenShift is prescriptive about running the user as part of the root group (so it can ensure containers have the right file permissions) and yet EKS recommends just the opposite (because it expects users to ensure file permissions are correct). Here are the Amazon docs that detail best practices for using a fsGroup:
rule: 'MustRunAs'
ranges:
# Forbid adding the root group.
- min: 1
max: 65535 The docker image produced by the I'm not entirely sure how to best accommodate both paradigms. While OpenShift allows users to override a pod's UID/GID by specifying
If you're deploying your bookkeeper pods on dedicated hosts, I think that removes their main concern because other namespaces won't be deployed to the nodes. I'm not sure how you're deploying these pods though. One thing that might be beneficial to all is to remove the
and they mention the limitation of derivative images here:
All that said, I don't know how essential it is to use the By removing the instruction, it'd make it easier for end users to make derivative images based on the official docker images instead of having to build their own image from source. It might also be relevant to note that kubernetes 1.20 now has a feature to manage the file permissions of volumes here. I haven't looked into this yet, but I'm wondering if it simplifies this problem by allowing the runtime to make sure the filesystem has the appropriate permissions set. Personally, I would think there are more users who are looking for it to run as a non root user and group out of the box, which is why I submitted #8796. Another option is to produce two images to satisfy the different requirements. @sijie - what are your thoughts here? I think we'll need some input from the larger pulsar community. |
Thanks, TIL about different (conflicting) paradigms on EKS and Openshift. Very curious indeed about what the broader pulsar community has to say about this. |
@fransguelinckx - it sounds like the way to solve this (and make the container usable on both platforms) is to remove the I believe the main reason this works for both platforms is that by leaving the volume open ended (and not declared in the Dockerfile), the file system permissions will be inherited from the host. In that way, it's up to end users (or the orchestration platform) to ensure their hosts are properly configured. I still need to double check that this is all correct, but that's my current understanding. |
I finally understand the nuance to this problem, and I am pretty sure I overstepped when I said that the two platforms were incompatible. There is a way to make this work for both platforms. I think some of my confusion came from the actual contents of this PR. The PR proposes the following: RUN chown -R 0:0 /pulsar \
&& chmod -R g=u /pulsar however, as @fransguelinckx shows, what is actually required is RUN chgrp -R 0 /pulsar && \
chmod -R g=u /pulsar The key nuance here is that members of the root group need to have write permissions. The user does not have to be root, as the PR proposes. Instead, we can have the user that owns all of the files be the That change will work properly for the OpenShift case because the root group will have sufficient permissions and it will work for the case where we're running as a non-root user and non-root group as long as the user is the As an aside, using the |
@michaeljmarshall sounds very sensible to me. I only added the root user If you implement this in your PR, I will close this one |
@roelrymenants - thanks for the context, that makes sense. A major part of the confusion is my inexperience with the nuances of file ownership for docker, declared volumes, and the different systems mentioned. I learned a number of things in finding the final solution mentioned above. I plan on updating my PR tonight to work for both use cases. |
This issue is resolved by #8796 |
Motivation
On OpenShift the user id in the container is random. In this case, the
/pulsar
folder has problematic permissions. TheVOLUME
statement even locks these into place for derivative images.e.g. as a random user the
/pulsar/conf
folder is read-only, while the pulsar helm chart assumes it to be present and writable.Modifications
Change ownership of everything in the
/pulsar
directory to beroot:root
, and make sure the owning group has the same rights as the owner.This solves the problem, as OpenShift ensures the user in the container is a member of the
root
group.