api icon indicating copy to clipboard operation
api copied to clipboard

Change dr MaxConnection data type from int32 to uint32

Open ningyougang opened this issue 2 years ago • 6 comments

  • [x] Change dr tcpSetting's MaxConnection int type from int32 to uint32

    Refer to: https://istio.io/v1.5/docs/reference/config/networking/destination-rule/#ConnectionPoolSettings-TCPSettings image

    When configure a value to above red part value: 4294967295, the value can't be parsed due to int32 can't meet it, so need to use int64 to meet it.

ningyougang avatar Nov 18 '21 05:11 ningyougang

Hi @ningyougang. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

istio-testing avatar Nov 18 '21 05:11 istio-testing

We could not change the type of an API. The confusing docs actually means, if no value set, the value that istio applied to envoy is max uint32(2^32-1)

hzxuzhonghu avatar Nov 18 '21 07:11 hzxuzhonghu

We could not change the type of an API. The confusing docs actually means, if no value set, the value that istio applied to envoy is max uint32(2^32-1)

Thanks for your reply.

ningyougang avatar Nov 18 '21 08:11 ningyougang

Thanks for the PR!

I am not sure this is backwards compatible in protobuf? Envoy's is a uint32 so I am not sure using int64 here is appropriate as well

uint32 type is in [0 , 4294967295] , also meet the scope. I already changed it to uint32

Consider the compatible issue, unit32 is better.

ningyougang avatar Nov 19 '21 01:11 ningyougang

Per proto, this does seem ok (https://developers.google.com/protocol-buffers/docs/proto3#updating). However, I am not sure I understand the motivation. Do you really need a number that int32 will not satisfy?

The motivation is the int32 is not enough for below value image so it is better to change it from int32 to uint32.

Not only maxConnection field exist the problem, but also other fields as below also exist the same issue. image

hm..it is better to change these fileds's data type from int32 to unit32 both.

btw, regarding Per proto, this does seem ok You mean need some changes for some proto file? but i didn't find it.

ningyougang avatar Nov 22 '21 00:11 ningyougang

@ningyougang: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

istio-testing avatar Mar 23 '22 22:03 istio-testing

🚧 This issue or pull request has been closed due to not having had activity from an Istio team member since 2021-11-19. If you feel this issue or pull request deserves attention, please reopen the issue. Please see this wiki page for more information. Thank you for your contributions.

Created by the issue and PR lifecycle manager.

istio-policy-bot avatar May 15 '24 23:05 istio-policy-bot