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.

Sign up for GitHub

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

Jump to bottom

Handle OPTIONS #16

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Handle OPTIONS #16

wants to merge 5 commits into from

Conversation

sboily
Copy link

@sboily sboily commented Apr 20, 2023 •
edited by pc-m
Loading

With the changes, we reduce by 50% the HTTP trafic on our services.

WAZO-3232

@sboily
Handle OPTIONS
bf91b42 
With the changes, we reduce by 50% the HTTP trafic on our services.
@wazo-community-zuul
Copy link
Contributor

Build succeeded.

@wazo-community-zuul
Copy link
Contributor

Build succeeded.

@wazo-community-zuul
Copy link
Contributor

Build succeeded.

@pc-m
Copy link
Member

pc-m commented Apr 21, 2023

This does not work as is because of the directives inheritance rules in nginx. ref https://www.nginx.com/blog/avoiding-top-10-nginx-configuration-mistakes/#directive-inheritance
Basically if the location contains an add_header directive, any add_header defined in the http or server function disappear and need to be re-defined in the location.
I'll rework the code to make it work

@pc-m
Move cors file handling to a file that needs to be included
d402252 
I could not get the version in the server block to work. One of the reason is
that add add_header directives disappear once we do a add_header in the
location.

I also introduced a directory named all-locations.d that is used to include some
logic that should replicated to all locations.

All service that want to benefit from this change will need to

include /etc/nginx/locations/https-available/all-locations.d/*;
@wazo-community-zuul
Copy link
Contributor

Build succeeded.

@pc-m pc-m mentioned this pull request Apr 21, 2023
Draft
@pc-m
Copy link
Member

pc-m commented Apr 21, 2023

This does not work as is because of the directives inheritance rules in nginx. ref https://www.nginx.com/blog/avoiding-top-10-nginx-configuration-mistakes/#directive-inheritance Basically if the location contains an add_header directive, any add_header defined in the http or server function disappear and need to be re-defined in the location. I'll rework the code to make it work

I've made a new version that can be included in all locations. It requires more changes since all locations need to include it. I also added a all-location.d that can be included in all locations to allow this kind of drop-in to be added "easily" (adding rate limits everywhere for example)

All service that want to leverage from this change would have to include the directory in there locations. I've made the change to wazo-auth as an example #16

@pc-m
Copy link
Member

pc-m commented Apr 21, 2023

This does not work as is because of the directives inheritance rules in nginx. ref https://www.nginx.com/blog/avoiding-top-10-nginx-configuration-mistakes/#directive-inheritance Basically if the location contains an add_header directive, any add_header defined in the http or server function disappear and need to be re-defined in the location. I'll rework the code to make it work

I've made a new version that can be included in all locations. It requires more changes since all locations need to include it. I also added a all-location.d that can be included in all locations to allow this kind of drop-in to be added "easily" (adding rate limits everywhere for example)

All service that want to leverage from this change would have to include the directory in there locations. I've made the change to wazo-auth as an example #16

After discussing this subject I've decided not to make the all-locations.d directory and include the cors.conf file explicitly. With the scalability work we are doing I'm confident we're going to need to do some tweaking to our nginx configuration and I'd like us to get more experience with the kind of changes we are going to need before deciding on a system to make the configuration more flexible.

@wazo-community-zuul
Copy link
Contributor

Build succeeded.

@pc-m
Copy link
Member

pc-m commented Apr 21, 2023

With the changes, we reduce by 50% the HTTP trafic on our services.

WAZO-3232

While testing this change I noticed that I get very few OPTIONS request on multiple stacks. I think we tend to see them as developers because we use a private browser when testing.

On a very busy stack I counted 2827 OPTIONS out of a total of 127980 requests on nginx 2.2%

@fblackburn1 fblackburn1 marked this pull request as draft February 8, 2024 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
to-discuss
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sboily @pc-m