Skip to content

Commit

Permalink
Fixed all_of and any_of HTTP filters (#2818)
Browse files Browse the repository at this point in the history
* Fixed layer and reverse portforwarder

* Doc fix

* Added integration test
  • Loading branch information
Razz4780 authored Oct 10, 2024
1 parent b973f47 commit abdee6d
Show file tree
Hide file tree
Showing 6 changed files with 274 additions and 19 deletions.
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions changelog.d/2817.fixed.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fixed a bug where `all_of` and `any_of` HTTP filters were stealing all HTTP traffic.
72 changes: 63 additions & 9 deletions mirrord/cli/src/port_forward.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@ use std::{
};

use futures::StreamExt;
use mirrord_config::feature::network::incoming::IncomingConfig;
use mirrord_config::feature::network::incoming::{
http_filter::{HttpFilterConfig, InnerFilter},
IncomingConfig,
};
use mirrord_intproxy::{
background_tasks::{BackgroundTasks, TaskError, TaskSender, TaskUpdate},
error::IntProxyError,
Expand Down Expand Up @@ -877,23 +880,74 @@ impl IncomingMode {

let ports = { http_filter_config.ports.iter().copied().collect() };

let filter = match (
&http_filter_config.path_filter,
&http_filter_config.header_filter,
) {
(Some(path), None) => StealHttpFilter::Filter(HttpFilter::Path(
// Matching all fields to make this check future-proof.
let filter = match http_filter_config {
HttpFilterConfig {
path_filter: Some(path),
header_filter: None,
all_of: None,
any_of: None,
ports: _ports,
} => StealHttpFilter::Filter(HttpFilter::Path(
Filter::new(path.into()).expect("invalid filter expression"),
)),
(None, Some(header)) => StealHttpFilter::Filter(HttpFilter::Header(

HttpFilterConfig {
path_filter: None,
header_filter: Some(header),
all_of: None,
any_of: None,
ports: _ports,
} => StealHttpFilter::Filter(HttpFilter::Header(
Filter::new(header.into()).expect("invalid filter expression"),
)),
(None, None) => StealHttpFilter::None,
_ => panic!("multiple HTTP filters specified"),

HttpFilterConfig {
path_filter: None,
header_filter: None,
all_of: Some(filters),
any_of: None,
ports: _ports,
} => StealHttpFilter::Filter(Self::make_composite_filter(true, filters)),

HttpFilterConfig {
path_filter: None,
header_filter: None,
all_of: None,
any_of: Some(filters),
ports: _ports,
} => StealHttpFilter::Filter(Self::make_composite_filter(false, filters)),

HttpFilterConfig {
path_filter: None,
header_filter: None,
all_of: None,
any_of: None,
ports: _ports,
} => StealHttpFilter::None,

_ => panic!("multiple HTTP filters specified, this is a bug"),
};

Self::Steal(StealHttpSettings { filter, ports })
}

fn make_composite_filter(all: bool, filters: &[InnerFilter]) -> HttpFilter {
let filters = filters
.iter()
.map(|filter| match filter {
InnerFilter::Path { path } => {
HttpFilter::Path(Filter::new(path.clone()).expect("invalid filter expression"))
}
InnerFilter::Header { header } => HttpFilter::Header(
Filter::new(header.clone()).expect("invalid filter expression"),
),
})
.collect();

HttpFilter::Composite { all, filters }
}

/// Returns [`PortSubscription`] request to be used for the given port.
fn subscription(&self, port: Port) -> PortSubscription {
let Self::Steal(steal) = self else {
Expand Down
1 change: 1 addition & 0 deletions mirrord/layer/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ chrono = { version = "0.4", features = ["clock"] }
http-body = { workspace = true }
hyper = { workspace = true }
rstest = "*"
tempfile = "3"
test-cdylib = "*"
tower = "0.4"
tokio = { version = "1", features = ["rt", "net", "macros"] }
Expand Down
77 changes: 67 additions & 10 deletions mirrord/layer/src/setup.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,13 @@ use mirrord_config::{
feature::{
env::EnvConfig,
fs::FsConfig,
network::{incoming::IncomingConfig, outgoing::OutgoingConfig},
network::{
incoming::{
http_filter::{HttpFilterConfig, InnerFilter},
IncomingConfig,
},
outgoing::OutgoingConfig,
},
},
target::Target,
LayerConfig,
Expand Down Expand Up @@ -211,7 +217,7 @@ pub enum IncomingMode {
}

impl IncomingMode {
/// Creates a new instance from the given [`LayerConfig`].
/// Creates a new instance from the given [`IncomingConfig`].
fn new(config: &IncomingConfig) -> Self {
if !config.is_steal() {
return Self::Mirror;
Expand All @@ -221,23 +227,74 @@ impl IncomingMode {

let ports = { http_filter_config.ports.iter().copied().collect() };

let filter = match (
&http_filter_config.path_filter,
&http_filter_config.header_filter,
) {
(Some(path), None) => StealHttpFilter::Filter(HttpFilter::Path(
// Matching all fields to make this check future-proof.
let filter = match http_filter_config {
HttpFilterConfig {
path_filter: Some(path),
header_filter: None,
all_of: None,
any_of: None,
ports: _ports,
} => StealHttpFilter::Filter(HttpFilter::Path(
Filter::new(path.into()).expect("invalid filter expression"),
)),
(None, Some(header)) => StealHttpFilter::Filter(HttpFilter::Header(

HttpFilterConfig {
path_filter: None,
header_filter: Some(header),
all_of: None,
any_of: None,
ports: _ports,
} => StealHttpFilter::Filter(HttpFilter::Header(
Filter::new(header.into()).expect("invalid filter expression"),
)),
(None, None) => StealHttpFilter::None,
_ => panic!("multiple HTTP filters specified"),

HttpFilterConfig {
path_filter: None,
header_filter: None,
all_of: Some(filters),
any_of: None,
ports: _ports,
} => StealHttpFilter::Filter(Self::make_composite_filter(true, filters)),

HttpFilterConfig {
path_filter: None,
header_filter: None,
all_of: None,
any_of: Some(filters),
ports: _ports,
} => StealHttpFilter::Filter(Self::make_composite_filter(false, filters)),

HttpFilterConfig {
path_filter: None,
header_filter: None,
all_of: None,
any_of: None,
ports: _ports,
} => StealHttpFilter::None,

_ => panic!("multiple HTTP filters specified, this is a bug"),
};

Self::Steal(StealHttpSettings { filter, ports })
}

fn make_composite_filter(all: bool, filters: &[InnerFilter]) -> HttpFilter {
let filters = filters
.iter()
.map(|filter| match filter {
InnerFilter::Path { path } => {
HttpFilter::Path(Filter::new(path.clone()).expect("invalid filter expression"))
}
InnerFilter::Header { header } => HttpFilter::Header(
Filter::new(header.clone()).expect("invalid filter expression"),
),
})
.collect();

HttpFilter::Composite { all, filters }
}

/// Returns [`PortSubscription`] request to be used for the given port.
pub fn subscription(&self, port: Port) -> PortSubscription {
let Self::Steal(steal) = self else {
Expand Down
141 changes: 141 additions & 0 deletions mirrord/layer/tests/issue2817.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
#![feature(assert_matches)]
#![warn(clippy::indexing_slicing)]

use std::{path::Path, time::Duration};

use mirrord_protocol::{
tcp::{Filter, HttpFilter, LayerTcpSteal, StealType},
ClientMessage,
};
use rand::Rng;
use rstest::rstest;

mod common;

pub use common::*;

#[derive(Clone, Copy, Debug)]
enum TestedStealVariant {
Unfiltered,
Header,
Path,
AllOf,
AnyOf,
}

impl TestedStealVariant {
fn as_json_config(self) -> serde_json::Value {
serde_json::json!({
"path_filter": matches!(self, Self::Path).then(|| "/some/path"),
"header_filter": matches!(self, Self::Header).then(|| "some: header"),
"all_of": matches!(self, Self::AllOf).then(|| serde_json::json!([
{ "path": "/some/path" },
{ "header": "some: header" },
])),
"any_of": matches!(self, Self::AnyOf).then(|| serde_json::json!([
{ "path": "/some/path" },
{ "header": "some: header" },
]))
})
}

fn assert_matches(self, steal_type: StealType) {
match (self, steal_type) {
(Self::Unfiltered, StealType::All(80)) => {}
(Self::Header, StealType::FilteredHttpEx(80, HttpFilter::Header(filter))) => {
assert_eq!(filter, Filter::new("some: header".into()).unwrap())
}
(Self::Path, StealType::FilteredHttpEx(80, HttpFilter::Path(filter))) => {
assert_eq!(filter, Filter::new("/some/path".into()).unwrap())
}
(
Self::AllOf,
StealType::FilteredHttpEx(80, HttpFilter::Composite { all: true, filters }),
) => {
assert_eq!(
filters,
vec![
HttpFilter::Path(Filter::new("/some/path".into()).unwrap()),
HttpFilter::Header(Filter::new("some: header".into()).unwrap()),
],
)
}
(
Self::AnyOf,
StealType::FilteredHttpEx(
80,
HttpFilter::Composite {
all: false,
filters,
},
),
) => {
assert_eq!(
filters,
vec![
HttpFilter::Path(Filter::new("/some/path".into()).unwrap()),
HttpFilter::Header(Filter::new("some: header".into()).unwrap()),
],
)
}
(.., steal_type) => panic!("received unexpected steal type: {steal_type:?}"),
}
}
}

/// Verify that the layer requests correct subscription type based on HTTP filter config.
#[rstest]
#[tokio::test]
#[timeout(Duration::from_secs(60))]
async fn test_issue2817(
#[values(Application::NodeHTTP)] application: Application,
dylib_path: &Path,
#[values(
TestedStealVariant::Unfiltered,
TestedStealVariant::Header,
TestedStealVariant::Path,
TestedStealVariant::AllOf,
TestedStealVariant::AnyOf
)]
config_variant: TestedStealVariant,
) {
let dir = tempfile::tempdir().unwrap();
let file_id = rand::thread_rng().gen::<u64>();
let config_path = dir.path().join(format!("{file_id:X}.json"));

let config = serde_json::json!({
"feature": {
"network": {
"incoming": {
"mode": "steal",
"http_filter": config_variant.as_json_config(),
}
}
}
});

tokio::fs::write(&config_path, serde_json::to_string_pretty(&config).unwrap())
.await
.expect("failed to saving layer config to tmp file");

let (mut test_process, mut intproxy) = application
.start_process_with_layer(
dylib_path,
Default::default(),
Some(config_path.to_str().unwrap()),
)
.await;

let message = intproxy.recv().await;
let steal_type = match message {
ClientMessage::TcpSteal(LayerTcpSteal::PortSubscribe(steal_type)) => steal_type,
other => panic!("unexpected message received from the app: {other:?}"),
};
config_variant.assert_matches(steal_type);

test_process
.child
.kill()
.await
.expect("failed to kill the app");
}

0 comments on commit abdee6d

Please sign in to comment.