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.

Sign up for GitHub

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
+40 −0
Merged

api-breaker plugin returns default response body #6949

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
7a33476
api-breaker plugin returns default response body
Apr 27, 2022
367be74
fix docs's lint error
Apr 27, 2022
51a7710
fix error in api-breaker's ut
Apr 28, 2022
2c2f937
Update apisix/plugins/api-breaker.lua
qihaiyan Apr 28, 2022
80aeb8a
add test case to cover break_response_content_type
Apr 29, 2022
f623360
Update docs/en/latest/plugins/api-breaker.md
qihaiyan Apr 29, 2022
e594c34
Update docs/zh/latest/plugins/api-breaker.md
qihaiyan Apr 29, 2022
b345605
adding the headers field to add any response headers
May 9, 2022
f2ff4d3
revert check_schema
May 10, 2022
6325416
Update apisix/plugins/api-breaker.lua
qihaiyan May 10, 2022
e207ca0
change headers attribute to break_response_headers
May 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions apisix/plugins/api-breaker.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,13 @@ local schema = {
minimum = 200,
maximum = 599,
},
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 •
edited
Loading

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:

    apisix/apisix/control/router.lua

    Line 81 in cc88a8d

    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 •
edited
Loading

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 •
edited
Loading

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.

type = "string",
default = "application/json"
},
max_breaker_sec = {
type = "integer",
minimum = 3,
Expand Down Expand Up @@ -158,6 +165,11 @@ function _M.access(conf, ctx)

-- breaker
if lasttime + breaker_time >= ngx.time() then
if conf.break_response_body then
core.response.clear_header_as_body_modified()
qihaiyan marked this conversation as resolved.
Show resolved Hide resolved
ngx.header["Content-Type"] = conf.break_response_content_type
qihaiyan marked this conversation as resolved.
Show resolved Hide resolved
return conf.break_response_code, conf.break_response_body
end
return conf.break_response_code
end

Expand Down
2 changes: 2 additions & 0 deletions docs/en/latest/plugins/api-breaker.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ In an unhealthy state, when a request is forwarded to an upstream service and th
| Name | Type | Requirement | Default | Valid | Description |
| ----------------------- | ------------- | ----------- | -------- | --------------- | --------------------------------------------------------------------------- |
| break_response_code | integer | required | | [200, ..., 599] | Return error code when unhealthy |
| break_response_body | string | optional | | | Return reponse body when unhealthy |
| break_response_content_type | string | optional | application/json | | Return body's content type when unhealthy |
qihaiyan marked this conversation as resolved.
Show resolved Hide resolved
| max_breaker_sec | integer | optional | 300 | >=3 | Maximum breaker time(seconds) |
| unhealthy.http_statuses | array[integer] | optional | {500} | [500, ..., 599] | Status codes when unhealthy |
| unhealthy.failures | integer | optional | 3 | >=1 | Number of consecutive error requests that triggered an unhealthy state |
Expand Down
2 changes: 2 additions & 0 deletions docs/zh/latest/plugins/api-breaker.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ title: api-breaker
| 名称 | 类型 | 必选项 | 默认值 | 有效值 | 描述 |
| ----------------------- | -------------- | ------ | ---------- | --------------- | -------------------------------- |
| break_response_code | integer | 必须 | 无 | [200, ..., 599] | 不健康返回错误码 |
| break_response_body | string | 可选 | 无 | | 不健康返回报文 |
| break_response_content_type | string | 可选 | application/json | | 不健康返回Content-Type |
| max_breaker_sec | integer | 可选 | 300 | >=3 | 最大熔断持续时间 |
| unhealthy.http_statuses | array[integer] | 可选 | {500} | [500, ..., 599] | 不健康时候的状态码 |
| unhealthy.failures | integer | 可选 | 3 | >=1 | 触发不健康状态的连续错误请求次数 |
Expand Down
9 changes: 6 additions & 3 deletions t/plugin/api-breaker.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ done
--- request
GET /t
--- response_body
{"break_response_code":502,"healthy":{"http_statuses":[200],"successes":3},"max_breaker_sec":300,"unhealthy":{"failures":3,"http_statuses":[500]}}
{"break_response_code":502,"break_response_content_type":"application/json","healthy":{"http_statuses":[200],"successes":3},"max_breaker_sec":300,"unhealthy":{"failures":3,"http_statuses":[500]}}
--- no_error_log
[error]

Expand All @@ -104,7 +104,7 @@ GET /t
--- request
GET /t
--- response_body
{"break_response_code":502,"healthy":{"http_statuses":[200],"successes":3},"max_breaker_sec":300,"unhealthy":{"failures":3,"http_statuses":[500]}}
{"break_response_code":502,"break_response_content_type":"application/json","healthy":{"http_statuses":[200],"successes":3},"max_breaker_sec":300,"unhealthy":{"failures":3,"http_statuses":[500]}}
--- no_error_log
[error]

Expand All @@ -131,7 +131,7 @@ GET /t
--- request
GET /t
--- response_body
{"break_response_code":502,"healthy":{"http_statuses":[200],"successes":3},"max_breaker_sec":300,"unhealthy":{"failures":3,"http_statuses":[500]}}
{"break_response_code":502,"break_response_content_type":"application/json","healthy":{"http_statuses":[200],"successes":3},"max_breaker_sec":300,"unhealthy":{"failures":3,"http_statuses":[500]}}
--- no_error_log
[error]

Expand Down Expand Up @@ -458,6 +458,7 @@ breaker_time: 2
"plugins": {
"api-breaker": {
"break_response_code": 502,
"break_response_body": "{\"message\":\"breaker opened.\"}",
"unhealthy": {
"failures": 3
},
Expand Down Expand Up @@ -509,6 +510,8 @@ GET /api_breaker?code=500
]
--- error_code eval
[200, 500, 503, 500, 500, 502]
--- response_body_like eval
[".*", ".*", ".*", ".*", ".*", "{\"message\":\"breaker opened\"}"]
--- no_error_log
[error]

Expand Down