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

Adjust plugin order to correct interaction with telemetry & traffic shaping #6757

Merged
merged 18 commits into from
Feb 13, 2025
Merged

Adjust plugin order to correct interaction with telemetry & traffic shaping #6757

merged 18 commits into from
Feb 13, 2025

Conversation

goto-bus-stop
Copy link
Member

@goto-bus-stop goto-bus-stop commented Feb 11, 2025 •
edited
Loading

  • Update the layer inventory document
    • This doesn't fully bring it up to date with the current state of 2.x, I think, but the plan for today will be to use this to audit the order of plugins and to document an understanding of the way requests flow through the router and the constraints on the placement of various layers.
  • Move CSRF to the router service, and after traffic shaping
    • It being at the supergraph service was likely a leftover from the pre-router service era.
    • Moving this to the router service prevents doing unnecessary work like GraphQL parsing and validation if we're just going to reject the request anyways.
    • Previously, telemetry "worked" for CSRF-rejected requests because CSRF worked at the supergraph service. Now,
      CSRF happens at the router service, so it must be applied after telemetry in the plugin list.
    • This results in a change to some error responses. Especially, if a request sends a wrong content-type, it may now be rejected by CSRF instead of being rejected by content negotiation.
  • Move body size limit to after traffic shaping.

Follow-ups (Maybe for router 2.0.1 if necessary):

  • Move CORS handling to a plugin so it can be done in-between telemetry and traffic shaping
  • Disable Router span creation for CORS requests

@svc-apollo-docs
Copy link
Collaborator

svc-apollo-docs commented Feb 11, 2025 •
edited
Loading

✅ Docs preview has no changes

The preview was not built because there were no changes.

Build ID: 98a6c186b8539804f9517339

@github-actions GitHub Actions
Copy link
Contributor

@goto-bus-stop, please consider creating a changeset entry in /.changesets/. These instructions describe the process and tooling.

@goto-bus-stop goto-bus-stop changed the title dev-docs: minor tweaks to the layer inventory Adjust plugin order to correct interaction with telemetry & traffic shaping Feb 11, 2025
bryn added 2 commits February 11, 2025 15:09
Move CSRF plugin code
7056a1d 
Migrate to new test harness.
Fix add missing tests.
Line endings
62917e8
@goto-bus-stop goto-bus-stop marked this pull request as ready for review February 11, 2025 16:03
@goto-bus-stop goto-bus-stop requested review from a team as code owners February 11, 2025 16:03
bnjjj
bnjjj approved these changes Feb 12, 2025
View reviewed changes
apollo-router/src/plugins/csrf/mod.rs Outdated
@@ -27,6 +26,7 @@ pub(crate) struct CSRFConfig {
/// set unsafe_disabled = true to disable the plugin behavior
/// Note that setting this to true is deemed unsafe.
/// See <https://developer.mozilla.org/en-US/docs/Glossary/CSRF>.
/// TODO rename this to enabled. This is in line with the other plugins and will be less confusing.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it be done before merging this PR ?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, 2.n or 3.0

goto-bus-stop
goto-bus-stop commented Feb 13, 2025
View reviewed changes
apollo-router/src/router_factory.rs
add_mandatory_apollo_plugin!("license_enforcement");
add_mandatory_apollo_plugin!("health_check");
// TODO: Introduce a CORS plugin here
Copy link
Member Author

Choose a reason for hiding this comment

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

This will be a follow-up, just recording our decision on where to put it as a code comment

garypen
garypen approved these changes Feb 13, 2025
View reviewed changes
...uter/src/configuration/snapshots/apollo_router__configuration__tests__schema_generation.snap
@@ -342,7 +343,7 @@ expression: "&schema"
},
"CSRFConfig": {
"additionalProperties": false,
"description": "CSRF Configuration.",
"description": "CSRF protection configuration.\n\nSee <https://owasp.org/www-community/attacks/csrf> for an explanation on CSRF attacks.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice improvement!

@goto-bus-stop goto-bus-stop enabled auto-merge (squash) February 13, 2025 13:35
@goto-bus-stop goto-bus-stop merged commit bee9a27 into dev Feb 13, 2025
15 checks passed
@goto-bus-stop goto-bus-stop deleted the renee/layer-inventory-tweaks branch February 13, 2025 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@garypen garypen garypen approved these changes

@bnjjj bnjjj bnjjj approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@goto-bus-stop @svc-apollo-docs @garypen @bnjjj