|
ZBX-15963, ZBX-16300, ZBX-16692 are related.
|
|
This only seems logical that you'd want to base the code action based on what the running environment reflects. This would save customers the extra hurdle of having to compile from source to use elasticsearch.
|
|
In addition, from libcurl tutorial:
Features libcurl Provides
It is considered best-practice to determine libcurl features at run-time rather than at build-time (if possible of course). By calling curl_version_info and checking out the details of the returned struct, your program can figure out exactly what the currently running libcurl supports.
|
|
Implementing this would avoid having issues like ZBX-22946
|
|
Especially so because now some distributions ship smaller libcurl-minimal packages, that miss many cURL features.
|
|
Related issue: ZBXNEXT-1896
|
|
An example of curl_version_info usage to detect if run-time cURL library supports "smtp" and "smtp" protocols required for sending "non-plain text" mail. Instead of "Unsupported protocol" user will see an error message like cURL library was compiled without support for "smtps" protocol:
ZBXNEXT-3623-zabbix-7.0.0alpha9-curl-protocols.patch
In case of "plain-text" (no SSL, no auth) Emil media type Zabbix does not use cURL but sends e-mail using TCP protocol.
|
Mini spec
In short, what needs to be done:
- move cURL feature and protocol checks from auto-tools (build-time) to the code (run-time):
- move other cURL version checks from auto-tools (build-time) to the code (run-time):
- it is also required to add few wrapper functions to:
- get cURL version
- check if protocol is supported
- check if feature is supported
In addition it is suggested to increase curl version requirement to 7.19.0 or later. It was lowered to 7.13.1 in ZBX-15963 because we supported RHEL 5. In 7.0 we do not support it anymore and RHEL 6 has libcurl version 7.19 .
Probably all the places that need to be checked:
Approvals on the spec:
|
|
This report conflates features and interfaces.
The first test, # if 0x071004 >= LIBCURL_VERSION_NUM, works around the fact that the CURLOPT_SSLKEYPASSWD option was renamed to CURLOPT_KEYPASSWD in 7.16.4, and itself was a rename of CURLOPT_CERTPASSWD in 7.9.2. This is a pre-processor symbol, which is obviously not something that can be decided upon in runtime. This is done to avoid #ifdef dances at call sites, as one, and only one, path will ever be valid.
The same applies to the tests for CURLOPT_ACCEPT_ENCODING (which was called CURLOPT_ENCODING prior to 7.21.6), and the CURLOPT_SPEED_DOWNLOAD* symbols, which is due to CURLOPT_SPEED_DOWNLOAD being deprecated (not renamed) in favor of CURLOPT_SPEED_DOWNLOAD_T, which returns the result as a different type than the former (curl_off_t vs double), and again, is done to avoid duplicating code at call sites.
These are interface issues, and their discrepancies need to be resolved during the pre-processing stage, as we'll fail compilation otherwise (missing symbols, data type mismatching).
The only test that is out of place is the SMTP auth one, but only due to the way it is implemented (should be done via autoconf, which is what it is for, and not this odd version check in a header). In this case, the correct way to deal with this should be via attempting to set the CURLOPT_PROTOCOLS_STR option on the transfer handle to the protocol(s) we're interested in (SMTP and SMTPS in this case), and checking the return value from the curl_easy_setopt() call for the CURLE_UNSUPPORTED_PROTOCOL value.
As for run-time feature detection - all libcurl functions returning a CURLcode type can return CURLE_NOT_BUILT_IN, to indicate that support for a particular protocol or feature was not built into the linked libcurl. This is the most straight forward and robust manner to do this.
Another way to detect feature presence would be to interrogating the curl_version_info_data structure, as returned by a call to curl_version_info(), but due to the structure member presence being version dependent, this could be unreliable. Most notably, in the case when the struct definition during compile-time comes from an older version of libcurl than the version linked during run-time, making the later added members inaccessible to the application/wrapper library (admittedly, this would be an unusual situation). In my opinion, this is an inferior solution to just checking for CURLE_NOT_BUILT_IN, as this would require all handle setup code to have a preamble performing these checks, resulting in sprinkling functions with them, instead of just trying to do what needs to be done and failing in a straight forward manner.
Overall, I have to concede that the libcurl wrapping code is a hot mess of hacks and should be refactored, but that is not really in the scope of this particular issue.
|
This report conflates features and interfaces.
What do you mean? I would say, that it is Zabbix code that conflates features and interfaces.
The fact, that code is compiled with libcurl headers >= 7.55 (and has CURLOPT_SPEED_DOWNLOAD_T defined) does not mean that it will run with libcurl shared library >= 7.55 and using CURLOPT_SPEED_DOWNLOAD_T is the best approach.
I didn't say it explicitly in the issue Description, but I meant that in case Zabbix code does not have a libcurl header that defines CURLOPT_SPEED_DOWNLOAD_T, Zabbix code should define this constant itself using a value looked up in the latest libcurl headers, e.g. (based on https://github.com/curl/curl/blob/524253dc9078e69300c5390eda496603067f325f/include/curl/curl.h):
# if 0x073700 <= LIBCURL_VERSION_NUM
# define ZBX_CURLINFO_SPEED_DOWNLOAD_T CURLINFO_SPEED_DOWNLOAD_T
# define ZBX_CURLINFO_SPEED_DOWNLOAD CURLINFO_DOUBLE + 9
# else
# define ZBX_CURLINFO_SPEED_DOWNLOAD_T CURLINFO_OFF_T + 9
# define ZBX_CURLINFO_SPEED_DOWNLOAD CURLINFO_SPEED_DOWNLOAD
# endif
At runtime I would expect Zabbix to try in turn both ZBX_CURLINFO_SPEED_DOWNLOAD_T and ZBX_CURLINFO_SPEED_DOWNLOAD before failing.
Of course, it is sufficient to check return values at call sites ("ask for forgiveness" and all that jazz), but it would be also nice and user-friendly to fail/detect issues as soon as possible. Over the years Zabbix already did the research, which versions of libcurl support what. curl_version_info_data is guaranteed to contain version number. It is a matter of a simple if on Zabbix startup to see if a certain libcurl-based functionality is supposed to work.
|
|
Hey, cyclone!
Allow me to clarify my earlier statement that "This report conflates features and interfaces". What I meant was runtime-features and source-interfaces.
These symbols (for example, CURLINFO_SPEED_DOWNLOAD_T) are resolved during the preprocessing stage, and depending on the version of the libcurl development header and library version on the build system, one or the other symbol may not be present; hence the conditional definition of ZBX_CURLINFO_SPEED_DOWNLOAD_T. This may be the case when the development header comes from a version of libcurl that didn't yet define CURLINFO_SPEED_DOWNLOAD_T (anything prior to 7.55.0), or the opposite case, when building with a header that no longer ships the old symbol (CURLINFO_SPEED_DOWNLOAD). The default preference is to use the newer option, if present. This is that best approach in action.
I'm not sure what you mean by the following:
The fact, that code is compiled with libcurl headers >= 7.55 (and has CURLOPT_SPEED_DOWNLOAD_T defined) does not mean that it will run with libcurl shared library >= 7.55 and using CURLOPT_SPEED_DOWNLOAD_T is the best approach.
As you yourself previously mentioned, libcurl's ABI is very stable, which is exactly why the above allows Zabbix to work with arbitrary versions of libcurl linked at runtime - newer or older. The immediate example of that here are the CURLOPT_CERTPASSWD / CURLOPT_SSLKEYPASSWD / CURLOPT_KEYPASSWD options, which changed names, but reused the value, to stay ABI compatible. Moreover, a library header version >= 7.55.x is much less likely to need as many of these wrappers to account for source interface changes than something like 7.13.1, which is the current minimum supported version. As soon as we raise the minimum version requirement, they should be removed.
Regarding:
I didn't say it explicitly in the issue Description, but I meant that in case Zabbix code does not have a libcurl header that defines CURLOPT_SPEED_DOWNLOAD_T, Zabbix code should define this constant itself using a value looked up in the latest libcurl headers, e.g. (based on https://github.com/curl/curl/blob/524253dc9078e69300c5390eda496603067f325f/include/curl/curl.h): [...] At runtime I would expect Zabbix to try in turn both ZBX_CURLINFO_SPEED_DOWNLOAD_T and ZBX_CURLINFO_SPEED_DOWNLOAD before failing.
As for the definition of the CURLINFO_SPEED_DOWNLOAD_T being done by Zabbix itself – I'm not entirely sure why we would want to do that. The development library installation already provides the header, and having a copy of a fragment, that then requires maintenance to stay in sync with upstream's choice of defining the symbol, is simply unnecessary. This is why we define the ZBX_CURLINFO_* symbols to whatever they resolve to, and only work around the fact that one or the other symbol may or may not be defined by the header.
To the second portion of the quote - this only applies to symbols that have been deprecated and not removed (as is the case with CURLINFO_SPEED_DOWNLOAD_T). We would still have to work with the fact that the installed library may be older than the runtime, and thus be unaware of the new option (as that symbol wasn't defined for the preprocessor) and type(s) (which were not defined by the interface header).
However, I agree that this is a good idea, and a case for which it could, or should, implement an attempt at fallback to the previous version of an option, if we intend to support a larger range of libcurl versions.
IMHO, at this point, I would consider creating a wrapper (i.e. libzbxcurl) for libcurl, that would perform the feature and option detection as needed, depending on the option requested. As the number of these cases is small, the wrapper would be terse and compact, and most cases would be mostly direct parameters passing. This would
- allow abstracting away these preproccesor symbol and type details,
- simplify Zabbix library and application code,
- and provide an extended interface to libraries and applications
- we kind of need to, as a result of the type abstraction
This will not, however, resolve the fact that the user code will still be dependent on what symbols were provided by the development header, and not all cases will always be covered.
|
allows Zabbix to work with arbitrary versions of libcurl linked at runtime - newer or older
Apparently, your definition of "work" is very generous. 
I would advice you to read through ZBX-11629 to get a taste of how Zabbix "works with arbitrary versions of libcurl". In short:
- Zabbix SIA shipped packages compiled with ancient libcurl headers (hard to tell exact version, but apparently something older than 7.20).
- User installed these packages.
- User realized, that packages do not support SMTP.
- User read somewhere in Zabbix documentation, that to support SMTP Zabbix requires newer libcurl.
- User updates libcurl to 7.51 (which, theoretically, should be more than enough).
- SMTP still does not work.
- User is... confused, to put it mildly.
If you would compile Zabbix with newer libcurl headers and then downgrade libcurl, the situation would be symmetric, but equally bad - Zabbix would claim that SMTP is supported, but would fail to send emails.
Zabbix only "works with arbitrary versions of libcurl" only if you are ready to compile Zabbix from source yourself. If you upgrade or downgrade libcurl after compiling Zabbix, Zabbix will not adjust.
The whole point of this feature request is to make Zabbix aware of libcurl version that is currently loaded. Zabbix should be able to work with arbitrary versions of libcurl without recompilation.
|
The development library installation already provides the header, and having a copy of a fragment, that then requires maintenance to stay in sync with upstream's choice of defining the symbol, is simply unnecessary.
It is necessary for cases when available development headers are too old to too new to provide all potentially useful constants.
Speaking of staying in sync, I don't see a problem there.
- libcurl maintainers have no way of changing numeric values of the constants without affecting compatibility between versions.
- libcurl maintainers value compatibility very high and successfully maintain a stable ABI.
In my opinion it is safe to conclude, that numeric values of the constants will not change in the foreseeable future, hence there will be no need to sync them.
|
|
Hi, cyclone.
Regarding your first bullet point in your first comment:
- Zabbix SIA shipped packages compiled with ancient libcurl headers (hard to tell exact version, but apparently something older than 7.20).
We build against the version of libcurl development library as shipped by the distributions we build packages for. If the user chooses to install the package on a system that ships a different version and feature set of a library, the user should expect discrepancies.
What you're asking, in brief, requires that we
- declare a minimum supported version of libcurl,
- build with a newer (or latest) version of libcurl,
- maintain support for use of deprecated and/or removed option macros starting from the minimum supported version.
We do (1) already, as the autoconf generated configure will refuse to generate the build system, if the minimum version is met. There is the issue that the current minimum version requirement is out of date, and needs to be updated, as we unconditionally rely on interfaces only available starting with 7.19.2. This is captured in ZBX-22649.
(2) is not free. For a start, some policy needs to be in place, deciding the upstream channel we intend to build against; most likely, a choice between current development and latest stable release. I personally would opt for stability, to have prep time for addressing any breaking changes that may occur in development.
We generally don't practice merging individual artifacts from our dependency upstream (with some key exceptions), nor would we want to, as it introduces additional maintenance burden, and is a clunky process to go about in general. This would thus require updating the build environment creation and possibly the build procedure itself, to include the building of the development library to be used in the building of Zabbix libraries and applications themselves. It is generally the responsibility of whomever builds the package to provide the build process with the appropriate build environment for their configuration.
I'm not against this, and believe this to be a generally sound approach, as it would allow the user to always make use of the most recent stable version features of the library, be it libcurl or any other library. This is also technically viable. I suppose the deciding factor here is work resource. I think it would be worthwhile consider this as a generally policy for all external library dependencies, where viable.
(3) we already do, except that the driver for introducing new abstractions are the versions provided by the distributions we build packages for, and not builds of any latest versions (see (2)).
Regarding your second comment:
Again, I think it to be unnecessary to maintain a copy of the interface header, as it is already provided by a built development library package, and (2) from the previous section would solve this issue, as the header used during pre-processing would not really be "too new" ever again. The additional necessary work I see this requiring is:
- to introduce a runtime check on the version of the dynamically linked libcurl , to enforce the minimum required version, matching the autoconf declaration,
- handle protocol and option support detection in runtime.
(1) requires further elaboration on when and where to do it, but it's safe to say this is viable as well. Makes me lean even more towards introducing a libzbxcurl for handling all these issues. (2) can be solved via CURLE_UNSUPPORTED_PROTOCOL and CURLE_NOT_BUILT_IN handling as I mentioned previously. Again, to reiterate, I see this as the best option.
In essence, to resolve the issues you mentioned, and avoid breaking user expectations as to feature availability, we should do (2) from this posts response to your first comment, and (2) from the second one. Both are technically viable and I personally agree with the final benefit.
|
|
Fixed in development branch
|
|
Great cURL page where you can seek information about every public symbol and cURL version in which it was added:
https://curl.se/libcurl/c/symbols-by-name.html
|
|
(5) Found one case where the feature was decided at build-time, file src/libs/zbxhttp/http.c:
Need to change that in run-time detection of CURLH.
<dimir> RESOLVED in 352b97a917d
<yurii> CLOSED
|
Overview
TL;DR
This changes the way Zabbix detects curl library features when working with it.
Before this change:
- Zabbix detected curl library features at build time
After this change:
- Zabbix detects curl library features at runtime
More details
Now when you e. g. upgrade your curl library that has new features these features will be available in Zabbix upon restart. Re-compilation of Zabbix is no longer required. This is true for the following Zabbix components:
In addition, what was changed:
- Minimum required build-time curl version was raised to 7.19.1
- When being built Zabbix just checks if curl library is available (if requested) and satisfies the version requirement
- When issuing a runtime curl library error the version being in use will be added (e. g. curl library does not support SSL/TLS (using version 7.88.1))
- When Zabbix server is started and the SMTP authentication is logged it will write availability of SMTP authentication of cURL library being used
Fixed in
|
|
Updated documentation:
|
Generated at Sun Apr 20 21:22:19 EEST 2025 using Jira 9.12.4#9120004-sha1:625303b708afdb767e17cb2838290c41888e9ff0.
ZBXNEXT-3623-zabbix-7.0.0alpha9-curl-protocols.patch
Team A
while API is being actively enhanced.