From bbde582368848dc404416e3a2581a81aac366666 Mon Sep 17 00:00:00 2001 From: Philip Jenvey Date: Mon, 30 Oct 2023 16:14:19 -0700 Subject: [PATCH] fix: split router endpoints into their own app (#491) * fix: split router endpoints into their own app which binds solely to the router port SYNC-3979 --- Cargo.lock | 2 ++ autoconnect/Cargo.toml | 2 ++ autoconnect/autoconnect-web/src/dockerflow.rs | 16 +++++++++- autoconnect/autoconnect-web/src/lib.rs | 24 +++++++-------- autoconnect/autoconnect-web/src/routes.rs | 8 ++--- autoconnect/autoconnect-web/src/test.rs | 4 +-- .../autoconnect-ws-sm/src/error.rs | 3 -- autoconnect/autoconnect-ws/src/ping.rs | 2 +- autoconnect/src/main.rs | 30 +++++++++++++++---- tests/test_integration_all_rust.py | 22 ++++++++++++++ 10 files changed, 82 insertions(+), 31 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index eafd8f279..d52b3d0e7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -528,6 +528,8 @@ dependencies = [ "actix-cors", "actix-http", "actix-rt", + "actix-server", + "actix-service", "actix-test", "actix-web", "actix-ws", diff --git a/autoconnect/Cargo.toml b/autoconnect/Cargo.toml index fdb976b8a..9aaf71060 100644 --- a/autoconnect/Cargo.toml +++ b/autoconnect/Cargo.toml @@ -48,6 +48,8 @@ autoconnect_web.workspace = true autoconnect_ws.workspace = true autopush_common.workspace = true +actix-server = "2.3" +actix-service = "2.0" docopt = "1.1" [features] diff --git a/autoconnect/autoconnect-web/src/dockerflow.rs b/autoconnect/autoconnect-web/src/dockerflow.rs index 050e95ffb..54684a1f0 100644 --- a/autoconnect/autoconnect-web/src/dockerflow.rs +++ b/autoconnect/autoconnect-web/src/dockerflow.rs @@ -2,7 +2,7 @@ use std::thread; use actix_web::{ - web::{Data, Json}, + web::{self, Data, Json}, HttpResponse, ResponseError, }; use serde_json::json; @@ -11,6 +11,20 @@ use autoconnect_settings::AppState; use crate::error::ApiError; +/// Configure the Dockerflow (and legacy monitoring) routes +pub fn config(config: &mut web::ServiceConfig) { + config + .service(web::resource("/status").route(web::get().to(status_route))) + .service(web::resource("/health").route(web::get().to(health_route))) + .service(web::resource("/v1/err/crit").route(web::get().to(log_check))) + // standardized + .service(web::resource("/__error__").route(web::get().to(log_check))) + // Dockerflow + .service(web::resource("/__heartbeat__").route(web::get().to(health_route))) + .service(web::resource("/__lbheartbeat__").route(web::get().to(lb_heartbeat_route))) + .service(web::resource("/__version__").route(web::get().to(version_route))); +} + /// Handle the `/health` and `/__heartbeat__` routes pub async fn health_route(state: Data) -> Json { let status = if state.db.health_check().await.is_ok() { diff --git a/autoconnect/autoconnect-web/src/lib.rs b/autoconnect/autoconnect-web/src/lib.rs index 6deb71824..9cc2be767 100644 --- a/autoconnect/autoconnect-web/src/lib.rs +++ b/autoconnect/autoconnect-web/src/lib.rs @@ -16,7 +16,7 @@ use actix_web::web; #[macro_export] macro_rules! build_app { - ($app_state: expr) => { + ($app_state: expr, $config: expr) => { actix_web::App::new() .app_data(actix_web::web::Data::new($app_state.clone())) .wrap(actix_web::middleware::ErrorHandlers::new().handler( @@ -28,25 +28,21 @@ macro_rules! build_app { >::new( $app_state.metrics.clone(), "error".to_owned() )) - .configure($crate::config) + .configure($config) }; } +/// The publicly exposed app config pub fn config(cfg: &mut web::ServiceConfig) { cfg // Websocket Handler .route("/", web::get().to(routes::ws_route)) - .service(web::resource("/push/{uaid}").route(web::put().to(routes::push_route))) + .service(web::scope("").configure(dockerflow::config)); +} + +/// The internal router app config +pub fn config_router(cfg: &mut web::ServiceConfig) { + cfg.service(web::resource("/push/{uaid}").route(web::put().to(routes::push_route))) .service(web::resource("/notif/{uaid}").route(web::put().to(routes::check_storage_route))) - .service(web::resource("/status").route(web::get().to(dockerflow::status_route))) - .service(web::resource("/health").route(web::get().to(dockerflow::health_route))) - .service(web::resource("/v1/err/crit").route(web::get().to(dockerflow::log_check))) - // standardized - .service(web::resource("/__error__").route(web::get().to(dockerflow::log_check))) - // Dockerflow - .service(web::resource("/__heartbeat__").route(web::get().to(dockerflow::health_route))) - .service( - web::resource("/__lbheartbeat__").route(web::get().to(dockerflow::lb_heartbeat_route)), - ) - .service(web::resource("/__version__").route(web::get().to(dockerflow::version_route))); + .service(web::scope("").configure(dockerflow::config)); } diff --git a/autoconnect/autoconnect-web/src/routes.rs b/autoconnect/autoconnect-web/src/routes.rs index 41551046c..fbb59ef05 100644 --- a/autoconnect/autoconnect-web/src/routes.rs +++ b/autoconnect/autoconnect-web/src/routes.rs @@ -19,14 +19,14 @@ pub async fn ws_route( pub async fn push_route( uaid: web::Path, notif: web::Json, - state: web::Data, + app_state: web::Data, ) -> HttpResponse { trace!( "⏩ push_route, uaid: {} channel_id: {}", uaid, notif.channel_id ); - let result = state + let result = app_state .clients .notify(uaid.into_inner(), notif.into_inner()) .await; @@ -40,10 +40,10 @@ pub async fn push_route( /// Notify a connected client to check storage for new notifications pub async fn check_storage_route( uaid: web::Path, - state: web::Data, + app_state: web::Data, ) -> HttpResponse { trace!("⏩ check_storage_route, uaid: {}", uaid); - let result = state.clients.check_storage(uaid.into_inner()).await; + let result = app_state.clients.check_storage(uaid.into_inner()).await; if result.is_ok() { HttpResponse::Ok().finish() } else { diff --git a/autoconnect/autoconnect-web/src/test.rs b/autoconnect/autoconnect-web/src/test.rs index a46872a14..a23bdf1c3 100644 --- a/autoconnect/autoconnect-web/src/test.rs +++ b/autoconnect/autoconnect-web/src/test.rs @@ -10,7 +10,7 @@ use autoconnect_common::test_support::{hello_again_db, hello_db, DUMMY_UAID, HEL use autoconnect_settings::{AppState, Settings}; use autopush_common::notification::Notification; -use crate::build_app; +use crate::{build_app, config}; #[ctor::ctor] fn init_test_logging() { @@ -18,7 +18,7 @@ fn init_test_logging() { } fn test_server(app_state: AppState) -> TestServer { - actix_test::start(move || build_app!(app_state)) + actix_test::start(move || build_app!(app_state, config)) } /// Extract the next message from the pending message queue and attempt to diff --git a/autoconnect/autoconnect-ws/autoconnect-ws-sm/src/error.rs b/autoconnect/autoconnect-ws/autoconnect-ws-sm/src/error.rs index 77989a2f0..5caa86fea 100644 --- a/autoconnect/autoconnect-ws/autoconnect-ws-sm/src/error.rs +++ b/autoconnect/autoconnect-ws/autoconnect-ws-sm/src/error.rs @@ -91,9 +91,6 @@ pub enum SMErrorKind { #[error("UAID dropped")] UaidReset, - #[error("Already connected to another node")] - AlreadyConnected, - #[error("New Client with the same UAID has connected to this node")] Ghost, diff --git a/autoconnect/autoconnect-ws/src/ping.rs b/autoconnect/autoconnect-ws/src/ping.rs index 930482589..7cab0e79d 100644 --- a/autoconnect/autoconnect-ws/src/ping.rs +++ b/autoconnect/autoconnect-ws/src/ping.rs @@ -95,7 +95,7 @@ impl PingManager { self.waiting, Waiting::ToPing ); - if matches!(self.waiting, Waiting::ForPong) { + if let Waiting::ForPong = self.waiting { self.set_waiting(Waiting::ToPing, settings).await; } } diff --git a/autoconnect/src/main.rs b/autoconnect/src/main.rs index ca3249ee6..07e6e563a 100644 --- a/autoconnect/src/main.rs +++ b/autoconnect/src/main.rs @@ -6,19 +6,22 @@ extern crate slog_scope; use std::{env, vec::Vec}; -use actix_web::HttpServer; +use actix_http::HttpService; +use actix_server::Server; +use actix_service::map_config; +use actix_web::dev::AppConfig; use docopt::Docopt; use serde::Deserialize; use autoconnect_settings::{AppState, Settings}; -use autoconnect_web::build_app; +use autoconnect_web::{build_app, config, config_router}; use autopush_common::{ errors::{ApcErrorKind, Result}, logging, }; const USAGE: &str = " -Usage: autopush_rs [options] +Usage: autoconnect [options] Options: -h, --help Show this message. @@ -79,9 +82,24 @@ async fn main() -> Result<()> { "Starting autoconnect on port {} (router_port: {})", port, router_port ); - HttpServer::new(move || build_app!(app_state)) - .bind(("0.0.0.0", port))? - .bind(("0.0.0.0", router_port))? + + let router_app_state = app_state.clone(); + Server::build() + .bind("autoconnect", ("0.0.0.0", port), move || { + let app = build_app!(app_state, config); + HttpService::build() + // XXX: AppConfig::default() does *not* have correct values + // https://github.com/actix/actix-web/issues/3180 + .finish(map_config(app, |_| AppConfig::default())) + .tcp() + })? + .bind("autoconnect-router", ("0.0.0.0", router_port), move || { + let app = build_app!(router_app_state, config_router); + HttpService::build() + // XXX: + .finish(map_config(app, |_| AppConfig::default())) + .tcp() + })? .run() .await .map_err(|e| e.into()) diff --git a/tests/test_integration_all_rust.py b/tests/test_integration_all_rust.py index f6079730d..a0779647a 100644 --- a/tests/test_integration_all_rust.py +++ b/tests/test_integration_all_rust.py @@ -1548,6 +1548,28 @@ def test_can_ping(self): assert not client.ws.connected yield self.shut_down(client) + @inlineCallbacks + def test_internal_endpoints(self): + """Ensure an internal router endpoint isn't exposed on the public CONNECTION_PORT""" + client = yield self.quick_register() + parsed = urlparse(self._ws_url)._replace(scheme="http")._replace(path=f"/notif/{client.uaid}") + + # We can't determine an AUTOPUSH_CN_SERVER's ROUTER_PORT + if not os.getenv("AUTOPUSH_CN_SERVER"): + url = parsed._replace(netloc=f"{parsed.hostname}:{ROUTER_PORT}").geturl() + # First ensure the endpoint we're testing for on the public port exists where + # we expect it on the internal ROUTER_PORT + requests.put(url).raise_for_status() + + try: + requests.put(parsed.geturl()).raise_for_status() + except requests.exceptions.ConnectionError: + pass + except requests.exceptions.HTTPError as e: + assert e.response.status_code == 404 + else: + assert False + class TestRustWebPushBroadcast(unittest.TestCase): max_endpoint_logs = 4