CLPR#3747 fixes the case of ACLs that refer to dropped elements, but it doesn't do anything special for replaced elements yet.

Ideally, when an element is replaced -- for example, when a location constraint with multiple top-level rules is replaced by two new constraints -- we want to preserve existing behavior. That means if an ACL refers to the original constraint ID either by reference or by xpath, that ACL refers to the replacement constraints when we're finished.

The reference case doesn't seem too difficult. Replace reference by xpath="<element1>|<element2>|...".

The xpath case is trickier, because XSLT 1.0 doesn't provide a way to check whether a given node is in the node set that an ACL's xpath attribute would match. One approach would be simply to log a warning that ACL behavior may have changed. However, I think it's worth a shot. Two options:

1. Use an EXSLT extension called <dyn:evaluate> that dynamically evaluates an XPath. This is the most straightforward option, but thus far we've avoided using anything from EXSLT, relying instead on standard XSLT 1.0. However, libxslt *does* support <dyn:evaluate>, and AFAIK we don't explicitly try to support other XSLT processors.
2. Build an auxiliary stylesheet file as XML output during one stage (or within the orchestrator like cts-schemas or Pacemaker's C schema code), and load it in the next transformation stylesheet. I have a loose picture of how we'd do this. This is the most portable way and doesn't rely on anything nonstandard. The tradeoff is increased code complexity and computational work.

Ref: https://stackoverflow.com/a/4647827

P.S. Regarding EXSLT... If we *were* to use EXSLT, then there would be no need for Pacemaker or cts-schemas to be aware of the numbering scheme (upgrade-3.10-0.xsl, upgrade-3.10-1.xsl, ...). Instead, we'd be able to use a wrapper stylesheet that handles all the dispatching to the stylesheets in the pipeline.

@kgaillot Can you weigh in when you have a chance? I know things are especially busy.
I figure it's unwise to work on the xpath case until we agree upon how we ought to address it -- if at all.

It looks like the only transformation that actually replaces one element with other elements that have different IDs, is the "location constraint with multiple top-level rules" case. Other transformations either drop an element or modify it (without changing its ID). New elements may also be created.

how

Option 1 or Option 2 above.

if at all

I think the best we could reasonably do, is to make sure that ACL permissions that previously matched the original constraint now match the replacement constraints. We would do this by adding |constraint1|constraint2... to the xpath attribute of ACL permissions that matched the original constraint. This is a non-trivial task (particularly if we use Option 2), but it looks doable.

More generally, I think it's impractical and perhaps impossible to preserve behavior for all possible ACL permissions that use xpath attributes. This is simply because XPath is so flexible -- it could match an element based on its name attribute, based on its children, etc. Lots of things that may change over the course of the transformation pipeline.

So we'd want to put a warning somewhere, telling the user that the behavior of ACL permissions with xpath might change. It's probably not even easy for the user to check the upgraded CIB when the scheduler (rather than cibadmin) is doing the upgrade, since that's happening in memory. The question would be whether we want to address the particular case of the location constraint with multiple top-level rules.

Since the risk of corner cases is so high, and our time for 3.0.0 is limited, I think the best approach would be to log a warning if a CIB contains ACLs with xpaths and the upgrade adds or renames any element or attribute. (We could possibly check whether an xpath exists that refers to an attribute, to reduce the scope a bit.) Something like: "WARNING: CIB syntax changes may invalidate ACLs that use 'xpath'. It is strongly recommended to run 'cibadmin --upgrade' then go through the updated CIB carefully to ensure ACLs still match the desired intent."

Do you know what version of libxslt added support for dyn:evaluate? We only require 2.9.2 currently. I wouldn't mind using it to reduce the scope of the log further, but it's not necessary.

In T898#14801, @kgaillot wrote:

Do you know what version of libxslt added support for dyn:evaluate? We only require 2.9.2 currently. I wouldn't mind using it to reduce the scope of the log further, but it's not necessary.

Apparently 1.0.19, via this commit in 2002. https://gitlab.gnome.org/GNOME/libxslt/-/commit/765957430fd4b748c2d8f70ee546308fad4359f8

Practically speaking, it seems unlikely to go away, so more a matter of principle whether we want to rely on anything nonstandard. Caution is probably paranoid (not a bad thing in this line of work).

In T898#14801, @kgaillot wrote:

Since the risk of corner cases is so high, and our time for 3.0.0 is limited, I think the best approach would be to log a warning if a CIB contains ACLs with xpaths and the upgrade adds or renames any element or attribute. (We could possibly check whether an xpath exists that refers to an attribute, to reduce the scope a bit.) Something like: "WARNING: CIB syntax changes may invalidate ACLs that use 'xpath'. It is strongly recommended to run 'cibadmin --upgrade' then go through the updated CIB carefully to ensure ACLs still match the desired intent."

I'd also be fine with warning at the end if any ACLs use xpath, period. It's broader scope than necessary, but it avoids the need to clutter the stylesheet and the logs with individual warnings for each relevant transformation. (We could some of the log clutter by having one warning per step in the pipeline, but that may be a gnarly template to catch every case in a single template.)

In T898#14811, @nrwahl2 wrote:

In T898#14801, @kgaillot wrote:

Since the risk of corner cases is so high, and our time for 3.0.0 is limited, I think the best approach would be to log a warning if a CIB contains ACLs with xpaths and the upgrade adds or renames any element or attribute. (We could possibly check whether an xpath exists that refers to an attribute, to reduce the scope a bit.) Something like: "WARNING: CIB syntax changes may invalidate ACLs that use 'xpath'. It is strongly recommended to run 'cibadmin --upgrade' then go through the updated CIB carefully to ensure ACLs still match the desired intent."

I'd also be fine with warning at the end if any ACLs use xpath, period. It's broader scope than necessary, but it avoids the need to clutter the stylesheet and the logs with individual warnings for each relevant transformation. (We could some of the log clutter by having one warning per step in the pipeline, but that may be a gnarly template to catch every case in a single template.)

Yeah I think that wins for simplicity and resilience