-
Notifications
You must be signed in to change notification settings - Fork 273
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
connnectors: feature warnings on incompatible plugins #6525
base: next
Are you sure you want to change the base?
Conversation
This PR is still missing some plugin warnings and a real documentation link to explain why these incompatibilities exist, so it's a draft for now. |
CI performance tests
|
This commit adds a few informative warnings when using connectors with certain plugins which are currently incompatible. These warnings should better inform router operators of certain features which aren't yet supported when used in tandem with connectors.
1cadd00
to
b49f6e4
Compare
✅ Docs preview has no changesThe preview was not built because there were no changes. Build ID: d7cf1e1ac84771deeb22569f |
// There are a few plugins which influence all subgraphs, regardless | ||
// of configuration, so we warn about these statically here if we have | ||
// any connector-enabled subgraphs. | ||
let incompatible_plugins = ["demand_control", "rhai"]; |
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.
It seems too broad to warn if the customer is using Rhai at all. It's not really incompatible - the only issue is that there is no way for them to hook into the connectors requests, which isn't something we need to warn about since they can't do it. They might mistakenly think that the subgraph lifecycle hook will be executed for a subgraph containing connectors, when it won't. But the other lifecycle hooks are still executed, and in fact we suggest that they use these for auth. I think warning here would be more confusing than anything.
My bar for a warning is that there should be a way for the customer to resolve the issue, or at least suppress it. Here, there isn't - if they need to use Rhai for auth or any other reason, they'd see this warning always.
coprocessor: | ||
url: http://127.0.0.1:8081 | ||
subgraph: | ||
all: |
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.
The all
case concerns me. I mentioned in another comment that for any warning we give, the customer should have a way to resolve the issue. The only way to resolve this would be to replace all
with the same config copy and pasted for every single subgraph that doesn't contain connectors. In large supergraphs, that could be very unwieldy. And similar to the Rhai case, there really isn't an incompatibility with coprocessors, there just isn't a way to execute a coprocessor at the connector level - it still works for router, supergraph, etc.
authentication: | ||
subgraph: | ||
all: | ||
aws_sig_v4: | ||
default_chain: | ||
profile_name: "my-test-profile" | ||
region: "us-east-1" | ||
service_name: "lambda" | ||
assume_role: | ||
role_arn: "test-arn" | ||
session_name: "test-session" | ||
external_id: "test-id" |
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.
We actually do have full support for this with connectors. It just needs to be configured at the connector source level:
authentication:
connector:
sources:
aws.lambda:
aws_sig_v4:
default_chain:
profile_name: "default"
region: "us-east-1"
service_name: "lambda"
assume_role:
role_arn: "arn:aws:iam::XXXXXXXXXXXX:role/helloexecute"
session_name: "graphql"
Maybe if they have authentication settings on subgraph
but don't have a connector
setting, there could be an INFO level message pointing them to how to configure it?
The ideal solution would be to detect if any connector base URL points to AWS, and warn if they haven't configured the corresponding source with authentication
.
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.
Does authentication.subgraph.all
apply to connector sources as well?
This commit adds a few informative warnings when using connectors with certain plugins which are currently incompatible. These warnings should better inform router operators of certain features which aren't yet supported when used in tandem with connectors.
Checklist
Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.
Exceptions
Note any exceptions here
Notes
Footnotes
It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this. ↩
Configuration is an important part of many changes. Where applicable please try to document configuration examples. ↩
Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. ↩