envoyproxy / envoy

Cloud-native high-performance edge/middle/service proxy
https://www.envoyproxy.io
Apache License 2.0
24.51k stars 4.73k forks source link

Switch to zlib-ng by default, remove zlib #13261

Open htuch opened 3 years ago

htuch commented 3 years ago

It would be ideal not to be on the hook for maintaining two distinct zlib implementations as external deps. If zlib-ng is faster or directionally where we want to go, maybe we can switch to that as the default library. Then after a period of burn time we can delete zlib support.

htuch commented 3 years ago

@junr03 @rojkov

rojkov commented 3 years ago

Even though it's pretty solid in the zlib compatibility mode I'd wait a bit still before making it the default choice.

I checked https://github.com/zlib-ng/zlib-ng/issues/90 and https://github.com/zlib-ng/zlib-ng/issues/488 and it seems zlib-ng is very close to making its first stable release. We can do the switch as soon as zlib-ng is officially stable.

Then after having zlib removed we could reimplement the de/compressor to use zlib-ng's native API and compile out the ZLIB_COMPAT code.

dio commented 3 years ago

we could reimplement the de/compressor to use zlib-ng's native API

Just be careful that gRPC uses zlib as well (and curl! But this one will be removed soon 🙏🏽)

Diatrus commented 3 years ago

An update on this, zlib-ng was released as stable 10 days ago.

moderation commented 3 years ago

https://github.com/zlib-ng/zlib-ng/releases/tag/2.0.2 bazel/repository_locations.bzl

    com_github_zlib_ng_zlib_ng = dict(
        project_name = "zlib-ng",
        project_desc = "zlib fork (higher performance)",
        project_url = "https://github.com/zlib-ng/zlib-ng",
        version = "2.0.2",
        sha256 = "dd37886f22ca6890e403ea6c1d60f36eab1d08d2f232a35f5b02126621149d28",
        release_date = "2021-03-23",
        strip_prefix = "zlib-ng-{version}",
        urls = ["https://github.com/zlib-ng/zlib-ng/archive/{version}.tar.gz"],
        use_category = ["controlplane", "dataplane_core"],
        cpe = "N/A",
    ),
snowp commented 2 years ago

At this point it seems like we can start thinking about dropping zlib support since the stable release is quite a while ago.

What would be the process for doing so? Would we want to go through a deprecation period where we switch the default to ng or are we comfortable just making the switch directly?

@keith also pointed out that we currently only support using zlib-ng on Linux, which might be a blocker for getting rid of zlib unless we can get that working.

@envoyproxy/dependency-shepherds @htuch for thoughts

htuch commented 2 years ago

This is definitely a papercut; i.e. all things being equal it would be better to have one library than two, but we probably need to find an owner who cares enough to do the work that @rojkov suggested and get this working across platforms. The marginal security advantage is probably somewhat slim, given how well tested and used zlib is, but I'd take the dependency elimination if all other things were equal.

This would have to be a build time switch. I'm not sure about switching to the native APIs though, I think we have much better support for zlib than zlib-ng internally and having build time flexibility would continue to be something we want to keep.

keith commented 2 years ago

I was looking at this a bit lately because the situation at this point is a bit complicated. The new outlier is com_googlesource_chromium_zlib, from that repo we use zlib_compression_utils, and v8 uses zlib from there as well. Currently our other zlib repos take precedence over this one, but I'm not sure if that's good or not, because I don't know what chromium's fork of zlib changes, and how much v8 or zlib_compression_utils relies on the behavior differences.

Given zlib-ng has improved I did another test to see if it could be built everywhere, and it looks like there are some issues on windows, which are hard to debug for me since I don't have a windows machine and the CI iteration time is kind of high. I imagine we could figure this out, and switch to zlib-ng, dropping the original zlib.

But I wonder at this point if we would be better off trying to use chromium's zlib everywhere given our general reliance on it through these other targets, and dropping zlib-ng + the original zlib. But I don't have context on why we were trying to switch to zlib-ng originally either so I would love to hear thoughts here so we can try to clean up this stuff!