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

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

Open
htuch opened this issue Sep 24, 2020 · 8 comments
Open

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

htuch opened this issue Sep 24, 2020 · 8 comments
Labels
area/compression enhancement Feature requests. Not bugs or questions. help wanted Needs help!

Comments

@htuch
Copy link
Member

htuch commented Sep 24, 2020

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 htuch added enhancement Feature requests. Not bugs or questions. triage Issue requires triage area/compression help wanted Needs help! and removed triage Issue requires triage labels Sep 24, 2020
@htuch
Copy link
Member Author

htuch commented Sep 24, 2020

@junr03 @rojkov

@rojkov
Copy link
Member

rojkov commented Sep 25, 2020

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 zlib-ng/zlib-ng#90 and zlib-ng/zlib-ng#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
Copy link
Member

dio commented Sep 25, 2020

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
Copy link

Diatrus commented Mar 26, 2021

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

@moderation
Copy link
Contributor

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",
    ),

keith added a commit to keith/envoy that referenced this issue Sep 2, 2021
First step of: envoyproxy#13261

Signed-off-by: Keith Smiley <[email protected]>
@snowp
Copy link
Contributor

snowp commented Sep 2, 2021

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
Copy link
Member Author

htuch commented Sep 3, 2021

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
Copy link
Member

keith commented Mar 16, 2022

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/compression enhancement Feature requests. Not bugs or questions. help wanted Needs help!
Projects
None yet
Development

No branches or pull requests

7 participants