-
Notifications
You must be signed in to change notification settings - Fork 527
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
feat: enhance worker deployments with additional packages and CA certificates #1626
base: develop
Are you sure you want to change the base?
feat: enhance worker deployments with additional packages and CA certificates #1626
Conversation
@@ -76,10 +76,18 @@ spec: | |||
- name: {{ .Chart.Name }}-worker | |||
image: "{{ template "sentry.image" . }}" | |||
imagePullPolicy: {{ default "IfNotPresent" .Values.images.sentry.pullPolicy }} | |||
command: ["sentry"] | |||
command: ["/bin/bash", "-c"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better to use init container and emptyDir. Нou will get the same result without changing the main container command
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What commands should I run in the init container?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same commands, but in different container and mount emptyDir for /usr/local/share/ca-certificates/
. Like init container in relay
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adonskoy could you create MR with you idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adonskoy there is no point in adding certificates to python storage using init container since python certificate storage in the main container is dynamic and can change from version to version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tried using the REQUESTS_CA_BUNDLE
env? Should work for sentry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also need to install additional python packages. They cannot be installed via init container
- "run" | ||
- "worker" | ||
- | | ||
{{- if .Values.sentry.worker.installAdditionalPackages }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Installing packages at runtime is not the best practice and will take much longer at startup compared to a custom image. Is this really necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I don't see any other options
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me that this can be solved in a more universal way: add the ability to set a custom command to launch the container. Then you can add what you need in your case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you create new MR with you idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adonskoy Can you create new MR with you idea?
pip install {{ range .Values.sentry.worker.installAdditionalPackages }}{{ . }} {{ end }} | ||
{{- end }} | ||
mkdir -p /usr/local/share/ca-certificates/ | ||
for c in $(ls -1 /usr/local/share/ca-certificates/); do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about adding this to the template and adding it to all sentry workloads?
After all, CAs may be needed throughout the entire system, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you write the example code?
This pull request introduces several enhancements to the Sentry worker deployments:
Additional Package Installation:
installAdditionalPackages
flag in thevalues.yaml
file.django-multidb-router
sentry-nodestore-elastic
sentry-s3-nodestore
(from a specific release)CA Certificates Management:
/usr/local/share/ca-certificates/
.update-ca-certificates
.caCertificatesSecret
field in thevalues.yaml
file.Changes
Deployment Templates:
command
andargs
sections in the worker deployment templates to include a bash script that handles the installation of additional packages and CA certificates.Values Configuration:
values.yaml
file:sentry.worker.installAdditionalPackages
: Boolean flag to enable/disable the installation of additional packages.sentry.worker.caCertificatesSecret
: Name of the secret containing custom CA certificates.Check pip packages
Check certificates