Skip to content

Commit

Permalink
fix: split router endpoints into their own app (#491)
Browse files Browse the repository at this point in the history
* fix: split router endpoints into their own app

which binds solely to the router port

SYNC-3979
  • Loading branch information
pjenvey authored Oct 30, 2023
1 parent ea30222 commit bbde582
Show file tree
Hide file tree
Showing 10 changed files with 82 additions and 31 deletions.
2 changes: 2 additions & 0 deletions Cargo.lock

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

2 changes: 2 additions & 0 deletions autoconnect/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
16 changes: 15 additions & 1 deletion autoconnect/autoconnect-web/src/dockerflow.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
use std::thread;

use actix_web::{
web::{Data, Json},
web::{self, Data, Json},
HttpResponse, ResponseError,
};
use serde_json::json;
Expand All @@ -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<AppState>) -> Json<serde_json::Value> {
let status = if state.db.health_check().await.is_ok() {
Expand Down
24 changes: 10 additions & 14 deletions autoconnect/autoconnect-web/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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));
}
8 changes: 4 additions & 4 deletions autoconnect/autoconnect-web/src/routes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,14 @@ pub async fn ws_route(
pub async fn push_route(
uaid: web::Path<Uuid>,
notif: web::Json<Notification>,
state: web::Data<AppState>,
app_state: web::Data<AppState>,
) -> 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;
Expand All @@ -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<Uuid>,
state: web::Data<AppState>,
app_state: web::Data<AppState>,
) -> 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 {
Expand Down
4 changes: 2 additions & 2 deletions autoconnect/autoconnect-web/src/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,15 @@ 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() {
autopush_common::logging::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
Expand Down
3 changes: 0 additions & 3 deletions autoconnect/autoconnect-ws/autoconnect-ws-sm/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,

Expand Down
2 changes: 1 addition & 1 deletion autoconnect/autoconnect-ws/src/ping.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
Expand Down
30 changes: 24 additions & 6 deletions autoconnect/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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())
Expand Down
22 changes: 22 additions & 0 deletions tests/test_integration_all_rust.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit bbde582

Please sign in to comment.