-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Add docs for QueryApiKey API #76521
Add docs for QueryApiKey API #76521
Conversation
f6277df
to
559a8a5
Compare
559a8a5
to
576fcde
Compare
Pinging @elastic/clients-team (Team:Clients) |
Pinging @elastic/es-docs (Team:Docs) |
Pinging @elastic/es-security (Team:Security) |
"id": "api-key-id-2", | ||
"id": "6wHJmcQpReKBa42EHV5SBw", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is not really related to the purpose of this PR. It's more of a bug fix of existing doc because this ID format is misleading.
"id": "dGhpcyBpcyBub3QgYSByZWFsIHRva2VuIGJ1dCBpdCBpcyBvbmx5IHRlc3QgZGF0YS4gZG8gbm90IHRyeSB0byByZWFkIHRva2VuIQ==", <2> | ||
"id": "0GF5GXsBCXxz2eDxWwFN", <2> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
Note it only support a subset of the query types including | ||
<<query-dsl-match-all-query>>, <<query-dsl-bool-query>>, | ||
<<query-dsl-term-query>>, <<query-dsl-terms-query>>, <<query-dsl-ids-query>>, | ||
<<query-dsl-prefix-query>>, <<query-dsl-wildcard-query>>, <<query-dsl-range-query>>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have some comments about the fields that can be queried? Maybe a list, or at least an explanation of how people would work out what the fields are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I added a list to explain the available fields.
+ | ||
By default, you cannot page through more than 10,000 hits using the `from` and | ||
`size` parameters. To page through more hits, use the | ||
<<search-after,`search_after`>> parameter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is repeated from above. Is that intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is indeed intentional. The same is repeated on the Search API doc page. The difference is that the two parameters, from
and size
, are further apart on the Search API page which makes the repitition more justifiable. I can consolidate them if you prefer.
(Optional, array) <<search-after,Search after>> definition. | ||
|
||
NOTE: If the user only has `manage_own_api_key` privilege, this API returns | ||
only those API keys that are owned by the user. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to state the reverse as well.
If the user has
manage_api_key
or higher (includingmanage_security
),
this API returns API keys that are owned by any user
(any user? all users?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I added
On the other hand, if the user
hasmanage_api_key
or greater privilege (includingmanage_security
), this
API returns API keys regardless of their owners.
?
Co-authored-by: Tim Vernum <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggested some changes -- will push some changes to this branch for a definition list under the query
parameter.
NOTE: If the user only has `manage_own_api_key` privilege, this API returns | ||
only those API keys that are owned by the user. On the other hand, if the user | ||
has `manage_api_key` or greater privilege (including `manage_security`), this | ||
API returns API keys regardless of their owners. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this note belongs in the Prerequisites section where we mention which privileges are required to use the API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved it under the prerequisites as a separate bullet point.
<1> The list of API keys that were retrieved for this request. | ||
<2> Id for the API key | ||
<3> Name of the API key | ||
<4> Creation time for the API key in milliseconds | ||
<5> Optional expiration time for the API key in milliseconds | ||
<6> Invalidation status for the API key. If the key has been invalidated, it has | ||
a value of `true`. Otherwise, it is `false`. | ||
<7> Principal for which this API key was created | ||
<8> Realm name of the principal for which this API key was created | ||
<9> Metadata of the API key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that these callouts should be removed in favor of a proper definition list under the query
parameter starting on line 44. I'll push changes to this branch with the formatted and updated definition list.
Co-authored-by: Adam Locke <[email protected]>
@elasticmachine update branch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 with the latest changes included
@elasticmachine update branch |
@tvernum Because I merged the rest spec PR (#76238) too soon, our docs build is currently broken because the rest spec file points to an URL yet to be added by this PR. To fix the issue, I am going to merge this PR once the CI passes. Both @lockewritesdocs and I are OK with where it is now. But I am happy to raise a follow-up PR if you spot any issues (commenting and review are still allowed by the UI after merging). Sorry for the inconvenience. I'll manage it better next time. |
@elasticmachine run elasticsearch-ci/part-2-fips The CI failure is irrelevant but seems legit, I raised #76689 |
💔 Backport failed
To backport manually run |
This PR adds documentation for the new QueryApiKey API added in elastic#75335 and elastic#76144 Relates: elastic#71023
This PR adds documentation for the new QueryApiKey API added in elastic#75335 and elastic#76144 Relates: elastic#71023
This PR adds documentation for the new QueryApiKey API added in #75335 and #76144
doc-preview: https://elasticsearch_76521.docs-preview.app.elstc.co/guide/en/elasticsearch/reference/master/security-api-query-api-key.html
Relates: #71023