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

Remove subdomain routing for the API service #116

Merged
merged 13 commits into from
Sep 24, 2024

Conversation

binaryoverload
Copy link
Member

@binaryoverload binaryoverload commented Sep 21, 2024

Changes:

  • Added config to allow each route to have the domains configured
  • Added a custom middleware to do routing on full domains, not just subdomains
  • Updated the build scripts to be usable on Windows
    • Added cross-env which allows setting of environment variables on all platforms
    • Added copyfiles to copy the static assets. I've used the following command to replace all the copy scripts: copyfiles -e \"src/**/*.ts\" -u 1 \"src/**/*\" dist
      • This copies all files from src/**/* to the dist folder including the assets, views and timezones
      • .ts files are excluded
      • -u 1 is included so the files aren't copied to dist/src which is the default behaviour
  • Update the config manager to only quit after all the config has been validated
  • Upgrade S3 client to v3 to stop the warning every time

@jonbarrow
Copy link
Member

I have reservations about allowing access to the API routes from any domain. They are restricted for a reason. Would the appropriate solution not be to just specify the internal subdomain as well as the public one?

@binaryoverload
Copy link
Member Author

I have reservations about allowing access to the API routes from any domain. They are restricted for a reason. Would the appropriate solution not be to just specify the internal subdomain as well as the public one?

Keep in mind that "allowed from any domain" doesn't mean that any domain will be able to use this - it will still only accessible through the correct host routing in the infra which will only allow the external domain and the internal domain

The service is not responsible for restricting the domains that can access it - that's the infra's job. In this case, the subdomain routing is only so the service can tell the difference between the different domains

@jonbarrow
Copy link
Member

The service is not responsible for restricting the domains that can access it - that's the infra's job. In this case, the subdomain routing is only so the service can tell the difference between the different domains

It's the service's job to restrict this when the service is split into multiple sub-services like ours, though. That's why we have subdomain routing to begin with, since the account server itself houses multiple services which are only accessible at specific domains

Right now the api service is setup for dealing with PNID related tasks, but what if we wanted to introduce other HTTP-based internal APIs? Right now most of the internal APIs are handled by internal gRPC connections, but what if we had a situation where we needed to expose, say, the server configurations through an HTTP based API for whatever reason? Without being able to differentiate between the existing account API and this new API, since subdomain routing was removed, how would it know which service to actually use?

It also feels a bit hacky, for lack of a better word? To organize everything neatly with subdomain routing, but for the account API to just go "assume literally anything else is for the account API"?

Copy link
Member

@jonbarrow jonbarrow left a comment

Choose a reason for hiding this comment

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

Looks great, thank you! Just a few nits tbh

src/config-manager.ts Outdated Show resolved Hide resolved
src/config-manager.ts Outdated Show resolved Hide resolved
src/config-manager.ts Show resolved Hide resolved
src/config-manager.ts Outdated Show resolved Hide resolved
src/middleware/host-limit.ts Outdated Show resolved Hide resolved
Copy link
Member

@jonbarrow jonbarrow left a comment

Choose a reason for hiding this comment

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

LGTM

@jonbarrow jonbarrow merged commit 616493f into PretendoNetwork:dev Sep 24, 2024
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