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.

Sign up for GitHub

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

Adds opentdf-client versions 1.4.0 and 1.5.0 #18896

Merged
merged 23 commits into from
Aug 17, 2023
Merged

Adds opentdf-client versions 1.4.0 and 1.5.0 #18896

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
a30a8ed
Adds versions 1.4.0 and 1.5.0
Jul 25, 2023
91a29d5
Need magic_enum for 1.4.0 also
Jul 25, 2023
abbaa72
Merge branch 'master' into opentdf-client-version-1.5.0
AbrilRBS Aug 10, 2023
1b9cb5a
Update to new release content
Aug 10, 2023
449351b
Merge branch 'opentdf-client-version-1.5.0' of github.com:patmantru/c…
Aug 10, 2023
fd6ce0e
Merge branch 'master' into opentdf-client-version-1.5.0
patmantru Aug 10, 2023
628a83e
Update to newest release tag content
Aug 10, 2023
e890d3f
Merge branch 'master' into opentdf-client-version-1.5.0
patmantru Aug 10, 2023
80a3261
Merge branch 'opentdf-client-version-1.5.0' of github.com:patmantru/c…
Aug 10, 2023
82c2a3f
Merge branch 'master' into opentdf-client-version-1.5.0
AbrilRBS Aug 11, 2023
32082cd
Change OpenSSL version references to ranges per CCI requirements
Aug 11, 2023
d27b840
fix typo
Aug 11, 2023
122bfa5
Try different range spec for 1.1.x
Aug 11, 2023
fd21b7f
Try another range spec for openssl 1.1.x
Aug 11, 2023
50342a7
Another try at version ranges
Aug 11, 2023
813e0df
Still another try at ranges for openssl 1.1.x
Aug 11, 2023
56cfc6d
Pick a version range that is also compatible with jwt-cpp
Aug 11, 2023
edce36d
openssl/[>=3.1 <3.2] range is not compatible with jwt-cpp. Use fixed…
Aug 11, 2023
e7593af
One more try with newer jwt-cpp and ranges
Aug 14, 2023
667d4c6
Avoid jwt-cpp complaint about OpenSSL 1.1.1u
Aug 14, 2023
1b273e1
Override 3.1.1 for openssl to avoid jwt-cpp complaint
Aug 14, 2023
ebb08c7
jwt-cpp requires are broken forr 0.5.0 and 0.6.0, avoid
Aug 14, 2023
61d8cf7
opentdf-client: update openssl version ranges
jcar87 Aug 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions recipes/opentdf-client/all/conandata.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,10 @@
sources:
"1.5.0":
url: "https://github.com/opentdf/client-cpp/archive/1.5.0.tar.gz"
sha256: "c0b5f0732742a539c6a4bf5e720da4cf5c318bb069cef69789dce91c9261e8a2"
"1.4.0":
url: "https://github.com/opentdf/client-cpp/archive/1.4.0.tar.gz"
sha256: "0ef9cdf5e49f83a8ddcde10d64cdf9108652e781396cfab000f50cc067e6f795"
"1.3.10":
url: "https://github.com/opentdf/client-cpp/archive/1.3.10.tar.gz"
sha256: "539bd5e64bceb86f63b3f7db75de470d5ea1d52ae6436a6a2d6789f7d0710dd4"
Expand Down
11 changes: 10 additions & 1 deletion recipes/opentdf-client/all/conanfile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,14 @@ def validate(self):
raise ConanInvalidConfiguration(f'{self.name} can not be built with MT or MTd at this time')

def requirements(self):
self.requires("openssl/1.1.1q")
# Uses openssl 3.x for 1.5.0 and newer
if Version(self.version) >= "1.5.0":
self.requires("openssl/3.1.1")
else:
self.requires("openssl/1.1.1q")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @patmantru, thanks a lot for the contribution. One comment: we are now accepting version ranges for openssl to avoid overrides: https://github.com/conan-io/conan-center-index/blob/master/docs/adding_packages/dependencies.md#version-ranges
So maybe you can use one for both those requires.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion, but it's not going to work in this case. Our code requires the OpenSSL 1.x interface for 1.4.0-and-older versions. The changes to support OpenSSL 3.x were introduced in our version 1.5.0. I need to leave that check as-is.

Copy link
Contributor

@czoido czoido Jul 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @patmantru, I may have not explained it properly, I mean that you can use separate version ranges for each Version(self.version) so that you avoid the need of doing overrides in other libraries that may be required in the same graph. Something similar to this:

if Version(self.version) >= "1.5.0":
    self.requires("openssl/[>=3.1 <4]")
else:
    self.requires("openssl/[>=1.1 <3]")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I understand now. The issue I have (especially for the 1.1.x stuff) is that I don't want someone to accidentally pull in a random version that we don't know anything about. Case in point, there are vulnerabilities in the 1.1.1[a-z] earlier versions that are fixed in later ones, so I'd rather go with a fixed version. Unless there's a strict CCI requirement that I change this, I'd really prefer to leave it as-is. Having a repeatable build (identical binaries every time you build it) is much more important to me for this package.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@czoido any additional feedback?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have been building this using a local recipe, and it works fine using pinned versions. Now that I am specifying a version range...the opentdf-client version 1.5.0 that includes the OpenSSL 3.1.1 upgrade changes fails because jwt-cpp disallows it. Going back to pinned versions to see if that works.

Copy link
Contributor Author

@patmantru patmantru Aug 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jcar87 After 12 (!!!) attempts and the better part of 2 full work days, I have this PR building.

Notes:
jwt-cpp has its requirement for OpenSSL set as the range [>1.1.1c, <1.1.1u] (note the less-than on the upper limit) which means any range spec that pulls in something newer than 1.1.1t will fail. The opentdf-client package uses OpenSSL 1.1.1 for releases up through 1.4.0. Since the current 1.x version is 1.1.1u, setting a range spec of [>1.1.1q <1.2] will pick 1.1.1u, which will fail jwt-cpp's range check. As a concession to your request, even though it is actually propagating the problem, I've added [>1.1.1q <1.1.1u] as the range for opentdf-client 1.4.0 and older, just so I can say I did my best here.

Similarly, specifying an OpenSSL 3.1.x version range for my release 1.5.0 recipe will fail, because it's ALSO not within jwt-cpp's [>1.1.1c, <1.1.1u] range check.

The ONLY WAY TO GET MY RELEASE 1.5.0 RECIPE FOR TO BUILD CLEANLY given the jwt-cpp recipe, is to pin the OpenSSL version as 3.1.1, which overrides the jwt-cpp range check. This is evident from looking at the 11 previous build attempts that all failed for one range-check reason or another.

I think I've done more than my share of attempting to comply with your request to use version ranges for OpenSSL. I've proven that it is not possible in this situation given all the constraints. All I wanted to do was to publish our release 1.4.0 and 1.5.0 recipe updates. That was supposed to be a couple of new SHA256 lines, and bumping some requirement versions. Now this has dragged on for weeks, and it's still not in.

Please approve this so it can be merged and I can move on to more important things...like upgrading the recipe to be conan 2.x compliant.

Thanks-
Pat

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As someone developing this code for a data protection company, security is job 1. That includes build repeatability. That means knowing exactly what is being used. That means version pinning. Everything.

Absolutely agreed, this is important, and possible with Conan Center and where we are headed. I think we have a disagreement as to where this version pinning should be happening. We want to make sure that it is consumers that are able to do version pinning. This is the purpose of lockfiles. This is a good summary of recommendations, that places the responsibility on consumers to be aware and known their entire dependency tree - we give them the options to both pin the version and update to a newer one if necessary, provided the newer one is known to be compatible (as is the case with the OpenSSL 3.x series).

That makes me sad, as I had hoped CCI would be like (as you mentioned) Pypi/Cargo/npm, a central 'this is the gold standard for package recipes' for C++ modules.

on the one hand, you have hopes that CCI would be like Pypi/Cargo/npm, but on the other hand these tools and repositories use version ranges by default, which you are arguing against - Some example

  • These are the requirements of boto3, the top most downloaded package in PyPI - notice how each of them uses a version range, as we are advocating to use in Conan Center for OpenSSL.
  • Cargo for Rust uses version ranges by default, even when a full version is specified - this is because it assumes SemVer compatibility, same as is the case for OpenSSL.
  • The semver npm package (which has 276 million weekly downloads) lists has a dependency that uses version ranges: https://github.com/npm/node-semver/blob/main/package.json#L51 - the ^ notation here is also exactly equivalent as what we are doing for OpenSSL.

In all these cases, it means that users doing pip install, npm install or cargo builds would get a combination of versions today, and will very likely get a completely different one if they run the same commands 2 weeks from now. In all of these cases there are well described mechanisms to lock the version resolver to specific versions:

The practices that we are proposing for Conan Center are in line with widely adopted practices elsewhere. Conan is no differente: https://docs.conan.io/2.0/tutorial/versioning/lockfiles.html

The CCI focus now seems to be on doing whatever is necessary to reduce the volume of recipe updates

This is categorically untrue.

including floating versions 'on some packages' to force more of the builds to work without intervention. 'Flexibility' is not always better.

The "some packages" is for packages that have known compatibility promises. For libraries that don't, we won't be using them. Widely adopted practice elsewhere.

then can I upload them to CCI with the recipe and ensure they are used when the recipe is referenced by a consumer? Guessing not, because that would defeat CCI's 'flexibility' goal.

While there are some differences, Cargo explains this better than I can: https://doc.rust-lang.org/cargo/faq.html#why-do-binaries-have-cargolock-in-version-control-but-not-libraries

We want the end consumers to be in control - they depend on OpenSSL, they should control which version, provided all the libraries they are using are compatible.

As I mentioned in another issue (#13994), it seems like the CCI guidance continues to be 'Please submit your recipes to CCI, but you should really make a local copy of everything and use that instead of CCI because we're unreliable', which falls far short of what CCI could be.

The PR is still open and subject to change. The advise is, and always has been for Conan Center, to avoid consuming recipes and binaries from Conan Center in a fully unconstrained manner. The advice (especially with Conan 2.0) would be to use lockfiles, and only update the lockfiles when you need to update your dependencies - then you can inspect and solve conflicts if needed or fix compatibility issues. It shouldn't happen "from under you" - this is in line with the recommendations for python packages, npm, cargo, and so on.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notably, the recipe for jwt-cpp specs "openssl/[>1.1.1c, <1.1.1u]", which means my specification of [>=1.1 <=1.2] fails because 1.1.1u (most recent openSSL version) is not acceptable to jwt-cpp. Rather than picking 1.1.1t, which would be acceptable to both my package and jwt-cpp, it just fails. This is like playing whack-a-mole, it moves the versioning problem instead of fixing it.

Thanks for reporting this. openssl/[>1.1.1c, <1.1.1u] is not accepted range in Conan Center. We have discovered there is a bug with the bump dependencies automerge - and the team is working to fix this. I have opened a PR to address this: #19220

I apologise you experienced this issues, I can understand this has been frustating.

The ONLY WAY TO GET MY RELEASE 1.5.0 RECIPE FOR TO BUILD CLEANLY given the jwt-cpp recipe, is to pin the OpenSSL version as 3.1.1, which overrides the jwt-cpp range check.

Unfortunately this is a side effect of the openssl/[>1.1.1c, <1.1.1u] dependency in the jwt-cpp recipe - this is malformed and should have not been merged. The team is looking into this to avoid this in the future. in Conan 2.0 this is interpreted as openssl/[>1.1.1c], hence why it works when you pin to 3.1.1: in Conan 1 it does an override, and in Conan 2.0 it just matches the range.

Please note that this silent overrides (as experienced in Conan 1.x) are no longer a thing in Conan 2.0: if jwt-cpp were only compatible with OpenSSL 1.x and opentdf-client is only compatible with OpenSSL 3.x onwards (or in the case where both depend on a specific version), I'm fairly certain this would result in a conflict immediately, and there is no override. Version ranges avoid the conflicts when things are known to be compatible.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are fixing this issue here: #19220 and will make sure opentdf-client is relaunched and merged today!

# Uses magic_enum for 1.4.0 and newer
if Version(self.version) >= "1.4.0":
self.requires("magic_enum/0.8.2")
self.requires("ms-gsl/2.1.0")
self.requires("nlohmann_json/3.11.1")
self.requires("jwt-cpp/0.4.0")
Expand Down Expand Up @@ -121,3 +128,5 @@ def package_info(self):
self.cpp_info.components["libopentdf"].requires = ["openssl::openssl", "boost::boost", "ms-gsl::ms-gsl", "libxml2::libxml2", "jwt-cpp::jwt-cpp", "nlohmann_json::nlohmann_json"]
if Version(self.version) < "1.1.0":
self.cpp_info.components["libopentdf"].requires.append("libarchive::libarchive")
if Version(self.version) >= "1.4.0":
self.cpp_info.components["libopentdf"].requires.append("magic_enum::magic_enum")
4 changes: 4 additions & 0 deletions recipes/opentdf-client/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
versions:
"1.5.0":
folder: all
"1.4.0":
folder: all
"1.3.10":
folder: all
"1.3.9":
Expand Down