Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow index name patterns in Privileges index fields #3127

Merged
merged 4 commits into from
Nov 18, 2024
Merged

Conversation

pquentin
Copy link
Member

Motivated by this YAML test I realized that index in various privileges classes can always be an index pattern, ie. a string.

For references, here is how IndexName and Indices are defined:

export type IndexName = string
export type Indices = IndexName | IndexName[]

This fixes security.bulk_put_role, as can be tested with make validate api=security.bulk_put_role branch=main.

This comment was marked as outdated.

@flobernd
Copy link
Member

flobernd commented Nov 14, 2024

That's unfortunate 😓 I recently changed these from Indices to IndexName[] without noticing it breaks validation..

Context:
#3016

I have to think about how I want to proceed with that. It seems like the documentation is wrong about only mentioning IndexName[] which means the problem is for .NET only. The .NET client hand-crafts the Indices type (but not IndexName[]) and implements special semantics to e.g. support _all. I'm pretty sure I won't be able to change this without introducing a massive breaking change.

Copy link
Contributor

@l-trotta l-trotta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I think for most static clients this should change nothing since we already simplify this union as an array

Copy link
Contributor

@jakelandis jakelandis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@flobernd
Copy link
Member

flobernd commented Nov 14, 2024

Could we please change this types to use IndexName | IndexName[] instead of the Indices alias for now and leave a comment that links to this issue?

That would give us some time to think about these special types that carry specific semantics, without breaking the .NET client.

IMO we should model these differences in the Spec to some extend in order to allow generators to produce better code. In this particular case, Indices usually supports the _all string constant which is why the .NET client treats that in a special way. The security APIs however do not accept this constant. They require the usage of a proper pattern (*) instead.

We had similar candidates in the past where the typing is the same, but the semantics are different. I remember e.g. StoredFields (#2415). Here we used a similar workaround, keeping the special StoredFields properties as Field[] while using Fields for everything else.

This comment was marked as outdated.

@pquentin pquentin requested a review from a team as a code owner November 15, 2024 05:56

This comment was marked as off-topic.

@pquentin pquentin removed the request for review from a team November 15, 2024 06:09
@pquentin
Copy link
Member Author

Thanks Florian, please take another look.

Copy link
Contributor

Following you can find the validation results for the APIs you have changed.

API Status Request Response
security.activate_user_profile 🟢 9/9 9/9
security.authenticate 🟢 30/30 30/30
security.bulk_delete_role 🟢 1/1 1/1
security.bulk_put_role 🟢 1/1 1/1
security.bulk_update_api_keys 🟠 Missing type Missing type
security.change_password 🟢 9/9 9/9
security.clear_api_key_cache 🟢 13/13 13/13
security.clear_cached_privileges 🟢 3/3 3/3
security.clear_cached_realms 🟢 1/1 1/1
security.clear_cached_roles 🟢 2/2 2/2
security.clear_cached_service_tokens 🟢 4/4 4/4
security.create_api_key 🔴 66/68 59/59
security.create_cross_cluster_api_key 🟢 3/3 3/3
security.create_service_token 🟢 3/3 3/3
security.delete_privileges 🟢 6/6 6/6
security.delete_role_mapping 🟢 9/9 9/9
security.delete_role 🟢 8/8 8/8
security.delete_service_token Missing test Missing test
security.delete_user 🟢 9/9 9/9
security.disable_user_profile 🟢 1/1 1/1
security.disable_user 🟢 3/3 3/3
security.enable_user_profile 🟢 1/1 1/1
security.enable_user 🟢 4/4 4/4
security.enroll_kibana Missing test Missing test
security.enroll_node Missing test Missing test
security.get_api_key 🔴 38/38 15/38
security.get_builtin_privileges 🟢 1/1 1/1
security.get_privileges 🟢 12/12 12/12
security.get_role_mapping 🔴 18/18 10/18
security.get_role 🔴 24/24 22/24
security.get_service_accounts Missing test Missing test
security.get_service_credentials 🟢 1/1 1/1
security.get_settings 🟠 Missing type Missing type
security.get_token 🟢 25/25 24/24
security.get_user_privileges 🔴 8/8 7/8
security.get_user_profile 🟢 8/8 8/8
security.get_user 🟢 25/25 25/25
security.grant_api_key 🟢 7/7 7/7
security.has_privileges_user_profile 🟢 3/3 3/3
security.has_privileges 🟢 24/24 24/24
security.invalidate_api_key 🟢 12/12 12/12
security.invalidate_token 🟢 11/11 11/11
security.oidc_authenticate 🟠 Missing type Missing type
security.oidc_logout 🟠 Missing type Missing type
security.oidc_prepare_authentication 🟠 Missing type Missing type
security.put_privileges 🟢 10/10 10/10
security.put_role_mapping 🔴 2/11 11/11
security.put_role 🟢 39/39 38/38
security.put_user 🟢 48/48 47/47
security.query_api_keys 🔴 14/14 1/14
security.query_role 🟢 2/2 2/2
security.query_user 🟢 4/4 4/4
security.saml_authenticate Missing test Missing test
security.saml_complete_logout Missing test Missing test
security.saml_invalidate Missing test Missing test
security.saml_logout Missing test Missing test
security.saml_prepare_authentication Missing test Missing test
security.saml_service_provider_metadata Missing test Missing test
security.suggest_user_profiles 🟢 1/1 1/1
security.update_api_key 🟢 5/5 5/5
security.update_cross_cluster_api_key 🟢 2/2 2/2
security.update_settings 🟠 Missing type Missing type
security.update_user_profile_data 🟢 1/1 1/1

You can validate these APIs yourself by using the make validate target.

Copy link
Member

@flobernd flobernd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! This looks good now 🙂

@pquentin pquentin merged commit dd55c46 into main Nov 18, 2024
7 checks passed
@pquentin pquentin deleted the indices-privilege branch November 18, 2024 06:42
Copy link
Contributor

The backport to 8.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-8.x 8.x
# Navigate to the new working tree
cd .worktrees/backport-8.x
# Create a new branch
git switch --create backport-3127-to-8.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 dd55c46bfe8334f57dfd831b56197ed3574a8a8b
# Push it to GitHub
git push --set-upstream origin backport-3127-to-8.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-8.x

Then, create a pull request where the base branch is 8.x and the compare/head branch is backport-3127-to-8.x.

pquentin added a commit that referenced this pull request Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants