[Alerting] Replaced single invalidateApiKey request with the bulk. #87401

YulNaumenko · 2021-01-05T22:46:20Z

Changes introduced based on the adding support to invalidate API keys in a bulk operation.

invalidateApiKey<InvalidateAPIKeyResult>({
          body: { ids: params.ids },
        })

Extended methods invalidate:

/**
   * Tries to invalidate an API keys.
   * @param request Request instance.
   * @param params The params to invalidate an API keys.
   */
  async invalidate(request: KibanaRequest, params: InvalidateAPIKeysParams) {

and invalidateAsInternalUser:

/**
   * Tries to invalidate the API keys by using the internal user.
   * @param params The params to invalidate the API keys.
   */
  async invalidateAsInternalUser(params: InvalidateAPIKeysParams) {
    if (!this.license.isEnabled()) {
      return null;
    }

of x-pack/plugins/security/server/authentication/api_keys/api_keys.ts to support invalidation by multiple ids parameters.
Introduced proper changes in the plugins which are using this API.
Resolves #83224

elasticmachine · 2021-01-05T22:46:22Z

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

azasypkin

LGTM with few nits, thanks!

x-pack/plugins/security/server/authentication/api_keys/api_keys.ts

azasypkin · 2021-01-06T08:21:44Z

x-pack/plugins/security/server/authentication/api_keys/api_keys.ts

@@ -253,10 +260,10 @@ export class APIKeys {
  }

  /**
-   * Tries to invalidate an API key by using the internal user.
-   * @param params The params to invalidate an API key.
+   * Tries to invalidate an API keys by using the internal user.


question: would you mind updating the signature/log-messages/etc for invalidate method as well to keep them consistent? I see only one place in fleet that uses invalidate, so hopefully it won't require too much work. But if not, feel free to keep it as is and I'll update it in the follow-up.

ymao1

LGTM! Left some minor comments about updating the log message and maybe adding a check for whether there are keys to invalidate before calling the bulk invalidate request.

x-pack/plugins/alerts/server/invalidate_pending_api_keys/task.ts

…fleets plugin usage of this API.

elasticmachine · 2021-01-07T01:49:02Z

Pinging @elastic/ingest-management (Team:Ingest Management)

jfsiii · 2021-01-07T02:18:51Z

x-pack/plugins/fleet/server/services/api_keys/security.ts

@@ -64,7 +64,7 @@ export async function authenticate(callCluster: CallESAsCurrentUser) {
  }
 }

-export async function invalidateAPIKey(soClient: SavedObjectsClientContract, id: string) {
+export async function invalidateAPIKey(soClient: SavedObjectsClientContract, ids: string[]) {


Since this now accepts ids: string[] I think the function should be pluralized to invalidateAPIKeys

kibanamachine · 2021-01-07T06:14:47Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: b012dff

Metrics [docs]

Distributable file count

id	before	after	diff
`default`	47260	48020	+760

History

💔 Build #97468 failed d6be4c8
💚 Build #97164 succeeded f3211eb
💔 Build #97134 failed 820e660

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

jfsiii

🚚 Thanks!

mikecote

Changes in the alerts plugin LGTM!

mikecote · 2021-01-07T12:48:20Z

x-pack/plugins/alerts/server/invalidate_pending_api_keys/task.ts

+  if (apiKeyIds.length > 0) {
+    const response = await invalidateAPIKeys({ ids: apiKeyIds }, securityPluginStart);
+    if (response.apiKeysEnabled === true && response.result.error_count > 0) {
+      logger.error(`Failed to invalidate API Keys [ids="${apiKeyIds.join(', ')}"]`);


nit: I think the intent was to join with double quotes?

Suggested change

logger.error(`Failed to invalidate API Keys [ids="${apiKeyIds.join(', ')}"]`);

logger.error(`Failed to invalidate API Keys [ids="${apiKeyIds.join(", ")}"]`);

TS formatter trying to replace back to a single quotes.

Ohh I see, I was thinking the quotes were part of the logs, oops. All good.

…lk. (#87401) (#87657) * resolve merge conflicts * fixed faling tests

Replaced single invalidateApiKey request with the bulk

820e660

YulNaumenko added Feature:Alerting v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.12.0 labels Jan 5, 2021

YulNaumenko self-assigned this Jan 5, 2021

YulNaumenko requested review from a team as code owners January 5, 2021 22:46

legrego requested a review from azasypkin January 5, 2021 23:15

fixed failing test

f3211eb

azasypkin approved these changes Jan 6, 2021

View reviewed changes

ymao1 approved these changes Jan 6, 2021

View reviewed changes

x-pack/plugins/alerts/server/invalidate_pending_api_keys/task.ts Outdated Show resolved Hide resolved

x-pack/plugins/alerts/server/invalidate_pending_api_keys/task.ts Outdated Show resolved Hide resolved

Extended invalidate method to support multiple invalidation. Updated …

d6be4c8

…fleets plugin usage of this API.

YulNaumenko requested a review from a team January 7, 2021 01:48

botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label Jan 7, 2021

jfsiii reviewed Jan 7, 2021

View reviewed changes

fixed due to comments

b012dff

YulNaumenko requested a review from jfsiii January 7, 2021 04:00

jfsiii approved these changes Jan 7, 2021

View reviewed changes

mikecote approved these changes Jan 7, 2021

View reviewed changes

YulNaumenko merged commit 794c6b3 into elastic:master Jan 7, 2021

YulNaumenko mentioned this pull request Jan 7, 2021

[7.x] [Alerting] Replaced single invalidateApiKey request with the bulk. (#87401) #87657

Merged

jen-huang mentioned this pull request Jan 7, 2021

[Security] Support invalidate API keys by multiple IDs #79714

Closed

YulNaumenko added a commit that referenced this pull request Jan 7, 2021

[7.x] [Alerting] Replaced single invalidateApiKey request with the bu…

3d275fe

…lk. (#87401) (#87657) * resolve merge conflicts * fixed faling tests

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Alerting] Replaced single invalidateApiKey request with the bulk. #87401

[Alerting] Replaced single invalidateApiKey request with the bulk. #87401

YulNaumenko commented Jan 5, 2021 •

edited

Loading

elasticmachine commented Jan 5, 2021

azasypkin left a comment

azasypkin Jan 6, 2021

ymao1 left a comment

elasticmachine commented Jan 7, 2021

jfsiii Jan 7, 2021

kibanamachine commented Jan 7, 2021

jfsiii left a comment

mikecote left a comment

mikecote Jan 7, 2021

YulNaumenko Jan 7, 2021

mikecote Jan 7, 2021

	logger.error(`Failed to invalidate API Keys [ids="${apiKeyIds.join(', ')}"]`);
	logger.error(`Failed to invalidate API Keys [ids="${apiKeyIds.join(", ")}"]`);

[Alerting] Replaced single invalidateApiKey request with the bulk. #87401

[Alerting] Replaced single invalidateApiKey request with the bulk. #87401

Conversation

YulNaumenko commented Jan 5, 2021 • edited Loading

elasticmachine commented Jan 5, 2021

azasypkin left a comment

Choose a reason for hiding this comment

azasypkin Jan 6, 2021

Choose a reason for hiding this comment

ymao1 left a comment

Choose a reason for hiding this comment

elasticmachine commented Jan 7, 2021

jfsiii Jan 7, 2021

Choose a reason for hiding this comment

kibanamachine commented Jan 7, 2021

💚 Build Succeeded

Metrics [docs]

Distributable file count

History

jfsiii left a comment

Choose a reason for hiding this comment

mikecote left a comment

Choose a reason for hiding this comment

mikecote Jan 7, 2021

Choose a reason for hiding this comment

YulNaumenko Jan 7, 2021

Choose a reason for hiding this comment

mikecote Jan 7, 2021

Choose a reason for hiding this comment

YulNaumenko commented Jan 5, 2021 •

edited

Loading