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

api-breaker plugin returns default response body #6949

Merged
merged 11 commits into from
May 10, 2022
Merged

api-breaker plugin returns default response body #6949

merged 11 commits into from
May 10, 2022

Conversation

qihaiyan
Copy link
Contributor

Description

When upstream is in the unhealthy state, the api-breaker returns unhealthy.http_statuses only, without a response body.
In order to be more compatible to the client, return a default response body is useful.

Fixes #6923

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

break_response_body = {
type = "string"
},
break_response_content_type = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Apache APISIX has a few plugins that define the response body but without setting the content type.

I'm thinking about that, if we need to keep the behaviors consistence?

Copy link
Contributor Author

@qihaiyan qihaiyan Apr 27, 2022

Choose a reason for hiding this comment

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

If the body is json, and the content-type is text/html, some APP clients may report error, and the terminal user would be affected.

Copy link
Contributor

Choose a reason for hiding this comment

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

This problem cannot be solved even if we provide this field. People have to know how to configure it.

For these kinds of returning body, I think we may need a unified way to handle the Content-Type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This problem cannot be solved even if we provide this field. People have to know how to configure it.

For these kinds of returning body, I think we may need a unified way to handle the Content-Type.

A unified way is good, but it's not that easy. Modern API returns application/json, but some legacy API returns application/soap+xml like SOAP.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the API owner should be responsible to configure route on the apigageway correctly , it's part of the developing work, and only the API owner knows what the correct Content-Type should be. If there's no way to config the Content-Type correctly, when break state is open, the client APP used this API may crash because of the unexpected Content-Type.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, i'll browse all the plugins which should be with a content-type field and submit an issue with a checklist.

It is not a good idea, as this check won't fix the non-open-source plugins.

One solution is to guess:

  1. according to the response body:
    if type(body) == "table" and ngx.header["Content-Type"] == nil then
    or using https://github.com/spacewander/lua-resty-mime-sniff
  2. according to the Accept header or the ext part of the uri

Another solution is to configure it in the route

Copy link
Member

Choose a reason for hiding this comment

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

As for this plugin, I would suggest adding a field to add any response headers instead of just content-type.

Copy link
Contributor Author

@qihaiyan qihaiyan May 2, 2022

Choose a reason for hiding this comment

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

As for this plugin, I would suggest adding a field to add any response headers instead of just content-type.

Is there any other plugin with such a field? I'm not sure of which field is suitable for representing any response headers. @spacewander

Copy link
Contributor Author

@qihaiyan qihaiyan May 8, 2022

Choose a reason for hiding this comment

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

As for this plugin, I would suggest adding a field to add any response headers instead of just content-type.

Can we use the headers field to represent any response headers? I found the echo plugin and also the response-rewrite plugin using this attribute. @spacewander

Name Type Requirement Default Valid Description
headers array optional     New headers for response. Using an array so multiple response headers with the same name can be used

Copy link
Member

Choose a reason for hiding this comment

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

Yes. And we should use an array so multiple response headers with the same name can be used.

@tzssangglass
Copy link
Member

Hi @qihaiyan , can we add a test cases to cover break_response_content_type ?

Co-authored-by: tzssangglass <tzssangglass@gmail.com>
@qihaiyan
Copy link
Contributor Author

Hi @qihaiyan , can we add a test cases to cover break_response_content_type ?

Sure, i'll add the test cases tomorrow.

@qihaiyan
Copy link
Contributor Author

Can you help to check why the chaos-test failed? @tzssangglass
https://github.com/apache/apisix/runs/6222619168?check_suite_focus=true
image

@spacewander
Copy link
Member

Can you help to check why the chaos-test failed? @tzssangglass https://github.com/apache/apisix/runs/6222619168?check_suite_focus=true image

The chaos test is not so stable. I have rerun it.

@qihaiyan
Copy link
Contributor Author

The chaos test is not so stable. I have rerun it.
ok, thanks.

docs/en/latest/plugins/api-breaker.md Outdated Show resolved Hide resolved
docs/zh/latest/plugins/api-breaker.md Outdated Show resolved Hide resolved
qihaiyan and others added 2 commits April 29, 2022 17:35
Co-authored-by: Alex Zhang <tokers@apache.org>
Co-authored-by: Alex Zhang <tokers@apache.org>
tokers
tokers previously approved these changes May 2, 2022
tzssangglass
tzssangglass previously approved these changes May 9, 2022
@qihaiyan
Copy link
Contributor Author

qihaiyan commented May 9, 2022

This PR‘s status is Changes approved current now. Can i still add more commits to this PR to aim at @spacewander 's suggestion? @tzssangglass .

As for this plugin, I would suggest adding a field to add any response headers instead of just content-type.

Copy link
Member

@spacewander spacewander left a comment

Choose a reason for hiding this comment

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

Yes, let's address #6949 (comment)

@qihaiyan qihaiyan dismissed stale reviews from tzssangglass and tokers via b345605 May 9, 2022 13:31
apisix/plugins/api-breaker.lua Outdated Show resolved Hide resolved
apisix/plugins/api-breaker.lua Outdated Show resolved Hide resolved
apisix/plugins/api-breaker.lua Outdated Show resolved Hide resolved
@spacewander spacewander merged commit 01a6ab4 into apache:master May 10, 2022
@qihaiyan qihaiyan deleted the feat-6923 branch May 10, 2022 06:58
Liu-Junlin pushed a commit to Liu-Junlin/apisix that referenced this pull request May 20, 2022
Co-authored-by: qihaiyan <qihaiyan@hisense.com>
Co-authored-by: tzssangglass <tzssangglass@gmail.com>
Co-authored-by: Alex Zhang <tokers@apache.org>
Co-authored-by: 罗泽轩 <spacewanderlzx@gmail.com>
spacewander added a commit that referenced this pull request Jun 30, 2022
Co-authored-by: qihaiyan <qihaiyan@hisense.com>
Co-authored-by: tzssangglass <tzssangglass@gmail.com>
Co-authored-by: Alex Zhang <tokers@apache.org>
Co-authored-by: 罗泽轩 <spacewanderlzx@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: As a user, I want to let api-breaker plugin return default response body
4 participants