I agree that using 'fsGroup' is the proper solution.
If we are concerned about PR#10743 setting the permissions to 644 as a security
risk, then why are we not also reverting it? If this is a legitimate problem,
then are we not introducing a security risk in 2.8.0? Also, why not just revert
it now so we don't have to remember to change it later and risk that we forget?
Cheers,Chris
On Tuesday, June 8, 2021, 03:22:07 AM EDT, Sijie Guo <guosi...@gmail.com>
wrote:
Enrico,
PR#10743 doesn't fix the problem and it actually made things worse. I am a
bit surprised that the change was merged without a review from other
committers who are more familiar with the Pulsar Functions codebase.
1) First of all, the problem can happen when you using an old version of a
broker to schedule functions with a new function docker image. In this way,
the new Functions will not able to run because the new Pulsar Functions
image is running under the `pulsar` user, which cannot read the token file
mounted under `root`.
2) During an upgrade process, people typically update the docker image
first before updating the function worker. The order is important here
because the command lines generated by the function worker are backward
compatible not forward compatible. If you update the function worker first,
it can easily break the running functions as the new function worker might
generate a command line that is not able to be recognized by the old
function image. So during an upgrade process, you would hit a similar issue
as 1).
3) PR#10743 didn't fix the problem. We used `400` for security concerns. We
want to limit the file to be only accessed by the user of the function
runner image. Changing 400 to 644 doesn't fix the problem. Instead, it
introduces other security concerns, because it makes the token file being
accessed by any users. A proper fix is to specify the proper `fsGroup` in
the security context of the functions pods.
4) I am not comfortable with rootless user change given the problem we have
identified. We should have proper fixes in place to address the permission
concerns before we switched to a rootless docker image. Because we also
don't know if it is going to break other changes or not.
Based on the concerns above, I believe the right approach here is to revert
#8796. We don't need to revert #10743. Because the user is `root` anyway.
But when we introduce the `rootless` change again, we need to remember to
change the permission from 644 to 400.
Let me know if you have any questions.
- Sijie
On Mon, Jun 7, 2021 at 10:48 PM Enrico Olivelli <eolive...@gmail.com> wrote:
> Il Mar 8 Giu 2021, 03:32 PengHui Li <peng...@apache.org> ha scritto:
>
> > Hi Sijie,
> >
> > Thanks for the feedback, cancel the RC1 for now.
> > Please don't vote for this thread again, thanks.
> >
> > This is the PR
> > https://github.com/apache/pulsar/pull/10861#pullrequestreview-678011678
> > for reverting https://github.com/apache/pulsar/pull/8796. After the PR
> > merged, I will send out the RC2 ASAP
> >
> > Penghui
> >
> > Sijie Guo <guosi...@gmail.com> 于2021年6月8日周二 上午9:26写道：
> >
> > > Penghui,
> > >
> > > Unfortunately, I think we have to cancel this vote.
> > >
> > > The change https://github.com/apache/pulsar/pull/8796 has broken the
> > > Pulsar
> > > Functions running on Kubernetes.
> > >
> > > The Pulsar Functions Kubernetes runtime generates a secret and mounts
> it
> > > using mode `256`. That means the secret is only able to read by the
> user.
> > > The StatefulSet created by Kubernetes runtime mounts the secrets under
> > the
> > > `root` user. Hence only the root user is able to read the secret. This
> > > results in any functions submitted will fail to read the authentication
> > > information.
> > >
> > > Because all the Kubernetes resources generated by the Kubernetes
> runtime
> > > are hardcoded. There is no easy way to change the security context for
> > the
> > > function statefulsets.
>
>
> Doesn't pr
> https://github.com/apache/pulsar/pull/10743
> Fix the problem?
>
> Can you share an error?
> Have you tracked the problem in a github issue?
>
> I am fine with reverting the patch
>
> But I am surprised because during the past days we (at Datastax) are
> testing the feature and we are able to run functions (apart from the
> problem, that I thought it was fixed by #10743 )
>
> Enrico
>
>
> My take here is to revert the change in
> > > https://github.com/apache/pulsar/pull/8796 to go back to the root user
> > > until we address the issues in the Kubernetes runtime.
> > >
> > > If there are other approaches to get around this issue, please let me
> > know.
> > > Otherwise, we have to cancel this vote.
> > >
> > > - Sijie
> > >
> > > On Mon, Jun 7, 2021 at 4:02 PM PengHui Li <peng...@apache.org> wrote:
> > >
> > > > Hi all,
> > > >
> > > > I have also pushed the docker image to my personal dockerhub account.
> > > > If you want to verify on docker, you use use following images
> > > >
> > > > https://hub.docker.com/repository/docker/lph890127/pulsar
> > > > https://hub.docker.com/repository/docker/lph890127/pulsar-all
> > > > https://hub.docker.com/repository/docker/lph890127/pulsar-standalone
> > > >
> > > > Thanks,
> > > > Penghui
> > > >
> > > > Matteo Merli <mme...@apache.org> 于2021年6月8日周二 上午3:31写道：
> > > >
> > > > > +1 binding
> > > > >
> > > > > Checked:
> > > > > * Signatures
> > > > > * Bin distribution:
> > > > > - NOTICE, README, LICENSE
> > > > > - Start standalone service and producer/consumer test
> > > > > * Src distribution:
> > > > > - NOTICE, README, LICENSE
> > > > > - Compile and unit tests
> > > > > - Start standalone service
> > > > > * Checked staging maven repository artifacts
> > > > >
> > > > >
> > > > > --
> > > > > Matteo Merli
> > > > > <mme...@apache.org>
> > > > >
> > > > > On Mon, Jun 7, 2021 at 6:21 AM PengHui Li <peng...@apache.org>
> > wrote:
> > > > > >
> > > > > > This is the first release candidate for Apache Pulsar, version
> > 2.8.0.
> > > > > >
> > > > > > It fixes the following
> > > > > > issues:
> > > > >
> > > >
> > >
> >
> https://github.com/apache/pulsar/pulls?q=is%3Apr+milestone%3A2.8.0+-label%3Arelease%2F2.7.1+-label%3Arelease%2F2.7.2+is%3Aclosed
> > > > > >
> > > > > > *** Please download, test and vote on this release. This vote
> will
> > > stay
> > > > > open
> > > > > > for at least 72 hours ***
> > > > > >
> > > > > > Note that we are voting upon the source (tag), binaries are
> > provided
> > > > for
> > > > > > convenience.
> > > > > >
> > > > > > Source and binary
> > > > > > files:
> > > > >
> > >
> https://dist.apache.org/repos/dist/dev/pulsar/pulsar-2.8.0-candidate-1/
> > > > > >
> > > > > > SHA-512 checksums:
> > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
> 48306629a261f78c560b449f85b58b6e66ae9c7464961ec3990784a97dcb75870f32bfe99393f60195224a66e6b29f06154230a96a7d5edecddb35618a2d69b2
> > > > > > apache-pulsar-2.8.0-SNAPSHOT-bin.tar.gz
> > > > > >
> > > > >
> > > >
> > >
> >
> 3fdab0dad99d7ef2fe9728c1b538d424ef95b208b5d1d01aa7fc23859fe8c8f82074be9ba6426f525159a33ea742ca892c34b87fa641f94c8ddbb84fbacab6eb
> > > > > > apache-pulsar-2.8.0-SNAPSHOT-src.tar.gz
> > > > > >
> > > > > > Maven staging repo:
> > > > >
> > >
> https://repository.apache.org/content/repositories/orgapachepulsar-1088/
> > > > > >
> > > > > > The tag to be voted upon:
> > > > > > v2.8.0-candidate-1
> > > > > > (73172334d15e29b7755e5792d7c577f48e54554d)
> > > > > https://github.com/apache/pulsar/releases/tag/v2.8.0-candidate-1
> > > > > >
> > > > > > Pulsar's KEYS file containing PGP keys we use to sign the
> > > > > > release:https://dist.apache.org/repos/dist/dev/pulsar/KEYS
> > > > > >
> > > > > > Please download the the source package, and follow the README to
> > > build
> > > > > > and run the Pulsar standalone service.
> > > > >
> > > >
> > >
> >
>
