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

HTTP Response StatusCodes from EventListener #931

Closed
dibyom opened this issue Jan 27, 2021 · 1 comment
Closed

HTTP Response StatusCodes from EventListener #931

dibyom opened this issue Jan 27, 2021 · 1 comment
Assignees
Labels
kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/feature Categorizes issue or PR as related to a new feature.
Milestone

Comments

@dibyom
Copy link
Member

dibyom commented Jan 27, 2021

Background

Today the EventListener waits for at least one resource to be created before returning a HTTP response. We return 201 if a resource is created and 202 otherwise. If we fail to create a resource due to an auth error, we return a 401 response instead.

Note that the 201 status code, the user still may not know exactly what resource was created. They'd still have to look into the EL logs (using the eventID from the HTTP response) to find out which resource was created.

Problem

  1. If an EL is handling many triggers (or a single trigger had many interceptor calls), the EL will take a long time to respond and may even time out. The webhook originator (e.g. GitHub) don't generally care about the response codes (unless its 5xx)

  2. The 201 status code gives the impression that everything succeeded while that may not be the case always e.g. one trigger might succeed while another failed (see EventListener return a 2xx status on failure #656, Add Error Msg and BadRequest for Parsing error #758)

Proposal

I'd propose we return a response back as soon as the EL has read the body. We'd return a 2xx if we can fully read the body as a valid JSON document, and 4xx otherwise. We'd still return an eventID (and the name and namespace of the EventListener) so that the user can track the progress of the trigger.

@dibyom dibyom added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/feature Categorizes issue or PR as related to a new feature. labels Jan 27, 2021
@dibyom dibyom added this to the Triggers v0.14 milestone Apr 21, 2021
@dibyom
Copy link
Member Author

dibyom commented May 17, 2021

We are going to publicize the status change a bit and then put it in a release.

@dibyom dibyom modified the milestones: Triggers v0.14, Triggers Beta May 17, 2021
dibyom pushed a commit to dibyom/triggers that referenced this issue Jun 24, 2021
In the current version of the product, the HTTP request to the
EventListener endpoint waits for all triggers to process to determine if
a resource was created. This is not in line with the suggestion by the
source control systems for webhook endpoints. This change updates the
endpoint to respond as soon as the triggers have been selected for
processing.

BREAKING CHANGE: With this change the EventListener will stop responding
with `201 Created` status code when it creates resources. Instead it
will always respond with a `202 Accepted` response code.

Fixes tektoncd#931
dibyom pushed a commit to dibyom/triggers that referenced this issue Jun 24, 2021
In the current version of the product, the HTTP request to the
EventListener endpoint waits for all triggers to process to determine if
a resource was created. This is not in line with the suggestion by the
source control systems for webhook endpoints. This change updates the
endpoint to respond as soon as the triggers have been selected for
processing.

BREAKING CHANGE: With this change the EventListener will stop responding
with `201 Created` status code when it creates resources. Instead it
will always respond with a `202 Accepted` response code.

Fixes tektoncd#931
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants