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

The CORS plugin should allow users to specify access-control-max-age in configuration #2212

Closed
osamra-rbi opened this issue Dec 5, 2022 · 8 comments

Comments

@osamra-rbi
Copy link
Contributor

Is it possible to add the method to the Request interface for Rhia? Is there a reason it isn't there in the first place?

We're in the process of migrating from Apollo Gateway to Apollo Router, but one thing we cannot do is cache CORS requests, which is doing something along the lines of setting:
Access-Control-Max-Age: 86400

This can't be done with the current Rhia API as method isn't exposed on the Request interface, therefore we can't check for OPTION requests to add the headers needed

@garypen
Copy link
Contributor

garypen commented Dec 6, 2022

The Rhai API only provides processing at the various stages of the router plugin lifecycle. An OPTION method wouldn't make it as far as the current plugin lifecycle.

I can think of two ways that this maybe could be addressed:

  • We could enhance our CORS configuration to allow specifying an access control max age
  • We are adding a new HTTP processing stage that will be invoked early in request processing. It should be possible to do what you want there.

I think it's probably best to do this in the CORS configuration, but I'll discuss with the team and also I'm interested in your feedback on the two alternatives.

@garypen garypen changed the title Adding method to the Request interface for Rhia API The CORS plugin should allow users to specify access-control-max-age in configuration Dec 6, 2022
@garypen
Copy link
Contributor

garypen commented Dec 6, 2022

I think this actually requires a change to our CORS plugin, to support amax-age property, so I'm going to update the title and change the labelling.

@osamra-rbi
Copy link
Contributor Author

osamra-rbi commented Dec 6, 2022

Definitely think it makes more sense to be in the CORS configuration, this is where I first looked to see if it was possible

Although another header that's needed is Cache-Control and that's not necessarily something that's under CORS, maybe it makes more sense for that to be set via Rhia if the method could be exposed? Not sure.

Here's this article that explains what we've been doing to provide more context

@garypen
Copy link
Contributor

garypen commented Dec 9, 2022

I think it may make sense to do that automatically if the CORS plugin is modified to support a max-age property. Alternatively, our HTTP processing stage will be delivered soon and I expect that may also help out here.

@osamra-rbi
Copy link
Contributor Author

I think it may make sense to do that automatically if the CORS plugin is modified to support a max-age property. Alternatively, our HTTP processing stage will be delivered soon and I expect that may also help out here.

That makes sense.

Meanwhile, I guess a temporary solution to achieve what we want is to make a native Rust plugin, until there's something in Apollo Router that allows us to do what we need

@osamra-rbi
Copy link
Contributor Author

Hi @garypen @bnjjj

I'll be opening up a PR that adds the max-age to the CORS configuration. I'm not sure of a solution for adding Cache-Control since it is somewhat independent of CORS

@abernix
Copy link
Member

abernix commented Jan 17, 2023

cache CORS requests

I'm curious why this is feature is inherently important to you because it's my understanding from my basic Google search that this behavior literally cannot be relied on in browsers: Firefox is the only browser that allows 24 hours and most other browsers put cache limit in place on of anywhere from 5 minutes to 2 hours. It also seems they offer no guarantees of that.

@osamra-rbi
Copy link
Contributor Author

cache CORS requests

I'm curious why this is feature is inherently important to you because it's my understanding from my basic Google search that this behavior literally cannot be relied on in browsers: Firefox is the only browser that allows 24 hours and most other browsers put cache limit in place on of anywhere from 5 minutes to 2 hours. It also seems they offer no guarantees of that.

Hey! You're right, each browser has different limits

Despite Firefox being the only browser that supports 24 hours, Chrome will see that and set it to its own limit (2 hours)

The reason we did this was for response improvements. Before we had this, each request that went to our gateway would have to do a CORS request

After adding the header, we saw improved latency and a 20% request reduction to our gateway

Honesty, we could probably lower the value since 24 hours is a bit high from my understanding

FWIW, we're going to be moving our gateway (router) to the same domain as our sites so we don't have to deal with CORS

Nevertheless, I do think it's worth having this option anyways since it fits under CORS

bnjjj added a commit that referenced this issue Jan 24, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Hi there

As discussed in this
[issue](#2212), the CORS
plugin in Apollo Router currently lacks setting the
Access-Control-Max-Age header. This PR adds a new option called max_age
that is used like so:
```
cors:
  max_age: 86400
```

This does not solve the Cache-Control header mentioned in issue as it's
a bit separate from CORS.

New test(s) and documentation changes haven't been done yet as I wanted
to confirm this is fine before proceeding further. If so, happy to do
them. Thanks!

(FYI I don't have the ability to assign a reviewer or anything, if we're
able to do that)

Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
Co-authored-by: Simon Sapin <simon@apollographql.com>
Co-authored-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
@bnjjj bnjjj closed this as completed Jan 24, 2023
@abernix abernix removed the triage label Feb 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants