-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Deprecate moonjit forked component of envoy in favor of luajit #13539
Comments
+100. I think we should just drop s390 support in upstream Envoy. If we don't have it in CI, we don't even know if it's actually working. I think downstream consumers of Envoy can add MoonJit back in as needed. WDYT @envoyproxy/dependency-shepherds? |
+1, let's drop moonjit and s390. If someone cares enough about it they can work on upstream support and CI. |
FYI @dio |
SGTM. Less dependencies, well maintained is 👍 |
See #13539 Signed-off-by: William A Rowe Jr <wrowe@vmware.com>
@wrowe sure! Let's remove it. |
Deprecating moonjit will cause failures for envoy builds on ppc64le architecture. Can the support for moonjit be retained.. |
@rashmi-ibm I think it's only reasonable for us to support dependencies for architectures that are in our CI. I suggest that IBM either works to add ppc64le to Envoy CI or takes on the job of producing an Envoy distribution that adds MoonJIT support outside of this repository. There is also the option of trying to add PPC support directly to LuaJIT, MoonJIT is maintainer-less and we're uncomfortable with its security posture as a result. |
https://github.com/envoyproxy/envoy/blob/master/DEPENDENCY_POLICY.md spells out the project policy. I imagine, @rashmi-ibm , that this dependency wouldn't be entertained in the first place, if presented today. While there are a number of approaches to supporting ppc64le and s390x, the most rational one would be to cherry pick the useful support from moonjit (which is already forked from luajit) and propose as a fresh PR to the https://github.com/luajit/luajit maintainers, who seems active and receptive as noted in this issue. The previous moonjit maintainer seems willing to lend a hand to whomever takes on such an effort. |
Just for clarification, 1.16.x and earlier would be unaffected (but not receive any security updates, as htuch mentions above, since moonjit has no maintainers.) 1.17.0 would be arriving in January, so if anyone takes on pushing moonjit fixes to the luajit project, I'm personally happy to help transition to a newer commit or release tag of 2.1.x branch of luajit in the process of deprecating moonjit. This should occur on a November timeline in order for me to commit to helping shepherd it into envoy 1.17.0 release. |
Reminder, 1.17.0 freezes soon. I'll propose the PR to drop moonjit at year end. While it's up for discussion, it seems prudent to avoid the moonjit dependency in that release since there's no way to get security updates anymore, rather than carry it on any extended EOL/service basis. |
Yes, with no maintainers I think it's untenable to continue to support MoonJIT. There is effectively zero security story for a data plane component involving a JIT and dynamic code loading. +1 on removal ASAP. |
This reverts commit 2c03496. See thread https://twitter.com/siddhesh_p/status/1308594269502885889?s=20 @siddhesh_p Sep 22 "It ought to generally be easy to revert to the upstream tree since they're both [moonjit and luajit] (AFAIK) on the 2.1.x branch. I'll be happy to help with the transition if needed." This is no longer maintained code, there is no way for envoy project to assume responsibility, and after many months, no maintainers have stepped forward. PR's to luajit effort are one appropriate way forward. Resolves: envoyproxy#13539 Signed-off-by: William A Rowe Jr <wrowe@vmware.com>
After discussing with deps maintainers, it doesn't seem this is much of a showstopper... PR 14603 still needs work to unwind the layers of the onion which have happened since moonjit support was introduced. Looking at other discussions, there's also the question of whether modern Lua 5.4 or the OpenResty/LuaJIT2 fork are appropriate alternatives to the very old (but now somewhat again maintained) LuaJIT project? This requires some extra attention, and the possibility of keeping the selection logic that was introduced for moonjit (modulo moonjit itself, which has no maintainer now.) Will research and come back to this subject shortly, after 1.17.0. |
See envoyproxy/envoy#13539 Signed-off-by: William A Rowe Jr <wrowe@vmware.com>
Since we are going to upgrade LuaJIT to the latest version (#25763), maybe it is time to drop moonjit? There are some bugs that are fixed in LuaJIT but not in moonjit. For example, |
I think there have been comments in the past about removing moonjit. I can't remember why it's there in the first place so I think it's probably fine to remove. |
+1. From a dependency maintenance point of view, we should remove |
A very important development has occurred to the moonjit fork;
https://github.com/moonjit/moonjit/blob/master/README.md
As stated in this thread;
https://twitter.com/siddhesh_p/status/1308594269502885889?s=20 @ siddhesh_p Sep 22
"It ought to generally be easy to revert to the upstream tree since they're both [moonjit and luajit] (AFAIK) on the 2.1.x branch. I'll be happy to help with the transition if needed."
It's clear from #13474 Envoy is not in a position to maintain multiple patches and appropriate level of security reviews against multiple old revisions based on whatever political discussions might be happening elsewhere. For context, the underlying Lua project from PUC-Rio has never seen it necessary to enable their basic makefile for dynamic linking or other linux build gymnastics, it's a truly KISS approach to getting the lua engine together. The various luajit forks each have relatively few or one active maintainers, if that many. But the core luajit fork has accepted a wide range of ports from a wide range of contributors and is the most frequently used upstream, as seen here; http://luajit.org/sponsors.html
I'd concur with Siddhesh that getting any s390 patches (31 or 64 bit compatibility) into luajit 2.1 branch should be a straightforward exercise, one which I don't have the machine access to accomplish anymore myself. I suspect Envoy needs only 64-bit compatibility at this late date, which is much less quirky than the 31 bit model. Since it seems that this deprecated moonjit is needed only for the s390 support introduced in #10265 - it seems unwise for the Envoy maintainers to manage this dependency moving forwards.
I'll offer a PR to detangle windows today from the existing moonjit support and lower the surface area required to maintain both in a short-term, but would encourage anyone with the facilities to review these dependencies and patches on moonjit to find us a path for s390 portability back into envoy's trusted upstream fork.
cc: @mattklein123 @htuch @moderation @iii-i
The text was updated successfully, but these errors were encountered: