Drop Mozilla LDAP SDK detection, update --with-ldap code #1710

yadij · 2024-02-27T20:14:41Z

Mozilla has not maintained their LDAP SDK since 2017:
https://wiki.mozilla.org/LDAP_C_SDK#Latest_News_-_10.2F13.2F2017

Admin needing to use that SDK can ./configure Squid with:

--with-ldap=/path/to/sdk CPPFLAGS=-I/path/to/sdk/mozldap

kinkie · 2024-02-28T12:05:01Z

Ok to test

configure.ac

kinkie · 2024-02-29T09:33:25Z

It seems that centos-8 and centos-stream-8 do not ship openldap's pkg-config file :\

rousskov · 2024-02-29T14:11:42Z

It seems that centos-8 and centos-stream-8 do not ship openldap's pkg-config file :\

AFAICT,

CentOS Linux 8 reached EOL on December 31st, 2021.
CentOS Stream 8 will stop being maintained in May 2024.

Perhaps we can treat LDAP SDKs on those two platforms the same way this PR currently treats Mozilla LDAP SDK (i.e. document how to build with those (arguably deficient) SDKs instead of hard-coding those instructions into Squid master/v7)?

kinkie · 2024-02-29T14:16:21Z

That's a fair point; should we drop CentOS 8 as a target platform now?
RHEL 8 is still supported

rousskov · 2024-02-29T15:02:51Z

should we drop CentOS 8 as a target platform now?

I do not know what that would mean, so I cannot answer that question. My comment was specific to Squid LDAP auto-detection code and to the desire to make that specific automation/convenience feature require pkg-config files (among other "standard" things in modern environments), assuming PR Squid can still build (using custom ./configure parameters) with LDAP support on all the exceptional/older platforms where official Squid builds with LDAP support now (by default).

yadij · 2024-03-01T01:56:30Z

Yes to Dropping CentOS 8 - should have happened with v6 release.

I am on the fence about the "Stream" flavours since they are RHEL now which we already support separately.

kinkie · 2024-03-01T14:17:41Z

Yes to Dropping CentOS 8 - should have happened with v6 release.

I am on the fence about the "Stream" flavours since they are RHEL now which we already support separately.

Let me try to be clearer:
This PR fails merge testing becasue centos-8 and centos-stream-8 lack a proper pkg-config file for libldap.

This means that the only ways to not regress our merge testing are to drop both or to do some platform-specific shenanigans in our jenkins (and github actions, if we migrate to them fully or partly) harnesses.

Which should it be?

rousskov · 2024-03-01T14:35:42Z

This PR fails merge testing becasue centos-8 and centos-stream-8 lack a proper pkg-config file for libldap.

Our options include:

Stop requiring¹ LDAP support in CI testing (on any platform).
Stop requiring¹ LDAP support in CI testing (on CentOs 8² platforms).
Stop requiring¹ LDAP support in CI testing (on CentOs 8 CI nodes).
Provide custom ./configure options (or some such) to work around the lack of CentOS-provided package files (on CentOS 8 CI nodes).
Do not run CI tests on CentOs 8 platforms.

My preferences, from the most to the least preferred, are 1 ("universal"), 4 ("eat our own dog food"), and 5 ("simple").

There are several ways to do that, and we would need to agree on the best way, but it is obvious that we do not have to require that LDAP is supported on every platform we may be interested in for CI testing purposes. ↩ ↩² ↩³
In this message, "CentOS 8" stands for centos-8 and/or centos-stream-8, whatever is left in CI after this discussion. ↩

kinkie · 2024-03-04T11:38:38Z

I have retired centos-8 from the build farm.
Running test-builds.sh with LIBLDAP_LIBS='-lldap -llber' is successful on Centos Stream 8, but that's not the correct solution; we currently accept to not mandate tests for features that have external dependencies (see comments in layer-02-maximus), so what we need to do is to have a better detection of functional requirements to build helpers.

kinkie · 2024-03-04T11:45:37Z

Commit 558b388 implements the stricter requirements checking for helpers; it passes the build on Centos Stream 8 and 9; a diff of the build log output for the two releases is attached

maximus-build-log.diff.gz

kinkie · 2024-03-04T14:54:03Z

@yadij this seems good to go; I'll approve it after my own changes, but please do recheck it

Mozilla has not maintained their LDAP SDK since 2017: https://wiki.mozilla.org/LDAP_C_SDK#Latest_News_-_10.2F13.2F2017 Admin needing to use that SDK can ./configure Squid with: --with-ldap=/path/to/sdk CPPFLAGS=-I/path/to/sdk/mozldap

rousskov · 2024-03-04T15:51:24Z

doc/release-notes/release-7.sgml.in

+	<p>Removed special support for outdated Mozilla LDAP SDK.
+	   <em>--with-ldap=/path/to/sdk CPPFLAGS="-I/path/to/sdk/mozldap"</em>
+	   can be used if needed.


I do not insist on this polishing, but it may help avoid the false implication that Squid no longer works with Mozilla LDAP SDK:

Suggested change

Removed special support for outdated Mozilla LDAP SDK.

--with-ldap=/path/to/sdk CPPFLAGS="-I/path/to/sdk/mozldap"

can be used if needed.

Removed workarounds for outdated Mozilla LDAP SDK. Use

--with-ldap=/path/to/sdk CPPFLAGS="-I/path/to/sdk/mozldap"

to ./configure Squid to build with that SDK.

Implication is correct: Squid default builds will not work with Mozilla LDAP SDK after this patch is applied.

The custom path and CPPFLAGS are required for that LDAP SDK to be working at all.

Implication is correct: Squid default builds will not work with Mozilla LDAP SDK after this patch is applied.

The above implication is not the false implication I recommend avoiding in my change request.

Then why did you use those words? "false implication that Squid no longer works with Mozilla LDAP SDK"

Then why did you use those words? "false implication that Squid no longer works with Mozilla LDAP SDK"

I used the words I used because I believed they accurately described the problem. I still believe that they do.

There is obviously a difference between my description and the description in your response. The two phrases do not describe the same thing. The description in your response is about whether "default builds" work with Mozilla LDAP SDK. The concept of "support" goes beyond default builds, of course -- Squid may support X even though Squid default build does not support X. My description is about that general "support" concept.

src/acl/external/LDAP_group/required.m4

src/acl/external/kerberos_ldap_group/required.m4

Mozilla has not maintained their LDAP SDK since 2017: https://wiki.mozilla.org/LDAP_C_SDK#Latest_News_-_10.2F13.2F2017 Admin needing to use that SDK can ./configure Squid with: --with-ldap=/path/to/sdk CPPFLAGS=-I/path/to/sdk/mozldap

rousskov · 2024-03-05T15:51:07Z

configure.ac

 SQUID_AUTO_LIB(ldap,[LDAP],[LIBLDAP])
+dnl On MinGW set Windows LDAP libraries using -lwldap32
+AS_IF([test "x$with_ldap" != "xno" -a "$squid_host_os" = "mingw"],[
+  LIBLDAP_LIBS="$LIBLDAP_LIBS -lwldap32"


Feels wrong to change LIBLDAP_LIBS that was set by admin. Should this be conditional on LIBLDAP_LIBS being empty?

Maybe, for now I am leaving the logic inside this case as-was. Testing changes to it need some testing on MinGW which we are not quite up to doing (yet)

That excuse itself is very weak, but when combined with the fact that this PR does change the condition, it becomes invalid.

AFAICT, the PRs in this series routinely introduce untested changes. That is just the nature of this work, given our current CI limitations. IMO, we should not apply "need some testing before I modify this further" logic when dealing with changes that appear to violate basic rules. Given the two evils, I recommend increasing the risk of breaking MinGW build instead of committing such violations. These changes should be moving us forward as far as code quality is concerned. We can fix marginal builds later, as needed.

rousskov · 2024-03-05T15:55:18Z

configure.ac

+dnl On MinGW set Windows LDAP libraries using -lwldap32
+AS_IF([test "x$with_ldap" != "xno" -a "$squid_host_os" = "mingw"],[
+  LIBLDAP_LIBS="$LIBLDAP_LIBS -lwldap32"


Why do we have to place this special code outside of SQUID_CHECK_LIB_WORKS()?

To extract it for now outside the other logic. I would like to later (n the MinGW work) identify whether it is actually needed, or if we should have a completely separate library option+check for the Windows LDAP.

Alex: Why do we have to place this special code outside of SQUID_CHECK_LIB_WORKS()?

Amos: To extract it for now outside the other logic

That sentence does not appear to answer the "why" question. It essentially restates the fact accepted by the question.

Amos: I would like to later identify whether it is actually needed, or if we should have a completely separate library option+check for the Windows LDAP.

I see no reason to move this special code to accomplish the stated "identify the need" and associated possible refactoring goals. AFAICT, the move itself contradicts the intent behind SQUID_CHECK_LIB_WORKS() approach. We should not move unless there is a very compelling argument for doing so. No such argument has been provided AFAICT.

Remove Mosilla LDAP SDK support which has apparently not been maintained since 2017.

yadij · 2024-03-13T17:03:04Z

Replaced by PR #1736

rousskov reviewed Feb 28, 2024

View reviewed changes

configure.ac Outdated Show resolved Hide resolved

rousskov changed the title ~~Maintenance: update --with-ldap detection~~ Drop Mozilla LDAP SDK detection, update --with-ldap code Feb 29, 2024

yadij added the S-waiting-for-QA QA team action is needed (and usually required) label Mar 1, 2024

rousskov added the S-waiting-for-author author action is expected (and usually required) label Mar 1, 2024

kinkie removed the S-waiting-for-QA QA team action is needed (and usually required) label Mar 4, 2024

kinkie previously approved these changes Mar 4, 2024

View reviewed changes

squid-anubis added the M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels label Mar 4, 2024

kinkie mentioned this pull request Mar 4, 2024

Maintenance: rework SASL detection #1694

Closed

squid-anubis removed the M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels label Mar 4, 2024

rousskov reviewed Mar 4, 2024

View reviewed changes

kinkie dismissed their stale review via 8f8ed03 March 5, 2024 09:26

yadij force-pushed the arc-autoconf-ng-26 branch 2 times, most recently from 84407d5 to 06c9839 Compare March 5, 2024 15:46

rousskov reviewed Mar 5, 2024

View reviewed changes

yadij added S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box and removed S-waiting-for-author author action is expected (and usually required) labels Mar 6, 2024

rousskov added S-waiting-for-author author action is expected (and usually required) and removed S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box labels Mar 6, 2024

Maintenance: update --with-ldap detection

13bcb18

Remove Mosilla LDAP SDK support which has apparently not been maintained since 2017.

yadij force-pushed the arc-autoconf-ng-26 branch from 9044f6f to 13bcb18 Compare March 13, 2024 16:36

yadij added S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box and removed S-waiting-for-author author action is expected (and usually required) labels Mar 13, 2024

yadij closed this Mar 13, 2024

yadij deleted the arc-autoconf-ng-26 branch March 13, 2024 17:03

squid-anubis added the M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels label Mar 13, 2024

rousskov mentioned this pull request Mar 13, 2024

Maintenance: update --with-ldap detection #1736

Closed

Drop Mozilla LDAP SDK detection, update --with-ldap code #1710

Drop Mozilla LDAP SDK detection, update --with-ldap code #1710

Uh oh!

Conversation

yadij commented Feb 27, 2024 • edited by rousskov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kinkie commented Feb 28, 2024

Uh oh!

Uh oh!

kinkie commented Feb 29, 2024

Uh oh!

rousskov commented Feb 29, 2024

Uh oh!

kinkie commented Feb 29, 2024

Uh oh!

rousskov commented Feb 29, 2024

Uh oh!

yadij commented Mar 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kinkie commented Mar 1, 2024

Uh oh!

rousskov commented Mar 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Footnotes

Uh oh!

kinkie commented Mar 4, 2024

Uh oh!

kinkie commented Mar 4, 2024

Uh oh!

kinkie commented Mar 4, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yadij commented Mar 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yadij commented Feb 27, 2024 •

edited by rousskov

Loading

yadij commented Mar 1, 2024 •

edited

Loading

rousskov commented Mar 1, 2024 •

edited

Loading