Skip to content

Fix ConnectionHandlerImpl matching correct any address#19802

Merged
ggreenway merged 6 commits intoenvoyproxy:mainfrom
soulxu:check_listener
Feb 9, 2022
Merged

Fix ConnectionHandlerImpl matching correct any address#19802
ggreenway merged 6 commits intoenvoyproxy:mainfrom
soulxu:check_listener

Conversation

@soulxu
Copy link
Copy Markdown
Member

@soulxu soulxu commented Feb 3, 2022

Commit Message: Fix ConnectionHandlerImpl matching correct any address
Additional Description:

Risk Level:low
Testing: unittest
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A

Signed-off-by: He Jie Xu hejie.xu@intel.com
Co-authored-by: Yuchen Dai silentdai@gmail.com

Signed-off-by: He Jie Xu <hejie.xu@intel.com>
@soulxu
Copy link
Copy Markdown
Member Author

soulxu commented Feb 3, 2022

@howardjohn I don't know if there is way to test this fix on the istio side, otherwise revert the #19362 is the right choice. Hope this can fix the problem. Sorry for making this regression.

cc @lambdai @mattklein123

@howardjohn
Copy link
Copy Markdown
Contributor

I can try it out hopefully sometime today

@soulxu
Copy link
Copy Markdown
Member Author

soulxu commented Feb 3, 2022

/wait

@soulxu
Copy link
Copy Markdown
Member Author

soulxu commented Feb 3, 2022

I can try it out hopefully sometime today

thanks

Copy link
Copy Markdown
Contributor

@lambdai lambdai left a comment

Choose a reason for hiding this comment

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

Thank you! I think that is the reasonable fix

Signed-off-by: He Jie Xu <hejie.xu@intel.com>
Signed-off-by: He Jie Xu <hejie.xu@intel.com>
@soulxu
Copy link
Copy Markdown
Member Author

soulxu commented Feb 3, 2022

/wait

waiting for istio side test.

@howardjohn
Copy link
Copy Markdown
Contributor

I think its still failing...

@soulxu soulxu closed this Feb 4, 2022
Signed-off-by: He Jie Xu <hejie.xu@intel.com>
Signed-off-by: He Jie Xu <hejie.xu@intel.com>
@soulxu soulxu reopened this Feb 7, 2022
@soulxu soulxu changed the title listener: check the listener isn't stopped when balance the request Fix ConnectionHandlerImpl matching correct any address Feb 7, 2022
@soulxu
Copy link
Copy Markdown
Member Author

soulxu commented Feb 7, 2022

cc @ggreenway

Signed-off-by: He Jie Xu <hejie.xu@intel.com>
@ggreenway
Copy link
Copy Markdown
Member

@howardjohn can you test this branch and see if it fixes the Istio issues?

@howardjohn
Copy link
Copy Markdown
Contributor

@howardjohn can you test this branch and see if it fixes the Istio issues?

We don't have the best infrastructure to test arbitrary Envoy builds right now, and the issue was not 100% reproducible (but close), but it appears to be fixed. About 90% confident (where 100% confidence will come after it lands and our CI pulls it in). Thanks for all the work here!

Copy link
Copy Markdown
Member

@ggreenway ggreenway left a comment

Choose a reason for hiding this comment

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

LGTM. @lambdai any other comments?

/wait-any

@soulxu
Copy link
Copy Markdown
Member Author

soulxu commented Feb 9, 2022

@howardjohn can you test this branch and see if it fixes the Istio issues?

We don't have the best infrastructure to test arbitrary Envoy builds right now, and the issue was not 100% reproducible (but close), but it appears to be fixed. About 90% confident (where 100% confidence will come after it lands and our CI pulls it in). Thanks for all the work here!

Is that possible I compiled an envoy and upload somewhere, then I trigger by a PR on istio side to modify those address
https://github.com/istio/istio/blob/e448c96013adc84de2864c586d9012f9aad7262c/Makefile.core.mk#L130-L134

@lambdai
Copy link
Copy Markdown
Contributor

lambdai commented Feb 9, 2022

LGTM. @lambdai any other comments?

/wait-any

LGTM. Thanks!

@lambdai
Copy link
Copy Markdown
Contributor

lambdai commented Feb 9, 2022

@howardjohn can you test this branch and see if it fixes the Istio issues?

We don't have the best infrastructure to test arbitrary Envoy builds right now, and the issue was not 100% reproducible (but close), but it appears to be fixed. About 90% confident (where 100% confidence will come after it lands and our CI pulls it in). Thanks for all the work here!

Is that possible I compiled an envoy and upload somewhere, then I trigger by a PR on istio side to modify those address https://github.com/istio/istio/blob/e448c96013adc84de2864c586d9012f9aad7262c/Makefile.core.mk#L130-L134

the istio/proxy commits could use any commit in envoyproxy/proxy, but only envoy maintainers can create such a commit.

Also, for this particular failure, istio/proxy doesn't detect either.

@ggreenway ggreenway merged commit 43f759b into envoyproxy:main Feb 9, 2022
@ggreenway
Copy link
Copy Markdown
Member

@howardjohn please let us know if the istio issue is resolved, after you're able to test with this commit.

@howardjohn
Copy link
Copy Markdown
Contributor

We are good to go 👍 . Thanks everyone

@soulxu
Copy link
Copy Markdown
Member Author

soulxu commented Feb 9, 2022

We are good to go 👍 . Thanks everyone

thanks!

joshperry pushed a commit to joshperry/envoy that referenced this pull request Feb 13, 2022
)

Signed-off-by: He Jie Xu <hejie.xu@intel.com>
Signed-off-by: Josh Perry <josh.perry@mx.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.

4 participants