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

[health_check] validate form item "checks.active.port" #2019

Closed
uriddle opened this issue Jul 30, 2021 · 12 comments · Fixed by #2042
Closed

[health_check] validate form item "checks.active.port" #2019

uriddle opened this issue Jul 30, 2021 · 12 comments · Fixed by #2042
Labels
bug Something isn't working good first issue Good for newcomers
Milestone

Comments

@uriddle
Copy link

uriddle commented Jul 30, 2021

Issue description

upstream 节点健康检查在不配置 active.port 的时候,校验出错

QV JGL)P 6NBXR3ZNGETC3

Expected behavior

官网介绍:
active.port: 用于发现 upstream 节点健康可用的自定义主机端口(可选),配置此项会覆盖 upstream 节点中的端口。

@uriddle uriddle added the bug Something isn't working label Jul 30, 2021
@juzhiyuan
Copy link
Member

cc @Baoyuantop to have a check

@uriddle
Copy link
Author

uriddle commented Jul 30, 2021

版本:
apisix-dashboard:2.7

@Baoyuantop
Copy link
Contributor

Hi @uriddle, In order to better help you solve the problem, can you provide me how to Reproduce ?

for example :

  1. Go to '...'
  2. Click on '....'
  3. Scroll down to '....'
  4. See error

@uriddle
Copy link
Author

uriddle commented Jul 30, 2021

How to Reproduce

  1. Go to "Upstream" and then click button "configure"
    image

  2. Click on 'configure ' and Scroll down to "Health Check", click button “Active”
    image

  3. leave “Port” empty.

the health check host port is optional ,so I don‘t want to change it.

finally,submit

  1. See error
    image

@Baoyuantop
Copy link
Contributor

Thank you @uriddle .

When create or update Upstream, the port option in Health Check is optional.

Dashboard will provide a default value 80, but when the user changes it to empty, dashboard will send null to api, this will cause this error.

The possible solution is to not pass the port option to the api when the user sets it to null, but how will we save the port status as null?

what do you think about it ? @uriddle @juzhiyuan

@uriddle
Copy link
Author

uriddle commented Jul 30, 2021

Maybe we can deal with “Port” just like “Host” option .

@juzhiyuan
Copy link
Member

Hi @Baoyuantop, all schema in Apache APISIX is in here[1].

I just checked the health_checker schema, and the port field is optional indeed, I'm not sure what will happen if we don't pass the port field 🤔 need more search on Apache APISIX.

BTW, the Web should omit all null values when sending a Request

[1] https://github.com/apache/apisix/blob/master/apisix/schema_def.lua#L139-L142

@Baoyuantop
Copy link
Contributor

Hi @Baoyuantop, all schema in Apache APISIX is in here[1].

I just checked the health_checker schema, and the port field is optional indeed, I'm not sure what will happen if we don't pass the port field 🤔 need more search on Apache APISIX.

BTW, the Web should omit all null values when sending a Request

[1] https://github.com/apache/apisix/blob/master/apisix/schema_def.lua#L139-L142

I tested the api, it returns 200 when I don't pass the port field, so it's a good idea to omit all null values

@juzhiyuan
Copy link
Member

ok, you may need to omit all null values from a nested object value

@juzhiyuan juzhiyuan added good first issue Good for newcomers and removed checking labels Jul 30, 2021
@juzhiyuan juzhiyuan added this to the 2.7.1 milestone Jul 30, 2021
@Baoyuantop
Copy link
Contributor

I found that in the entire web if there is an optional InputNumber component, this bug can be reproduced.

Therefore, I think that all attributes in an object with null values in the request body should be filtered out by requestInterceptors.

What do y'all think?

@juzhiyuan
Copy link
Member

yes, the Request Interceptor is also a good place to omit all undefined & null values.

@liuxiran
Copy link
Contributor

liuxiran commented Aug 7, 2021

Hi @uriddle , since pr #2042 got merged, you can retry this issue after version 2.7.1 released or just cherry-pick the related patch to your local env to hot fix this issue. Hope that will help you, reopen it when you have any questions, thanks a lot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants