[Fleet] Enforce 10 min cooldown for agent upgrade #168606

jillguyonnet · 2023-10-11T13:57:58Z

Summary

This PR adds a check based on the agent.upgraded_at field and the time a request to upgrade the issue. If the request is issued sooner than 10 minutes after the last upgrade, it is rejected, even if force: true is passed:

POST agents/{agentId}/upgrade will fail with 400
agents included in POST agents/bulk_upgrade will not be upgraded

Checklist

Unit or functional tests were updated or added to match the most common scenarios

apmmachine · 2023-10-11T13:58:18Z

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

/oblt-deploy : Deploy a Kibana instance using the Observability test environments.
/oblt-deploy-serverless : Deploy a serverless Kibana instance using the Observability test environments.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

jillguyonnet · 2023-10-11T16:06:48Z

@elasticmachine merge upstream

jillguyonnet · 2023-10-12T07:42:55Z

@elasticmachine merge upstream

elasticmachine · 2023-10-12T08:01:15Z

Pinging @elastic/fleet (Team:Fleet)

jillguyonnet · 2023-10-12T08:02:01Z

Opening this for review as CI failures seem unrelated.

Can someone confirm whether backport:prev-minor is what we want here?

jlind23 · 2023-10-12T08:05:51Z

@jillguyonnet Yes we should backport it to 8.11 as it is needed for #135539
cc @cmacknz @kpollich

criamico · 2023-10-12T08:09:57Z

x-pack/plugins/fleet/common/services/is_agent_upgradeable.ts

+  }
+
+  const elaspedSinceUpgradeInMillis = Date.now() - Date.parse(agent.upgraded_at);
+  const upgradeCooldownInMin = 10;


Nit: could you uppercase the name so it's clear that is a constant?

criamico · 2023-10-12T08:11:51Z

x-pack/plugins/fleet/server/services/agents/upgrade_action_runner.ts

@@ -78,7 +78,8 @@ export async function upgradeBatch(
    agentsToCheckUpgradeable.map(async (agent) => {
      // Filter out agents currently unenrolling, unenrolled, or not upgradeable b/c of version check


could you also edit this comment to cover the new case?

criamico

LGTM 🚢

kpollich

🚀

kpollich · 2023-10-12T12:35:48Z

x-pack/plugins/fleet/server/routes/agent/upgrade_handler.ts

+      return response.customError({
+        statusCode: 400,
+        body: {
+          message: `agent ${request.params.agentId} was upgraded less than 10 minutes ago`,


This error message should probably interpolate the UPGRADE_COOLDOWN_IN_MIN constant so we don't forget to update it if that constant ever changes.

Good catch! Thanks for reviewing 👍

jillguyonnet · 2023-10-12T14:48:03Z

@elasticmachine merge upstream

cmacknz · 2023-10-12T17:48:30Z

x-pack/plugins/fleet/server/routes/agent/upgrade_handler.ts

+      return response.customError({
+        statusCode: 400,
+        body: {
+          message: `agent ${request.params.agentId} was upgraded less than ${AGENT_UPGRADE_COOLDOWN_IN_MIN} minutes ago`,


Suggested change

message: `agent ${request.params.agentId} was upgraded less than ${AGENT_UPGRADE_COOLDOWN_IN_MIN} minutes ago`,

message: `agent ${request.params.agentId} was upgraded less than ${AGENT_UPGRADE_COOLDOWN_IN_MIN} minutes ago, confirming the upgrade will not roll back`,

I think we need to explain why we have this cooldown or it will just lead to questions. Possibly the explanation should link directly back to our documentation.

Would it also be possible to have the response contain how long the user needs to wait until they can try again? Otherwise they will just be spamming these requests until one of them goes through.

It could be more correct to return a 429 with a Retry-After header in the response: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Retry-After

I've added some basic text for now to explain the cooldown.

I've added elastic/ingest-docs#605 to track the docs for this, and will create a follow-up issue once this PR is merged to capture the need to come back and add a docs link to this text.

I've also updated the API to return a 429 Too Many Requests and to include a time string in the format XXmYYs to indicate how long to wait for a retry as well as a retry-after header with the value in seconds.

I also give you a gold medal #168606 (review)

jillguyonnet · 2023-10-13T06:51:50Z

@elasticmachine merge upstream

jillguyonnet · 2023-10-13T10:22:01Z

@elasticmachine merge upstream

kpollich · 2023-10-17T15:34:37Z

@elasticmachine merge upstream

kpollich · 2023-10-17T17:35:04Z

Looks like there's some FTR failures related to the cooldown change, as we have tests that upgrade an agent quickly. Will take a look soon.

…-ref HEAD~1..HEAD --fix'

kpollich · 2023-10-18T16:30:31Z

@elasticmachine merge upstream

kibana-ci · 2023-10-18T17:36:05Z

💚 Build Succeeded

Buildkite Build
Commit: fdfe7c9

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`fleet`	1.2MB	1.2MB	+382.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`fleet`	147.3KB	147.6KB	+265.0B

History

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

cc @jillguyonnet

kpollich

Approving latest changes that I made

## Summary Closes elastic#168233 This PR adds a check based on the `agent.upgraded_at` field and the time a request to upgrade the issue. If the request is issued sooner than 10 minutes after the last upgrade, it is rejected, even if `force: true` is passed: - `POST agents/{agentId}/upgrade` will fail with 400 - agents included in `POST agents/bulk_upgrade` will not be upgraded ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kyle Pollich <kyle.pollich@elastic.co> (cherry picked from commit 4fffedd)

kibanamachine · 2023-10-18T18:41:55Z

💚 All backports created successfully

Status	Branch	Result
✅	8.11

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…169295) # Backport This will backport the following commits from `main` to `8.11`: - [[Fleet] Enforce 10 min cooldown for agent upgrade (#168606)](#168606)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Jill Guyonnet <jill.guyonnet@elastic.co>

[Fleet] Enforce 10 min cooldown for agent upgrade

fcc1eb9

jillguyonnet added the Team:Fleet Team label for Observability Data Collection Fleet team label Oct 11, 2023

jillguyonnet self-assigned this Oct 11, 2023

Merge branch 'main' into fleet/agent-upgrade-cooldown

3291e86

Merge branch 'main' into fleet/agent-upgrade-cooldown

353650a

jillguyonnet added release_note:skip Skip the PR/issue when compiling release notes v8.12.0 backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Oct 12, 2023

jillguyonnet marked this pull request as ready for review October 12, 2023 08:01

jillguyonnet requested a review from a team as a code owner October 12, 2023 08:01

criamico reviewed Oct 12, 2023

View reviewed changes

Feedback

3e5ca34

criamico approved these changes Oct 12, 2023

View reviewed changes

kpollich approved these changes Oct 12, 2023

View reviewed changes

Extract cooldown duration to constant

5687538

Merge branch 'main' into fleet/agent-upgrade-cooldown

a8a02ab

cmacknz reviewed Oct 12, 2023

View reviewed changes

Merge branch 'main' into fleet/agent-upgrade-cooldown

bb37f35

Merge branch 'main' into fleet/agent-upgrade-cooldown

8c764f8

kpollich mentioned this pull request Oct 17, 2023

[REQUEST]: Document "restart upgrade" cooldown for agents elastic/ingest-docs#605

Closed

Update status code + add help text + include retry-after

2bb6a66

Fix retry-after rounding

914c188

Merge branch 'main' into fleet/agent-upgrade-cooldown

a783783

kpollich and others added 3 commits October 18, 2023 10:40

Fix some failing tests + rename utility

4c90e73

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

3456e84

…-ref HEAD~1..HEAD --fix'

Update error message + modal message with better explainer

d4a660a

Merge branch 'main' into fleet/agent-upgrade-cooldown

fdfe7c9

kpollich approved these changes Oct 18, 2023

View reviewed changes

kpollich merged commit 4fffedd into elastic:main Oct 18, 2023

kibanamachine mentioned this pull request Oct 18, 2023

[8.11] [Fleet] Enforce 10 min cooldown for agent upgrade (#168606) #169295

Merged

kibanamachine added the v8.11.0 label Oct 18, 2023

jillguyonnet deleted the fleet/agent-upgrade-cooldown branch October 23, 2023 10:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fleet] Enforce 10 min cooldown for agent upgrade #168606

[Fleet] Enforce 10 min cooldown for agent upgrade #168606

jillguyonnet commented Oct 11, 2023 •

edited by kibanamachine

Loading

apmmachine commented Oct 11, 2023

jillguyonnet commented Oct 11, 2023

jillguyonnet commented Oct 12, 2023

elasticmachine commented Oct 12, 2023

jillguyonnet commented Oct 12, 2023

jlind23 commented Oct 12, 2023

criamico Oct 12, 2023

criamico Oct 12, 2023

criamico left a comment

kpollich left a comment

kpollich Oct 12, 2023

jillguyonnet Oct 12, 2023

jillguyonnet commented Oct 12, 2023

cmacknz Oct 12, 2023

kpollich Oct 17, 2023 •

edited

Loading

cmacknz Oct 18, 2023

cmacknz Oct 18, 2023

jillguyonnet commented Oct 13, 2023

jillguyonnet commented Oct 13, 2023

kpollich commented Oct 17, 2023

kpollich commented Oct 17, 2023

kpollich commented Oct 18, 2023

kibana-ci commented Oct 18, 2023

kpollich left a comment

kibanamachine commented Oct 18, 2023

		@@ -78,7 +78,8 @@ export async function upgradeBatch(
		agentsToCheckUpgradeable.map(async (agent) => {
		// Filter out agents currently unenrolling, unenrolled, or not upgradeable b/c of version check

	message: `agent ${request.params.agentId} was upgraded less than ${AGENT_UPGRADE_COOLDOWN_IN_MIN} minutes ago`,
	message: `agent ${request.params.agentId} was upgraded less than ${AGENT_UPGRADE_COOLDOWN_IN_MIN} minutes ago, confirming the upgrade will not roll back`,

[Fleet] Enforce 10 min cooldown for agent upgrade #168606

[Fleet] Enforce 10 min cooldown for agent upgrade #168606

Conversation

jillguyonnet commented Oct 11, 2023 • edited by kibanamachine Loading

Summary

Checklist

apmmachine commented Oct 11, 2023

🤖 GitHub comments

jillguyonnet commented Oct 11, 2023

jillguyonnet commented Oct 12, 2023

elasticmachine commented Oct 12, 2023

jillguyonnet commented Oct 12, 2023

jlind23 commented Oct 12, 2023

criamico Oct 12, 2023

Choose a reason for hiding this comment

criamico Oct 12, 2023

Choose a reason for hiding this comment

criamico left a comment

Choose a reason for hiding this comment

kpollich left a comment

Choose a reason for hiding this comment

kpollich Oct 12, 2023

Choose a reason for hiding this comment

jillguyonnet Oct 12, 2023

Choose a reason for hiding this comment

jillguyonnet commented Oct 12, 2023

cmacknz Oct 12, 2023

Choose a reason for hiding this comment

kpollich Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

cmacknz Oct 18, 2023

Choose a reason for hiding this comment

cmacknz Oct 18, 2023

Choose a reason for hiding this comment

jillguyonnet commented Oct 13, 2023

jillguyonnet commented Oct 13, 2023

kpollich commented Oct 17, 2023

kpollich commented Oct 17, 2023

kpollich commented Oct 18, 2023

kibana-ci commented Oct 18, 2023

💚 Build Succeeded

Metrics [docs]

Async chunks

Page load bundle

History

kpollich left a comment

Choose a reason for hiding this comment

kibanamachine commented Oct 18, 2023

💚 All backports created successfully

Questions ?

jillguyonnet commented Oct 11, 2023 •

edited by kibanamachine

Loading

kpollich Oct 17, 2023 •

edited

Loading