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

Handle readonly containers #284

Closed
wants to merge 1 commit into from
Closed

Handle readonly containers #284

wants to merge 1 commit into from

Conversation

srudin
Copy link

@srudin srudin commented Jan 20, 2023

As discussed here: #281

This is NOT intended to be merged yet - I would just like to initiate the discussion about the readonly containers with some sample.

In the discussion linked above I can see that you are somewhat sceptical about including the changes for the readonly containers as you worry it will complicate the chart significantly without being a requirement for most people.

I would like to convince you that this ain't the case by showing you what was required to get the db container to work. You can see that it merely required some directories to be specified as emptyDir (which will put a writeable memory drive in place of these folders that will be discarded when the container stops). From my point of view this is a very small change that will do no harm to anybody not running the containers readonly and benefit everyone who likes to do so. My impression is the real problem with this requirement is the time required to find the issues and fix them and not the resulting solution. But feel free to share your thoughts.

I am currently working on the web container which is slightly more complicated. However I think the final solution will be comparable to this one here once I figure out the cause for #283

@srudin srudin mentioned this pull request Jan 20, 2023
@batpad
Copy link
Member

batpad commented Jan 20, 2023

@srudin extremely grateful for your clear explanations and pull requests here! Definitely a lot for me to learn about security and best practices, especially when running in clusters with other production workloads - apologies if it might take me a bit of time to do more reading and get up-to-speed in properly understanding the mechanics and consequences of these changes.

Definitely seeing this PR helps a LOT to understand how this works, and definitely reduces my skepticism a lot :-) - this looks great, and I think mostly kindof makes sense to me . Am still curious about how one would handle cases where the application needs to write to non temp folders, but I think I'm also seeing the value in this approach a lot more and would be super happy to spend some time on our end to help test and merge this - these changes do seem super valuable if we do want osm-seed to be easier and more practical to install in larger multi-tenant clusters: so far we have almost always ran separate clusters for osm-seed instances, and I can see a lot of the changes you are making will be required if we want to more confidently run in multi-tenant scenarios, which would definitely be a strong goal for us.

I do apologize in advance though: osm-seed is not our main focus or a funded project at the moment, so while we will try to get to things as soon as possible, it is possible there will be delays when we get over-run with other work - but in principle I'm definitely very keen to work through these and thank you again for your contributions!

@srudin
Copy link
Author

srudin commented Jan 20, 2023

Ok - I will be working on this for some more time and update this PR with the required changes for the other containers (once I get them running).

@srudin srudin closed this by deleting the head repository Sep 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants