Skip to content

Fix H2 URI path matching in ALB health check layer #4288

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.

Sign up for GitHub

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Fix H2 URI path matching in ALB health check layer #4288

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
12bd774
For h2, uri path should be explicitly matched
Sep 7, 2025
0543dab
add changelog entry
Sep 7, 2025
aef9c71
Bump smithy server version
Sep 7, 2025
fc73951
Add uri_with_only_path test
Sep 9, 2025
1a5d7d5
Merge branch 'main' into fahadzub/health
drganjoo Sep 10, 2025
e26efed
Merge branch 'main' into fahadzub/health
drganjoo Sep 10, 2025
7052e9f
Merge branch 'main' into fahadzub/health
drganjoo Sep 10, 2025
95e4742
Change minor version only for aws-smithy-http-server
drganjoo Sep 11, 2025
d713193
Merge branch 'main' into fahadzub/health
drganjoo Sep 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions .changelog/1757207291.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
---
applies_to: [server]
authors: [drganjoo]
references: []
breaking: false
new_feature: false
bug_fix: true
---
Fixed URI path matching for H2 protocol in the ALB health check layer to ensure explicit path comparison.

2 changes: 1 addition & 1 deletion rust-runtime/aws-smithy-http-server/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "aws-smithy-http-server"
version = "0.65.5"
version = "0.66.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

why did you make a new breaking version? isn't this going to cause problems? This change seems backwards compatible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're absolutely right - this is a backwards compatible change. I bumped to 0.66 as a precautionary measure because I wanted to avoid any potential versioning issues during integration testing. At the time, this was planned to be included in the next code generator release, so the version bump seemed appropriate. However, since this now needs to be released independently of the code generator, you're correct that 0.65.6 would be more suitable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

well and it seems like making a new breaking change release of the libraries when we don't need it only causes lots of pain?

Copy link
Contributor Author

@drganjoo drganjoo Sep 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

We typically never release intermediate crate versions of aws-smithy-http-server independently of the code generator, so bumping the major version, combined with a code-generator release, is usually the safer approach - it eliminates any risk of breaking users due to potential CI tooling issues. When releases are tied to the code generator, a major version bump has minimal downstream impact compared to accidentally introducing breaking changes.

However, since this particular release now needs to happen independently of the code generator, I've changed it to only bump the minor version.

authors = ["Smithy Rust Server <[email protected]>"]
edition = "2021"
license = "Apache-2.0"
Expand Down
31 changes: 30 additions & 1 deletion rust-runtime/aws-smithy-http-server/src/layer/alb_health_check.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ where
}

fn call(&mut self, req: Request<Body>) -> Self::Future {
if req.uri() == self.layer.health_check_uri.as_ref() {
if req.uri().path() == self.layer.health_check_uri.as_ref() {
let clone = self.layer.health_check_handler.clone();
let service = std::mem::replace(&mut self.layer.health_check_handler, clone);
let handler_future = service.oneshot(req);
Expand Down Expand Up @@ -179,3 +179,32 @@ where
}
}
}

#[cfg(test)]
mod tests {
use super::*;
use http::Uri;
use tower::{service_fn, ServiceExt};

#[tokio::test]
async fn health_check_matches_path_with_scheme_and_authority() {
Copy link
Contributor

Choose a reason for hiding this comment

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

For my own edification where is the "with scheme and authority" part of this test coming from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The scheme and authority are intentionally hard-coded in this test. I've renamed the test to health_check_matches_path_when_uri_contains_scheme_authority_and_path to better reflect its purpose.

This test specifically verifies that the health check correctly matches requests based solely on their path component, regardless of whether the URI also includes scheme and authority components. This ensures our health check behavior remains consistent across different URI.

let layer = AlbHealthCheckLayer::from_handler("/ping", |_req| async { StatusCode::OK });

let inner_service = service_fn(|_req| async {
Ok::<_, std::convert::Infallible>(
Response::builder()
.status(StatusCode::IM_A_TEAPOT)
.body(crate::body::empty())
.expect("infallible"),
)
});

let service = layer.layer(inner_service);

let uri: Uri = "https://example.com/ping".parse().unwrap();
let request = Request::builder().uri(uri).body(Body::empty()).unwrap();

let response = service.oneshot(request).await.unwrap();
assert_eq!(response.status(), StatusCode::OK);
}
}
Loading