apollographql / router

A configurable, high-performance routing runtime for Apollo Federation 🚀
https://www.apollographql.com/docs/router/
Other
807 stars 272 forks source link

Client headers are parsed into the context before coprocessor hook #5251

Open smyrick opened 5 months ago

smyrick commented 5 months ago

EDIT: The initial use case can now be solved with a work-around, see below, but there still exists the issue, even if non-urgent that there is not some external hook mechanism before the Router reads HTTP request data

Describe the bug In 1.40.2 the populate_context function, which is called in the SupergraphService lifecycle, looks up the client_name_header and stores it in the context.

In 1.45.1, it no longer looks up that header in that function. Glancing through, it looks like this occurred in this commit, "Remove Legacy Validation".

So, it looks like the client name is now extracted in an Apollo-owned Router Service plugin. To my knowledge, this will run before any plugin lifecycle we can possibly create - Router Service is the earliest lifecycle, and Apollo space ones will always run before user space plugins I think. So, in the current state I don't think modifications to the client name header are possible from our code. Is this intended, or is there a workaround to ensure we can update headers prior to the header being collected for telemetry?

To Reproduce

telemetry:
  apollo:
    client_name_header: x-caller-id
    client_version_header: x-caller-version

x-foobar-name -> x-caller-id x-foobar-version -> x-caller-version

Expected behavior I can use Rust, Rhai, or Coprocessor plugins at some stage to mutate the headers before Apollo uses them

Desktop (please complete the following information):

Screenshot 2024-05-24 at 5 29 54 PM

smyrick commented 5 months ago

I was able to reproduce the bug here: https://github.com/apollosolutions/router-playground/pull/25

Geal commented 4 months ago

Could a router level rhai script fix it? Here would be the order of operations:

smyrick commented 4 months ago

@Geal That works! I modified the coprocessor to manually set the context value from the header

payload.context.entries['apollo_telemetry::client_name'] = headerValue;

If this is our recommended way to set the client reporting values then that works for this, but we still have the issue that we can't mutate any other headers before Router reads them. Shouldn't the coprocessor be the first call out in the the RouterRequest lifecycle?

Geal commented 4 months ago

for various reasons, telemetry has to be called before everything else. We're planning changes to the pipeline that might help here