Fix geoip databases index access after system feature migration

[joegallo]

There's a difference in the index resolution between a net new system index and a net new system index behind an alias. This PR adds to an integration test in order to capture the wider scenario, but it also adds a unit test that hits just this one precise detail.

The change to the IndexAbstractionResolver is very small, and I think it makes sense and is defensible, but this isn't an area of the code I know especially well, so I'd love an expert's eyes in the review.

[elasticsearchmachine]

Hi @joegallo, I've created a changelog YAML for you.

[joegallo]

I think there's at least one implied missing test in IndexAbstractionResolverTests that could cover this scenario. It'd be nice to have a unit test for it. (edit: Done, see later comment.)

[joegallo]

I added a unit test in 4308559 that fails in the commit on which it was written (because that commit is based on main) but that passed on this branch after the merge commit because it works with the changes from this branch.

[joegallo]

Backporting this PR to 8.18 and 8.x is going to look slightly different than the code that hits main. I've pre-gamed that as 8.x...joegallo:elasticsearch:fix-geoip-access-after-reindex-8.x.

[gmarouli]

The change to the IndexAbstractionResolver is very small, and I think it makes sense and is defensible, but this isn't an area of the code I know especially well, so I'd love an expert's eyes in the review.

So this is a bit of a weird territory, the combination of access and system indices is not well established. I tried to get to the bottom of this when I was refactoring the IndexNameExpressionResolver but I did not go far.

According to system level access:

For system data streams and "net-new" system indices (see {@link SystemIndexDescriptor#isNetNew()}), access levels should be used to reject requests entirely (see javadoc).

So if I read the above correctly what we are doing here, is not what we want. Our main issue is that the legacy indices that were allowed access now become net-new and they lose access. Right?

I have documented in IndexNameExpressionResolver.SystemResourceAccess that we already have contradicting code here, but before we make it even more complicated, let's think it through a bit. What do you think?

However there is backwards compatibility mode and I think this is what we count here, right?

... there was a desire for "net-new" system indices to opt in to the post-8.0 behavior of having access blocked in most cases, but this caused problems with certain APIs (see #74687), so this access level was added as a
workaround. Once we no longer have to support accessing existing system indices, this can and should be removed, along with the net-new property of system indices in general.

So, I am definitely not an expert but at least what you did matches what I read in the javadoc.

[elasticsearchmachine]

Pinging @elastic/es-data-management (Team:Data Management)

[masseyke]

I am fairly sure that this is correct. If I am missing something, it is easily-enough reversible. So LGTM.

[elasticsearchmachine]

💔 Backport failed

Status	Branch	Result
✅	9.0
❌	8.18	Commit could not be cherrypicked due to conflicts
❌	8.x	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 121196