Move GetCheckpointAction under the monitor namespace #84473
Conversation
In elastic#80984, a new action is added to the "view_index_privilege" index privilege. This PR adds it under "manage" as well and also adds test to ensure "view_index_metadata" is always a subset of "manage".
ywangd
added
>non-issue
:Security/Authorization
|
Pinging @elastic/es-security (Team:Security) |
ywangd
changed the title
|
I kept the explicit grant of |
|
The test failure is unrelated. It does not even have security enabled. |
Yes, that's correct. |
There was a problem hiding this comment.
LGTM
However, I am asking you to add documentation to the code, e.g. in Constants.java you could add a code doc header explaining the different namespaces and purposes. Although this is test code, every new action has to be added here, so a developer would automatically stumble upon this documentation (or you find a better place and link it, whatever). Your explanations in the PR are great, but will be lost.
| /** | ||
| * The name of an index related action always being with `indices:` followed by a sequence of slash-separated terms | ||
| * that generally describes the hierarchy (from broader to more specific) of the action. For example, the | ||
| * first level comprises `admin`, `monitor`, `data` which generally categorize an action into either an admin | ||
| * related function, or a monitoring related function or a user-data related function. Subsequent levels further | ||
| * narrow down the category until the meaning is specific enough. | ||
| * | ||
| * Note that these terms are meant to categorize what the action does, *not* how it should be invoked. This means | ||
| * whether an action is accessible via REST API should not contribute to its naming. | ||
| * | ||
| * Also note that the `internal:transport/proxy/` prefix is automatically added and stripped for actions that go | ||
| * through a CCR/CCS proxy. No action should be explicitly named like that. | ||
| */ |
There was a problem hiding this comment.
However, I am asking you to add documentation to the code, e.g. in Constants.java you could add a code doc header explaining the different namespaces and purposes. Although this is test code, every new action has to be added here, so a developer would automatically stumble upon this documentation (or you find a better place and link it, whatever). Your explanations in the PR are great, but will be lost.
@hendrikmuhs I added this comment to IndexPrivilege. I hope this read right to you and potentially be useful in future. Thanks!
In elastic#80984, a new GetCheckpointAction is added under the namespace of indices:internal/. This is a new namespace and there it is not automatically covered by the manage index privilege. Since it is explicilty added to view_index_metadata, it means view_index_metadata is no longer a subset of manage. This is unexpected. After discussion, instead of adding this new namespace to manage, we agreed to move the new action under monitor and drop the new namespace. IIUC, the internal is used to indicate that this action is internal to a bigger process and it cannot be called on its own and should be kept as an implementation detail. However, what privilege an action should have is an orthogonal concern to how it should be used. The function of this action is more of monitor (similar to the existing GetGlobalCheckpoint API) This PR also adds a few tests to ensure certain subset relationships between privileges, e.g. "view_index_privilege" is a subset of "manage".
In elastic#80984, a new GetCheckpointAction is added under the namespace of indices:internal/. This is a new namespace and there it is not automatically covered by the manage index privilege. Since it is explicilty added to view_index_metadata, it means view_index_metadata is no longer a subset of manage. This is unexpected. After discussion, instead of adding this new namespace to manage, we agreed to move the new action under monitor and drop the new namespace. IIUC, the internal is used to indicate that this action is internal to a bigger process and it cannot be called on its own and should be kept as an implementation detail. However, what privilege an action should have is an orthogonal concern to how it should be used. The function of this action is more of monitor (similar to the existing GetGlobalCheckpoint API) This PR also adds a few tests to ensure certain subset relationships between privileges, e.g. "view_index_privilege" is a subset of "manage".
In #80984, a new GetCheckpointAction is added under the namespace of
indices:internal/. This is a new namespace and there it is not automaticallycovered by the
manageindex privilege. Since it is explicilty added toview_index_metadata, it meansview_index_metadatais no longer asubset of
manage. This is unexpected.After discussion, instead of adding this new namespace to
manage, we agreedto move the new action under
monitorand drop the new namespace.IIUC, the
internalis used to indicate that this action is internal to a bigger processand it cannot be called on its own and should be kept as an implementation detail.
However, what privilege an action should have is an orthogonal concern to how it
should be used. The function of this action is more of
monitor(similar to the existingGetGlobalCheckpoint API)
This PR also adds a few tests to ensure certain subset relationships between
privileges, e.g. "view_index_privilege" is a subset of "manage".