Skip to content
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

Closed
wrowe opened this issue Oct 13, 2020 · 15 comments · Fixed by #25923
Closed

Deprecate moonjit forked component of envoy in favor of luajit #13539

wrowe opened this issue Oct 13, 2020 · 15 comments · Fixed by #25923
Assignees

Comments

@wrowe
Copy link
Contributor

wrowe commented Oct 13, 2020

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

@wrowe wrowe added the triage Issue requires triage label Oct 13, 2020
@htuch
Copy link
Member

htuch commented Oct 13, 2020

+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?

@htuch htuch added help wanted Needs help! tech debt area/lua and removed triage Issue requires triage labels Oct 13, 2020
@mattklein123
Copy link
Member

+1, let's drop moonjit and s390. If someone cares enough about it they can work on upstream support and CI.

@wrowe
Copy link
Contributor Author

wrowe commented Oct 13, 2020

FYI @dio

@moderation
Copy link
Contributor

moderation commented Oct 13, 2020

SGTM. Less dependencies, well maintained is 👍

htuch pushed a commit that referenced this issue Oct 16, 2020
See #13539

Signed-off-by: William A Rowe Jr <wrowe@vmware.com>
@dio
Copy link
Member

dio commented Oct 16, 2020

@wrowe sure! Let's remove it.

@rashmi-ibm
Copy link

Deprecating moonjit will cause failures for envoy builds on ppc64le architecture. Can the support for moonjit be retained..

@htuch
Copy link
Member

htuch commented Oct 21, 2020

@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.

@wrowe
Copy link
Contributor Author

wrowe commented Oct 23, 2020

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.

@wrowe
Copy link
Contributor Author

wrowe commented Oct 23, 2020

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.

@wrowe wrowe added this to the 1.17.0 milestone Oct 23, 2020
@wrowe
Copy link
Contributor Author

wrowe commented Dec 16, 2020

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.

@htuch
Copy link
Member

htuch commented Dec 17, 2020

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.

wrowe added a commit to wrowe/envoy that referenced this issue Jan 8, 2021
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>
@wrowe wrowe modified the milestones: 1.17.0, 1.18.0 Jan 8, 2021
@wrowe
Copy link
Contributor Author

wrowe commented Jan 8, 2021

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.

@mattklein123 mattklein123 modified the milestones: 1.18.0, 1.19.0 Apr 25, 2021
@mattklein123 mattklein123 removed this from the 1.19.0 milestone Jul 7, 2021
rexengineering pushed a commit to rexengineering/istio-envoy that referenced this issue Oct 15, 2021
See envoyproxy/envoy#13539

Signed-off-by: William A Rowe Jr <wrowe@vmware.com>
@spacewander
Copy link
Contributor

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,
LuaJIT solves CVE-2020-24372 with commits LuaJIT/LuaJIT@12ab596 and LuaJIT/LuaJIT@e296f56, which are not in the moonjit repo.

@mattklein123
Copy link
Member

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.

@moderation
Copy link
Contributor

+1. From a dependency maintenance point of view, we should remove

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants