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

[Ingest Manager] [Discuss] success property in API responses #73221

Closed
jfsiii opened this issue Jul 25, 2020 · 6 comments
Closed

[Ingest Manager] [Discuss] success property in API responses #73221

jfsiii opened this issue Jul 25, 2020 · 6 comments
Assignees
Labels
Team:Defend Workflows “EDR Workflows” sub-team of Security Solution Team:Fleet Team label for Observability Data Collection Fleet team

Comments

@jfsiii
Copy link
Contributor

jfsiii commented Jul 25, 2020

After some searching, ${API_ROOT}/check-permissions is the only route I can find where an API response has success: false at the top-level.

The other 30+ success: booleans seem to only be success: true. Our tests seem to only include the true case and after confirming a 200 response.

success: false is not in many (most? any?) errors. We don't include it in our response.customError or response.notFound options and it's not included by the platform.

e.g. 404
> curl http://localhost:5601/api/ingest_manager/fleet/enrollment-api-keys/missing-key-id --user elastic:changeme
{"statusCode":404,"error":"Not Found","message":"EnrollmentAPIKey missing-key-id not found"}
> curl http://localhost:5601/api/ingest_manager/package_configs/missing-key-id --user elastic:changeme
{"statusCode":404,"error":"Not Found","message":"Package config missing-key-id not found"}
> curl http://localhost:5601/api/ingest_manager/fleet/agents/missing-key-id --user elastic:changeme
{"statusCode":404,"error":"Not Found","message":"Agent missing-key-id not found"}

I see some references to it in the client-side code but I haven't dug into them. Perhaps they never receive false. These could/would be updated to use try/catch for await or the .catch if using the Promise.

Questions

Do we need/want this property?

Should we add more success: false or can we remove it as a default/required top-level response property?

@jfsiii jfsiii added the Team:Fleet Team label for Observability Data Collection Fleet team label Jul 25, 2020
@elasticmachine
Copy link
Contributor

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

@nchaulet
Copy link
Member

I am in favor if removing it. Relying on status code is probably a lot better (and we are already doing it)

@jfsiii
Copy link
Contributor Author

jfsiii commented Jul 26, 2020

I opened a draft PR after doing a find/replace for all but check-permissions #73223

@elasticmachine
Copy link
Contributor

Pinging @elastic/endpoint-management (Team:Endpoint Management)

@paul-tavares
Copy link
Contributor

Did a quick search in security_solution code base and did not see anything on our end that seems to be using t. Looks like your PR already accounts for updating our mocks 👍

@jfsiii
Copy link
Contributor Author

jfsiii commented Aug 17, 2020

Closing the discussion part of this. Implementation started in #73223

@jfsiii jfsiii closed this as completed Aug 17, 2020
@MindyRS MindyRS added Team:Defend Workflows “EDR Workflows” sub-team of Security Solution and removed Team:Endpoint Management labels Oct 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Defend Workflows “EDR Workflows” sub-team of Security Solution Team:Fleet Team label for Observability Data Collection Fleet team
Projects
None yet
Development

No branches or pull requests

5 participants