-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
tls: fix duplicated ':TLS_error_end' suffix in socket failure reason #34799
Conversation
This fix also prevents duplicated debug logs, since they are always generated after the end suffix is appended to the failure reason. Signed-off-by: Joe Kralicky <joekralicky@gmail.com>
/assign @botengyao Could you take a first pass on this? as you worked on this before. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks great, thanks for working on this!
} else { | ||
EXPECT_THAT(log_result, | ||
StartsWith("DOWNSTREAM_TRANSPORT_FAILURE_REASON=TLS_error:|268435640:" | ||
"SSL_routines:OPENSSL_internal:NO_SHARED_CIPHER:TLS_error_end")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is previous test ending with 2 TLS_error_ends here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the full log_result text here (in main
) is
"DOWNSTREAM_TRANSPORT_FAILURE_REASON=TLS_error:|268435640:SSL_routines:OPENSSL_internal:NO_SHARED_CIPHER:TLS_error_end:TLS_error_end FILTER_CHAIN_NAME=-"
but it passed since it was only checking the prefix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm, thanks for the fix!
defer to @tyxia for another pass.
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thanks!
…nvoyproxy#34799) This fix also prevents duplicated debug logs, since they are always generated after the end suffix is appended to the failure reason. Signed-off-by: Joe Kralicky <joekralicky@gmail.com> Signed-off-by: Fernando Cainelli <fernando.cainelli-external@getyourguide.com>
…nvoyproxy#34799) This fix also prevents duplicated debug logs, since they are always generated after the end suffix is appended to the failure reason. Signed-off-by: Joe Kralicky <joekralicky@gmail.com> Signed-off-by: Fernando Cainelli <fernando.cainelli-external@getyourguide.com>
…nvoyproxy#34799) This fix also prevents duplicated debug logs, since they are always generated after the end suffix is appended to the failure reason. Signed-off-by: Joe Kralicky <joekralicky@gmail.com> Signed-off-by: Fernando Cainelli <fernando.cainelli-external@getyourguide.com>
…nvoyproxy#34799) This fix also prevents duplicated debug logs, since they are always generated after the end suffix is appended to the failure reason. Signed-off-by: Joe Kralicky <joekralicky@gmail.com> Signed-off-by: Fernando Cainelli <fernando.cainelli-external@getyourguide.com>
Commit Message: tls: fix duplicated ':TLS_error_end' suffix in socket failure reason
This fix also prevents duplicated debug logs, since they are always generated after the end suffix is appended to the failure reason.
Additional Description:
Follow-up to #34741
Risk Level: low (no guarantees are made about the format of this message); searching the string
:TLS_error_end:TLS_error_end
in github returns no results other than the test code in the PR linked aboveTesting: updated existing tests that checked this string
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]