Deprecate cluster's and listener's tls_context in favor of transport socket#17163
Merged
htuch merged 13 commits intoenvoyproxy:mainfrom Jul 9, 2021
Merged
Deprecate cluster's and listener's tls_context in favor of transport socket#17163htuch merged 13 commits intoenvoyproxy:mainfrom
tls_context in favor of transport socket#17163htuch merged 13 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Tianyu Xia <tyxia@google.com>
Signed-off-by: Tianyu Xia <tyxia@google.com>
socket Signed-off-by: Tianyu Xia <tyxia@google.com>
Signed-off-by: Tianyu Xia <tyxia@google.com>
tyxia
requested review from
alyssawilk,
asraa,
ggreenway,
lizan and
mattklein123
as code owners
Member
Author
|
@htuch Please take a look. Thanks! |
… output parameters and remove default arg Signed-off-by: Tianyu Xia <tyxia@google.com>
Member
Author
|
/retest |
|
Retrying Azure Pipelines: |
htuch
suggested changes
Jun 29, 2021
Member
htuch
left a comment
htuch
left a comment
There was a problem hiding this comment.
Looks good! A few comments..
/wait
Signed-off-by: Tianyu Xia <tyxia@google.com>
Member
Author
Signed-off-by: Tianyu Xia <tyxia@google.com>
Signed-off-by: Tianyu Xia <tyxia@google.com>
htuch
suggested changes
Jul 1, 2021
Member
htuch
left a comment
htuch
left a comment
There was a problem hiding this comment.
LGTM modulo few remaining nits and CI passing.
/wait
Signed-off-by: Tianyu Xia <tyxia@google.com>
Member
Author
|
/retest |
|
Retrying Azure Pipelines: |
Member
Author
|
/retest |
|
Retrying Azure Pipelines: |
Member
Author
|
Thanks for the review! The failures in previous CI run were caused by tests' flakiness. All the tests passed in the most recent CI run |
yanavlasov
approved these changes
Jul 8, 2021
htuch
approved these changes
Jul 9, 2021
leyao-daily
pushed a commit
to leyao-daily/envoy
that referenced
this pull request
Sep 30, 2021
…t socket (envoyproxy#17163) Remove cluster's and listener's tls_context since we are using transport socket's tls_context/ Refactor the sslSocketTest : centralize the server configuration in one place --configureServerAndExpiredClientCertificate function. Note: The test could be further refactored by leveraging this function I modified, but I feel that is not worth the large amount of effort and current code also provides a bit flexibility of configuration (e.g. specifies various config like cert_hash, cert_spki in place) Move createProtocolTestOptions to unnamed namespace. Even though superiority of unnamed namespace over static is more applied to user-defined types rather than variables and functions (i.e. static no longer deprecated in standard and should do the same thing for latter two), it is still good to keep it in the unnamed namespace like other internal helper functions . Also, unnamed namespace should be encouraged for such usage in general Risk Level: Low Testing: Local tests and CI run (All tests passed) Signed-off-by: Tianyu Xia <tyxia@google.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Remove cluster's and listener's
tls_contextsince we are using transport socket's tls_context/Refactor the sslSocketTest : centralize the server configuration in one place --
configureServerAndExpiredClientCertificatefunction. Note: The test could be further refactored by leveraging this function I modified, but I feel that is not worth the large amount of effort and current code also provides a bit flexibility of configuration (e.g. specifies various config like cert_hash, cert_spki in place)Move
createProtocolTestOptionsto unnamed namespace. Even though superiority of unnamed namespace over static is more applied to user-defined types rather than variables and functions (i.e.staticno longer deprecated in standard and should do the same thing for latter two), it is still good to keep it in the unnamed namespace like other internal helper functions . Also, unnamed namespace should be encouraged for such usage in generalSigned-off-by: Tianyu Xia tyxia@google.com
Risk Level: Low
Testing: Local tests and CI run (All tests passed)