Skip to content
Merged
Show file tree
Hide file tree
Changes from 13 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.

httpCallAsync()
^^^^^^^^^^^^^^^

.. code-block:: lua

headers, body = handle:httpCallAsync(cluster, headers, body, timeout)

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.

drive-by comment; I know nothing about Lua but I would've expected, and would hope you would not return headers/body from an async function, but would instead have the user supply another optional argument, which would be some sort of closure or lambda to run when the async fetch was complete, passing the headers and body to that function.

Even better, I think 2 lambdas should be supplied: one that is passed the headers when available. The second is called with body-chunks, until there are no more. It would receive a bool param that would be 'true' when the request is complete.

Actually I haven't looked deeply at the Envoy infrastructure for HTTP Fetches, but modeling the Lua interface based on that pattern (which might differ from my suggestion above) would be even better.

The phrase 'fire and forget' doesn't seem good to me from a resource usage perspective.

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.

Ah, you're correct, this does not return headers/body. Pushed a change. Thanks for noticing that!

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.

Resource/memory wise, it looks like there are checks in the pipeline for this (I ran into it in previous pushes, and resolved them).

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.

@jmarantz and I talked about this offline. We will rename this httpCallNonblocking and proceed without callbacks.


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 `httpCallAsync` function. Same as `httpCall` but fire-and-forget.
* 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
153 changes: 88 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");
}
}) {}
}),
fireAndForgetWriter_(new FireAndForgetWriter(filter)) {}

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

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::luaHttpCallAsync(lua_State* state) {
return fireAndForgetWriter_->luaHttpCallAsync(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,84 @@ int StreamHandleWrapper::luaImportPublicKey(lua_State* state) {
return 1;
}

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

int FireAndForgetWriter::luaHttpCallAsync(lua_State* state) {
if (LuaFilterUtil::makeHttpCall(state, filter_, *this)) {
return 0;
} else {

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.

Superfluous else? We can remove this, but I don't feel strongly about it.

return 2;
}
}

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
48 changes: 46 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 FireAndForgetWriter;

/**
* 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},
{"httpCallAsync", static_luaHttpCallAsync},
{"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 an asynchronous 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, luaHttpCallAsync);

/**
* 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,38 @@ class StreamHandleWrapper : public Filters::Common::Lua::BaseLuaObject<StreamHan
State state_{State::Running};
std::function<void()> yield_callback_;
Http::AsyncClient::Request* http_request_{};
FireAndForgetWriter* fireAndForgetWriter_;

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.

WDYT of making this a unique_ptr?

};

/**
* Implementation that takes incoming requests and implements

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.

Sorry, could you expand this a little bit? I'm a bit confused by "takes incoming requests".

* "fire and forget" behavior using an async client.
*/
class FireAndForgetWriter : public Filters::Common::Lua::BaseLuaObject<FireAndForgetWriter>,

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.

Since this writer currently is an HTTP async client "writer". Can we name it as FireAndForgetHttpWriter or HttpAsyncClientWriter or similar?

public Http::AsyncClient::Callbacks {
public:
FireAndForgetWriter(Filter& filter);

int luaHttpCallAsync(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

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.

A super nit,

Suggested change
* A class with shared code for building and making http calls
* 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 Asynchronous, Fire and Forget HTTP request flow.

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.

Suggested change
// Basic Asynchronous, Fire and Forget HTTP request flow.
// Basic asynchronous, fire-and-forget HTTP request flow.

TEST_F(LuaHttpFilterTest, HttpCallAsync) {
const std::string SCRIPT{R"EOF(
function envoy_on_request(request_handle)
local headers, body = request_handle:httpCallAsync(
"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