tls: future-proof Utility::getErrorDescription#16553
Merged
ggreenway merged 1 commit intoenvoyproxy:mainfrom May 27, 2021
Merged
tls: future-proof Utility::getErrorDescription#16553ggreenway merged 1 commit intoenvoyproxy:mainfrom
ggreenway merged 1 commit intoenvoyproxy:mainfrom
Conversation
As with any other dependency, BoringSSL is not a fixed thing. envoyproxy#14600 added an enumeration over all BoringSSL errors. This incorrectly assumes we'd never add more errors, and unnecessarily adds an dependency on errors (e.g. SSL_ERROR_WANT_CHANNEL_ID_LOOKUP) that Envoy will never encounter and may be removed in the future. Instead, the correct function is SSL_error_description. The original code enumerated errors because Envoy tries to support an old version of BoringSSL, but in that case the future-proof scheme would be to use a BORINGSSL_API_VERSION ifdef. Next, this rewrites the test. The tests assume SSL_ERROR_* constants are stable, which is invalid, and they assume that 19 will never be allocated when it has been and, in fact, we allocate them consecutively. Instead, use the constants, test a few error codes that Envoy already depends on, and use -1 as the sample unknown error. This ensures Envoy's logging reflect future values BoringSSL may add and avoids this code breaking Envoy in a future version of BoringSSL. Signed-off-by: David Benjamin <davidben@google.com>
ggreenway
approved these changes
May 26, 2021
Member
ggreenway
left a comment
ggreenway
left a comment
There was a problem hiding this comment.
This looks great; thanks for the cleanup!
/retest
|
Retrying Azure Pipelines: |
leyao-daily
pushed a commit
to leyao-daily/envoy
that referenced
this pull request
Sep 30, 2021
As with any other dependency, BoringSSL is not a fixed thing. envoyproxy#14600 added an enumeration over all BoringSSL errors. This incorrectly assumes we'd never add more errors, and unnecessarily adds an dependency on errors (e.g. SSL_ERROR_WANT_CHANNEL_ID_LOOKUP) that Envoy will never encounter and may be removed in the future. Instead, the correct function is SSL_error_description. The original code enumerated errors because Envoy tries to support an old version of BoringSSL, but in that case the future-proof scheme would be to use a BORINGSSL_API_VERSION ifdef. Next, this rewrites the test. The tests assume SSL_ERROR_* constants are stable, which is invalid, and they assume that 19 will never be allocated when it has been and, in fact, we allocate them consecutively. Instead, use the constants, test a few error codes that Envoy already depends on, and use -1 as the sample unknown error. This ensures Envoy's logging reflect future values BoringSSL may add and avoids this code breaking Envoy in a future version of BoringSSL. Signed-off-by: David Benjamin <davidben@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
3 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.
Commit Message:
As with any other dependency, BoringSSL is not a fixed thing.
#14600 added an enumeration over
all BoringSSL errors. This incorrectly assumes we'd never add more
errors, and unnecessarily adds an dependency on errors (e.g.
SSL_ERROR_WANT_CHANNEL_ID_LOOKUP) that Envoy will never encounter and
may be removed in the future.
Instead, the correct function is SSL_error_description. The original
code enumerated errors because Envoy tries to support an old version of
BoringSSL, but in that case the future-proof scheme would be to use a
BORINGSSL_API_VERSION ifdef.
Next, this rewrites the test. The tests assume SSL_ERROR_* constants are
stable, which is invalid, and they assume that 19 will never be
allocated when it has been and, in fact, we allocate them consecutively.
Instead, use the constants, test a few error codes that Envoy already
depends on, and use -1 as the sample unknown error.
This ensures Envoy's logging reflect future values BoringSSL may add and
avoids this code breaking Envoy in a future version of BoringSSL.
Signed-off-by: David Benjamin davidben@google.com
Additional Description:
Risk Level: Low
Testing: bazel test //test/extensions/transport_sockets/tls/...
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A