Skip to content
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

[ENH] Update reverse proxy recipe with nginx conf #254

Merged
merged 7 commits into from
Jan 15, 2025

Conversation

surchs
Copy link
Contributor

@surchs surchs commented Dec 20, 2024

To have longer request timeouts

Changes proposed in this pull request:

Checklist

Please leave checkboxes empty for PR reviewers

  • PR has an interpretable title with a prefix ([ENH], [FIX], [REF], [TST], [CI], [MNT], [INF]) see our Contributing Guidelines for more info)
  • PR links to GitHub issue with mention Closes #XXXX
  • Checks pass

To have longer request timeouts
Copy link

netlify bot commented Dec 20, 2024

Deploy Preview for neurobagel-documentation ready!

Name Link
🔨 Latest commit 68632b4
🔍 Latest deploy log https://app.netlify.com/sites/neurobagel-documentation/deploys/6787e86d20046c0007928351
😎 Deploy Preview https://deploy-preview-254--neurobagel-documentation.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@alyssadai alyssadai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @surchs! One question about the way we're mounting the custom nginx config file - maybe you can take a quick look in case I'm misunderstanding something

docs/user_guide/config.md Show resolved Hide resolved
docs/user_guide/config.md Outdated Show resolved Hide resolved
@surchs surchs marked this pull request as ready for review January 9, 2025 16:09
@surchs surchs requested a review from alyssadai January 9, 2025 18:18
@surchs
Copy link
Contributor Author

surchs commented Jan 14, 2025

  • remove conf volume
  • @alyssadai to figure out docker mounting specifics

@surchs surchs added the flag:check Issue needs attention before further action label Jan 15, 2025
Copy link
Contributor

@alyssadai alyssadai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @surchs, this LGTG 🧑‍🍳

Re: Docker bind mounting of files, the closest documentation of our observed behaviour I could find was https://docs.docker.com/engine/storage/bind-mounts/#considerations-and-constraints

Bind mounts have write access to files on the host by default.

One side effect of using bind mounts is that you can change the host filesystem via processes running in a container, including creating, modifying, or deleting important system files or directories. This capability can have security implications. For example, it may affect non-Docker processes on the host system.

You can use the readonly or ro option to prevent the container from writing to the mount.

From what I understand, it is expected that when you bind-mount a file into a container, the mounted file's parent directory is effectively exposed within the container. So if the container writes to the parent directory, those changes are reflected on the host (unless we add the ro option).

@surchs surchs merged commit 4070966 into main Jan 15, 2025
6 checks passed
@surchs surchs deleted the add_nginx_timeout_recipe branch January 15, 2025 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flag:check Issue needs attention before further action
Projects
Status: Review - Done
Development

Successfully merging this pull request may close these issues.

Add example nginx .conf file with extended timeouts to recipe with reverse proxy containers
2 participants