Fix xmlsec version pin

[jedcunningham]

xmlsec is only used when the python3-saml additional extra is used in the Amazon provider. We don't need it as a requirement for the Amazon provider in general.

Related: #39103
Closes: #39437

[o-nikolas]

👍

[ferruzzi]

LGTM. Currently failing a CI test but it looks like unrelated flake.

[potiuk]

As discussed in #39437 (comment) - I am fine with merging it as is, if the conflicts with xmlsec1/2 is nothing that AWS team is worried about :)

[potiuk]

BTW. It's rather strange that the build does not fail now. I wonder if there is some caching effect - because 3 weeks ago (we do not see the outputs any more) - libxmlsec 1.3.14 caused the compatibility error (it basically failed the main build by the sheer fact of it being installed in the image. But it seems to work fine now. I wonder if it will not start faiing after merging to main due to some caching effect, but let's see :).

[jedcunningham]

My CI image still ends up with 1.3.13, so it must be built using the extra. That's my theory at least :)

[jedcunningham]

Seems we also have a saml extra:

airflow/hatch_build.py

Lines 132 to 135 in 4f13f1b

	"saml": [
	# This is required for support of SAML which might be used by some providers (e.g. Amazon)
	"python3-saml>=1.16.0",
	],

Should we add the xmlsec pin there also?

That was also added in #35488.

[potiuk]

Should we add the xmlsec pin there also?

🤷 :)

I am not even sure while libxmlsec1 in the image was needed in the first place :). It's been added specifically on request by @vincbeck (I remember I helped to explain how to add it) - but I have no idea why it was needed at all (possibly for System Tests to run with AWS Executor) #35488 - that's why when we encountered failing main I added the limit to fix it and left the issue to investigate, but to be perfectly honest, I have no slightest idea what are consequences of leaving it in/adding limit saml extra 😱

All I know that 3 weeks ago - until I added xmlsec<1.3.14 - it broke main with error - xmlsec 1.3.14 started to complaine that it needs libxmlsec1.2+ if you look at the release notes: https://github.com/xmlsec/python-xmlsec/releases - 1.3.14 has a change "Improved libxml2 version check" - which is this change:

https://github.com/xmlsec/python-xmlsec/pull/299/files

and our CI builds started to fail with lxml & xmlsec libxml2 library version mismatch.

That's all I know about it :). That's why I am surprised we do not see that error in this PR (despite evidently 1.3.14 being installed as you can see in generated constraints output.

I cannot see the original failure - it's gone as action logs are kept for 2 weeks, so my guts feeling tells me it will start failing our main again after merging, but let's see.

That's all I know about it. Maybe @vincbeck could explain why we needed to add `libxmlsec1 to our image at all ?

[potiuk]

BTW also it could be that this has been fixed in the meantime by other means - for example base image for python got refreshed with newer version of some libraries that got updated. So maybe we do not need the limit at all any more - all that is possible.

[eladkal]

This will create an extra dependency is it not?
if so then when we want to remove it (as it might not be needed in the future) it will require having a major release for the provider?

[jedcunningham]

xmlsec is a dependency of python3-saml, so it's always been there, and likely will always be.

The change 3 weeks ago was just to set an upper bound. This PR just moves it into the python3-saml extra.

[potiuk]

If you look at the summary: https://github.com/apache/airflow/actions/runs/9020420427?pr=39528 1.3.14 got updated:

So I'd say - we should remove the limit altogether and keep our 🤞 that after merging it will not break main.

[jedcunningham]

why we needed to add `libxmlsec1 to our image at all?

The chain goes like this: amazon provider -> python3-saml (this is the optional extra) -> xmlsec -> libxmlsec1.

Even weirder, I just downloaded the generated constraints for this run...

constraints-3.12/constraints-source-providers-3.12.txt
600:xmlsec==1.3.14

constraints-3.12/constraints-3.12.txt
709:xmlsec==1.3.13

[potiuk]

Even weirder, I just downloaded the generated constraints for this run...

This is expected. Source constraints reflect what is in main. The "constraints" are PyPI constraints so they are limited by amazon provder released few days ago.

[jedcunningham]

Gotcha. I clearly know enough to think I might know what's going on, but don't :)

[ferruzzi]

Just FYI, @vincbeck is away until Monday. I know he added that as part of the AWS Auth Manager work, and it wasn't related to the system tests stuff. It was related to allowing cloud-native auth managers other than relying on flask, and I think it was more of a lead-up to the multi-tenancy stuff, but for accurate info and more detail we'd need to wait till he is back on Monday.

[potiuk]

Superseded by #39534

[vincbeck]

Sorry I was away for the week so I could not really help here. As mentioned by Jed, the only reason xmlsec was part of the image is because it is a dependency of python3-saml. We don't use xmlsec directly. I dont see any objections of removing the pin. I'll do some further tests but overall, LGTM