-
Notifications
You must be signed in to change notification settings - Fork 107
Allow RootContext to create child contexts for streams. #34
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.
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
Changes from all commits
28ba351
94ecd03
1cad709
6488b8a
79e8621
5f0302c
9756371
46cf563
a4658e8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
// Copyright 2020 Google LLC | ||
// | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// You may obtain a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
|
||
use proxy_wasm::traits::*; | ||
use proxy_wasm::types::*; | ||
|
||
#[no_mangle] | ||
pub fn _start() { | ||
proxy_wasm::set_log_level(LogLevel::Trace); | ||
proxy_wasm::set_root_context(|_| -> Box<dyn RootContext> { | ||
Box::new(HttpConfigHeaderRoot { | ||
header_content: String::new(), | ||
}) | ||
}); | ||
} | ||
|
||
struct HttpConfigHeader { | ||
header_content: String, | ||
} | ||
|
||
impl Context for HttpConfigHeader {} | ||
|
||
impl HttpContext for HttpConfigHeader { | ||
fn on_http_response_headers(&mut self, _num_headers: usize) -> Action { | ||
self.add_http_response_header("custom-header", self.header_content.as_str()); | ||
Action::Continue | ||
} | ||
} | ||
|
||
struct HttpConfigHeaderRoot { | ||
header_content: String, | ||
} | ||
|
||
impl Context for HttpConfigHeaderRoot {} | ||
|
||
impl RootContext for HttpConfigHeaderRoot { | ||
fn on_configure(&mut self, _plugin_configuration_size: usize) -> bool { | ||
if let Some(config_bytes) = self.get_configuration() { | ||
self.header_content = String::from_utf8(config_bytes).unwrap() | ||
} | ||
true | ||
} | ||
|
||
fn create_http_context(&self, _context_id: u32) -> Option<Box<dyn HttpContext>> { | ||
Some(Box::new(HttpConfigHeader { | ||
header_content: self.header_content.clone(), | ||
})) | ||
} | ||
|
||
fn get_type(&self) -> Option<ContextType> { | ||
Some(ContextType::HttpContext) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -80,6 +80,17 @@ impl Dispatcher { | |
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) { | ||
let new_context = match self.new_root.get() { | ||
Some(f) => f(context_id), | ||
|
@@ -96,12 +107,15 @@ 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) => match self.new_stream.get() { | ||
Some(f) => f(context_id, root_context_id), | ||
None => match root_context.create_stream_context(context_id) { | ||
Some(stream_context) => stream_context, | ||
None => panic!("create_stream_context returned None"), | ||
}, | ||
}, | ||
None => panic!("invalid root_context_id"), | ||
}; | ||
if self | ||
.streams | ||
|
@@ -114,12 +128,15 @@ 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) => match self.new_http_stream.get() { | ||
Some(f) => f(context_id, root_context_id), | ||
None => match root_context.create_http_context(context_id) { | ||
Some(stream_context) => stream_context, | ||
None => panic!("create_http_context returned None"), | ||
}, | ||
}, | ||
None => panic!("invalid root_context_id"), | ||
}; | ||
if self | ||
.http_streams | ||
|
@@ -131,26 +148,25 @@ 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) | ||
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); | ||
} else if let Some(root_context) = self.roots.borrow().get(&root_context_id) { | ||
match root_context.get_type() { | ||
Some(ContextType::HttpContext) => { | ||
self.create_http_context(context_id, root_context_id) | ||
} | ||
Some(ContextType::StreamContext) => { | ||
self.create_stream_context(context_id, root_context_id) | ||
} | ||
None => panic!("missing ContextType on root_context"), | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that would also allow us to remove There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 and missing constructors"); | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,6 +47,13 @@ pub enum Status { | |
InternalFailure = 10, | ||
} | ||
|
||
#[repr(u32)] | ||
#[derive(Debug)] | ||
pub enum ContextType { | ||
HttpContext = 0, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you re-add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why? How would that be different from returning There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I meant only adding enum value here, not using it in this PR, since the |
||
StreamContext = 1, | ||
} | ||
|
||
#[repr(u32)] | ||
#[derive(Debug)] | ||
pub enum BufferType { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems expensive, since presumably each context will own a copy of the header value.
It's fine in this example, since the value is small, but in general, wouldn't it be better to store a reference to the parent context and access configuration from there?