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

Add v7 restCompat for invalidating API key with the id field #78664

Merged
merged 5 commits into from
Oct 11, 2021

Conversation

ywangd
Copy link
Member

@ywangd ywangd commented Oct 5, 2021

This PR restore the id field for InvaliateApiKey API so it can be used
if the request explicitly requires v7 compatibility.

Relates: #66671

This PR restore the id field for InvaliateApiKey API so it can be used
if the request explicitly requires v7 compatibility.

Relates: elastic#66671
@ywangd ywangd added >enhancement :Security/Security Security issues without another label v8.0.0 labels Oct 5, 2021
@ywangd ywangd requested a review from tvernum October 5, 2021 05:50
@elasticmachine elasticmachine added the Team:Security Meta label for security team label Oct 5, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security (Team:Security)

@ywangd
Copy link
Member Author

ywangd commented Oct 5, 2021

The 7.x branch currently does not have any YAML tests to excercise the deprecated id field. Because back then, YAML test does not allow match warnings with changing content (field position). This is now possible with the new allowed_warnings_regex directive. I'll raise a separate PR to add a new YAML test in 7.x so that the deprecated field is tested.

@ywangd
Copy link
Member Author

ywangd commented Oct 6, 2021

@elasticmachine run elasticsearch-ci/rest-compatibility

@ywangd
Copy link
Member Author

ywangd commented Oct 6, 2021

@elasticmachine update branch

ywangd added a commit that referenced this pull request Oct 6, 2021
Copy link
Contributor

@tvernum tvernum left a comment

Choose a reason for hiding this comment

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

LGTM


private ConstructingObjectParser<InvalidateApiKeyRequest, Void> getObjectParser(RestRequest request) {
if (request.getRestApiVersion() == RestApiVersion.V_7) {
final ConstructingObjectParser<InvalidateApiKeyRequest, Void> objectParser = new ConstructingObjectParser<>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason this isn't a static final field like the non-compat parser?
I'm not particularly fussed either way, it just surprised me.

Copy link
Member Author

Choose a reason for hiding this comment

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

My thinking is to minimize the overhead of restCompat to normal v8 usage (at the same time accept a bit more overhead for restCompat). So this v7 parser does not have to exist if client never uses v7 rest compat. It's probably a marginal saving. But recent focus on performance issue led me to this.

@ywangd ywangd merged commit d2cae33 into elastic:master Oct 11, 2021
weizijun added a commit to weizijun/elasticsearch that referenced this pull request Oct 11, 2021
* upstream/master: (250 commits)
  [Transform] HLRC cleanups (elastic#78909)
  [ML] Make ML indices hidden when the node becomes master (elastic#77416)
  Introduce a Few Settings Singleton Instances (elastic#78897)
  Simplify TestCluster extraJar configuration (elastic#78837)
  Add @OverRide annotations to methods in EnrichPlugin class (elastic#76873)
  Add v7 restCompat for invalidating API key with the id field (elastic#78664)
  EQL: Refine repeatable queries (elastic#78895)
  Fix DataTierTests package and add a validation test (elastic#78880)
  Fix split package org.elasticsearch.common.xcontent (elastic#78831)
  Store DataTier Preference directly on IndexMetadata (elastic#78668)
  [DOCS] Fixes typo in calendar API example (elastic#78867)
  Improve Node Shutdown Observability (elastic#78727)
  Convert encrypted snapshot license object to LicensedFeature (elastic#78731)
  Revert "Make nodePaths() singular (elastic#72514)" (elastic#78801)
  Fix incorrect generic type in PolicyStepsRegistry (elastic#78628)
  [DOCS] Fixes ML get calendars API (elastic#78808)
  Implement GET API for System Feature Upgrades (elastic#78642)
  [TEST] More MetadataStateFormat tests (elastic#78577)
  Add support for rest compatibility headers to the HLRC (elastic#78490)
  Un-ignoring tests after backporting fix (elastic#78830)
  ...

# Conflicts:
#	server/src/main/java/org/elasticsearch/ingest/IngestService.java
#	server/src/test/java/org/elasticsearch/ingest/IngestServiceTests.java
ywangd added a commit to ywangd/elasticsearch that referenced this pull request Oct 29, 2021
RestCompat for invalidate API key with a single id is merged. The test
can now be enabled.
Relates: elastic#78664
ywangd added a commit that referenced this pull request Nov 3, 2021
RestCompat for invalidate API key with a single id is merged. The test
can now be enabled.

Relates: #78664
ywangd added a commit to ywangd/elasticsearch that referenced this pull request Nov 3, 2021
)

RestCompat for invalidate API key with a single id is merged. The test
can now be enabled.

Relates: elastic#78664
elasticsearchmachine pushed a commit that referenced this pull request Nov 3, 2021
…80237)

RestCompat for invalidate API key with a single id is merged. The test
can now be enabled.

Relates: #78664
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement :Security/Security Security issues without another label Team:Security Meta label for security team v8.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants