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

[Fleet] Remove deprecated APIs for agents endpoints #198313

Merged
merged 30 commits into from
Nov 1, 2024

Conversation

nchaulet
Copy link
Member

@nchaulet nchaulet commented Oct 30, 2024

Summary

Related to #189746

Remove API endpoints

  • POST /service-tokens in favor of POST /service_tokens
  • GET /agent-status in favor GET /agent_status
  • PUT /agents/:agentid/reassign in favor of POST /agents/:agentid/reassign

Remove deprecated parameters or response

  • Remove total from GET /agent_status response
  • Remove list from GET /agents response

Our UI was still consuming the total field in some places, I made the changes to not use anymore, this could cause some errors during the upgrade if two kibana version are running but it seems code handle that missing field well.

To keep those removal PR readable and easy to review I will do separate similar PR for epm endpoints and enrollment api keys endpoints

Did some manual testing of Fleet, nothing seems to be broken and looking at telemetry those deletions seems relatively safe.

nchaulet and others added 5 commits October 29, 2024 17:26
…t --include-path /api/status --include-path /api/alerting/rule/ --include-path /api/alerting/rules --include-path /api/actions --include-path /api/security/role --include-path /api/spaces --include-path /api/fleet --update'
@nchaulet nchaulet self-assigned this Oct 30, 2024
@nchaulet nchaulet added release_note:breaking backport:skip This commit does not require backporting labels Oct 30, 2024
@nchaulet nchaulet changed the title [Fleet] Remove deprecated APIs [Fleet] Remove deprecated APIs for agents endpoints Oct 30, 2024
nchaulet and others added 3 commits October 30, 2024 10:30
…t --include-path /api/status --include-path /api/alerting/rule/ --include-path /api/alerting/rules --include-path /api/actions --include-path /api/security/role --include-path /api/spaces --include-path /api/fleet --update'
@nchaulet nchaulet marked this pull request as ready for review October 30, 2024 17:55
@nchaulet nchaulet requested review from a team as code owners October 30, 2024 17:55
@botelastic botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label Oct 30, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

nchaulet and others added 5 commits October 30, 2024 15:04
…t --include-path /api/status --include-path /api/alerting/rule/ --include-path /api/alerting/rules --include-path /api/actions --include-path /api/security/role --include-path /api/spaces --include-path /api/fleet --update'
@nchaulet
Copy link
Member Author

nchaulet commented Oct 31, 2024

@criamico @juliaElastic it seems observability-perf is relying a lot on calling GET /agent_status with a kuery parameter I am wondering if it really make sense to remove that API parameter?

Copy link
Contributor

@gergoabraham gergoabraham left a comment

Choose a reason for hiding this comment

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

@elastic/security-defend-workflows relevant changes: active is used instead of the now removed total from the response of GET /agent_status, and based on the code snippet below, they are the same, so the related changes look good! thanks for updating the code 🙌

const allActive = allStatuses - combinedStatuses.unenrolled - combinedStatuses.inactive;
return {
...combinedStatuses,
all: allStatuses,
active: allActive,
/* @deprecated no agents will have other status */
other: 0,
/* @deprecated Agent events do not exists anymore */
events: 0,
/* @deprecated use active instead */
total: allActive,
};

nchaulet and others added 4 commits October 31, 2024 08:47
…t --include-path /api/status --include-path /api/alerting/rule/ --include-path /api/alerting/rules --include-path /api/actions --include-path /api/security/role --include-path /api/spaces --include-path /api/fleet --update'
@nchaulet nchaulet requested a review from criamico October 31, 2024 14:07
@criamico
Copy link
Contributor

it seems observability-perf is relying a lot on calling GET /agent_status with a kuery parameter I am wondering if it really make sense to remove that API parameter?

As discussed in slack, this deprecation was firstly introduced as preliminary work for #161064, but it was then decided to keep the kuery parameter, as highlighted in the PR description. This deprecation was then mistakenly kept, but seeing that it's still used and useful I think it's ok to keep it.

@nchaulet
Copy link
Member Author

@elasticmachine merge upstream

@@ -218,8 +217,10 @@ export function useFetchAgentsData() {
getStatusSummary: true,
withMetrics: displayAgentMetrics,
}),
sendGetAgentStatus({
sendGetAgents({
Copy link
Contributor

Choose a reason for hiding this comment

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

As we are not going to deprecate usage of kuery in agent_status, is this change still needed? Or you prefer to change it anyway?

Copy link
Member Author

Choose a reason for hiding this comment

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

I will remove those changes 👍

@@ -135,10 +135,11 @@ export const AgentUpgradeAgentModal: React.FunctionComponent<AgentUpgradeAgentMo

// if selection is a query, do an api call to get updating agents
try {
const res = await sendGetAgentStatus({
const res = await sendGetAgentsQuery({
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

Copy link
Contributor

@maxcold maxcold left a comment

Choose a reason for hiding this comment

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

cloud_defend and cloud_security_posture changes lgtm

Copy link
Contributor

@criamico criamico left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

nchaulet and others added 5 commits October 31, 2024 12:29
…t --include-path /api/status --include-path /api/alerting/rule/ --include-path /api/alerting/rules --include-path /api/actions --include-path /api/security/role --include-path /api/spaces --include-path /api/fleet --update'
@nchaulet
Copy link
Member Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
fleet 1301 1300 -1

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.7MB 1.7MB +6.0B
securitySolution 21.0MB 21.0MB +1.0B
total +7.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 173.1KB 173.0KB -110.0B
Unknown metric groups

API count

id before after diff
fleet 1426 1423 -3

ESLint disabled line counts

id before after diff
fleet 48 47 -1

Total ESLint disabled count

id before after diff
fleet 60 59 -1

History

cc @nchaulet

@nchaulet nchaulet merged commit 115dbec into elastic:main Nov 1, 2024
55 checks passed
@nchaulet nchaulet deleted the feature-remove-deprecated-apis branch November 1, 2024 12:00
nreese pushed a commit to nreese/kibana that referenced this pull request Nov 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:breaking Team:Fleet Team label for Observability Data Collection Fleet team v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants