From bdfc697f5443410543dbca7c424cae3f9e7ea039 Mon Sep 17 00:00:00 2001 From: James Peach Date: Thu, 13 Oct 2016 09:52:46 -0700 Subject: [PATCH] TS-4947: Fix log collation hosts configuration. In the Lua logging configuration, we were failing to correctly configure log collation hosts. The main fix is to simply return true when we actually succeeded. Fixed a bug in lua_scoped_stack where we failed to count the number of values we pushed, which messes up the stack on return. Updated the LogHost display to show failover hosts. Added some tests for the supported syntaxes. --- lib/bindings/bindings.cc | 15 +++++ lib/bindings/bindings.h | 3 + lib/bindings/lua.h | 1 + proxy/logging/LogBindings.cc | 117 ++++++++++++++++++++++++++++++----- proxy/logging/LogHost.cc | 6 ++ 5 files changed, 127 insertions(+), 15 deletions(-) diff --git a/lib/bindings/bindings.cc b/lib/bindings/bindings.cc index 557a661a64a..32e996f86cd 100644 --- a/lib/bindings/bindings.cc +++ b/lib/bindings/bindings.cc @@ -250,6 +250,21 @@ BindingInstance::require(const char *path) return true; } +bool +BindingInstance::eval(const char *chunk) +{ + ink_release_assert(this->lua != NULL); + + if (luaL_dostring(this->lua, chunk) != 0) { + const char *w = lua_tostring(this->lua, -1); + Warning("%s", w); + lua_pop(this->lua, 1); + return false; + } + + return true; +} + BindingInstance * BindingInstance::self(lua_State *lua) { diff --git a/lib/bindings/bindings.h b/lib/bindings/bindings.h index 55f78a0a84c..21bd6d7d330 100644 --- a/lib/bindings/bindings.h +++ b/lib/bindings/bindings.h @@ -38,6 +38,9 @@ struct BindingInstance { // Import a Lua file. bool require(const char *path); + // Eval a chunk of Lua code. + bool eval(const char *chunk); + // Bind values to the specified global name. If the name contains '.' // separators, intermediate tables are constucted and the value is bound // to the final path component. diff --git a/lib/bindings/lua.h b/lib/bindings/lua.h index edbf7babf5a..db5c732e75d 100644 --- a/lib/bindings/lua.h +++ b/lib/bindings/lua.h @@ -89,6 +89,7 @@ struct lua_scoped_stack { push_value(int value) { lua_pushvalue(L, value); + nvals++; } private: diff --git a/proxy/logging/LogBindings.cc b/proxy/logging/LogBindings.cc index ce263a11367..c27e3b3cd19 100644 --- a/proxy/logging/LogBindings.cc +++ b/proxy/logging/LogBindings.cc @@ -27,6 +27,8 @@ #include "LogObject.h" #include "LogConfig.h" +#include "ts/TestBox.h" + static int refcount_object_new(lua_State *L, const char *type_name, RefCountObj *obj) { @@ -130,13 +132,17 @@ create_wipe_filter_object(lua_State *L) } static LogHost * -make_log_host(LogHost *parent, LogObject *log, const char *spec) +make_log_host(LogHost *parent, LogObject *log, const char *s) { ats_scoped_obj lh; + // set_name_or_ipstr() silently mmodifies its argument, so make + // a copy to avoid writing into memory owned by the Lua VM. + std::string spec(s); + lh = new LogHost(log->get_full_filename(), log->get_signature()); - if (!lh->set_name_or_ipstr(spec)) { - Error("invalid collation host specification '%s'", spec); + if (!lh->set_name_or_ipstr(spec.c_str())) { + Error("invalid collation host specification '%s'", s); return NULL; } @@ -148,7 +154,7 @@ make_log_host(LogHost *parent, LogObject *log, const char *spec) last = last->failover_link.next; } - Debug("lua", "added failover host %p to %p for %s", lh.get(), last, spec); + Debug("lua", "added failover host %p to %p for %s", lh.get(), last, s); last->failover_link.next = lh.release(); return parent; } @@ -172,6 +178,7 @@ log_object_add_hosts(lua_State *L, LogObject *log, int value, bool top) if (lua_istable(L, value)) { lua_scoped_stack saved(L); + int count = luaL_getn(L, value); LogHost *lh = NULL; @@ -182,23 +189,29 @@ log_object_add_hosts(lua_State *L, LogObject *log, int value, bool top) // We allow one level of array nesting to represent failover hosts. Puke if // a nested array contains anything other than strings. - if (!top && !lua_isstring(L, value)) { - luaL_error(L, "bad type, expected 'string' but found '%s'", lua_typename(L, lua_type(L, value))); + if (!top && !lua_isstring(L, -1)) { + luaL_error(L, "bad type, expected 'string' but found '%s'", lua_typename(L, lua_type(L, -1))); } - if (lua_isstring(L, value)) { - LogHost *tmp = make_log_host(top ? NULL : lh, log, lua_tostring(L, -1)); - if (tmp) { - lh = tmp; - } - } else if (lua_istable(L, value)) { + switch (lua_type(L, -1)) { + case LUA_TSTRING: + // This is a collation host address. Add it as a peer host if + // we are on the top level, or as a failover host if we are + // in a nested array. + lh = make_log_host(top ? NULL : lh, log, lua_tostring(L, -1)); + break; + + case LUA_TTABLE: + // Recurse to construct a failover group from a nested array. if (!log_object_add_hosts(L, log, -1, false /* nested */)) { lua_pop(L, 1); // Pop the element. - return false; } - } else { - luaL_error(L, "bad type, expected 'string' or 'array' but found '%s'", lua_typename(L, lua_type(L, value))); + + break; + + default: + luaL_error(L, "bad type, expected 'string' or 'array' but found '%s'", lua_typename(L, lua_type(L, -1))); } // If this is the top level array, then each entry is a LogHost. For nested arrays, we aggregate @@ -211,7 +224,10 @@ log_object_add_hosts(lua_State *L, LogObject *log, int value, bool top) lua_pop(L, 1); // Pop the element. } + // Attach the log host to this log object. lh will only be non-null if we + // are dealing with a nested array of failover hosts. log->add_loghost(lh, false /* take ownership */); + return true; } return false; @@ -342,6 +358,10 @@ create_log_object(lua_State *L, const char *name, LogFileFormat which) lua_pop(L, 1); + if (is_debug_tag_set("log-config")) { + log->display(stderr); + } + // Now the object is complete, give it to the object manager. conf->log_object_manager.manage_object(log.get()); @@ -420,3 +440,70 @@ MakeLogBindings(BindingInstance &binding, LogConfig *conf) return true; } + +EXCLUSIVE_REGRESSION_TEST(LogConfig_CollationHosts)(RegressionTest *t, int /* atype ATS_UNUSED */, int *pstatus) +{ + TestBox box(t, pstatus); + + LogConfig config; + BindingInstance binding; + + const char single[] = R"LUA( + log.ascii { + Format = "%", + Filename = "one-collation-host", + CollationHosts = "127.0.0.1:8080", + } + )LUA"; + + const char multi[] = R"LUA( + log.ascii { + Format = "%", + Filename = "many-collation-hosts", + CollationHosts = { "127.0.0.1:8080", "127.0.0.1:8081" }, + } + )LUA"; + + const char failover[] = R"LUA( + log.ascii { + Format = "%", + Filename = "many-collation-failover", + CollationHosts = { + { '127.0.0.1:8080', '127.0.0.1:8081' }, + { '127.0.0.2:8080', '127.0.0.2:8081' }, + { '127.0.0.3:8080', '127.0.0.3:8081' }, + } + } + )LUA"; + + const char combined[] = R"LUA( + log.ascii { + Format = "%", + Filename = "mixed-collation-failover", + CollationHosts = { + { '127.0.0.1:8080', '127.0.0.1:8081' }, + { '127.0.0.2:8080', '127.0.0.2:8081' }, + { '127.0.0.3:8080', '127.0.0.3:8081' }, + '127.0.0.4:8080', + '127.0.0.5:8080', + } + } + )LUA"; + + (void)single; + (void)multi; + (void)failover; + (void)combined; + + box = REGRESSION_TEST_PASSED; + + box.check(binding.construct(), "construct Lua binding instance"); + box.check(MakeLogBindings(binding, &config), "load Lua log configuration API"); + + box.check(binding.eval(single), "configuring a single log host"); + box.check(binding.eval(multi), "configuring multiple log hosts"); + box.check(binding.eval(failover), "configuring a multiple hosts with failover"); + box.check(binding.eval(combined), "configuring a multiple hosts some with failover"); + + config.display(stderr); +} diff --git a/proxy/logging/LogHost.cc b/proxy/logging/LogHost.cc index a93fd0df5db..7ee53305e3c 100644 --- a/proxy/logging/LogHost.cc +++ b/proxy/logging/LogHost.cc @@ -310,6 +310,12 @@ void LogHost::display(FILE *fd) { fprintf(fd, "LogHost: %s:%u, %s\n", name(), port(), (connected(NOPING)) ? "connected" : "not connected"); + + LogHost *host = this; + while (host->failover_link.next != NULL) { + fprintf(fd, "Failover: %s:%u, %s\n", host->name(), host->port(), (host->connected(NOPING)) ? "connected" : "not connected"); + host = host->failover_link.next; + } } void