You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/08/17 02:36:15 UTC
[GitHub] [pulsar-helm-chart] acjohnson opened a new pull request #150: added additionalCommand parameter
acjohnson opened a new pull request #150:
URL: https://github.com/apache/pulsar-helm-chart/pull/150
Fixes #148
### Motivation
Added a `additionalCommand` parameter for each statefulset so that the chart user can optionally specific a custom command such as `update-ca-certificates` to run prior to starting the JVM and other container processes. This is required to avoid custom image builds simply for shipping an internal CA certificate file.
### Modifications
* Added `additionalCommand` parameter to values.yaml for each statefulset
* Added conditionals to each statefulset allowing the chart user to specify a command to be ran at the beginning of the command line(s)
### Verifying this change
- [ ] Make sure that the change passes the CI 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.
To unsubscribe, e-mail: dev-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-helm-chart] michaeljmarshall merged pull request #150: added additionalCommand parameter
Posted by GitBox <gi...@apache.org>.
michaeljmarshall merged pull request #150:
URL: https://github.com/apache/pulsar-helm-chart/pull/150
--
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.
To unsubscribe, e-mail: dev-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-helm-chart] michaeljmarshall commented on pull request #150: added additionalCommand parameter
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #150:
URL: https://github.com/apache/pulsar-helm-chart/pull/150#issuecomment-1005876996
This PR adds value, even though the original goal of this feature might break when we move to a non-root docker image, so I am going to merge it now.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-helm-chart] acjohnson commented on a change in pull request #150: added additionalCommand parameter
Posted by GitBox <gi...@apache.org>.
acjohnson commented on a change in pull request #150:
URL: https://github.com/apache/pulsar-helm-chart/pull/150#discussion_r693293606
##########
File path: charts/pulsar/templates/bookkeeper-statefulset.yaml
##########
@@ -162,6 +162,9 @@ spec:
command: ["sh", "-c"]
args:
- >
+ {{- if .Values.bookkeeper.additionalCommand }}
+ {{ .Values.bookkeeper.additionalCommand }}
+ {{- end }}
Review comment:
Yes it renders properly for me @michaeljmarshall. I'm running Helm v3.6.3
```shell
# helm version --short
v3.6.3+gd506314
# helm template pulsar/ | tail
# software distributed under the License is distributed on an
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
#
# deploy zookeeper only when `components.zookeeper` is true
# define the storage class for data directory
# echo $?
0
```
--
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.
To unsubscribe, e-mail: dev-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-helm-chart] michaeljmarshall commented on a change in pull request #150: added additionalCommand parameter
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #150:
URL: https://github.com/apache/pulsar-helm-chart/pull/150#discussion_r693247604
##########
File path: charts/pulsar/templates/bookkeeper-statefulset.yaml
##########
@@ -162,6 +162,9 @@ spec:
command: ["sh", "-c"]
args:
- >
+ {{- if .Values.bookkeeper.additionalCommand }}
+ {{ .Values.bookkeeper.additionalCommand }}
+ {{- end }}
Review comment:
Did you verify that this renders properly using `helm template`? I copied this into my repo and seem to be getting an error.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-helm-chart] acjohnson commented on pull request #150: added additionalCommand parameter
Posted by GitBox <gi...@apache.org>.
acjohnson commented on pull request #150:
URL: https://github.com/apache/pulsar-helm-chart/pull/150#issuecomment-907207056
> @acjohnson - are you only looking to supply the a custom trust store to the JVM? If so, it is possible to configure the JVM's keystore and truststore using the following jvm args: `-Djavax.net.ssl.keyStore` and `-Djavax.net.ssl.trustStore`, respectively. These can be added to the proxy/function worker/broker command via the `PULSAR_EXTRA_OPTS`environment variable. They need to be formatted as `.jks` files, and it is possible to put them in a secret then mount them into the pod as files for use by the JVM.
Good idea but no that will not be enough for our use case. We are using go pulsar functions and the JVM keystore being updated wouldn't solve for go functions unfortunately.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-helm-chart] acjohnson commented on pull request #150: added additionalCommand parameter
Posted by GitBox <gi...@apache.org>.
acjohnson commented on pull request #150:
URL: https://github.com/apache/pulsar-helm-chart/pull/150#issuecomment-906774294
I've experimented with a couple alternatives including simply mounting an already updated copy of `/etc/ssl/certs/ca-certificates.crt` via a secret volume file but that didn't work...
If non-root container is the future, I might be forced to build new docker images with every pulsar release unless someone comes up with a work around...
--
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.
To unsubscribe, e-mail: dev-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-helm-chart] michaeljmarshall commented on a change in pull request #150: added additionalCommand parameter
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #150:
URL: https://github.com/apache/pulsar-helm-chart/pull/150#discussion_r697152552
##########
File path: charts/pulsar/templates/bookkeeper-statefulset.yaml
##########
@@ -162,6 +162,9 @@ spec:
command: ["sh", "-c"]
args:
- >
+ {{- if .Values.bookkeeper.additionalCommand }}
+ {{ .Values.bookkeeper.additionalCommand }}
+ {{- end }}
Review comment:
I tested this properly, and I can see that it works. Thanks for confirming things on your end, @acjohnson.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-helm-chart] michaeljmarshall commented on pull request #150: added additionalCommand parameter
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #150:
URL: https://github.com/apache/pulsar-helm-chart/pull/150#issuecomment-906921598
@acjohnson - are you only looking to supply the a custom trust store to the JVM? If so, it is possible to configure the JVM's keystore and truststore using the following jvm args: `-Djavax.net.ssl.keyStore` and `-Djavax.net.ssl.trustStore`, respectively. These can be added to the proxy/function worker/broker command via the `PULSAR_EXTRA_OPTS`environment variable. They need to be formatted as `.jks` files, and it is possible to put them in a secret then mount them into the pod as files for use by the JVM.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-helm-chart] acjohnson commented on pull request #150: added additionalCommand parameter
Posted by GitBox <gi...@apache.org>.
acjohnson commented on pull request #150:
URL: https://github.com/apache/pulsar-helm-chart/pull/150#issuecomment-903044844
> Thank you for your contribution, @acjohnson. I am not opposed to adding a value like this, but I should point out that we are going to modify the pulsar docker image so that it runs with a non-root user by default. This work hasn't completed yet. When it does, a command like `update-ca-certificates` will fail because it requires elevated permission.
Hmm... yeah that's too bad it won't work after that non-root change...
--
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.
To unsubscribe, e-mail: dev-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org