Skip to content
This repository has been archived by the owner on Nov 26, 2022. It is now read-only.

feat(upstream): use upstream nginx scaffolding #105

Merged
merged 3 commits into from
Apr 9, 2022

Conversation

fzipi
Copy link
Member

@fzipi fzipi commented Feb 1, 2022

Signed-off-by: Felipe Zipitria felipe.zipitria@owasp.org

Should fix #104 .

  • Still lacks better processing of base nginx.conf I think
  • Other optimizations that we can throw there also

@fzipi fzipi force-pushed the use-nginx-upstream-capabilities branch from b03948d to 9afa8b3 Compare March 7, 2022 15:41
@fzipi fzipi mentioned this pull request Mar 8, 2022
@fzipi fzipi force-pushed the use-nginx-upstream-capabilities branch 6 times, most recently from 3eae0f4 to dc1990a Compare March 26, 2022 12:02
@fzipi fzipi marked this pull request as ready for review March 26, 2022 12:54
@fzipi fzipi force-pushed the use-nginx-upstream-capabilities branch 6 times, most recently from fdfab04 to 7fdc0f7 Compare March 30, 2022 20:14
@fzipi fzipi requested a review from theseion April 6, 2022 19:10
Copy link
Contributor

@theseion theseion left a comment

Choose a reason for hiding this comment

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

I don't understand why you duplicate the script that the NGiNX image already provides. The whole template magic works out of the box by placing files into the designated directory. I understand that it doesn't just work for templating files in other directories, but even there one could argue that you could just place all template files into the special templates directory and then move them somewhere else after envsubst has done its work. You could take advantage of the fact that the script handles subdirectories gracefully, so you could place other templates into subdirectories. That way you know where they end up.

v3-nginx/Dockerfile Outdated Show resolved Hide resolved
v3-nginx/docker-entrypoint.d/90-nginx-extras-envsubst.sh Outdated Show resolved Hide resolved
v3-nginx/docker-entrypoint.d/90-nginx-extras-envsubst.sh Outdated Show resolved Hide resolved
v3-nginx/docker-entrypoint.d/90-nginx-extras-envsubst.sh Outdated Show resolved Hide resolved
v3-nginx/docker-entrypoint.d/90-nginx-extras-envsubst.sh Outdated Show resolved Hide resolved
v3-nginx/docker-entrypoint.d/91-modsecurity-envsubst.sh Outdated Show resolved Hide resolved
v3-nginx/docker-entrypoint.d/91-modsecurity-envsubst.sh Outdated Show resolved Hide resolved
v3-nginx/docker-entrypoint.d/91-modsecurity-envsubst.sh Outdated Show resolved Hide resolved
v3-nginx/docker-entrypoint.d/91-modsecurity-envsubst.sh Outdated Show resolved Hide resolved
Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org>
@fzipi fzipi force-pushed the use-nginx-upstream-capabilities branch from 7fdc0f7 to 1f386ec Compare April 7, 2022 14:15
@fzipi
Copy link
Member Author

fzipi commented Apr 7, 2022

Ok, you are right, that worked. And simplifies that we don't need to change Apache.

Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org>
theseion
theseion previously approved these changes Apr 7, 2022
Copy link
Contributor

@theseion theseion left a comment

Choose a reason for hiding this comment

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

Nice! LGTM.

@fzipi
Copy link
Member Author

fzipi commented Apr 8, 2022

@theseion Needed to fix dns resolver because exporting the variable in a previous script didn't worked (which makes sense, once that you think about it). So ended up adding a new script that does sed on a fixed string instead, after templates are copied.

Copy link
Contributor

@theseion theseion left a comment

Choose a reason for hiding this comment

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

Yeah, makes sense to replace DNS resolver statically.

@fzipi fzipi requested a review from theseion April 8, 2022 18:07
v3-nginx/Dockerfile Outdated Show resolved Hide resolved
Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org>
@fzipi fzipi force-pushed the use-nginx-upstream-capabilities branch from 467f026 to abbd092 Compare April 8, 2022 19:20
@fzipi fzipi requested a review from theseion April 9, 2022 11:05
@fzipi fzipi merged commit ef9432b into coreruleset:master Apr 9, 2022
@fzipi fzipi deleted the use-nginx-upstream-capabilities branch April 9, 2022 12:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docker-entrypoint.d is unused
2 participants