Skip to content
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

Reconcile dev after merge to main for v1.45.1 #5037

Merged
merged 10 commits into from
Apr 29, 2024
16 changes: 8 additions & 8 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -48,14 +48,14 @@ executors:
# See https://circleci.com/docs/xcode-policy along with the support matrix
# at https://circleci.com/docs/using-macos#supported-xcode-versions.
# We use the major.minor notation to bring in compatible patches.
xcode: 14.2
xcode: "14.2.0"
resource_class: macos.m1.medium.gen1
macos_test: &macos_test_executor
macos:
# See https://circleci.com/docs/xcode-policy along with the support matrix
# at https://circleci.com/docs/using-macos#supported-xcode-versions.
# We use the major.minor notation to bring in compatible patches.
xcode: 14.2
xcode: "14.2.0"
resource_class: macos.m1.medium.gen1
windows_build: &windows_build_executor
machine:
Expand Down Expand Up @@ -201,7 +201,7 @@ commands:
find xtask/src -type f | while read name; do md5sum $name; done | sort -k 2 | md5sum > ~/.xtask_version
# The closest common ancestor to the default branch, so that test jobs can take advantage previous compiles
git remote set-head origin -a
TARGET_BRANCH=$(git rev-parse --abbrev-ref origin/HEAD)
TARGET_BRANCH=$(git rev-parse --abbrev-ref origin/HEAD)
echo "Target branch is ${TARGET_BRANCH}"
COMMON_ANCESTOR_REF=$(git merge-base HEAD "${TARGET_BRANCH}")
echo "Common ancestor is ${COMMON_ANCESTOR_REF}"
Expand Down Expand Up @@ -737,8 +737,8 @@ jobs:
# save containers for analysis
mkdir built-containers
docker save -o built-containers/router_${VERSION}-debug.tar ${ROUTER_TAG}:${VERSION}-debug
docker save -o built-containers/router_${VERSION}.tar ${ROUTER_TAG}:${VERSION}
docker save -o built-containers/router_${VERSION}.tar ${ROUTER_TAG}:${VERSION}

- persist_to_workspace:
root: .
paths:
Expand Down Expand Up @@ -993,8 +993,8 @@ workflows:
# Disables all PR comments from this job
do-pr-comments: false
# Scan job will return 1 if findings violating the Wiz policy are found.
# Toggle off to prevent any CI failures OR
# contact Apollo's Security team to adjust what violates the
# Toggle off to prevent any CI failures OR
# contact Apollo's Security team to adjust what violates the
# Wiz policy used in this scan.
fail-on-findings: true
# Configure scan job to use a policy specific to apollo-router.
Expand Down Expand Up @@ -1091,7 +1091,7 @@ workflows:
ignore: /.*/
tags:
only: /v.*/

security-scans:
when:
not: << pipeline.parameters.nightly >>
Expand Down
34 changes: 34 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,37 @@ All notable changes to Router will be documented in this file.

This project adheres to [Semantic Versioning v2.0.0](https://semver.org/spec/v2.0.0.html).

# [1.45.1] - 2024-04-26

## 🐛 Fixes

### Correct v1.44.0 regression in query plan cache ([PR #5028](https://github.com/apollographql/router/pull/5028))

Correct a critical regression that was introduced in [v1.44.0](https://github.com/apollographql/router/pull/4883) which could lead to execution of an incorrect query plan. This issue only affects Routers that use [distributed query plan caching](https://www.apollographql.com/docs/router/configuration/distributed-caching/#distributed-query-plan-caching), enabled via the `supergraph.query_planning.cache.redis.urls` configuration property.

By [@o0Ignition0o](https://github.com/o0Ignition0o) in https://github.com/apollographql/router/pull/5028

### Use entire schema when hashing an introspection query ([Issue #5006](https://github.com/apollographql/router/issues/5006))

Correct a _different_ hashing bug which impacted introspection queries which was also introduced in [v1.44.0](https://github.com/apollographql/router/pull/4883). This other hashing bug failed to account for introspection queries, resulting in introspection results being misaligned to the current schema. This issue only affects Routers that use [distributed query plan caching](https://www.apollographql.com/docs/router/configuration/distributed-caching/#distributed-query-plan-caching), enabled via the `supergraph.query_planning.cache.redis.urls` configuration property.

This release fixes the hashing mechanism by adding the schema string to hashed data if an introspection field is encountered. As a result, the entire schema is taken into account and the correct introspection result is returned.

By [@Geal](https://github.com/Geal) in https://github.com/apollographql/router/pull/5007

### Fix subgraph name mapping of Datadog exporter ([PR #5012](https://github.com/apollographql/router/pull/5012))

Previously in the router v1.45.0, subgraph name mapping didn't work correctly in the router's Datadog exporter. The exporter used the incorrect value `apollo.subgraph.name` for mapping attributes when it should have used the value `subgraph.name`. This issue has been fixed in this release.

By [@garypen](https://github.com/garypen) in https://github.com/apollographql/router/pull/5012


# [1.45.0] - 2024-04-22

> **Warning**
>
> **This version has a critical bug impacting users of [distributed query plan caching](https://www.apollographql.com/docs/router/configuration/distributed-caching/#distributed-query-plan-caching). See the _Fixes_ in [v1.45.1](https://github.com/apollographql/router/releases/tag/v1.45.1) for details. We highly recommend using v1.45.1 or v1.43.2 over v1.45.0.**

## 🚀 Features

### Query validation process with Rust ([PR #4551](https://github.com/apollographql/router/pull/4551))
Expand Down Expand Up @@ -154,6 +183,11 @@ By [@bonnici](https://github.com/bonnici) in https://github.com/apollographql/ro

# [1.44.0] - 2024-04-12

> **Warning**
>
> **This version has a critical bug impacting users of [distributed query plan caching](https://www.apollographql.com/docs/router/configuration/distributed-caching/#distributed-query-plan-caching). See the _Fixes_ in [v1.45.1](https://github.com/apollographql/router/releases/tag/v1.45.1) for details. We highly recommend using v1.45.1 or v1.43.2 over v1.44.0.**


## 🚀 Features

### Add details to `router service call failed` errors ([Issue #4899](https://github.com/apollographql/router/issues/4899))
Expand Down
6 changes: 3 additions & 3 deletions Cargo.lock
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ dependencies = [

[[package]]
name = "apollo-router"
version = "1.45.0"
version = "1.45.1"
dependencies = [
"access-json",
"anyhow",
Expand Down Expand Up @@ -412,7 +412,7 @@ dependencies = [

[[package]]
name = "apollo-router-benchmarks"
version = "1.45.0"
version = "1.45.1"
dependencies = [
"apollo-parser",
"apollo-router",
Expand All @@ -428,7 +428,7 @@ dependencies = [

[[package]]
name = "apollo-router-scaffold"
version = "1.45.0"
version = "1.45.1"
dependencies = [
"anyhow",
"cargo-scaffold",
Expand Down
2 changes: 1 addition & 1 deletion apollo-router-benchmarks/Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "apollo-router-benchmarks"
version = "1.45.0"
version = "1.45.1"
authors = ["Apollo Graph, Inc. <packages@apollographql.com>"]
edition = "2021"
license = "Elastic-2.0"
Expand Down
2 changes: 1 addition & 1 deletion apollo-router-scaffold/Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "apollo-router-scaffold"
version = "1.45.0"
version = "1.45.1"
authors = ["Apollo Graph, Inc. <packages@apollographql.com>"]
edition = "2021"
license = "Elastic-2.0"
Expand Down
2 changes: 1 addition & 1 deletion apollo-router-scaffold/templates/base/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ apollo-router = { path ="{{integration_test}}apollo-router" }
apollo-router = { git="https://github.com/apollographql/router.git", branch="{{branch}}" }
{{else}}
# Note if you update these dependencies then also update xtask/Cargo.toml
apollo-router = "1.45.0"
apollo-router = "1.45.1"
{{/if}}
{{/if}}
async-trait = "0.1.52"
Expand Down
2 changes: 1 addition & 1 deletion apollo-router-scaffold/templates/base/xtask/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ apollo-router-scaffold = { path ="{{integration_test}}apollo-router-scaffold" }
{{#if branch}}
apollo-router-scaffold = { git="https://github.com/apollographql/router.git", branch="{{branch}}" }
{{else}}
apollo-router-scaffold = { git = "https://github.com/apollographql/router.git", tag = "v1.45.0" }
apollo-router-scaffold = { git = "https://github.com/apollographql/router.git", tag = "v1.45.1" }
{{/if}}
{{/if}}
anyhow = "1.0.58"
Expand Down
2 changes: 1 addition & 1 deletion apollo-router/Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "apollo-router"
version = "1.45.0"
version = "1.45.1"
authors = ["Apollo Graph, Inc. <packages@apollographql.com>"]
repository = "https://github.com/apollographql/router/"
documentation = "https://docs.rs/apollo-router"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ expression: query_plan
"id": null,
"inputRewrites": null,
"outputRewrites": null,
"schemaAwareHash": "9358047754b11522aac502a3c6a668cd4286c07d489680834e63d6e033db4eb5",
"schemaAwareHash": "12dda6193654ae4fe6e38bc09d4f81cc73d0c9e098692096f72d2158eef4776f",
"authorization": {
"is_authenticated": false,
"scopes": [],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ expression: query_plan
"id": null,
"inputRewrites": null,
"outputRewrites": null,
"schemaAwareHash": "8f445761c0bcdda90b8da35ccd13fd98e474514f3efc071bd2c39495b5af94e5",
"schemaAwareHash": "00ad582ea45fc1bce436b36b21512f3d2c47b74fdbdc61e4b349289722c9ecf2",
"authorization": {
"is_authenticated": false,
"scopes": [],
Expand Down Expand Up @@ -61,7 +61,7 @@ expression: query_plan
"id": null,
"inputRewrites": null,
"outputRewrites": null,
"schemaAwareHash": "9a1feab7ee8c57c8a4ab4db29712412a9cfe94009bfcb40dc0d22ea54c410865",
"schemaAwareHash": "a8ebdc2151a2e5207882e43c6906c0c64167fd9a8e0c7c4becc47736a5105096",
"authorization": {
"is_authenticated": false,
"scopes": [],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ expression: "serde_json::to_value(response).unwrap()"
"id": null,
"inputRewrites": null,
"outputRewrites": null,
"schemaAwareHash": "34be619a78867ab9d0670048f4c93574e38cd9253e9cc032f567078355b25086",
"schemaAwareHash": "7245d488e97c3b2ac9f5fa4dd4660940b94ad81af070013305b2c0f76337b2f9",
"authorization": {
"is_authenticated": false,
"scopes": [],
Expand Down Expand Up @@ -107,7 +107,7 @@ expression: "serde_json::to_value(response).unwrap()"
"id": null,
"inputRewrites": null,
"outputRewrites": null,
"schemaAwareHash": "f1582d942020b23347d84f6ae46c018492ae7c59c9b1472e0b442121ddf16368",
"schemaAwareHash": "6e0b4156706ea0cf924500cfdc99dd44b9f0ed07e2d3f888d4aff156e6a33238",
"authorization": {
"is_authenticated": false,
"scopes": [],
Expand Down Expand Up @@ -153,7 +153,7 @@ expression: "serde_json::to_value(response).unwrap()"
"id": null,
"inputRewrites": null,
"outputRewrites": null,
"schemaAwareHash": "6fa5a74c5af2b18f343e9e69bbcbc9335e9faaa46c3d8964d199002dfeb0026f",
"schemaAwareHash": "ff649f3d70241d5a8cd5f5d03ff4c41ecff72b0e4129a480207b05ac92318042",
"authorization": {
"is_authenticated": false,
"scopes": [],
Expand Down Expand Up @@ -196,7 +196,7 @@ expression: "serde_json::to_value(response).unwrap()"
"id": null,
"inputRewrites": null,
"outputRewrites": null,
"schemaAwareHash": "6fa5a74c5af2b18f343e9e69bbcbc9335e9faaa46c3d8964d199002dfeb0026f",
"schemaAwareHash": "bf9f3beda78a7a565e47c862157bad4ec871d724d752218da1168455dddca074",
"authorization": {
"is_authenticated": false,
"scopes": [],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ expression: "serde_json::to_value(response).unwrap()"
"id": null,
"inputRewrites": null,
"outputRewrites": null,
"schemaAwareHash": "34be619a78867ab9d0670048f4c93574e38cd9253e9cc032f567078355b25086",
"schemaAwareHash": "7245d488e97c3b2ac9f5fa4dd4660940b94ad81af070013305b2c0f76337b2f9",
"authorization": {
"is_authenticated": false,
"scopes": [],
Expand Down Expand Up @@ -107,7 +107,7 @@ expression: "serde_json::to_value(response).unwrap()"
"id": null,
"inputRewrites": null,
"outputRewrites": null,
"schemaAwareHash": "f1582d942020b23347d84f6ae46c018492ae7c59c9b1472e0b442121ddf16368",
"schemaAwareHash": "6e0b4156706ea0cf924500cfdc99dd44b9f0ed07e2d3f888d4aff156e6a33238",
"authorization": {
"is_authenticated": false,
"scopes": [],
Expand Down Expand Up @@ -153,7 +153,7 @@ expression: "serde_json::to_value(response).unwrap()"
"id": null,
"inputRewrites": null,
"outputRewrites": null,
"schemaAwareHash": "6fa5a74c5af2b18f343e9e69bbcbc9335e9faaa46c3d8964d199002dfeb0026f",
"schemaAwareHash": "ff649f3d70241d5a8cd5f5d03ff4c41ecff72b0e4129a480207b05ac92318042",
"authorization": {
"is_authenticated": false,
"scopes": [],
Expand Down Expand Up @@ -196,7 +196,7 @@ expression: "serde_json::to_value(response).unwrap()"
"id": null,
"inputRewrites": null,
"outputRewrites": null,
"schemaAwareHash": "6fa5a74c5af2b18f343e9e69bbcbc9335e9faaa46c3d8964d199002dfeb0026f",
"schemaAwareHash": "bf9f3beda78a7a565e47c862157bad4ec871d724d752218da1168455dddca074",
"authorization": {
"is_authenticated": false,
"scopes": [],
Expand Down
21 changes: 14 additions & 7 deletions apollo-router/src/query_planner/bridge_query_planner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -631,7 +631,7 @@ impl BridgeQueryPlanner {
plan_success
.data
.query_plan
.hash_subqueries(&self.subgraph_schemas);
.hash_subqueries(&self.subgraph_schemas, &self.schema.raw_sdl);
plan_success
.data
.query_plan
Expand Down Expand Up @@ -830,19 +830,21 @@ impl Service<QueryPlannerRequest> for BridgeQueryPlanner {
Some(d) => d,
};

let schema = this.schema.api_schema();
match add_defer_labels(schema, &doc.ast) {
let api_schema = this.schema.api_schema();
match add_defer_labels(api_schema, &doc.ast) {
Err(e) => {
return Err(QueryPlannerError::SpecError(SpecError::TransformError(
e.to_string(),
)))
}
Ok(modified_query) => {
let executable_document = modified_query
.to_executable_validate(schema)
.to_executable_validate(api_schema)
// Assume transformation creates a valid document: ignore conversion errors
.map_err(|e| SpecError::ValidationError(e.into()))?;
let hash = QueryHashVisitor::hash_query(
schema,
this.schema.supergraph_schema(),
&this.schema.raw_sdl,
Comment on lines +846 to +847
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: here's an additional fix to the query hash I just found: the hashing mechanism has to be done on the supergraph schema, not the API schema. If needed I can split that change in a separate PR, that we would merge immediately after

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just want to clarify that while this is an additional fix necessary for the re-introduction of #5028, it does not constitute a necessary fix right now given that #5028 was exhaustive enough in and of itself.

&executable_document,
operation_name.as_deref(),
)
Expand Down Expand Up @@ -962,6 +964,7 @@ impl BridgeQueryPlanner {
.map_err(|e| SpecError::ValidationError(e.into()))?;
let hash = QueryHashVisitor::hash_query(
self.schema.supergraph_schema(),
&self.schema.raw_sdl,
&executable_document,
key.operation_name.as_deref(),
)
Expand Down Expand Up @@ -1054,9 +1057,13 @@ pub(super) struct QueryPlan {
}

impl QueryPlan {
fn hash_subqueries(&mut self, subgraph_schemas: &SubgraphSchemas) {
fn hash_subqueries(
&mut self,
subgraph_schemas: &SubgraphSchemas,
supergraph_schema_hash: &str,
) {
if let Some(node) = self.node.as_mut() {
node.hash_subqueries(subgraph_schemas);
node.hash_subqueries(subgraph_schemas, supergraph_schema_hash);
}
}

Expand Down
5 changes: 5 additions & 0 deletions apollo-router/src/query_planner/caching_query_planner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ where
hash,
metadata,
plan_options,
..
},
_,
)| WarmUpCachingQueryKey {
Expand Down Expand Up @@ -206,6 +207,7 @@ where
query: query.clone(),
operation: operation.clone(),
hash: doc.hash.clone(),
sdl: Arc::clone(&self.schema.raw_sdl),
metadata,
plan_options,
};
Expand Down Expand Up @@ -386,6 +388,7 @@ where
query: request.query.clone(),
operation: request.operation_name.to_owned(),
hash: doc.hash.clone(),
sdl: Arc::clone(&self.schema.raw_sdl),
metadata,
plan_options,
};
Expand Down Expand Up @@ -522,6 +525,7 @@ fn stats_report_key_hash(stats_report_key: &str) -> String {
#[derive(Debug, Clone, PartialEq, Eq)]
pub(crate) struct CachingQueryKey {
pub(crate) query: String,
pub(crate) sdl: Arc<String>,
pub(crate) operation: Option<String>,
pub(crate) hash: Arc<QueryHash>,
pub(crate) metadata: CacheKeyMetadata,
Expand All @@ -541,6 +545,7 @@ impl std::fmt::Display for CachingQueryKey {
hasher.update(
&serde_json::to_vec(&self.plan_options).expect("serialization should not fail"),
);
hasher.update(&serde_json::to_vec(&self.sdl).expect("serialization should not fail"));
let metadata = hex::encode(hasher.finalize());

write!(
Expand Down
14 changes: 11 additions & 3 deletions apollo-router/src/query_planner/fetch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -607,12 +607,20 @@ impl FetchNode {
&self.operation_kind
}

pub(crate) fn hash_subquery(&mut self, subgraph_schemas: &SubgraphSchemas) {
pub(crate) fn hash_subquery(
&mut self,
subgraph_schemas: &SubgraphSchemas,
supergraph_schema_hash: &str,
) {
let doc = self.parsed_operation(subgraph_schemas);
let schema = &subgraph_schemas[self.service_name.as_str()];

if let Ok(hash) = QueryHashVisitor::hash_query(schema, doc, self.operation_name.as_deref())
{
if let Ok(hash) = QueryHashVisitor::hash_query(
schema,
supergraph_schema_hash,
doc,
self.operation_name.as_deref(),
) {
self.schema_aware_hash = Arc::new(QueryHash(hash));
}
}
Expand Down