De-root nginx (and grpc-proxy) #2847
Conversation
JamesXNelson
requested review from
devinrsmith and
jcferretti
as code owners
| USER 0 | ||
|
|
||
| # We want to install sudo, and start the sudo service (update policy-rc.d to exit 0, instead of 101) | ||
| RUN ls -la /usr/sbin/policy-rc.d ; \ |
There was a problem hiding this comment.
convention is to prefix any multi-line RUN commands with set -eux to avoid surprises when one command fails, then the &&s aren't required. Alternatively, move contents into a .sh file, and start that with set -eux, invoke that directly (though that involves copying it first, etc)
|
For documentation, I'm thinking to just update anywhere we might reference |
| --server_http_tls_port="${PROXY_TLS_PORT}" \ | ||
| --server_tls_cert_file="$TLS_CRT" \ | ||
| --server_tls_key_file="$TLS_KEY" \ | ||
| --server_tls_client_ca_files="$TLS_CA,/etc/ssl/certs/ca-certificates.crt,/var/run/secrets/kubernetes.io/serviceaccount/ca.crt" \ |
There was a problem hiding this comment.
Is this kubernetes path going to break anybody who may want to do TLS on non-kube?
There was a problem hiding this comment.
Nope; I don't think so at least. It's just a list of places where there are CA files.
If you want, I can build the list and check if the files are readable first, that way it will work in kube but won't risk breaking someone w/o that ca cert.
There was a problem hiding this comment.
I checked if they exist before adding to the flag, so there's no way it can break non-kube users now.
| #expires -1; | ||
| #add_header Cache-Control 'must-revalidate, max-age=0'; |
There was a problem hiding this comment.
Don't think these should be commented out?
There was a problem hiding this comment.
Seems to work w/ these uncommented out.
There was a problem hiding this comment.
Even w/ these uncommented tho, a page using this feature like https://jxn-test-changes.demo.deephaven.app/ide/notebook/03%20Kafka%20Stream%20vs%20Append.md still gives me 304 cached status...
|
Ok, I think from the comments I've received, everything that is done is everything that is needed. |
|
Nm... I see now the examples for the images are in the deephaven-core repo, and will need updates. One more commit coming. |
|
ok, all pushed, conflicts resolved, should be g2g |
|
Be sure to hit the "Re-request review" button for each reviewer when it is ready to go. |
Also adds /ide routing in nginx, so we always serve index.html, but preserve full url so js can read the /ide/path.