Skip to content
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
871ee1b
extract http call functions into a new class, LuaFilterLibrary
mindyor Feb 23, 2020
df9df09
create fire and forget listener; lua function httpCallAsync
mindyor Feb 23, 2020
541bc4b
group class functions together
mindyor Feb 23, 2020
8b5b84d
add documentation for httpCallAsync
mindyor Feb 24, 2020
323d728
add release note to version history
mindyor Feb 24, 2020
e892eaa
formatting
mindyor Feb 25, 2020
29464ba
naming. s/LuaFilterLibrary/LuaFilterUtil/
mindyor Feb 25, 2020
b566eab
formatting: lengthen underline
mindyor Feb 25, 2020
4cb0ecc
move instantiation of fire and forget writer into stream handle wrapp…
mindyor Feb 25, 2020
27a0f3c
makeHttpCall static function
mindyor Feb 25, 2020
11c1301
whitespace
mindyor Feb 25, 2020
21aa51e
alphabetic order
mindyor Feb 25, 2020
bf9998d
delete fire and forget writer in destructor
mindyor Feb 26, 2020
3e0f6c4
correct docs: httpCallAsync return
mindyor Feb 26, 2020
38a8a99
rename functions Nonblocking as implementation does not use callbacks
mindyor Feb 27, 2020
67aee6e
rename FireAndForgetWriter to FireAndForgetHttpWriter
mindyor Feb 27, 2020
e5bee45
comment nits
mindyor Feb 27, 2020
ac562dd
comment clarity for fire and forget http wrier
mindyor Feb 27, 2020
aa04eb8
remove superfluous else
mindyor Feb 28, 2020
d838492
formatting
mindyor Feb 28, 2020
7b234f3
put helper methods in anonymous namespace
mindyor Feb 28, 2020
f4cd746
rename callbacksListener to callbacks
mindyor Feb 28, 2020
d6e452b
use simple noop callbacks class rather than fire and forget writer
mindyor Feb 28, 2020
d022924
httpCall take flag to indicate asynchronous. Remove httpCallNonblocking
mindyor Mar 3, 2020
5ad50d1
test covering async = false case
mindyor Mar 3, 2020
55d1682
remove lua declaration of functions; update naming and comments to us…
mindyor Mar 3, 2020
1ebaac1
Merge branch 'master' into async-lua-http-call
mindyor Mar 3, 2020
f279ad2
use static noopcallback - thanks snowp
mindyor Mar 3, 2020
1594207
remove unused lines in test
mindyor Mar 3, 2020
9945ac5
integration test assert on header values - thanks snowp
mindyor Mar 3, 2020
1d4ba85
comment tweak
mindyor Mar 3, 2020
6dcdcf5
pr comments
mindyor Mar 4, 2020
8459416
type check for async flag
mindyor Mar 4, 2020
d190e36
formatting
mindyor Mar 4, 2020
9a1dbd7
review feedback nits
mindyor Mar 5, 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
17 changes: 17 additions & 0 deletions docs/root/configuration/http/http_filters/lua_filter.rst
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,23 @@ data to send. *timeout* is an integer that specifies the call timeout in millise
Returns *headers* which is a table of response headers. Returns *body* which is the string response
body. May be nil if there is no body.

httpCallNonblocking()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i'm not a huge fan of this name (it sounds a bit clunky/verbose) and it worries me a bit if we are going to end up with one new function per variation of a HTTP call (httpCallNonblockingWithCallbacks kinda situation)

are we able to/can we add parameters to existing LUA filters? it'd be nice to either have one or two (e.g. httpCall and asyncHttpCall) functions with options for whether the response should be dropped, callbacks to invoke on response etc.

@mattklein123 @jmarantz @dio

@dio dio Feb 28, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are we able to/can we add parameters to existing LUA filters?

I think we can, yeah. This is a reasonable/great ask, yeah we should do this.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes I think we can do this and SGTM

@mindyor mindyor Mar 3, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Makes sense.

Removed the new function, adding a parameter asynchronous. Hope that looks cleaner!

^^^^^^^^^^^^^^^^^^^^^

.. code-block:: lua

handle:httpCallNonblocking(cluster, headers, body, timeout)

Makes an HTTP call to an upstream host. Same behavior as httpCall, except that Envoy will fire and forget.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would suggest spelling out what fire and forget means here wrt continuation of the filter chain


*cluster* is a string which maps to a configured cluster manager cluster. *headers*
is a table of key/value pairs to send (the value can be a string or table of strings). Note that
the *:method*, *:path*, and *:authority* headers must be set. *body* is an optional string of body
data to send. *timeout* is an integer that specifies the call timeout in milliseconds.

Returns *headers* which is a table of response headers. Returns *body* which is the string response
body. May be nil if there is no body.

respond()
^^^^^^^^^^

Expand Down
1 change: 1 addition & 0 deletions docs/root/intro/version_history.rst
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ Version history
minimum.
* config: use type URL to select an extension whenever the config type URL (or its previous versions) uniquely identify a typed extension, see :ref:`extension configuration <config_overview_extension_configuration>`.
* http: fixing a bug in HTTP/1.0 responses where Connection: keep-alive was not appended for connections which were kept alive.
* lua: added `httpCallNonblocking` function. Same as `httpCall` but fire-and-forget.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this needs to be updated

* network filters: network filter extensions use the "envoy.filters.network" name space. A mapping
of extension names is available in the :ref:`deprecated <deprecated>` documentation.
* rbac: added :ref:`url_path <envoy_api_field_config.rbac.v2.Permission.url_path>` for matching URL path without the query and fragment string.
Expand Down
150 changes: 85 additions & 65 deletions source/extensions/filters/http/lua/lua_filter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,10 @@ StreamHandleWrapper::StreamHandleWrapper(Filters::Common::Lua::Coroutine& corout
if (state_ == State::Running) {
throw Filters::Common::Lua::LuaException("script performed an unexpected yield");
}
}) {}
}),
fireAndForgetHttpWriter_(new FireAndForgetHttpWriter(filter)) {}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

no reason to heap allocate this, just use it as a non-pointer value

FireAndForgetHttpWriter fire_and_forget_http_writer_;

and just fire_and_forget_http_writer_(filter) in the constructor

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This actually does need to be heap allocated because the success/failure callbacks can live beyond the filter, but not in the way it is done here. The Lua filter config needs to have a fire and forget writer that works identically to the router's shadow writer. Take a look at the pattern that is used there.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(Or yes per other comment just pass static callbacks and that should be fine also).


StreamHandleWrapper::~StreamHandleWrapper() { delete fireAndForgetHttpWriter_; }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if you are going to be doing heap allocations please use std::unique_ptr/std::shared_ptr to manage the memory


Http::FilterHeadersStatus StreamHandleWrapper::start(int function_ref) {
// We are on the top of the stack.
Expand Down Expand Up @@ -116,7 +119,7 @@ int StreamHandleWrapper::luaRespond(lua_State* state) {
size_t body_size;
const char* raw_body = luaL_optlstring(state, 3, nullptr, &body_size);
auto headers = std::make_unique<Http::ResponseHeaderMapImpl>();
buildHeadersFromTable(*headers, state, 2);
LuaFilterUtil::buildHeadersFromTable(*headers, state, 2);

uint64_t status;
if (headers->Status() == nullptr ||
Expand All @@ -138,71 +141,9 @@ int StreamHandleWrapper::luaRespond(lua_State* state) {
return lua_yield(state, 0);
}

void StreamHandleWrapper::buildHeadersFromTable(Http::HeaderMap& headers, lua_State* state,
int table_index) {
// Build a header map to make the request. We iterate through the provided table to do this and
// check that we are getting strings.
lua_pushnil(state);
while (lua_next(state, table_index) != 0) {
// Uses 'key' (at index -2) and 'value' (at index -1).
const char* key = luaL_checkstring(state, -2);
// Check if the current value is a table, we iterate through the table and add each element of
// it as a header entry value for the current key.
if (lua_istable(state, -1)) {
lua_pushnil(state);
while (lua_next(state, -2) != 0) {
const char* value = luaL_checkstring(state, -1);
headers.addCopy(Http::LowerCaseString(key), value);
lua_pop(state, 1);
}
} else {
const char* value = luaL_checkstring(state, -1);
headers.addCopy(Http::LowerCaseString(key), value);
}
// Removes 'value'; keeps 'key' for next iteration. This is the input for lua_next() so that
// it can push the next key/value pair onto the stack.
lua_pop(state, 1);
}
}

int StreamHandleWrapper::luaHttpCall(lua_State* state) {
ASSERT(state_ == State::Running);

const std::string cluster = luaL_checkstring(state, 2);
luaL_checktype(state, 3, LUA_TTABLE);
size_t body_size;
const char* body = luaL_optlstring(state, 4, nullptr, &body_size);
int timeout_ms = luaL_checkint(state, 5);
if (timeout_ms < 0) {
return luaL_error(state, "http call timeout must be >= 0");
}

if (filter_.clusterManager().get(cluster) == nullptr) {
return luaL_error(state, "http call cluster invalid. Must be configured");
}

auto headers = std::make_unique<Http::RequestHeaderMapImpl>();
buildHeadersFromTable(*headers, state, 3);
Http::RequestMessagePtr message(new Http::RequestMessageImpl(std::move(headers)));

// Check that we were provided certain headers.
if (message->headers().Path() == nullptr || message->headers().Method() == nullptr ||
message->headers().Host() == nullptr) {
return luaL_error(state, "http call headers must include ':path', ':method', and ':authority'");
}

if (body != nullptr) {
message->body() = std::make_unique<Buffer::OwnedImpl>(body, body_size);
message->headers().setContentLength(body_size);
}

absl::optional<std::chrono::milliseconds> timeout;
if (timeout_ms > 0) {
timeout = std::chrono::milliseconds(timeout_ms);
}

http_request_ = filter_.clusterManager().httpAsyncClientForCluster(cluster).send(
std::move(message), *this, Http::AsyncClient::RequestOptions().setTimeout(timeout));
http_request_ = LuaFilterUtil::makeHttpCall(state, filter_, *this);
if (http_request_) {
state_ = State::HttpCall;
return lua_yield(state, 0);
Expand All @@ -213,6 +154,10 @@ int StreamHandleWrapper::luaHttpCall(lua_State* state) {
}
}

int StreamHandleWrapper::luaHttpCallNonblocking(lua_State* state) {
return fireAndForgetHttpWriter_->luaHttpCallNonblocking(state);
}

void StreamHandleWrapper::onSuccess(Http::ResponseMessagePtr&& response) {
ASSERT(state_ == State::HttpCall || state_ == State::Running);
ENVOY_LOG(debug, "async HTTP response complete");
Expand Down Expand Up @@ -485,6 +430,81 @@ int StreamHandleWrapper::luaImportPublicKey(lua_State* state) {
return 1;
}

FireAndForgetHttpWriter::FireAndForgetHttpWriter(Filter& filter) : filter_(filter) {}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i don't think this class buys us a whole lot, how about just having a NoopCallbacks that we pass to makeHttpCall?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hum, true.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good idea. Pushed.


int FireAndForgetHttpWriter::luaHttpCallNonblocking(lua_State* state) {
LuaFilterUtil::makeHttpCall(state, filter_, *this);
return 0;
}

Http::AsyncClient::Request*
LuaFilterUtil::makeHttpCall(lua_State* state, Filter& filter,
Http::AsyncClient::Callbacks& callbacksListener) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: callbacksListener -> callbacks (or at the very least snake case parameter names)

const std::string cluster = luaL_checkstring(state, 2);
luaL_checktype(state, 3, LUA_TTABLE);
size_t body_size;
const char* body = luaL_optlstring(state, 4, nullptr, &body_size);
int timeout_ms = luaL_checkint(state, 5);
if (timeout_ms < 0) {
luaL_error(state, "http call timeout must be >= 0");
}

if (filter.clusterManager().get(cluster) == nullptr) {
luaL_error(state, "http call cluster invalid. Must be configured");
}

auto headers = std::make_unique<Http::RequestHeaderMapImpl>();
LuaFilterUtil::buildHeadersFromTable(*headers, state, 3);
Http::RequestMessagePtr message(new Http::RequestMessageImpl(std::move(headers)));

// Check that we were provided certain headers.
if (message->headers().Path() == nullptr || message->headers().Method() == nullptr ||
message->headers().Host() == nullptr) {
luaL_error(state, "http call headers must include ':path', ':method', and ':authority'");
}

if (body != nullptr) {
message->body() = std::make_unique<Buffer::OwnedImpl>(body, body_size);
message->headers().setContentLength(body_size);
}

absl::optional<std::chrono::milliseconds> timeout;
if (timeout_ms > 0) {
timeout = std::chrono::milliseconds(timeout_ms);
}

return filter.clusterManager().httpAsyncClientForCluster(cluster).send(
std::move(message), callbacksListener,
Http::AsyncClient::RequestOptions().setTimeout(timeout));
}

void LuaFilterUtil::buildHeadersFromTable(Http::HeaderMap& headers, lua_State* state,
int table_index) {
// Build a header map to make the request. We iterate through the provided table to do this and
// check that we are getting strings.
lua_pushnil(state);
while (lua_next(state, table_index) != 0) {
// Uses 'key' (at index -2) and 'value' (at index -1).
const char* key = luaL_checkstring(state, -2);
// Check if the current value is a table, we iterate through the table and add each element of
// it as a header entry value for the current key.
if (lua_istable(state, -1)) {
lua_pushnil(state);
while (lua_next(state, -2) != 0) {
const char* value = luaL_checkstring(state, -1);
headers.addCopy(Http::LowerCaseString(key), value);
lua_pop(state, 1);
}
} else {
const char* value = luaL_checkstring(state, -1);
headers.addCopy(Http::LowerCaseString(key), value);
}
// Removes 'value'; keeps 'key' for next iteration. This is the input for lua_next() so that
// it can push the next key/value pair onto the stack.
lua_pop(state, 1);
}
}

FilterConfig::FilterConfig(const std::string& lua_code, ThreadLocal::SlotAllocator& tls,
Upstream::ClusterManager& cluster_manager)
: cluster_manager_(cluster_manager), lua_state_(lua_code, tls) {
Expand Down
47 changes: 45 additions & 2 deletions source/extensions/filters/http/lua/lua_filter.h
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,8 @@ class FilterCallbacks {

class Filter;

class FireAndForgetHttpWriter;

/**
* A wrapper for a currently running request/response. This is the primary handle passed to Lua.
* The script interacts with Envoy entirely through this handle.
Expand Down Expand Up @@ -117,6 +119,7 @@ class StreamHandleWrapper : public Filters::Common::Lua::BaseLuaObject<StreamHan

StreamHandleWrapper(Filters::Common::Lua::Coroutine& coroutine, Http::HeaderMap& headers,
bool end_stream, Filter& filter, FilterCallbacks& callbacks);
~StreamHandleWrapper();

Http::FilterHeadersStatus start(int function_ref);
Http::FilterDataStatus onData(Buffer::Instance& data, bool end_stream);
Expand All @@ -142,6 +145,7 @@ class StreamHandleWrapper : public Filters::Common::Lua::BaseLuaObject<StreamHan
{"logErr", static_luaLogErr},
{"logCritical", static_luaLogCritical},
{"httpCall", static_luaHttpCall},
{"httpCallNonblocking", static_luaHttpCallNonblocking},
{"respond", static_luaRespond},
{"streamInfo", static_luaStreamInfo},
{"connection", static_luaConnection},
Expand All @@ -160,6 +164,16 @@ class StreamHandleWrapper : public Filters::Common::Lua::BaseLuaObject<StreamHan
*/
DECLARE_LUA_FUNCTION(StreamHandleWrapper, luaHttpCall);

/**
* Perform a non-blocking HTTP call to an upstream host. Fires and forgets.
* @param 1 (string): The name of the upstream cluster to call. This cluster must be configured.
* @param 2 (table): A table of HTTP headers. :method, :path, and :authority must be defined.
* @param 3 (string): Body. Can be nil.
* @param 4 (int): Timeout in milliseconds for the call.
* @return headers (table), body (string/nil)
*/
DECLARE_LUA_FUNCTION(StreamHandleWrapper, luaHttpCallNonblocking);

/**
* Perform an inline response. This call is currently only valid on the request path. Further
* filter iteration will stop. No further script code will run after this call.
Expand Down Expand Up @@ -247,8 +261,6 @@ class StreamHandleWrapper : public Filters::Common::Lua::BaseLuaObject<StreamHan
*/
DECLARE_LUA_CLOSURE(StreamHandleWrapper, luaBodyIterator);

static void buildHeadersFromTable(Http::HeaderMap& headers, lua_State* state, int table_index);

// Filters::Common::Lua::BaseLuaObject
void onMarkDead() override {
// Headers/body/trailers wrappers do not survive any yields. The user can request them
Expand Down Expand Up @@ -285,6 +297,37 @@ class StreamHandleWrapper : public Filters::Common::Lua::BaseLuaObject<StreamHan
State state_{State::Running};
std::function<void()> yield_callback_;
Http::AsyncClient::Request* http_request_{};
FireAndForgetHttpWriter* fireAndForgetHttpWriter_;
};

/**
* HTTP client that performs non-blocking HTTP calls and ignores the response.
*/
class FireAndForgetHttpWriter : public Filters::Common::Lua::BaseLuaObject<FireAndForgetHttpWriter>,
public Http::AsyncClient::Callbacks {
public:
FireAndForgetHttpWriter(Filter& filter);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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


int luaHttpCallNonblocking(lua_State* state);

// Http::AsyncClient::Callbacks
void onSuccess(Http::ResponseMessagePtr&&) override {}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: del newline

void onFailure(Http::AsyncClient::FailureReason) override {}

private:
Filter& filter_;
};

/**
* A class with shared code for building and making HTTP calls.
*/
class LuaFilterUtil : public Filters::Common::Lua::BaseLuaObject<LuaFilterUtil> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Instead of making these utility functions exposed outside of this class/translation unit i'd suggest putting them in the anonymous namespace in the .cc file. This tends to be the preferred way of doing helper functions that are only used in one .cc file and that doesn't benefit from being a member function

you can look for namespace { in existing code to see what an anonymous namespace looks like

public:
static Http::AsyncClient::Request* makeHttpCall(lua_State* state, Filter& filter,
Http::AsyncClient::Callbacks& callbacksListener);

static void buildHeadersFromTable(Http::HeaderMap& headers, lua_State* state, int table_index);
};

/**
Expand Down
53 changes: 53 additions & 0 deletions test/extensions/filters/http/lua/lua_filter_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -799,6 +799,59 @@ TEST_F(LuaHttpFilterTest, HttpCall) {
callbacks->onSuccess(std::move(response_message));
}

// Basic nonblocking, fire-and-forget HTTP request flow.
TEST_F(LuaHttpFilterTest, HttpCallNonblocking) {
const std::string SCRIPT{R"EOF(
function envoy_on_request(request_handle)
local headers, body = request_handle:httpCallNonblocking(
"cluster",
{
[":method"] = "POST",
[":path"] = "/",
[":authority"] = "foo",
["set-cookie"] = { "flavor=chocolate; Path=/", "variant=chewy; Path=/" }
},
"hello world",
5000)
end
)EOF"};

InSequence s;
setup(SCRIPT);

Http::TestRequestHeaderMapImpl request_headers{{":path", "/"}};
Http::MockAsyncClientRequest request(&cluster_manager_.async_client_);
Http::AsyncClient::Callbacks* callbacks;
EXPECT_CALL(cluster_manager_, get(Eq("cluster")));
EXPECT_CALL(cluster_manager_, httpAsyncClientForCluster("cluster"));
EXPECT_CALL(cluster_manager_.async_client_, send_(_, _, _))
.WillOnce(
Invoke([&](Http::RequestMessagePtr& message, Http::AsyncClient::Callbacks& cb,
const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* {
EXPECT_EQ((Http::TestHeaderMapImpl{{":path", "/"},
{":method", "POST"},
{":authority", "foo"},
{"set-cookie", "flavor=chocolate; Path=/"},
{"set-cookie", "variant=chewy; Path=/"},
{"content-length", "11"}}),
message->headers());
callbacks = &cb;
return &request;
}));

EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(request_headers, false));

Buffer::OwnedImpl data("hello");
EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(data, false));

Http::TestRequestTrailerMapImpl request_trailers{{"foo", "bar"}};
EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->decodeTrailers(request_trailers));

Http::ResponseMessagePtr response_message(new Http::ResponseMessageImpl(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this seems unused?

Http::ResponseHeaderMapPtr{new Http::TestResponseHeaderMapImpl{{":status", "200"}}}));
response_message->body() = std::make_unique<Buffer::OwnedImpl>("response");
}

// Double HTTP call. Responses before request body.
TEST_F(LuaHttpFilterTest, DoubleHttpCall) {
const std::string SCRIPT{R"EOF(
Expand Down
Loading