-
Notifications
You must be signed in to change notification settings - Fork 434
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
Include openssl and ICU statically linked #7956
Conversation
e62fab2
to
599385d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tristan957 I'm trying to build static binary and would like to know if this option is enough to have libssl?
3192 tests run: 3053 passed, 0 failed, 139 skipped (full report)Flaky tests (2)Postgres 16
Postgres 14
Code coverage* (full report)
* collected from Rust tests only The comment gets automatically updated with the latest test results
3f62675 at 2024-06-07T17:39:05.665Z :recycle: |
599385d
to
e89059d
Compare
I wouldn't think so. Wouldn't you need something as discussed in https://stackoverflow.com/questions/6578484/telling-gcc-directly-to-link-a-library-statically? |
2f347c9
to
384cc75
Compare
What is the output of ldd after the latest push? |
aa6c567
to
16e0f33
Compare
Why can we just not recompile the |
Thtat's good point and we have encountered this libs missing issue when creating new project. its uses Maybe someone from Storage team can explain this better on why its uses |
9d2c21e
to
b9d1172
Compare
Result from my local build:
old build:
|
4809701
to
5231aa2
Compare
So we are bringing newer openssl and icu to match versions on the destination Linux. IMO it should work with both static and dynamic linking. Static is more reliable, but if it causes troubles we can roll that back to dynamic. |
f0c2bd2
to
cab653a
Compare
416b0cc
to
d540a4f
Compare
b08c064
to
d9a1c67
Compare
Due to the upcoming End of Life (EOL) for Debian 11, we need to upgrade the base OS for Pageservers from Debian 11 to Debian 12 for security reasons. When deploying a new Pageserver on Debian 12 with the same binary built on Debian 11, we encountered the following errors: ``` could not execute operation: pageserver error, status: 500, msg: Command failed with status ExitStatus(unix_wait_status(32512)): /usr/local/neon/v16/bin/initdb: error while loading shared libraries: libicuuc.so.67: cannot open shared object file: No such file or directory ``` and ``` could not execute operation: pageserver error, status: 500, msg: Command failed with status ExitStatus(unix_wait_status(32512)): /usr/local/neon/v14/bin/initdb: error while loading shared libraries: libssl.so.1.1: cannot open shared object file: No such file or directory ``` These issues occur when creating new projects. Closes: #12648 - To address these issues, we configured PostgreSQL build to use statically linked OpenSSL and ICU libraries. - This resolves the missing shared library errors when running the binaries on Debian 12.
We need to rebuild Postgres for changes in Dockerfile.build-tools as well this commit was override by my force push, so adding it again.
a69ca5b
to
3f62675
Compare
OpenSSL seems fine. ICU might be problematic. When the ICU version number changes, you will start to get a notice like this when you connect to a database that was created with the older version:
That's probably OK, I'm not aware of any ICU collation changes that would be likely to hit (glibc updates have been more problematic in the past). But an entry in the public-facing changelog and a heads up to the support team would be in order. |
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.
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.
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
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>
Problem
Due to the upcoming End of Life (EOL) for Debian 11, we need to upgrade
the base OS for Pageservers from Debian 11 to Debian 12 for security reasons.
When deploying a new Pageserver on Debian 12 with the same binary built on
Debian 11, we encountered the following errors:
and
These issues occur when creating new projects.
Summary of changes
To address these issues, we configured PostgreSQL build to use
statically linked OpenSSL and ICU libraries.
This resolves the missing shared library errors when running the
binaries on Debian 12.
Closes: https://github.com/neondatabase/cloud/issues/12648
Checklist before requesting a review
Checklist before merging