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

feat: cache cors requests #2048

Merged
merged 5 commits into from
Jul 11, 2024
Merged

feat: cache cors requests #2048

merged 5 commits into from
Jul 11, 2024

Conversation

capJavert
Copy link
Contributor

@capJavert capJavert commented Jul 9, 2024

front will again ping api. and so introducing caching for cors requests dailydotdev/apps#3332

  • /v1/a does not support OPTIONS method so CORS fails
  • /e already supports CORS
  • /scrape does not support OPTIONS method so CORS fails

@omBratteng
Copy link
Member

In production, /v1/a and /v1/a/* is sent to monetization service, and not handled by API

@omBratteng
Copy link
Member

Same goes for /e and /e/*, which is sent to analytics-api service

@omBratteng
Copy link
Member

That being said, it should be possible to add a global CORS rule to our api ingress.
https://cloud.google.com/kubernetes-engine/docs/how-to/ingress-configuration#response_headers

@capJavert
Copy link
Contributor Author

In production, /v1/a and /v1/a/* is sent to monetization service, and not handled by API

Yes, thats my TODO for tomorrow.

@@ -85,8 +85,9 @@ export default async function app(

app.register(helmet);
app.register(cors, {
origin: isProd ? /daily\.dev$/ : true,
origin: isProd ? /^(?:https:\/\/)?(?:[\w-]+\.)*daily\.dev$/ : true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

before it would allow adaily.dev

Copy link
Member

Choose a reason for hiding this comment

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

Good catch, otherwise we'd have to buy *daily.dev domains there are 💸

@capJavert capJavert marked this pull request as ready for review July 11, 2024 08:51
@capJavert capJavert requested a review from a team as a code owner July 11, 2024 08:51
@capJavert capJavert merged commit 326505c into main Jul 11, 2024
8 checks passed
@capJavert capJavert deleted the AS-441-cors-caching branch July 11, 2024 10:31
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.

3 participants