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.

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

Allow RootContext to create child contexts for streams. #34

Merged
merged 9 commits into from
Dec 4, 2020
Merged

Allow RootContext to create child contexts for streams. #34

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
28ba351
Move context creation to fn of RootContext instead of callback
dgn Aug 21, 2020
94ecd03
Make rustfmt happy
dgn Aug 21, 2020
1cad709
Addressing review comments
dgn Nov 16, 2020
6488b8a
React to review comments
dgn Nov 18, 2020
79e8621
Add examples/http_config.rs
dgn Nov 18, 2020
5f0302c
Add backwards compatibility, use Option<> where appropriate
dgn Nov 20, 2020
9756371
rustfmt
dgn Nov 20, 2020
46cf563
Revert unnecessary change in http_auth_random.rs, use String
dgn Nov 20, 2020
a4658e8
Merge branch 'master' into master
PiotrSikora Dec 4, 2020
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
51 changes: 32 additions & 19 deletions examples/http_auth_random.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,31 +20,21 @@ use std::time::Duration;
#[no_mangle]
pub fn _start() {
proxy_wasm::set_log_level(LogLevel::Trace);
proxy_wasm::set_http_context(|_, _| -> Box<dyn HttpContext> { Box::new(HttpAuthRandom) });
proxy_wasm::set_root_context(|_| -> Box<dyn RootContext> { Box::new(HttpAuthRandomRoot) });
Copy link
Member

Choose a reason for hiding this comment

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

Looking at this example, I wonder if we couldn't do something type-specific here, e.g.

proxy_wasm::set_root_context(|_| -> Box<dyn RootContext> { Box::new(HttpHeadersRoot) });
proxy_wasm::set_http_context(|root_context, context_id| -> Box<dyn HttpContext> {
    root_context.new_http_context(context_id)
});

with dispatcher taking care of resolving root_context_id to root_context before calling this callback?

Copy link
Member

Choose a reason for hiding this comment

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

On the second thought, that might be useless, since we cannot expose multiple interfaces right now anyway.

}

struct HttpAuthRandom;
PiotrSikora marked this conversation as resolved.
Show resolved Hide resolved
struct HttpAuthRandomRoot;

impl HttpContext for HttpAuthRandom {
fn on_http_request_headers(&mut self, _: usize) -> Action {
self.dispatch_http_call(
"httpbin",
vec![
(":method", "GET"),
(":path", "/bytes/1"),
(":authority", "httpbin.org"),
],
None,
vec![],
Duration::from_secs(5),
)
.unwrap();
Action::Pause
impl Context for HttpAuthRandomRoot {}

impl RootContext for HttpAuthRandomRoot {
fn get_type(&self) -> ContextType {
ContextType::HttpContext
}

fn on_http_response_headers(&mut self, _: usize) -> Action {
self.set_http_response_header("Powered-By", Some("proxy-wasm"));
Action::Continue
fn create_http_context(&self, _context_id: u32) -> Box<dyn HttpContext> {
Box::new(HttpAuthRandom)
}
}

Expand All @@ -65,3 +55,26 @@ impl Context for HttpAuthRandom {
);
}
}

PiotrSikora marked this conversation as resolved.
Show resolved Hide resolved
impl HttpContext for HttpAuthRandom {
fn on_http_request_headers(&mut self, _: usize) -> Action {
self.dispatch_http_call(
"httpbin",
vec![
(":method", "GET"),
(":path", "/bytes/1"),
(":authority", "httpbin.org"),
],
None,
vec![],
Duration::from_secs(5),
)
.unwrap();
Action::Pause
}

fn on_http_response_headers(&mut self, _: usize) -> Action {
self.set_http_response_header("Powered-By", Some("proxy-wasm"));
Action::Continue
}
}
15 changes: 14 additions & 1 deletion examples/http_body.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,23 @@ use proxy_wasm::types::*;
#[no_mangle]
pub fn _start() {
proxy_wasm::set_log_level(LogLevel::Trace);
proxy_wasm::set_http_context(|_, _| -> Box<dyn HttpContext> { Box::new(HttpBody) });
proxy_wasm::set_root_context(|_| -> Box<dyn RootContext> { Box::new(HttpBodyRoot) });
}

struct HttpBody;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: move struct HttpBody; after definition of HttpBodyRoot.

struct HttpBodyRoot;

impl Context for HttpBodyRoot {}

impl RootContext for HttpBodyRoot {
fn get_type(&self) -> ContextType {
ContextType::HttpContext
}

fn create_http_context(&self, _context_id: u32) -> Box<dyn HttpContext> {
Box::new(HttpBody)
}
}

impl Context for HttpBody {}

Expand Down
20 changes: 17 additions & 3 deletions examples/http_headers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,23 @@ use proxy_wasm::types::*;
#[no_mangle]
pub fn _start() {
proxy_wasm::set_log_level(LogLevel::Trace);
proxy_wasm::set_http_context(|context_id, _| -> Box<dyn HttpContext> {
Box::new(HttpHeaders { context_id })
});
proxy_wasm::set_root_context(|_| -> Box<dyn RootContext> { Box::new(HttpHeadersRoot) });
}

struct HttpHeadersRoot;

impl Context for HttpHeadersRoot {}

impl RootContext for HttpHeadersRoot {
fn get_type(&self) -> ContextType {
ContextType::HttpContext
}

fn create_http_context(&self, _context_id: u32) -> Box<dyn HttpContext> {
Box::new(HttpHeaders {
context_id: _context_id,
})
}
}

struct HttpHeaders {
Expand Down
72 changes: 25 additions & 47 deletions src/dispatcher.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,6 @@ pub(crate) fn set_root_context(callback: NewRootContext) {
DISPATCHER.with(|dispatcher| dispatcher.set_root_context(callback));
}

pub(crate) fn set_stream_context(callback: NewStreamContext) {
DISPATCHER.with(|dispatcher| dispatcher.set_stream_context(callback));
}

pub(crate) fn set_http_context(callback: NewHttpContext) {
DISPATCHER.with(|dispatcher| dispatcher.set_http_context(callback));
}

pub(crate) fn register_callout(token_id: u32) {
DISPATCHER.with(|dispatcher| dispatcher.register_callout(token_id));
}
Expand All @@ -46,9 +38,7 @@ impl RootContext for NoopRoot {}
struct Dispatcher {
new_root: Cell<Option<NewRootContext>>,
roots: RefCell<HashMap<u32, Box<dyn RootContext>>>,
new_stream: Cell<Option<NewStreamContext>>,
streams: RefCell<HashMap<u32, Box<dyn StreamContext>>>,
new_http_stream: Cell<Option<NewHttpContext>>,
http_streams: RefCell<HashMap<u32, Box<dyn HttpContext>>>,
active_id: Cell<u32>,
callouts: RefCell<HashMap<u32, u32>>,
Expand All @@ -59,9 +49,7 @@ impl Dispatcher {
Dispatcher {
new_root: Cell::new(None),
roots: RefCell::new(HashMap::new()),
new_stream: Cell::new(None),
streams: RefCell::new(HashMap::new()),
new_http_stream: Cell::new(None),
http_streams: RefCell::new(HashMap::new()),
active_id: Cell::new(0),
callouts: RefCell::new(HashMap::new()),
Expand All @@ -72,12 +60,15 @@ impl Dispatcher {
self.new_root.set(Some(callback));
}

fn set_stream_context(&self, callback: NewStreamContext) {
self.new_stream.set(Some(callback));
}

fn set_http_context(&self, callback: NewHttpContext) {
self.new_http_stream.set(Some(callback));
fn register_callout(&self, token_id: u32) {
if self
.callouts
.borrow_mut()
.insert(token_id, self.active_id.get())
.is_some()
{
panic!("duplicate token_id")
}
}

fn create_root_context(&self, context_id: u32) {
Expand All @@ -96,12 +87,9 @@ impl Dispatcher {
}

fn create_stream_context(&self, context_id: u32, root_context_id: u32) {
if !self.roots.borrow().contains_key(&root_context_id) {
panic!("invalid root_context_id")
}
let new_context = match self.new_stream.get() {
Some(f) => f(context_id, root_context_id),
None => panic!("missing constructor"),
let new_context = match self.roots.borrow().get(&root_context_id) {
Some(root_context) => root_context.create_stream_context(context_id),
None => panic!("invalid root_context_id"),
};
if self
.streams
Expand All @@ -114,12 +102,9 @@ impl Dispatcher {
}

fn create_http_context(&self, context_id: u32, root_context_id: u32) {
if !self.roots.borrow().contains_key(&root_context_id) {
panic!("invalid root_context_id")
}
let new_context = match self.new_http_stream.get() {
Some(f) => f(context_id, root_context_id),
None => panic!("missing constructor"),
let new_context = match self.roots.borrow().get(&root_context_id) {
Some(root_context) => root_context.create_http_context(context_id),
None => panic!("invalid root_context_id"),
};
if self
.http_streams
Expand All @@ -131,26 +116,19 @@ impl Dispatcher {
}
}

fn register_callout(&self, token_id: u32) {
if self
.callouts
.borrow_mut()
.insert(token_id, self.active_id.get())
.is_some()
{
panic!("duplicate token_id")
}
}

fn on_create_context(&self, context_id: u32, root_context_id: u32) {
if root_context_id == 0 {
self.create_root_context(context_id)
} else if self.new_http_stream.get().is_some() {
self.create_http_context(context_id, root_context_id);
} else if self.new_stream.get().is_some() {
self.create_stream_context(context_id, root_context_id);
self.create_root_context(context_id);
} else if let Some(root_context) = self.roots.borrow().get(&root_context_id) {
match root_context.get_type() {
ContextType::HttpContext => self.create_http_context(context_id, root_context_id),
ContextType::StreamContext => {
self.create_stream_context(context_id, root_context_id)
}
PiotrSikora marked this conversation as resolved.
Show resolved Hide resolved
ContextType::RootContext => panic!("missing ContextType on root_context"),
}
Copy link
Member

Choose a reason for hiding this comment

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

One more thing. I think it would be useful if we could retain backwards compatibility, and merge this without breaking all existing plugins, even if there aren't too many in the wild right now.

One way to do this is to fallback to the standalone constructors in case root_context.get_type() returns ContextType::RootContext or None, and perhaps revert changes to examples/http_auth_random.rs to make sure that the old code path still works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1 to remain backwards-compatible, but I think we should then deprecate the old way to create contexts and remove it in 0.2.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that would also allow us to remove NoopRoot in 0.2.0

Copy link
Member

Choose a reason for hiding this comment

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

The "old way" is fine for plugins that don't have configuration (e.g. where everything is already in the code), and the "new way" can double the code size with unnecessary boilerplate.

But it's something to consider for sure.

} else {
panic!("missing constructors")
panic!("invalid root_context_id");
}
}

Expand Down
8 changes: 0 additions & 8 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,5 @@ pub fn set_root_context(callback: types::NewRootContext) {
dispatcher::set_root_context(callback);
}

pub fn set_stream_context(callback: types::NewStreamContext) {
dispatcher::set_stream_context(callback);
}

pub fn set_http_context(callback: types::NewHttpContext) {
dispatcher::set_http_context(callback);
}

#[no_mangle]
pub extern "C" fn proxy_abi_version_0_1_0() {}
22 changes: 22 additions & 0 deletions src/traits.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,18 @@ pub trait RootContext: Context {
fn on_queue_ready(&mut self, _queue_id: u32) {}

fn on_log(&mut self) {}

fn create_http_context(&self, _context_id: u32) -> Box<dyn HttpContext> {
Box::new(EmptyHttpContext)
Copy link
Member

Choose a reason for hiding this comment

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

Box<dyn HttpContext> -> Option<Box<dyn HttpContext>> and return None.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}

fn create_stream_context(&self, _context_id: u32) -> Box<dyn StreamContext> {
Box::new(EmptyStreamContext)
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}

fn get_type(&self) -> ContextType {
ContextType::RootContext
Copy link
Member

Choose a reason for hiding this comment

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

Option<ContextType> and return None.

}
PiotrSikora marked this conversation as resolved.
Show resolved Hide resolved
}

pub trait StreamContext: Context {
Expand Down Expand Up @@ -159,6 +171,11 @@ pub trait StreamContext: Context {
fn on_log(&mut self) {}
}

struct EmptyStreamContext;

impl StreamContext for EmptyStreamContext {}
impl Context for EmptyStreamContext {}
Copy link
Member

Choose a reason for hiding this comment

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

This won't be needed with Option<T>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


pub trait HttpContext: Context {
fn on_http_request_headers(&mut self, _num_headers: usize) -> Action {
Action::Continue
Expand Down Expand Up @@ -303,3 +320,8 @@ pub trait HttpContext: Context {

fn on_log(&mut self) {}
}

struct EmptyHttpContext;

impl HttpContext for EmptyHttpContext {}
impl Context for EmptyHttpContext {}
Copy link
Member

Choose a reason for hiding this comment

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

This won't be needed with Option<T>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

6 changes: 6 additions & 0 deletions src/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,3 +85,9 @@ pub enum MetricType {
}

pub type Bytes = Vec<u8>;

pub enum ContextType {
Copy link
Member

Choose a reason for hiding this comment

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

Please add:

#[repr(u32)]
#[derive(Debug)]

(it's not used in the ABI right now, but it's going to be soon, so we might as well define it properly now)

Also, could you move it below Status?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

RootContext = 0,
HttpContext = 1,
StreamContext = 2,
}