You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2020/12/02 20:03:17 UTC
[GitHub] [pulsar] michaeljmarshall opened a new pull request #8796: [Issue 8751] Update Dockerfile for Pulsar and Dashboard to Create and Use pulsar User (nonroot user)
michaeljmarshall opened a new pull request #8796:
URL: https://github.com/apache/pulsar/pull/8796
<!--
### Contribution Checklist
- Name the pull request in the form "[Issue XYZ][component] Title of the pull request", where *XYZ* should be replaced by the actual issue number.
Skip *Issue XYZ* if there is no associated github issue for this pull request.
Skip *component* if you are unsure about which is the best component. E.g. `[docs] Fix typo in produce method`.
- Fill out the template below to describe the changes contributed by the pull request. That will give reviewers the context they need to do the review.
- Each pull request should address only one issue, not mix up code from multiple issues.
- Each commit in the pull request has a meaningful commit message
- Once all items of the checklist are addressed, remove the above text and this checklist, leaving only the filled out template below.
**(The sections below can be removed for hotfixes of typos)**
-->
Fixes #8751
### Motivation
Pulsar does not need to run as the root user. This PR updates the pulsar and the pulsar dashboard images to make them run as a new `pulsar` user (user 1000). This change increases the security of pulsar images.
### Modifications
Update two `Dockerfile`s to create a pulsar user, chown the appropriate directories, and then use that user by default.
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
I manually verified that the docker images run with the correct user and file permissions. As this is my first commit, I'm not familiar with pulsar testing. Are there tests that run against the produced docker images? If so, then there is likely no further testing needed.
### Does this pull request potentially affect one of the following parts:
*If `yes` was chosen, please highlight the changes*
- Dependencies (does it add or upgrade a dependency): no
- The public API: no
- The schema: no
- The default values of configurations: no
- The wire protocol: no
- The rest endpoints: no
- The admin cli options: no
- Anything that affects deployment: yes
In order to deploy this new docker container, users will need to update the `journal` and `ledgers` directories on the host to be owned by user `1000` and group `1000`.
### Documentation
Documentation will be required for this change because users will need to set up their hosts differently to properly give permission to user 1000 and group 1000 in order to allow the container to write to the appropriate host volumes. However, I'd like to see if this approach is accepted before adding documentation.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] sijie commented on a change in pull request #8796: [Issue 8751] Update Dockerfile for Pulsar and Dashboard to Create and Use pulsar User (nonroot user)
Posted by GitBox <gi...@apache.org>.
sijie commented on a change in pull request #8796:
URL: https://github.com/apache/pulsar/pull/8796#discussion_r539033618
##########
File path: docker/pulsar/Dockerfile
##########
@@ -53,21 +55,25 @@ RUN python3.7 get-pip.py
RUN update-alternatives --install /usr/bin/python python /usr/bin/python3 10
-ADD target/python-client/ /pulsar/pulsar-client
-ADD target/cpp-client/ /pulsar/cpp-client
+ADD --chown=pulsar:pulsar target/python-client/ /pulsar/pulsar-client
+ADD --chown=pulsar:pulsar target/cpp-client/ /pulsar/cpp-client
RUN echo networkaddress.cache.ttl=1 >> $JAVA_HOME/jre/lib/security/java.security
RUN apt-get update \
&& apt install -y /pulsar/cpp-client/*.deb \
&& apt-get clean \
+ && chown -R pulsar:pulsar /pulsar/cpp-client/
&& rm -rf /var/lib/apt/lists/*
+# Start using the pulsar user to ensure container defaults to run as non root user
+USER pulsar
+
+# Directories will have correct permission because we switched to the pulsar user
+RUN mkdir /pulsar/conf /pulsar/data
Review comment:
@michaeljmarshall when the docker image is used in Kubernetes, the helm chart will mount the disks to `/pulsar/data`. Does it change the permission?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #8796: [Issue 8751] Update Dockerfile for Pulsar and Dashboard to Create and Use pulsar User (nonroot user)
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #8796:
URL: https://github.com/apache/pulsar/pull/8796#issuecomment-777208905
Rebased to see if any of the recent test improvements help make this PR pass tests.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #8796: [Issue 8751] Update Dockerfile for Pulsar and Dashboard to Create and Use pulsar User (nonroot user)
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #8796:
URL: https://github.com/apache/pulsar/pull/8796#issuecomment-773115205
After discussion in https://github.com/apache/pulsar/pull/8242 and some additional research, I found a better way to meet all requirements to build this image to run with a non root user. I have the solution half implemented and will need to do some testing to make sure it looks as I expect before I add another commit.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #8796: [Issue 8751] Update Dockerfile for Pulsar and Dashboard to Create and Use pulsar User (nonroot user)
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #8796:
URL: https://github.com/apache/pulsar/pull/8796#issuecomment-752354459
I had missed a `\` in my previous commit, and that caused an error while running the docker build step. After fixing that, as well as a permissions issue on creating directories, it looks like there are some additional issues during container initialization where pulsar tries to create or write to files not in the `/pulsar` directory. I'll look into this soon and add a new commit to fix it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #8796: [Issue 8751] Update Dockerfile for Pulsar and Dashboard to Create and Use pulsar User (nonroot user)
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #8796:
URL: https://github.com/apache/pulsar/pull/8796#issuecomment-753227507
This latest commit includes a fix to make the pulsar python client available to the `root` and the `pulsar` user. This is necessary for pulsar functions and was exposed by the pulsar function integration tests, which currently run as the root user in the `pulsar-test-latest-version` docker container (this image defaults to run as root).
I think we should convert the pulsar function tests to run as the `pulsar` user. I'll look at adding that to a later commit.
These test failures (related to problems running as root vs pulsar) raise an important question about backwards compatibility and the published docker images. I think it adds value to let the container run as either the root user or the pulsar user to allow for end users to transition to the non-root version. As such, this commit installs the pulsar python client so that it's available to the root and pulsar.
However, I don't think it is necessary to run tests as the root user because it is unlikely that the root user will have any other problems. Based on a limited understanding of the tests, the pulsar functions are likely one of the only processes that rely on python, making it the only process that use the pip dependencies. It'd be great to have some perspective from people more familiar with the project and any other edge cases in different deployments.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #8796: [Issue 8751] Update Dockerfile for Pulsar and Dashboard to Create and Use pulsar User (nonroot user)
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #8796:
URL: https://github.com/apache/pulsar/pull/8796#issuecomment-803572304
@michaeljmarshall @sijie I reported the issue as https://github.com/apache/pulsar-helm-chart/issues/110 in pulsar-helm-chart so that it gets tracked.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #8796: [Issue 8751] Update Dockerfile for Pulsar and Dashboard to Create and Use pulsar User (nonroot user)
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #8796:
URL: https://github.com/apache/pulsar/pull/8796#issuecomment-773115205
After discussion in https://github.com/apache/pulsar/pull/8242 and some additional research, I found a better way to meet all requirements to build this image to run with a non root user. I have the solution half implemented and will need to do some testing to make sure it looks as I expect before I add another commit.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] sijie commented on pull request #8796: [Issue 8751] Update Dockerfile for Pulsar and Dashboard to Create and Use pulsar User (nonroot user)
Posted by GitBox <gi...@apache.org>.
sijie commented on pull request #8796:
URL: https://github.com/apache/pulsar/pull/8796#issuecomment-779570279
@michaeljmarshall Thank you for your contribution!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #8796: [Issue 8751] Update Dockerfile for Pulsar and Dashboard to Create and Use pulsar User (nonroot user)
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #8796:
URL: https://github.com/apache/pulsar/pull/8796#issuecomment-741595590
@sijie - I rebased and pushed, but it looks like the tests still failed. I'll try to take a closer look at the tests tomorrow.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #8796: [Issue 8751] Update Dockerfile for Pulsar and Dashboard to Create and Use pulsar User (nonroot user)
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #8796:
URL: https://github.com/apache/pulsar/pull/8796#issuecomment-774608357
This PR should now satisfy the requirements for OpenShift as well as running as a non root user and non root group.
I've assumed that users will need to be able to read/write to the `/pulsar` directory. It's possible that this is more permission than necessary.
I also added the `VOLUME` directive to standalone Dockerfile, as this was the original reason there was a `VOLUME` directive in the `pulsar` Dockerfile.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #8796: [Issue 8751] Update Dockerfile for Pulsar and Dashboard to Create and Use pulsar User (nonroot user)
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #8796:
URL: https://github.com/apache/pulsar/pull/8796#discussion_r551707216
##########
File path: docker/pulsar/Dockerfile
##########
@@ -53,21 +55,25 @@ RUN python3.7 get-pip.py
RUN update-alternatives --install /usr/bin/python python /usr/bin/python3 10
-ADD target/python-client/ /pulsar/pulsar-client
-ADD target/cpp-client/ /pulsar/cpp-client
+ADD --chown=pulsar:pulsar target/python-client/ /pulsar/pulsar-client
+ADD --chown=pulsar:pulsar target/cpp-client/ /pulsar/cpp-client
RUN echo networkaddress.cache.ttl=1 >> $JAVA_HOME/jre/lib/security/java.security
RUN apt-get update \
&& apt install -y /pulsar/cpp-client/*.deb \
&& apt-get clean \
+ && chown -R pulsar:pulsar /pulsar/cpp-client/
&& rm -rf /var/lib/apt/lists/*
+# Start using the pulsar user to ensure container defaults to run as non root user
+USER pulsar
+
+# Directories will have correct permission because we switched to the pulsar user
+RUN mkdir /pulsar/conf /pulsar/data
Review comment:
@sijie - after thinking about this, I think it might be helpful, and possibly necessary, to make a corresponding update to the helm chart to allow end users to choose how the container is run in kubernetes. Because docker allows an end user to choose the user that a container runs with, it is trivial for end users to run the container as root until they're able to update the host file system to have the appropriate permissions for the `/puslar/conf` and `/pulsar/data` directories. Let me know what you think.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #8796: [Issue 8751] Update Dockerfile for Pulsar and Dashboard to Create and Use pulsar User (nonroot user)
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #8796:
URL: https://github.com/apache/pulsar/pull/8796#issuecomment-802923466
@lhotari - good catch. I think the easiest way to prevent a breaking change in the helm chart is to default to using the `root` user in the helm chart's deployment for the pulsar proxy. We could then add a configuration that allows users to opt in to running that container as a non root user.
Essentially, we need to default a pulsar proxy pod's security context to the following:
```yaml
securityContext:
runAsUser: 0
runAsGroup: 0
```
I believe all other pulsar containers should still be able to run as the non root user.
(I am assuming that we want to prevent breaking changes here as the 2.8.0 release is only a minor version bump.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] sijie commented on pull request #8796: [Issue 8751] Update Dockerfile for Pulsar and Dashboard to Create and Use pulsar User (nonroot user)
Posted by GitBox <gi...@apache.org>.
sijie commented on pull request #8796:
URL: https://github.com/apache/pulsar/pull/8796#issuecomment-740262556
@addisonj ^^
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #8796: [Issue 8751] Update Dockerfile for Pulsar and Dashboard to Create and Use pulsar User (nonroot user)
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #8796:
URL: https://github.com/apache/pulsar/pull/8796#issuecomment-742237886
Of the 14 failed checks, there are 3 unique errors being logged. The first two seem like symptoms of an earlier failures. Here are the stages being run, and the errors output:
`run integration tests` -> `Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:3.0.0-M3:test (default-test) on project integration: There are test failures.`
`build pulsar image` -> `Failed to execute goal com.spotify:dockerfile-maven-plugin:1.4.13:build (default) on project pulsar-docker-image: Execution default of goal com.spotify:dockerfile-maven-plugin:1.4.13:build failed: Could not acquire image ID or digest following build -> [Help 1]`
`run unit test 'OTHER'` -> `Error: TestOpenAndWriteSink(org.apache.pulsar.io.rabbitmq.sink.RabbitMQSinkTest) Time elapsed: 0.01 s <<< FAILURE!
java.net.ConnectException: Connection refused (Connection refused)`
@sijie - any chance that the `run unit test 'OTHER'` failure is related to a flaky test? If not, perhaps something I changed is affecting that test.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #8796: [Issue 8751] Update Dockerfile for Pulsar and Dashboard to Create and Use pulsar User (nonroot user)
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #8796:
URL: https://github.com/apache/pulsar/pull/8796#issuecomment-740419815
I haven't yet had the time to dig into the tests to understand why they're failing. I think I'll be able to look into it later this week--although, I am new to the project, so any direction as to where things might be failing would be greatly appreciated.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #8796: [Issue 8751] Update Dockerfile for Pulsar and Dashboard to Create and Use pulsar User (nonroot user)
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #8796:
URL: https://github.com/apache/pulsar/pull/8796#issuecomment-802884368
Today I started running some tests for 2.8.0-SNAPSHOT images. There's a permission issue in pulsar-proxy since the default configuration uses port 80 and now when the default user is "pulsar", it cannot bind to port 80.
This is the error log in pulsar-proxy-0 pod:
```
14:38:21.477 [main] ERROR org.apache.pulsar.proxy.server.ProxyServiceStarter - Failed to start pulsar proxy service. error msg Failed to start HTTP server on ports [80]
java.io.IOException: Failed to start HTTP server on ports [80]
at org.apache.pulsar.proxy.server.WebServer.start(WebServer.java:246) ~[org.apache.pulsar-pulsar-proxy-2.8.0-SNAPSHOT.jar:2.8.0-SNAPSHOT]
at org.apache.pulsar.proxy.server.ProxyServiceStarter.<init>(ProxyServiceStarter.java:177) [org.apache.pulsar-pulsar-proxy-2.8.0-SNAPSHOT.jar:2.8.0-SNAPSHOT]
at org.apache.pulsar.proxy.server.ProxyServiceStarter.main(ProxyServiceStarter.java:186) [org.apache.pulsar-pulsar-proxy-2.8.0-SNAPSHOT.jar:2.8.0-SNAPSHOT]
Caused by: java.net.SocketException: Permission denied
at sun.nio.ch.Net.bind0(Native Method) ~[?:1.8.0_282]
at sun.nio.ch.Net.bind(Net.java:461) ~[?:1.8.0_282]
at sun.nio.ch.Net.bind(Net.java:453) ~[?:1.8.0_282]
at sun.nio.ch.ServerSocketChannelImpl.bind(ServerSocketChannelImpl.java:222) ~[?:1.8.0_282]
at sun.nio.ch.ServerSocketAdaptor.bind(ServerSocketAdaptor.java:85) ~[?:1.8.0_282]
at org.eclipse.jetty.server.ServerConnector.openAcceptChannel(ServerConnector.java:345) ~[org.eclipse.jetty-jetty-server-9.4.35.v20201120.jar:9.4.35.v20201120]
at org.eclipse.jetty.server.ServerConnector.open(ServerConnector.java:310) ~[org.eclipse.jetty-jetty-server-9.4.35.v20201120.jar:9.4.35.v20201120]
at org.eclipse.jetty.server.AbstractNetworkConnector.doStart(AbstractNetworkConnector.java:80) ~[org.eclipse.jetty-jetty-server-9.4.35.v20201120.jar:9.4.35.v20201120]
at org.eclipse.jetty.server.ServerConnector.doStart(ServerConnector.java:234) ~[org.eclipse.jetty-jetty-server-9.4.35.v20201120.jar:9.4.35.v20201120]
at org.eclipse.jetty.util.component.AbstractLifeCycle.start(AbstractLifeCycle.java:73) ~[org.eclipse.jetty-jetty-util-9.4.35.v20201120.jar:9.4.35.v20201120]
at org.eclipse.jetty.server.Server.doStart(Server.java:401) ~[org.eclipse.jetty-jetty-server-9.4.35.v20201120.jar:9.4.35.v20201120]
at org.eclipse.jetty.util.component.AbstractLifeCycle.start(AbstractLifeCycle.java:73) ~[org.eclipse.jetty-jetty-util-9.4.35.v20201120.jar:9.4.35.v20201120]
at org.apache.pulsar.proxy.server.WebServer.start(WebServer.java:224) ~[org.apache.pulsar-pulsar-proxy-2.8.0-SNAPSHOT.jar:2.8.0-SNAPSHOT]
... 2 more
2021-03-19 02:38:21,480 [sun.misc.Launcher$AppClassLoader@7ef20235] error Uncaught exception in thread main: java.io.IOException: Failed to start HTTP server on ports [80]
```
The problem goes away after changing the port to 8080 in values.yaml:
```
proxy:
ports:
http: 8080
```
@michaeljmarshall @sijie I wonder how to address this breaking change in pulsar-helm-chart?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #8796: [Issue 8751] Update Dockerfile for Pulsar and Dashboard to Create and Use pulsar User (nonroot user)
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #8796:
URL: https://github.com/apache/pulsar/pull/8796#issuecomment-738591133
I pushed a commit to clean up my initial commit and to try to make the tests pass.
Note: I removed the `RUN chown -R pulsar:pulsar /pulsar` line from the `Dockerfile` in favor of "chowning" directories and files as they are created because it added about 350 mb to the produced image (credit to @devoncrouse for pointing out that optimization).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #8796: [Issue 8751] Update Dockerfile for Pulsar and Dashboard to Create and Use pulsar User (nonroot user)
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #8796:
URL: https://github.com/apache/pulsar/pull/8796#issuecomment-756528535
@sijie - on this previous commit, it looks like the tests that failed only failed due to timeouts. On the commit before the latest one, the only test failure was from a maven issue that was unrelated to my change. At this point, it seems to me that the underlying change is valid, but I'm wondering if there is any chance that I'm running into flaky tests. If you think it's likely a problem with my changes, is there anyone who can help me troubleshoot these test failures? I'm happy to keep digging, but I think it might be more productive if I can get some guidance. Thanks!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] sijie commented on pull request #8796: [Issue 8751] Update Dockerfile for Pulsar and Dashboard to Create and Use pulsar User (nonroot user)
Posted by GitBox <gi...@apache.org>.
sijie commented on pull request #8796:
URL: https://github.com/apache/pulsar/pull/8796#issuecomment-756538399
@michaeljmarshall the integration tests passed. so that means your change is good. Other failures are flaky tests. I will rebase your branch to latest master (as there are some fixes in master to improve tests).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #8796: [Issue 8751] Update Dockerfile for Pulsar and Dashboard to Create and Use pulsar User (nonroot user)
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #8796:
URL: https://github.com/apache/pulsar/pull/8796#discussion_r539069260
##########
File path: docker/pulsar/Dockerfile
##########
@@ -53,21 +55,25 @@ RUN python3.7 get-pip.py
RUN update-alternatives --install /usr/bin/python python /usr/bin/python3 10
-ADD target/python-client/ /pulsar/pulsar-client
-ADD target/cpp-client/ /pulsar/cpp-client
+ADD --chown=pulsar:pulsar target/python-client/ /pulsar/pulsar-client
+ADD --chown=pulsar:pulsar target/cpp-client/ /pulsar/cpp-client
RUN echo networkaddress.cache.ttl=1 >> $JAVA_HOME/jre/lib/security/java.security
RUN apt-get update \
&& apt install -y /pulsar/cpp-client/*.deb \
&& apt-get clean \
+ && chown -R pulsar:pulsar /pulsar/cpp-client/
&& rm -rf /var/lib/apt/lists/*
+# Start using the pulsar user to ensure container defaults to run as non root user
+USER pulsar
+
+# Directories will have correct permission because we switched to the pulsar user
+RUN mkdir /pulsar/conf /pulsar/data
Review comment:
Kubernetes will not change the file system permissions by default. If the volume is owned by `root`, the container will also view the mounted volume as owned by `root`, which would be a problem for the container produced from this docker image.
In Linux, the mounted volume will need to be writable by user 1000. I'm not certain how Docker users map to host users in other operating systems.
[Here](https://kubernetes.io/docs/tasks/configure-pod-container/security-context/#configure-volume-permission-and-ownership-change-policy-for-pods) is the Kubernetes documentation on permissions and pods. It looks like there has been active development in the mounted volume permission domain.
I'm not sure how the pulsar project handles this type of change. Given that administrators of pulsar clusters should have access to the nodes running pulsar, it seems reasonable to me that those administrators could change the ownership of the relevant directories before upgrading to the next version of pulsar. The current containers would have root permissions, and would be able to write to directories/files owned by user 1000, so a rolling upgrade could still be achieved.
Is there any precedent for this type of change? I would really like to get this into the 2.8.0 release, so that we can increase the security of pulsar containers and I can stop maintaining my own fork of the project. However, I also recognize the importance of carefully releasing major changes to a project.
As I mentioned in my initial PR description, it might be relevant to provide migration documentation, but I'm not sure where to put it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] sijie commented on pull request #8796: [Issue 8751] Update Dockerfile for Pulsar and Dashboard to Create and Use pulsar User (nonroot user)
Posted by GitBox <gi...@apache.org>.
sijie commented on pull request #8796:
URL: https://github.com/apache/pulsar/pull/8796#issuecomment-757614213
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #8796: [Issue 8751] Update Dockerfile for Pulsar and Dashboard to Create and Use pulsar User (nonroot user)
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #8796:
URL: https://github.com/apache/pulsar/pull/8796#issuecomment-737681442
I pushed updates to use the user's id instead of name, and I pushed an update to properly chown the added directories in the `pulsar-all` `Dockerfile`. After fully building the project tonight, I noticed that the `pulsar-standalone` image still runs as the root user and that all of its directories are owned by `root` or by `postgres`. That docker image looks a bit more complicated than the `pulsar` and `pulsar-all` images, so I am going to refrain from updating that `Dockerfile`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #8796: [Issue 8751] Update Dockerfile for Pulsar and Dashboard to Create and Use pulsar User (nonroot user)
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #8796:
URL: https://github.com/apache/pulsar/pull/8796#issuecomment-737681891
This PR would also fix issue https://github.com/apache/pulsar/issues/7210.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] sijie merged pull request #8796: [Issue 8751] Update Dockerfile for Pulsar and Dashboard to Create and Use pulsar User (nonroot user)
Posted by GitBox <gi...@apache.org>.
sijie merged pull request #8796:
URL: https://github.com/apache/pulsar/pull/8796
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #8796: [Issue 8751] Update Dockerfile for Pulsar and Dashboard to Create and Use pulsar User (nonroot user)
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #8796:
URL: https://github.com/apache/pulsar/pull/8796#issuecomment-752785842
After digging into this morning's test failures, I can see that the tests failed because we're using `supervisord` to spawn the pulsar processes (broker, bookie, etc.) and our configuration script needed permission to edit `supervisord` configuration files. My latest commit updates the docker image used to run tests (`pulsar-test-latest-version`) so that the container runs as the root user, `supervisord` runs as root, and `supervisord` will spawn pulsar components as the pulsar user.
I am slightly concerned that this approach requires configuration in multiple places (each of the pulsar component `conf` files) to maintain the correctness of the tests. Given that new components are likely rare, I think it should be simple enough to maintain, but I want to call this risk out. The risk is that a new test could accidentally run as root and work because it somehow relies on running as root.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] sijie commented on pull request #8796: [Issue 8751] Update Dockerfile for Pulsar and Dashboard to Create and Use pulsar User (nonroot user)
Posted by GitBox <gi...@apache.org>.
sijie commented on pull request #8796:
URL: https://github.com/apache/pulsar/pull/8796#issuecomment-741552652
> I haven't yet had the time to dig into the tests to understand why they're failing.
That's probably related to flaky test environment. Can you try to rebase your branch to latest master to include some of recent fixes around tests?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #8796: [Issue 8751] Update Dockerfile for Pulsar and Dashboard to Create and Use pulsar User (nonroot user)
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #8796:
URL: https://github.com/apache/pulsar/pull/8796#issuecomment-756542749
@sijie - thanks for the insight. I just rebased and pushed my branch.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] sijie commented on pull request #8796: [Issue 8751] Update Dockerfile for Pulsar and Dashboard to Create and Use pulsar User (nonroot user)
Posted by GitBox <gi...@apache.org>.
sijie commented on pull request #8796:
URL: https://github.com/apache/pulsar/pull/8796#issuecomment-744173864
@michaeljmarshall It seems that all integration tests failed. It seems that it is related to this change. I am trying it locally.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall edited a comment on pull request #8796: [Issue 8751] Update Dockerfile for Pulsar and Dashboard to Create and Use pulsar User (nonroot user)
Posted by GitBox <gi...@apache.org>.
michaeljmarshall edited a comment on pull request #8796:
URL: https://github.com/apache/pulsar/pull/8796#issuecomment-742237886
Of the 14 failed checks, there are 3 unique errors being logged. The first two seem like symptoms of an earlier failures. Here are the stages being run, and the errors output:
`run integration tests` -> `Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:3.0.0-M3:test (default-test) on project integration: There are test failures.`
`build pulsar image` -> `Failed to execute goal com.spotify:dockerfile-maven-plugin:1.4.13:build (default) on project pulsar-docker-image: Execution default of goal com.spotify:dockerfile-maven-plugin:1.4.13:build failed: Could not acquire image ID or digest following build -> [Help 1]`
`run unit test 'OTHER'` -> `Error: TestOpenAndWriteSink(org.apache.pulsar.io.rabbitmq.sink.RabbitMQSinkTest) Time elapsed: 0.01 s <<< FAILURE!
java.net.ConnectException: Connection refused (Connection refused)`
@sijie - any chance that the `run unit test 'OTHER'` failure is related to a flaky test? If not, perhaps something I changed is affecting that test. ([Here](https://github.com/apache/pulsar/pull/8796/checks?check_run_id=1522259376) is that failing check.)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #8796: [Issue 8751] Update Dockerfile for Pulsar and Dashboard to Create and Use pulsar User (nonroot user)
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #8796:
URL: https://github.com/apache/pulsar/pull/8796#issuecomment-754380277
Based on a recent blogpost, https://github.com/hexops/dockerfile#do-not-use-a-uid-below-10000, I decided to change the pulsar user id to 10000 and the group id to 10001 in this most recent commit. Further, I added some documentation for the getting started with docker.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on pull request #8796: [Issue 8751] Update Dockerfile for Pulsar and Dashboard to Create and Use pulsar User (nonroot user)
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #8796:
URL: https://github.com/apache/pulsar/pull/8796#issuecomment-781296650
@michaeljmarshall after this commit I am no more able to build the Docker image locally
```
cd docker
mvn package -Pdocker
[INFO] --- exec-maven-plugin:1.6.0:exec (build-pulsar-clients-python-35) @ pulsar-docker-image ---
--------- Build Python wheel for 3.7 -- cp37-cp37m
Using image: apachepulsar/pulsar-build:manylinux-cp37-cp37m
bash: /pulsar/pulsar-client-cpp/docker/build-wheel-file-within-docker.sh: No such file or directory
[ERROR] Command execution failed.
org.apache.commons.exec.ExecuteException: Process exited with an error: 127 (Exit value: 127)
at org.apache.commons.exec.DefaultExecutor.executeInternal (DefaultExecutor.java:404)
at org.apache.commons.exec.DefaultExecutor.execute (DefaultExecutor.java:166)
at org.codehaus.mojo.exec.ExecMojo.executeCommandLine (ExecMojo.java:804)
at org.codehaus.mojo.exec.ExecMojo.executeCommandLine (ExecMojo.java:751)
at org.codehaus.mojo.exec.ExecMojo.execute (ExecMojo.java:313)
at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo (DefaultBuildPluginManager.java:137)
at org.apache.maven.lifecycle.internal.MojoExecutor.execute (MojoExecutor.java:210)
```
Are you able to build it locally ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on pull request #8796: [Issue 8751] Update Dockerfile for Pulsar and Dashboard to Create and Use pulsar User (nonroot user)
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #8796:
URL: https://github.com/apache/pulsar/pull/8796#issuecomment-781360448
The error is not due to this patch, I reproduce it on previous commits.
There must be something wrong in my env.
Sorry for the noise
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #8796: [Issue 8751] Update Dockerfile for Pulsar and Dashboard to Create and Use pulsar User (nonroot user)
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #8796:
URL: https://github.com/apache/pulsar/pull/8796#issuecomment-753390598
Rebased my branch with master.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org