[ROUTER-890] Ability to skip safelisting enforcement via plugin #6403
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ Docs Preview ReadyConfiguration{
"repoOverrides": {
"apollographql/router@main": {
"remote": {
"owner": "apollographql",
"repo": "router",
"branch": "glasser/pq-safelist-override"
}
}
}
}
9 pages published. Build will be available for 30 days. |
|
(PR still needs a small doc update and changeset, but otherwise should be good. Note that this is built on top of #6198.) |
|
CI performance tests
|
glasser
force-pushed
the
glasser/pq-client-name
branch
from
bf7ba2c to
8b459b7
Compare
glasser
force-pushed
the
glasser/pq-safelist-override
branch
2 times, most recently
from
909ebfb to
4cee254
Compare
6 tasks
glasser
commented
Dec 5, 2024
|
|
||
| let (_mock_guard, uplink_config) = mock_pq_uplink(&manifest).await; | ||
| assert_counter!( |
There was a problem hiding this comment.
Note that I wrote and tested all the assert_counter calls on a clean checkout before I rewrote the u64_counter calls to use the attributes vector because I was a bit paranoid that I would do something accidental like change bool true to string "true".
glasser
commented
Dec 5, 2024
| @@ -106,7 +106,7 @@ expression: yaml | |||
| - fields: | |||
| alg: ES256 | |||
| reason: "invalid type: string \"Hmm\", expected a sequence" | |||
| index: 5 | |||
| index: 5 | |||
There was a problem hiding this comment.
Oops this is a typo I should revert I think
glasser
commented
Dec 5, 2024
| @@ -130,7 +130,7 @@ Use `with_subscriber` to attach a subscriber to an async block. | |||
| ```rust | |||
| #[tokio::test] | |||
| async fn test_async() { | |||
| async{...}.with_subscriber(assert_snapshot_subscriber!()) | |||
| async{...}.with_subscriber(assert_snapshot_subscriber!()).await | |||
There was a problem hiding this comment.
Despite my need to make minor tweaks I really appreciated both of these dev docs!
glasser
force-pushed
the
glasser/pq-safelist-override
branch
from
4cee254 to
acafab0
Compare
|
!docs preview main |
1 similar comment
|
!docs preview main |
glasser
force-pushed
the
glasser/pq-client-name
branch
from
8b459b7 to
0797d3e
Compare
glasser
force-pushed
the
glasser/pq-safelist-override
branch
from
acafab0 to
0655ba2
Compare
glasser
force-pushed
the
glasser/pq-client-name
branch
from
0797d3e to
2bbe77c
Compare
glasser
force-pushed
the
glasser/pq-safelist-override
branch
from
0655ba2 to
abbde21
Compare
|
!docs preview main |
glasser
force-pushed
the
glasser/pq-safelist-override
branch
from
abbde21 to
a81aab8
Compare
|
!docs preview main |
|
(btw this |
glasser
force-pushed
the
glasser/pq-safelist-override
branch
from
a81aab8 to
8a14702
Compare
|
!docs preview main |
|
OK, this one is ready for review as well. Docs and changesets should be done. This merges into #6198. |
glasser
force-pushed
the
glasser/pq-client-name
branch
from
2bbe77c to
54e79ca
Compare
If safelisting is enabled, a `router_service` plugin can skip enforcement of the safelist (including the `require_id` check) by adding the key `apollo_persisted_queries::safelist::skip_enforcement` with value `true` to the request context. (This does not affect the logging of unknown operations by the `persisted_queries.log_unknown` option.) In cases where an operation would have been denied but is allowed due to the context key existing, the attribute `persisted_queries.safelist.enforcement_skipped` is set on the `apollo.router.operations.persisted_queries` metric with value true. This PR improves the testing of that metric as well. When writing the tests, I discovered that the `persisted_queries.safelist.rejected.unknown` attribute had its value set to `false` when the operation is denied but not logged, and to `true` when denied and logged. (You can also tell whether it is logged via the `persisted_queries.logged` attribute.) This dated to the creation of this metric in #3609 and seems to be a mistake. This PR normalizes this attribute to always be `true` if it is set. The metric was described as unstable when released in v1.28.1, so this seems reasonable.
glasser
force-pushed
the
glasser/pq-safelist-override
branch
from
8a14702 to
8b1fe2a
Compare
|
!docs preview main |
BrynCooke
approved these changes
Dec 6, 2024
BrynCooke
reviewed
Dec 6, 2024
| true, | ||
| )); | ||
| } | ||
| u64_counter!( |
There was a problem hiding this comment.
Wonderful! One callsite for metrics.
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If safelisting is enabled, a
router_serviceplugin can skip enforcement of the safelist (including therequire_idcheck) by adding the keyapollo_persisted_queries::safelist::skip_enforcementwith valuetrueto the request context.(This does not affect the logging of unknown operations by the
persisted_queries.log_unknownoption.)In cases where an operation would have been denied but is allowed due to the context key existing, the attribute
persisted_queries.safelist.enforcement_skippedis set on theapollo.router.operations.persisted_queriesmetric with value true.This PR improves the testing of that metric as well.
When writing the tests, I discovered that the
persisted_queries.safelist.rejected.unknownattribute had its value set tofalsewhen the operation is denied but not logged, and totruewhen denied and logged. (You can also tell whether it is logged via thepersisted_queries.loggedattribute.) This dated to the creation of this metric in #3609 and seems to be a mistake. This PR normalizes this attribute to always betrueif it is set. The metric was described as unstable when released in v1.28.1, so this seems reasonable.