Il giorno mar 8 giu 2021 alle ore 09:22 Sijie Guo <guosi...@gmail.com> ha scritto: > > 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.
We are on the same page. We should think more about this change. I raised other concerns about the lack of tools when you try to operate on the pods (for instance you miss a text editor and netstat). > > 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. Makes sense > > Let me know if you have any questions. Very clear, thanks The only thing I would add is to create a github issue in order to track the upgrade issue and tell the story, this way when we start a new implementation everyone will remember the story and possibly we will write good integration tests Enrico > > - 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. > > > > > > > > > > > > > > > > > > > >
