build gRPC against OpenSSL 3#1188
Open
tmckayus wants to merge 3 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds OpenSSL 3 detection and build/runtime wiring: new OpenSSL hint variables, RHEL8 EPEL/install for OpenSSL3, pass explicit OpenSSL 3 paths into gRPC and wheel CMake builds, exclude OpenSSL3 libs from wheels, add a runtime installer, and invoke it from CI jobs. ChangesOpenSSL 3 Build Integration
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ci/utils/install_protobuf_grpc.sh`:
- Around line 93-99: The script currently installs libssl-dev unconditionally in
the ubuntu/debian branch of install_protobuf_grpc.sh; add a distro/version check
(parse /etc/os-release VERSION_ID and ID) before installing libssl-dev and
enforce minimum versions (Ubuntu >=22.04 or Debian >=12); if the host is older,
either install an OpenSSL 3 provider/backport or exit with a clear error asking
the user to upgrade or enable a PPA/backport—implement this logic in the
ubuntu/debian install branch so the script only installs libssl-dev when
VERSION_ID meets the minimum, otherwise perform the alternative install or fail
with an actionable message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2313efb6-6e29-491b-96a8-d29fcf687b7a
📒 Files selected for processing (2)
ci/build_wheel_libcuopt.shci/utils/install_protobuf_grpc.sh
tmckayus
added
non-breaking
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ci/utils/install_openssl3_runtime.sh`:
- Around line 37-39: The current linker-path check uses ldconfig -p piped to
grep -qE "libssl\.so\.3|libcrypto\.so\.3" which passes if either library is
present; change it to require both by testing for each symbol separately (e.g.,
invoke ldconfig -p | grep -q "libssl\.so\.3" and ldconfig -p | grep -q
"libcrypto\.so\.3" or count both matches via grep -Eo and wc -l) and only echo
the error/exit if one or both are missing; keep the existing error message text
but ensure the conditional uses both libssl.so.3 and libcrypto.so.3 checks
instead of the single alternation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 70d96dd6-5a1c-488c-af1e-40be4ba0c273
📒 Files selected for processing (4)
ci/test_wheel_cuopt.shci/test_wheel_cuopt_server.shci/utils/install_openssl3_runtime.shci/utils/install_protobuf_grpc.sh
tmckayus
force-pushed
the
feature/openssl3-build
branch
from
25f4cfc to
893e701
Compare
Switch the wheel build to link gRPC against OpenSSL 3: * install_protobuf_grpc.sh: on Rocky/RHEL 8, install epel-release and openssl3-devel. Rocky/RHEL 9 and Ubuntu/Debian already have openssl-devel at version 3 so use that. * build_wheel_libcuopt.sh: Add libssl.so.3 / libcrypto.so.3 to the auditwheel exclude list so the wheel does not bundle them; runtime resolves to the host's OpenSSL 3, ensuring libcrypto and any FIPS provider (system or mounted) stay byte-version-matched. Verified that the resulting binary links require only OPENSSL_3.0.0 symbol versions (159 refs, 0 newer), so the wheel is ABI-compatible with the cuopt container's Ubuntu 22.04 (libssl 3.0.2), RHEL/Rocky 9 (3.0.7), and any OpenSSL 3.x runtime. Hosts must provide libssl.so.3 / libcrypto.so.3 (Ubuntu 22.04+, Debian 12+, RHEL/Rocky/Alma 9+, Fedora 36+ provides). Stock RHEL/Rocky 8 and Ubuntu 20.04 are not covered by this; users on those systems should either use the cuopt container image or install OpenSSL 3 from EPEL or a backport before pip-installing cuopt.
tmckayus
force-pushed
the
feature/openssl3-build
branch
from
d0f670a to
d65d905
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ci/utils/install_protobuf_grpc.sh (1)
109-121:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFedora dependency installation can silently no-op.
Line 109 includes
fedora, but Line 111/118 only install deps for major versions8or9. On Fedora (e.g., 36+), this branch skips installs entirely when--skip-depsis not used.Suggested minimal fix
- if [[ "$ID" == "rocky" || "$ID" == "centos" || "$ID" == "rhel" || "$ID" == "fedora" ]]; then + if [[ "$ID" == "rocky" || "$ID" == "centos" || "$ID" == "rhel" || "$ID" == "fedora" ]]; then # Enable PowerTools (Rocky 8) or CRB (Rocky 9) for some packages if [[ "${VERSION_ID%%.*}" == "8" ]]; then dnf config-manager --set-enabled powertools || dnf config-manager --set-enabled PowerTools || true # EPEL provides 'openssl3-devel' in parallel with the system OpenSSL 1.1.x. dnf install -y epel-release dnf install -y git cmake ninja-build gcc gcc-c++ openssl3-devel zlib-devel c-ares-devel OPENSSL_INCLUDE_DIR_HINT="/usr/include/openssl3" OPENSSL_LIB_DIR_HINT="/usr/lib64/openssl3" elif [[ "${VERSION_ID%%.*}" == "9" ]]; then dnf config-manager --set-enabled crb || true dnf install -y git cmake ninja-build gcc gcc-c++ openssl-devel zlib-devel c-ares-devel + elif [[ "$ID" == "fedora" ]]; then + dnf install -y git cmake ninja-build gcc gcc-c++ openssl-devel zlib-devel c-ares-devel fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ci/utils/install_protobuf_grpc.sh` around lines 109 - 121, The branch that checks [[ "$ID" == "rocky" || "$ID" == "centos" || "$ID" == "rhel" || "$ID" == "fedora" ]] currently only handles VERSION_ID major 8 and 9, so Fedora (e.g., 36+) skips dependency installation; update the nested VERSION_ID conditional in that block to include an else (or an explicit case for Fedora) that runs the dnf installs (git, cmake, ninja-build, gcc, gcc-c++, openssl-devel or openssl3-devel as appropriate, zlib-devel, c-ares-devel) and enable any needed repos (e.g., crb or epel) and set OPENSSL_INCLUDE_DIR_HINT/OPENSSL_LIB_DIR_HINT only where applicable (keep existing behavior for Rocky 8 vs 9).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@ci/utils/install_protobuf_grpc.sh`:
- Around line 109-121: The branch that checks [[ "$ID" == "rocky" || "$ID" ==
"centos" || "$ID" == "rhel" || "$ID" == "fedora" ]] currently only handles
VERSION_ID major 8 and 9, so Fedora (e.g., 36+) skips dependency installation;
update the nested VERSION_ID conditional in that block to include an else (or an
explicit case for Fedora) that runs the dnf installs (git, cmake, ninja-build,
gcc, gcc-c++, openssl-devel or openssl3-devel as appropriate, zlib-devel,
c-ares-devel) and enable any needed repos (e.g., crb or epel) and set
OPENSSL_INCLUDE_DIR_HINT/OPENSSL_LIB_DIR_HINT only where applicable (keep
existing behavior for Rocky 8 vs 9).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 219a2d75-0315-4915-89ad-162acb8eb641
📒 Files selected for processing (5)
ci/build_wheel_libcuopt.shci/test_wheel_cuopt.shci/test_wheel_cuopt_server.shci/utils/install_openssl3_runtime.shci/utils/install_protobuf_grpc.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- ci/test_wheel_cuopt.sh
- ci/test_wheel_cuopt_server.sh
Contributor
Author
|
/ok to test d65d905 |
Collaborator
rgsl888prabhu
left a comment
rgsl888prabhu
left a comment
There was a problem hiding this comment.
Changes looks good, but we may have to add this detail to docs or faqs to install openssl3 so it is always covered, and also did you check if the container needs this update ?
And does conda needs this update as well?
Contributor
Author
I can work on docs. conda already has openssl3, I believe the situation was that the container had it but libcuopt was linked against openssl 1.1.1 so it didn't use openssl 3 even though it was there. I will confirm |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Switch the wheel build to link gRPC against OpenSSL 3. This is good for modernization, and also necessary to enable FIPS compliance on many current OS platforms.
install_protobuf_grpc.sh: on Rocky/RHEL 8, install epel-release and openssl3-devel. Rocky/RHEL 9 and Ubuntu/Debian already have openssl-devel at version 3 so use that.
build_wheel_libcuopt.sh: Add libssl.so.3 / libcrypto.so.3 to the auditwheel exclude list so the wheel does not bundle them; runtime resolves to the host's OpenSSL 3, ensuring libcrypto and any FIPS provider (system or mounted) stay byte-version-matched.
Verified that the resulting binary links require only OPENSSL_3.0.0 symbol versions (159 refs, 0 newer), so the wheel is ABI-compatible with the cuopt container's Ubuntu 22.04 (libssl 3.0.2), RHEL/Rocky 9 (3.0.7), and any OpenSSL 3.x runtime.
Hosts must provide libssl.so.3 / libcrypto.so.3 (Ubuntu 22.04+, Debian 12+, RHEL/Rocky/Alma 9+, Fedora 36+ provides). Stock RHEL/Rocky 8 and Ubuntu 20.04 are not covered by this; users on those systems should either use the cuopt container image or install OpenSSL 3 from EPEL or a backport before pip-installing cuopt.