[ROUTER-890] Ability to skip safelisting enforcement via plugin

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.

.changesets/breaking_glasser_pq_metric_attribute_consistency.md

@@ -0,0 +1,9 @@
+### More consistent attributes on `apollo.router.operations.persisted_queries` metric ([PR #6403](https://github.com/apollographql/router/pull/6403))
+
+Version 1.28.1 added several *unstable* metrics, including `apollo.router.operations.persisted_queries`.
+
+When an operation is rejected, Router includes a `persisted_queries.safelist.rejected.unknown` attribute on the metric. Previously, this attribute had the value `true` if the operation is logged (via `log_unknown`), and `false` if the operation is not logged. (The attribute is not included at all if the operation is not rejected.) This appears to have been a mistake, as you can also tell whether it is logged via the `persisted_queries.logged` attribute.
+
+Router now only sets this attribute to true, and never to false. This may be a breaking change for your use of metrics; note that these metrics should be treated as unstable and may change in the future.
+
+By [@glasser](https://github.com/glasser) in https://github.com/apollographql/router/pull/6403

.changesets/feat_glasser_pq_safelist_override.md

@@ -0,0 +1,10 @@
+### Ability to skip Persisted Query List safelisting enforcement via plugin ([PR #6403](https://github.com/apollographql/router/pull/6403))
+
+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.
+
+By [@glasser](https://github.com/glasser) in https://github.com/apollographql/router/pull/6403

apollo-router/src/metrics/mod.rs

@@ -525,9 +525,10 @@ pub(crate) fn meter_provider() -> AggregateMeterProvider {
 ///     frobbles.color = "blue"
 /// );
 /// // Count a thing with dynamic attributes:
-/// let attributes = [
-///     opentelemetry::KeyValue::new("frobbles.color".to_string(), "blue".into()),
-/// ];
+/// let attributes = vec![];
+/// if (frobbled) {
+///     attributes.push(opentelemetry::KeyValue::new("frobbles.color".to_string(), "blue".into()));
+/// }
 /// u64_counter!(
 ///     "apollo.router.operations.frobbles",
 ///     "The amount of frobbles we've operated on",
@@ -939,6 +940,11 @@ macro_rules! assert_counter {
         assert_metric!(result, $name, Some($value.into()), None, &attributes);
     };
 
+    ($name:literal, $value: expr, $attributes: expr) => {
+        let result = crate::metrics::collect_metrics().assert($name, crate::metrics::test_utils::MetricType::Counter, $value, $attributes);
+        assert_metric!(result, $name, Some($value.into()), None, &$attributes);
+    };
+
     ($name:literal, $value: expr) => {
         let result = crate::metrics::collect_metrics().assert($name, crate::metrics::test_utils::MetricType::Counter, $value, &[]);
         assert_metric!(result, $name, Some($value.into()), None, &[]);
@@ -1209,6 +1215,7 @@ mod test {
         let attributes = vec![KeyValue::new("attr", "val")];
         u64_counter!("test", "test description", 1, attributes);
         assert_counter!("test", 1, "attr" = "val");
+        assert_counter!("test", 1, &attributes);
     }
 
     #[test]

apollo-router/src/services/layers/persisted_queries/manifest_poller.rs

@@ -34,6 +34,13 @@ pub struct FullPersistedQueryOperationId {
 /// An in memory cache of persisted queries.
 pub type PersistedQueryManifest = HashMap<FullPersistedQueryOperationId, String>;
 
+/// Describes whether the router should allow or deny a given request.
+/// with an error, or allow it but log the operation as unknown.
+pub(crate) struct FreeformGraphQLAction {
+    pub(crate) should_allow: bool,
+    pub(crate) should_log: bool,
+}
+
 /// How the router should respond to requests that are not resolved as the IDs
 /// of an operation in the manifest. (For the most part this means "requests
 /// sent as freeform GraphQL", though it also includes requests sent as an ID
@@ -58,49 +65,43 @@ pub(crate) enum FreeformGraphQLBehavior {
     },
 }
 
-/// Describes what the router should do for a given request: allow it, deny it
-/// with an error, or allow it but log the operation as unknown.
-pub(crate) enum FreeformGraphQLAction {
-    Allow,
-    Deny,
-    AllowAndLog,
-    DenyAndLog,
-}
-
 impl FreeformGraphQLBehavior {
     fn action_for_freeform_graphql(
         &self,
         ast: Result<&ast::Document, &str>,
     ) -> FreeformGraphQLAction {
         match self {
-            FreeformGraphQLBehavior::AllowAll { .. } => FreeformGraphQLAction::Allow,
+            FreeformGraphQLBehavior::AllowAll { .. } => FreeformGraphQLAction {
+                should_allow: true,
+                should_log: false,
+            },
             // Note that this branch doesn't get called in practice, because we catch
             // DenyAll at an earlier phase with never_allows_freeform_graphql.
-            FreeformGraphQLBehavior::DenyAll { log_unknown, .. } => {
-                if *log_unknown {
-                    FreeformGraphQLAction::DenyAndLog
-                } else {
-                    FreeformGraphQLAction::Deny
-                }
-            }
+            FreeformGraphQLBehavior::DenyAll { log_unknown, .. } => FreeformGraphQLAction {
+                should_allow: false,
+                should_log: *log_unknown,
+            },
             FreeformGraphQLBehavior::AllowIfInSafelist {
                 safelist,
                 log_unknown,
                 ..
             } => {
                 if safelist.is_allowed(ast) {
-                    FreeformGraphQLAction::Allow
-                } else if *log_unknown {
-                    FreeformGraphQLAction::DenyAndLog
+                    FreeformGraphQLAction {
+                        should_allow: true,
+                        should_log: false,
+                    }
                 } else {
-                    FreeformGraphQLAction::Deny
+                    FreeformGraphQLAction {
+                        should_allow: false,
+                        should_log: *log_unknown,
+                    }
                 }
             }
             FreeformGraphQLBehavior::LogUnlessInSafelist { safelist, .. } => {
-                if safelist.is_allowed(ast) {
-                    FreeformGraphQLAction::Allow
-                } else {
-                    FreeformGraphQLAction::AllowAndLog
+                FreeformGraphQLAction {
+                    should_allow: true,
+                    should_log: !safelist.is_allowed(ast),
                 }
             }
         }