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

Improve cpp-client-lib: provide another libpulsarwithdeps.a in dep/rpm #6458

Merged
merged 5 commits into from
Mar 18, 2020

Conversation

jiazhai
Copy link
Member

@jiazhai jiazhai commented Mar 2, 2020

Fix #6439
We shouldn't static link libssl in libpulsar.a, as this is a security red flag. we should just use whatever the libssl the system provides. Because if there is a security problem in libssl, all the machines can just update their own libssl library without rebuilding libpulsar.a.
As suggested, this change not change the old behavior, and mainly provides 2 other additional pulsar cpp client library in deb/rpm, and add related docs of how to use 4 libs in doc.
The additional 2 libs:

  • pulsarSharedNossl (libpulsarnossl.so), similar to pulsarShared(libpulsar.so), with no ssl statically linked.
  • pulsarStaticWithDeps(libpulsarwithdeps.a), similar to pulsarStatic(libpulsar.a), and archived in the dependencies libraries of libboost_regex, libboost_system, libcurl, libprotobuf, libzstd and libz statically.

Passed 4 libs rpm/deb build, install, and compile with a pulsar-client example code.

@sijie sijie added doc-required Your PR changes impact docs and you will update later. release/2.5.1 labels Mar 2, 2020
@sijie sijie added this to the 2.6.0 milestone Mar 2, 2020
@sijie
Copy link
Member

sijie commented Mar 2, 2020

@jiazhai Can you make sure the documentation is updated?

@aahmed-se
Copy link
Contributor

@merlimat can you please take a look.
I think it should be configurable but we should have the option to include libssl in the static binary if the user would like.

@aahmed-se
Copy link
Contributor

@jiazhai @sijie This will most likely break the python wheel files for mac, by default osx does not have ssl installed.

@sijie
Copy link
Member

sijie commented Mar 4, 2020

@jiazhai @aahmed-se how about introducing a new static library that doesn't link libssl?

@jiazhai
Copy link
Member Author

jiazhai commented Mar 4, 2020

@sijie Ok. I will try to add a new static library

@jiazhai jiazhai changed the title change to not include openssl in libpulsar.a [WIP]change to not include openssl in libpulsar.a Mar 5, 2020
@jiazhai jiazhai force-pushed the issue_6439 branch 2 times, most recently from a4dc4e8 to 723667b Compare March 11, 2020 16:50
@jiazhai jiazhai changed the title [WIP]change to not include openssl in libpulsar.a Improve cpp-client-lib: provide another libpulsarwithdeps.a in dep/rpm Mar 12, 2020
@jiazhai
Copy link
Member Author

jiazhai commented Mar 15, 2020

rebased with latest master

@jiazhai
Copy link
Member Author

jiazhai commented Mar 15, 2020

@aahmed-se @sijie updated to create separate additional libs for requirements, not changed the old behavior.

@jiazhai jiazhai merged commit 33eea88 into apache:master Mar 18, 2020
@jiazhai jiazhai removed the doc-required Your PR changes impact docs and you will update later. label Mar 18, 2020
@tuteng tuteng mentioned this pull request Mar 21, 2020
1 task
tuteng pushed a commit to AmateurEvents/pulsar that referenced this pull request Mar 21, 2020
…rpm (apache#6458)

Fix apache#6439 
We shouldn't static link libssl in libpulsar.a, as this is a security red flag. we should just use whatever the libssl the system provides. Because if there is a security problem in libssl, all the machines can just update their own libssl library without rebuilding libpulsar.a.
As suggested, this change not change the old behavior, and mainly provides 2 other additional pulsar cpp client library in deb/rpm, and add related docs of how to use 4 libs in doc.
The additional 2 libs: 
- pulsarSharedNossl (libpulsarnossl.so), similar to pulsarShared(libpulsar.so), with no ssl statically linked.
- pulsarStaticWithDeps(libpulsarwithdeps.a), similar to pulsarStatic(libpulsar.a), and archived in the dependencies libraries of `libboost_regex`,  `libboost_system`, `libcurl`, `libprotobuf`, `libzstd` and `libz` statically.

Passed 4 libs rpm/deb build, install, and compile with a pulsar-client example code.

* also add libpulsarwithdeps.a together with libpulsar.a into cpp client release

* add documentation for libpulsarwithdeps.a, add g++ build examples

* add pulsarSharedNossl target to build libpulsarnossl.so

* update doc

* verify 4 libs in rpm/deb build, installed, use all good

(cherry picked from commit 33eea88)
tuteng pushed a commit that referenced this pull request Apr 6, 2020
…rpm (#6458)

Fix #6439 
We shouldn't static link libssl in libpulsar.a, as this is a security red flag. we should just use whatever the libssl the system provides. Because if there is a security problem in libssl, all the machines can just update their own libssl library without rebuilding libpulsar.a.
As suggested, this change not change the old behavior, and mainly provides 2 other additional pulsar cpp client library in deb/rpm, and add related docs of how to use 4 libs in doc.
The additional 2 libs: 
- pulsarSharedNossl (libpulsarnossl.so), similar to pulsarShared(libpulsar.so), with no ssl statically linked.
- pulsarStaticWithDeps(libpulsarwithdeps.a), similar to pulsarStatic(libpulsar.a), and archived in the dependencies libraries of `libboost_regex`,  `libboost_system`, `libcurl`, `libprotobuf`, `libzstd` and `libz` statically.

Passed 4 libs rpm/deb build, install, and compile with a pulsar-client example code.

* also add libpulsarwithdeps.a together with libpulsar.a into cpp client release

* add documentation for libpulsarwithdeps.a, add g++ build examples

* add pulsarSharedNossl target to build libpulsarnossl.so

* update doc

* verify 4 libs in rpm/deb build, installed, use all good

(cherry picked from commit 33eea88)
tuteng pushed a commit that referenced this pull request Apr 13, 2020
…rpm (#6458)

Fix #6439 
We shouldn't static link libssl in libpulsar.a, as this is a security red flag. we should just use whatever the libssl the system provides. Because if there is a security problem in libssl, all the machines can just update their own libssl library without rebuilding libpulsar.a.
As suggested, this change not change the old behavior, and mainly provides 2 other additional pulsar cpp client library in deb/rpm, and add related docs of how to use 4 libs in doc.
The additional 2 libs: 
- pulsarSharedNossl (libpulsarnossl.so), similar to pulsarShared(libpulsar.so), with no ssl statically linked.
- pulsarStaticWithDeps(libpulsarwithdeps.a), similar to pulsarStatic(libpulsar.a), and archived in the dependencies libraries of `libboost_regex`,  `libboost_system`, `libcurl`, `libprotobuf`, `libzstd` and `libz` statically.

Passed 4 libs rpm/deb build, install, and compile with a pulsar-client example code.

* also add libpulsarwithdeps.a together with libpulsar.a into cpp client release

* add documentation for libpulsarwithdeps.a, add g++ build examples

* add pulsarSharedNossl target to build libpulsarnossl.so

* update doc

* verify 4 libs in rpm/deb build, installed, use all good

(cherry picked from commit 33eea88)
jerrypeng pushed a commit to jerrypeng/incubator-pulsar that referenced this pull request May 15, 2020
…rpm (apache#6458)

Fix apache#6439
We shouldn't static link libssl in libpulsar.a, as this is a security red flag. we should just use whatever the libssl the system provides. Because if there is a security problem in libssl, all the machines can just update their own libssl library without rebuilding libpulsar.a.
As suggested, this change not change the old behavior, and mainly provides 2 other additional pulsar cpp client library in deb/rpm, and add related docs of how to use 4 libs in doc.
The additional 2 libs:
- pulsarSharedNossl (libpulsarnossl.so), similar to pulsarShared(libpulsar.so), with no ssl statically linked.
- pulsarStaticWithDeps(libpulsarwithdeps.a), similar to pulsarStatic(libpulsar.a), and archived in the dependencies libraries of `libboost_regex`,  `libboost_system`, `libcurl`, `libprotobuf`, `libzstd` and `libz` statically.

Passed 4 libs rpm/deb build, install, and compile with a pulsar-client example code.

* also add libpulsarwithdeps.a together with libpulsar.a into cpp client release

* add documentation for libpulsarwithdeps.a, add g++ build examples

* add pulsarSharedNossl target to build libpulsarnossl.so

* update doc

* verify 4 libs in rpm/deb build, installed, use all good
jiazhai added a commit to jiazhai/pulsar that referenced this pull request May 18, 2020
…rpm (apache#6458)

Fix apache#6439
We shouldn't static link libssl in libpulsar.a, as this is a security red flag. we should just use whatever the libssl the system provides. Because if there is a security problem in libssl, all the machines can just update their own libssl library without rebuilding libpulsar.a.
As suggested, this change not change the old behavior, and mainly provides 2 other additional pulsar cpp client library in deb/rpm, and add related docs of how to use 4 libs in doc.
The additional 2 libs:
- pulsarSharedNossl (libpulsarnossl.so), similar to pulsarShared(libpulsar.so), with no ssl statically linked.
- pulsarStaticWithDeps(libpulsarwithdeps.a), similar to pulsarStatic(libpulsar.a), and archived in the dependencies libraries of `libboost_regex`,  `libboost_system`, `libcurl`, `libprotobuf`, `libzstd` and `libz` statically.

Passed 4 libs rpm/deb build, install, and compile with a pulsar-client example code.

* also add libpulsarwithdeps.a together with libpulsar.a into cpp client release

* add documentation for libpulsarwithdeps.a, add g++ build examples

* add pulsarSharedNossl target to build libpulsarnossl.so

* update doc

* verify 4 libs in rpm/deb build, installed, use all good
(cherry picked from commit 33eea88)
huangdx0726 pushed a commit to huangdx0726/pulsar that referenced this pull request Aug 24, 2020
…rpm (apache#6458)

Fix apache#6439 
We shouldn't static link libssl in libpulsar.a, as this is a security red flag. we should just use whatever the libssl the system provides. Because if there is a security problem in libssl, all the machines can just update their own libssl library without rebuilding libpulsar.a.
As suggested, this change not change the old behavior, and mainly provides 2 other additional pulsar cpp client library in deb/rpm, and add related docs of how to use 4 libs in doc.
The additional 2 libs: 
- pulsarSharedNossl (libpulsarnossl.so), similar to pulsarShared(libpulsar.so), with no ssl statically linked.
- pulsarStaticWithDeps(libpulsarwithdeps.a), similar to pulsarStatic(libpulsar.a), and archived in the dependencies libraries of `libboost_regex`,  `libboost_system`, `libcurl`, `libprotobuf`, `libzstd` and `libz` statically.

Passed 4 libs rpm/deb build, install, and compile with a pulsar-client example code.

* also add libpulsarwithdeps.a together with libpulsar.a into cpp client release

* add documentation for libpulsarwithdeps.a, add g++ build examples

* add pulsarSharedNossl target to build libpulsarnossl.so

* update doc

* verify 4 libs in rpm/deb build, installed, use all good
BewareMyPower added a commit to BewareMyPower/pulsar-client-cpp that referenced this pull request Oct 9, 2022
### Motivation

See discussions here: apache#28 (comment)

The original purpose to not include static OpenSSL library in
`libpulsarwithnossl.so` and `libpulsarwithdeps.a` is
apache/pulsar#6458. However, the ABI
compatibility of OpenSSL is not good. If the Pulsar C++ library links
dynamically to OpenSSL and the user only changes the OpenSSL dynamic
library, some symbols might not be found.

### Modifications

Use `LINK_STATIC` option to determine whether to link to OpenSSL library
statically. After that, there are only 3 libraries generated when
`LINK_STATIC` is ON.
- `libpulsar.so`: the dynamic pulsar-client-cpp library.
- `libpulsar.a`: the static pulsar-client-cpp library. When it's used,
  users must link to all 3rd party dependencies (OpenSSL, curl, etc.)
- `libpulsarwithdeps.a`: the static pulsar-client-cpp library.
merlimat pushed a commit to apache/pulsar-client-cpp that referenced this pull request Oct 10, 2022
* Link to OpenSSL statically when LINK_STATIC is ON

### Motivation

See discussions here: #28 (comment)

The original purpose to not include static OpenSSL library in
`libpulsarwithnossl.so` and `libpulsarwithdeps.a` is
apache/pulsar#6458. However, the ABI
compatibility of OpenSSL is not good. If the Pulsar C++ library links
dynamically to OpenSSL and the user only changes the OpenSSL dynamic
library, some symbols might not be found.

### Modifications

Use `LINK_STATIC` option to determine whether to link to OpenSSL library
statically. After that, there are only 3 libraries generated when
`LINK_STATIC` is ON.
- `libpulsar.so`: the dynamic pulsar-client-cpp library.
- `libpulsar.a`: the static pulsar-client-cpp library. When it's used,
  users must link to all 3rd party dependencies (OpenSSL, curl, etc.)
- `libpulsarwithdeps.a`: the static pulsar-client-cpp library.

* Remove unnecessary code

* Remove libpulsarnossl.so build
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

libssl.so should be linked dynamically into the libpulsar.a
3 participants