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

Renamed config file and hard linked it into the nginx conf #51

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

StevusPrimus
Copy link
Contributor

The reason is that likely we would want to put more conf files into /etc/nginx/conf.d , so if this folder is linked to a local folder the crowdsec_openresty.conf always got lost. Like this, it is much easier to use.

@StevusPrimus
Copy link
Contributor Author

StevusPrimus commented Sep 4, 2024

image is available at ghcr.io/stevusprimus/cs-openresty-bouncer:buildx-latest for testing

@blotus
Copy link
Member

blotus commented Oct 3, 2024

Hello,

Thanks for the PR, and sorry about the delay in reviewing it.

I'm a bit unsure about this change: if the user mounts a custom nginx.conf, the bouncer will not be loaded.

But at the same time, I do agree with you than putting it in conf.d also risks to override the config file, with the same consequences.

What do you think about storing the config file in a path the user has no reason to override (we use /staging in the crowdsec docker image to store some files in the image), and copy the config file to /etc/nginx/conf.d from there in docker_start.sh ?

@StevusPrimus
Copy link
Contributor Author

Hi!
Agree with all points.
I do think, if we want to copy the file from somewhere like staging it should be a one time copy at the first container run. Otherwise, the file would get magically overwritten at each container start/restart, which is not a behaviour, I would expect.
What about coping it only, if it does not exist yet? We would not need a separate initialization script then ...
I'll push a proposal soon.

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