Dispatch inline Lua code in per filter config of Lua filter by RDS

[wbpcode]

This is a supplement to PR #11235. Due to thread safety issues, the function of directly dispatch Lua code in per filter config through RDS is removed from #11235. Now, I am going to take some time to finalize it.

Although I have done some tests in my own environment, I am still worried about whether there will be problems that I have not considered. So I hope to get some opinions from the community.

For related background, please check issue #3124, #9279 and PR #11235.

[wbpcode]

https://github.com/wbpcode/envoy/blob/route-lua/source/extensions/filters/common/lua/lua.h#L361

class ThreadLocalState : Logger::Loggable<Logger::Id::lua>, public Event::DeferredDeletable {
public:
  ThreadLocalState(const std::string& code, ThreadLocal::SlotAllocator& tls);

  /**
   * @return CoroutinePtr a new coroutine.
   */
  CoroutinePtr createCoroutine();
// ......

https://github.com/wbpcode/envoy/blob/route-lua/source/extensions/filters/http/lua/lua_filter.h#L23

class PerLuaCodeSetup : Logger::Loggable<Logger::Id::lua> {
public:
  PerLuaCodeSetup(const std::string& lua_code, ThreadLocal::SlotAllocator& tls,
                  Event::Dispatcher& dispatcher);
  ~PerLuaCodeSetup() { dispatcher_.deferredDelete(std::move(lua_state_)); }


  Extensions::Filters::Common::Lua::CoroutinePtr createCoroutine() {
    return lua_state_->createCoroutine();
  }

  int requestFunctionRef() { return lua_state_->getGlobalRef(request_function_slot_); }
  int responseFunctionRef() { return lua_state_->getGlobalRef(response_function_slot_); }

  uint64_t runtimeBytesUsed() { return lua_state_->runtimeBytesUsed(); }
  void runtimeGC() { return lua_state_->runtimeGC(); }

private:
  uint64_t request_function_slot_{};
  uint64_t response_function_slot_{};

  Filters::Common::Lua::ThreadLocalStatePtr lua_state_;
  Event::Dispatcher& dispatcher_;
};

In order to ensure that TLS will not be modified on the worker thread, I destroy ThreadLocalState by using the dispatcher of the main thread.
In this way, both the creation and destruction of ThreadLocalState will be completed in the main thread.
I am not sure if I have missed other points that may cause security problems. I hope you two @dio @mattklein123 can help confirm whether this solution is feasible?

[mattklein123]

Sorry I'm not sure I follow the proposal. Can you explain in more detail potentially?

[wbpcode]

Sorry I'm not sure I follow the proposal. Can you explain in more detail potentially?

In Lua filter, we use PerLuaCodeSetup (PerLuaCodeSetup contains a ThreadLocalState member) to parse and manage a Lua script.

When we need to dynamically deliver Lua scripts through RDS, we will also create a PerLuaCodeSetup object associated with a specific route.

The problem is that when the routes table is updated, the PerLuaCodeSetup in the old route configuration will be destructed in the worker thread, which will lead to the modification of TLS in the same worker thread, which will eventually bring thread safety issues.

In the new implementation, I plan to use the dispatch (dispatch.deferredDelete) of the main thread to assist in the destruction of PerLuaCodeSetup. Ensure that all ThreadLocalState memeber of PerLuaCodeSetup can only be destructed on the main thread to avoid thread safety issues.

But now I cannot fully confirm whether this modification can circumvent all the problems. @mattklein123

PS: Maybe I submit a new PR directly and let the code explain how I did it (very simple way), if there is a problem, slowly improve it ?

[mattklein123]

Don't you need a notification of route change though? I think that is what is missing. Otherwise you have to do a dance in which you bounce back and forth between the workers and main thread. I think that if you were able to get a callback when the route table is updated you could potentially walk the table and pre-create Lua code for all routes and then TLS update that out to worker threads? cc @kyessenov @jplevyak @lizan @dio as this is a similar issue that we would eventually want to solve for WASM also I think.

[wbpcode]

I think that if you were able to get a callback when the route table is updated you could potentially walk the table and pre-create Lua code for all routes and then TLS update that out to worker threads

If I understand what you mean correctly, in fact, this is what I do now.

When the route table is updated, all Lua codes in the new route table will be parsed and corresponding PerLuaCodeSetup will be created (the dispatcher of the main thread will also be referenced by PerLuaCodeSetup at this time).

When all the creation is completed, TLS will update the route table to each worker thread. At the same time, the old route table will be destroyed on a thread (the thread that updated the route table last).

All PerLuaCodeSetup in the old route table will also be destroyed. But ThreadLocalState in the PerLuaCodeSetup will be handled specially and will be destructed by the dispatcher of the main thread.

[mattklein123]

If I understand what you mean correctly, in fact, this is what I do now.

You mean in the current code or in your proposed patch? We don't have any route table update callbacks right now. I think to allow RDS updates you would need this.

[stale]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or other activity occurs. Thank you for your contributions.

[dio]

This is done. #11235.