Support for security remote_cluster and associated privileges #3123
Conversation
|
|
||
| export class Response { | ||
| body: { cluster: string[]; index: IndexName[] } | ||
| body: { cluster: ClusterPrivilege[]; index: IndexName[]; remote_cluster: ClusterPrivilegeForRemote[] } |
There was a problem hiding this comment.
I am unsure about changing cluster from string[] to the proper enumeration
This should be the preferred model, where we guide users to only add named privileges from the enum. However, it is technically possible (but not recommended) to add the action names directly which are just string[]. Other places use the enum and this makes them the same (and the validation still passes).
There was a problem hiding this comment.
where we guide users to only add named privileges from the enum
This one is a response class, which means these properties are never set by the user.
Anyways: Are we sure that only valid privileges from the enum are returned here? If yes, we should definitely prefer the enum (however, this will be a breaking change for all statically typed language clients, so we have to check with everybody else first).
There was a problem hiding this comment.
From this API, yes, only values from the enum are ever returned.
There was a problem hiding this comment.
For me it would be ok to introduce a breaking change by changing the return type to the correct enum type. What about the rest @elastic/devtools-team ?
There was a problem hiding this comment.
See #3125 (comment) - turns out it's not breaking after all.
| } from '@security/_types/Privileges' | ||
| import { RoleTemplate } from '@security/_types/RoleTemplate' | ||
| import { Dictionary } from '@spec_utils/Dictionary' | ||
| import { UserDefinedValue } from '@spec_utils/UserDefinedValue' | ||
| import { Metadata } from '@_types/common' | ||
|
|
||
| export class Role { | ||
| cluster: string[] | ||
| cluster: ClusterPrivilege[] |
There was a problem hiding this comment.
same question (as above) here for the change from string[] to emum
There was a problem hiding this comment.
This type is as well only used in the response of get_role, so same situation as above.
There was a problem hiding this comment.
from this API it can be an action name not present in the enum. But the same applies for the indices which can be field too. However, that is undocumented and users should always use values from the enum. I would prefer to steer folks to use what values from the enum even if it is technically possible in the API to use other values. I will close this PR so I can use a branch instead of fork and will recomment on the new PR.
|
Could someone help me to navigate the failures here ? It's my first PR to this repo and all the validation either passed locally or failed in the same manner before my change. Not sure how to interpert the build failure here. |
|
Can you please run Additionally, the errors are in fact different on main and in your pull request. main$ make validate api=security.get_builtin_privileges branch=main
Validating endpoints
Validating security.get_builtin_privileges
It looks like security.get_builtin_privileges request has some errors, take a look at the workbench.
../clients-flight-recorder/scripts/types-validator/workbench/a6156899cdec2b75c536ba9ed5a3723b.test-d.ts:96:2
Argument of type '{ cluster: string[]; index: string[]; remote_cluster: string[]; }' is not assignable to parameter of type 'ErrorResponseBase | SecurityGetBuiltinPrivilegesRespo
nse'.
Object literal may only specify known properties, and '"remote_cluster"' does not exist in type 'ErrorResponseBase | SecurityGetBuiltinPrivilegesResponse'.
✔ 2 out of 2 test request cases are passing.
✖ 1 out of 2 test response cases are passing.In other words, this was failing because This pull request$ make validate api=security.get_builtin_privileges branch=main
Validating endpoints
Validating security.get_builtin_privileges
It looks like security.get_builtin_privileges request has some errors, take a look at the workbench.
../clients-flight-recorder/scripts/types-validator/workbench/scripts/types-validator/workbench/47a1eec20d0f6734ef46080eababed2a.test-d.ts:7:79
Argument of type '{ cluster: string[]; index: string[]; }' is not assignable to parameter of type 'ErrorResponseBase | SecurityGetBuiltinPrivilegesResponse'.
Property 'remote_cluster' is missing in type '{ cluster: string[]; index: string[]; }' but required in type 'SecurityGetBuiltinPrivilegesResponse'.
✔ 2 out of 2 test request cases are passing.
✖ 1 out of 2 test response cases are passing.Here, we're seeing the opposite failure! The spec requires |
| * The subset of cluster level privileges that can be defined for remote clusters. | ||
| * @availability stack | ||
| */ | ||
| export enum ClusterPrivilegeForRemote { |
There was a problem hiding this comment.
Let's rename this to RemoteClusterPrivilege. I think that matches the existing schema:
ClusterPrivilege(enum) ->ClusterPrivileges(class)RemoteClusterPrivilege(enum) ->RemoteClusterPrivileges(class)
|
I can confirm |
|
accidental close! 😆 |
|
closing in favor #3125 to allow CI to work. Will make changes on that PR. |
This commit adds support for the remote_cluster in the role and role descriptors.
Additionally: