Implement system index access level that includes everything except net-new indices #74803
Conversation
gwbrown
added
>non-issue
:Core/Infra/Core
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
williamrandolph
left a comment
williamrandolph
left a comment
There was a problem hiding this comment.
Great work, and tests look good. I've got a handful of small suggestions; otherwise, LGTM.
| return false; | ||
| case RESTRICTED: | ||
| return resolver.getSystemIndexAccessPredicate().test(indexAbstraction.getName()); | ||
| case NON_NET_NEW_ONLY: |
There was a problem hiding this comment.
NON_NET_NEW_ONLY is a little but clunky as a name. I have a few alternate suggestion:
BWC_SYSTEM_INDICES_ONLYorBACKWARDS_COMPATIBLE_ONLY- makes clear that this behavior is bwc-relatedLEGACY_BEHAVIORorLEGACY_ONLY- I don't love introducing the term "legacy," because the indices themselves aren't legacies.DEPRECATED_ACCESS_ONLY- fairly clear, right? This one indicates that access will disappear, which I like.
What do you think? If we keep the current name, I would want to put Javadoc on its enum entry so that confused devs can click through for an explanation of what the level means.
There was a problem hiding this comment.
I wrote the above review comment earlier; if it's urgent to merge I think we can avoid a back-and-forth discussion and stick with NON_NET_NEW_ONLY.
There was a problem hiding this comment.
I changed this to BACKWARDS_COMPATIBLE_ONLY as well as adding some javadoc: 26cbb39
I like that name much better, I knew NET_NEW_ONLY was bad I just didn't know what to do instead.
| throw systemIndices.netNewSystemIndexAccessException(threadPool.getThreadContext(), List.of(indexName)); | ||
| } | ||
| } else { | ||
| // NON_NET_NEW_ONLY should never be a possibility here, as it cannot be returned from getSystemIndexAccessLevel. |
There was a problem hiding this comment.
If it's not a possibility, should we assert on that?
There was a problem hiding this comment.
There's an assert immediately below this line that the access level should be NONE, which includes that it shouldn't be BACKWARDS_COMPATIBLE_ONLY by implication. I also added asserts to other places this method is used, though, and a comment in getSystemIndexAccessLevel noting it's intentional that it can't be returned from that method: 8bd9f81
| assertThat(result.get("logs-foo"), contains(new DataStreamAlias("logs", List.of("logs-bar", "logs-foo"), null))); | ||
| } | ||
|
|
||
| public void testNetNetSystemIndicesDontErrorWhenNotRequested() { |
There was a problem hiding this comment.
| public void testNetNetSystemIndicesDontErrorWhenNotRequested() { | |
| public void testNetNewSystemIndicesDontErrorWhenNotRequested() { |
|
Test failures appear to be caused by #74358 and #75221. @elasticmachine run elasticsearch-ci/part-1 |
|
Another instance of #75221. @elasticmachine run elasticsearch-ci/part-1 |
…et-new indices (elastic#74803) This PR implements a system index access level that includes all system indices, except net-new system indices. This new access level is leveraged to fix the error in the Get Alias API described in elastic#74687.
…et-new indices (elastic#74803) This PR implements a system index access level that includes all system indices, except net-new system indices. This new access level is leveraged to fix the error in the Get Alias API described in elastic#74687.
…et-new indices (elastic#74803) This PR implements a system index access level that includes all system indices, except net-new system indices. This new access level is leveraged to fix the error in the Get Alias API described in elastic#74687.
4 participants
This PR implements a system index access level that includes all system indices, except net-new system indices.
This new access level is leveraged to fix the error in the Get Alias API described in #74687.