-
Notifications
You must be signed in to change notification settings - Fork 2
feat(router): Override Subgraph URLs #471
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
base: main
Are you sure you want to change the base?
Conversation
✅
|
🐋 This PR was built and pushed to the following Docker images: Image Names: Platforms: Image Tags: Docker metadata{
"buildx.build.ref": "builder-483316d8-e78c-44a2-93b5-669183dd8476/builder-483316d8-e78c-44a2-93b5-669183dd84760/qxt5m3ibfgq6oekhw4vzwu52j",
"containerimage.descriptor": {
"mediaType": "application/vnd.oci.image.index.v1+json",
"digest": "sha256:91dc9853abc3bb6a222ce969f885b9e6096d77070f86abbbcec024b4c7c93f96",
"size": 1609
},
"containerimage.digest": "sha256:91dc9853abc3bb6a222ce969f885b9e6096d77070f86abbbcec024b4c7c93f96",
"image.name": "ghcr.io/graphql-hive/router:pr-471,ghcr.io/graphql-hive/router:sha-9a7348d"
} |
@gemini-code-assist review and summary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a powerful feature to override subgraph URLs, either statically or dynamically using VRL expressions. The implementation is well-structured, introducing a new ExpressionHTTPExecutor
to handle dynamic resolution and cleanly integrating it with the existing HTTPSubgraphExecutor
. The configuration is defined in a new override_subgraph_urls.rs
file and integrated into the main router config. The changes also include necessary refactoring in the executor map and semaphore handling to support this new functionality. The documentation has been updated to reflect the new feature.
I've found a critical issue that will prevent compilation due to a missing From
trait implementation, and a medium-severity issue in the documentation that should be addressed to make it complete.
03c7b48
to
84919f6
Compare
In the above configuration example, it appears that only the headers approach is supported, yes? In other words, the JWT token claims can't be used, correct? This should work for my use case, since I'm passing in a REGION header. The original reason I used the header is because the Apollo router only supports JWT decoding with the enterprise license. Otherwise, I would have used the JWT directly. In reality, the header was a workaround. |
JWT Plugin is still WIP --> #455 |
products: | ||
expression: |2- | ||
if .request.headers."x-region" == "us-east" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a case sensitive matching that is occurring? Or is it case-insensitive?
Are there any other request attributes that can be used here? Looks like the entire request object is available for use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, does the statement have to return a value here?
Or will the default automatically be reused if no value is returned in the expression?
For example, suppose my default url is http://hello.us.com
.
Is this a valid configuration for the products
subgraph?:
override_subgraph_urls:
subgraphs:
products:
expression: |2-
if .request.headers."x-region" == "eu-east" {
"https://hello.eu.eu/graphql"
}
I want to point out TWO things here:
- Notice that the default is
http
(plain http) and the EU version of the URL ishttps
(secure). Is this sort of change to the scheme supported? This was not originally supported in the Apollo router and but I submitted a PR for it to be supported here Feature/6897 - Allow scheme for subgraph to be inspected and updated apollographql/router#6906
Since Apollo didn't support it by default, I don't want to assume that it is supported here.
- Notice that I didn't include an
else
statement in the expression in the configuration above. I would expect that the default URL would be used in this case. Is this supported, or does the expression have to return a value, even the default value, or will the default value be used if no value is returned?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My Rust isn't good at all so I can't quite look at the code myself to answer the above questions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notice that I didn't include an else statement in the expression in the configuration above. I would expect that the default URL would be used in this case. Is this supported, or does the expression have to return a value, even the default value, or will the default value be used if no value is returned?
Yes the URL in the supergraph will be used if the expression doesn't return anything.
Notice that the default is http (plain http) and the EU version of the URL is https (secure). Is this sort of change to the scheme supported? This was not originally supported in the Apollo router and but I submitted a PR for it to be supported here
Scheme doesn't matter in our implementation currently. It is possible to switch in between http and https.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a case sensitive matching that is occurring? Or is it case-insensitive?
Header names are always lower-cased actually. So they are case-insensitive.
Are there any other request attributes that can be used here? Looks like the entire request object is available for use.
It has details like URL(search params etc), headers, method and the original router operation. After the implementation of JWT, it will have the jwt payload there too. Once we have a proper documentation then we'll explain the available variables in the VRL Context!
router/lib/executor/src/execution/plan.rs
Line 54 in 3b7a28c
pub struct ClientRequestDetails<'a> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds great! This is perfect. Because my organization chose to NOT use Apollo enterprise, we're only using basic query features of the router so switching to this new router now should be super easy.
The only thing we'll want to wait for is the ability to tie it to the Hive Registry for the supergraph schema. I saw there is an issue open for that and that it's already on the timeline.
3b7a28c
to
5435ea1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see we have a new subgraph executor, but I don't think we should.
The executor_map
is a hashmap where keys are endpoints, and I think that's why you did it this way, but it could be changed into subgraph's name instead and then the endpoint would be dynamic.
This way we just add resolve_endpoint
to existing executor and move on.
About expression
returning nothing and falling back to original value. I don't like this, it could be more explicit and done with returning .original_url
value or something like that.
Fixes #461
Ref ROUTER-149
This PR implements the feature to override existing subgraph urls defined in the supergraph;
Cosmo -> https://cosmo-docs.wundergraph.com/router/proxy-capabilities/override-subgraph-config
Apollo Router -> https://www.apollographql.com/docs/graphos/routing/configuration/yaml#override_subgraph_url
Configuration;