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

Revert "Include openssl and ICU statically linked" #8003

Merged
merged 1 commit into from
Jun 10, 2024

Conversation

problame
Copy link
Contributor

Reverts #7956

Rationale: compute incompatibilties

Slack thread: https://neondb.slack.com/archives/C033RQ5SPDH/p1718011276665839?thread_ts=1718008160.431869&cid=C033RQ5SPDH

Relevant quotes from @hlinnaka

If we go through with the current release candidate, but the compute is pinned, people who create new projects will get that warning, which is silly. To them, it looks like the ICU version was downgraded, because initdb was run with newer version.

We should upgrade the ICU version eventually. And when we do that, users with old projects that use ICU will start to see that warning. I think that's acceptable, as long as we do homework, notify users, and communicate that properly.
When do that, we should to try to upgrade the storage and compute versions at roughly the same time.

Copy link

3192 tests run: 3053 passed, 0 failed, 139 skipped (full report)


Flaky tests (2)

Postgres 14

Code coverage* (full report)

  • functions: 31.5% (6606 of 20963 functions)
  • lines: 48.4% (51064 of 105401 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
d039488 at 2024-06-10T10:34:11.107Z :recycle:

@problame problame merged commit ae5badd into main Jun 10, 2024
68 checks passed
@problame problame deleted the revert-7956-static-build-binaries branch June 10, 2024 11:20
problame added a commit that referenced this pull request Jun 10, 2024
Reverts #7956

Rationale: compute incompatibilties

Slack thread:
https://neondb.slack.com/archives/C033RQ5SPDH/p1718011276665839?thread_ts=1718008160.431869&cid=C033RQ5SPDH

Relevant quotes from @hlinnaka 

> If we go through with the current release candidate, but the compute
is pinned, people who create new projects will get that warning, which
is silly. To them, it looks like the ICU version was downgraded, because
initdb was run with newer version.

> We should upgrade the ICU version eventually. And when we do that,
users with old projects that use ICU will start to see that warning. I
think that's acceptable, as long as we do homework, notify users, and
communicate that properly.
> When do that, we should to try to upgrade the storage and compute
versions at roughly the same time.
problame pushed a commit that referenced this pull request Jun 17, 2024
We had to revert the earlier static linking change due to libicu version
incompatibilities:

- original PR: #7956
- revert PR: #8003

Specifically, the problem manifests for existing projects as error

```
DETAIL:  The collation in the database was created using version 153.120.42, but the operating system provides version 153.14.37.
```

So, this PR reintroduces the original change but with the exact same
libicu version as in Debian `bullseye`, i.e., the libicu version that
we're using today.
This avoids the version incompatibility.

Additional changes made by Christian:
- validation of the libicu tarball checksum
- parallel build (`-j $(nproc)`) for openssl and libicu

Ultimately, we'll have to figure out an upgrade store for libciu.
That work is tracked in epic TODO.

Details
=======

Debian bullseye has a few patches on top of libicu:
https://sources.debian.org/patches/icu/67.1-7/

Eyeballing them (we don't have the expertise to confidently judge
whether any of them affect Postgres), the following two stand out:

* https://sources.debian.org/patches/icu/67.1-7/ICU-13786_Fix_addLikelySubtags_minimizeSubtags.patch/
    * This seems to pull in the patch from unicode-org/icu#1140
    * It's a bugfix (not CVE?) for upstream issue https://unicode-org.atlassian.net/browse/ICU-13786
    * Unsure whether it's relevant to Postgres.
    * Explanation of "minimize subtags" functionality fixed in this patch: https://developer.android.com/ndk/reference/group/icu4c#uloc_minimizesubtags
* https://sources.debian.org/patches/icu/67.1-7/locid_operators.patch/
    * The patch doesn't mention why it's been added.
    * A similar patch was committed upstream as commit `2dc5bea9061b4fb05cd03e21b775dd944a0eb81d`, `ICU-21587 Fix memory bug w/ baseName`
    * Looks like this is the fix for CVE-2021-30535
    * Upstream issue tracker link: https://unicode-org.atlassian.net/browse/ICU-21587
    * We should probably have this patch. But there's no proper patch release from upstream.

Solution for integrating these patches: TBD, they're not integrated in this PR.
Ideas:
1. Build the bullseye package in bookworm (easiest way to maintain exact Debian patchstack)
2. Fork libicu on github, apply patches manually, create a release, make Dockerfile download our fork's release.

Refs
====

Co-authored-by: Christian Schwarz <christian@neon.tech>

refs neondatabase/cloud#12648
rahulinux added a commit that referenced this pull request Jun 18, 2024
We had to revert the earlier static linking change due to libicu version
incompatibilities:

- original PR: #7956
- revert PR: #8003

Specifically, the problem manifests for existing projects as error

```
DETAIL:  The collation in the database was created using version 153.120.42, but the operating system provides version 153.14.37.
```

So, this PR reintroduces the original change but with the exact same
libicu version as in Debian `bullseye`, i.e., the libicu version that
we're using today.
This avoids the version incompatibility.


Additional changes made by Christian
====================================
- `hashFiles` can take multiple arguments, use that feature
- validation of the libicu tarball checksum
- parallel build (`-j $(nproc)`) for openssl and libicu

Follow-ups
==========

Debian bullseye has a few patches on top of libicu:
https://sources.debian.org/patches/icu/67.1-7/
We still decide whether we need to include these patches or not.
=> neondatabase/cloud#14527

Eventually, we'll have to figure out an upgrade story for libicu.
That work is tracked in epic
neondatabase/cloud#14525.

The OpenSSL version in this PR is arbitrary.
We should use `1.1.1w` + Debian patches if applicable.
See neondatabase/cloud#14526.

Longer-term:
* neondatabase/cloud#14519
* neondatabase/cloud#14525

Refs
====

Co-authored-by: Christian Schwarz <christian@neon.tech>

refs neondatabase/cloud#12648

---------

Co-authored-by: Rahul Patil <rahul@neon.tech>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants