From 3aaac4566faba96de0bf9f8cb0dbed69e826ea6e Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Tue, 16 Jun 2020 21:31:05 +0200 Subject: [PATCH 1/5] Avoid log warning for using tls_context if the user didn't actually set it. Fixes #364 Signed-off-by: Otto van der Schaaf --- source/client/options_impl.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/source/client/options_impl.cc b/source/client/options_impl.cc index f2c8bd197..cce221353 100644 --- a/source/client/options_impl.cc +++ b/source/client/options_impl.cc @@ -671,7 +671,10 @@ CommandLineOptionsPtr OptionsImpl::toCommandLineOptionsInternal() const { request_options->mutable_request_body_size()->set_value(requestBodySize()); } } - *(command_line_options->mutable_tls_context()) = tls_context_; + if (!Envoy::MessageUtil()(tls_context_, + envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext())) { + *(command_line_options->mutable_tls_context()) = tls_context_; + } if (transport_socket_.has_value()) { *(command_line_options->mutable_transport_socket()) = transport_socket_.value(); } From 8660ab771657ba4c4ecd28f1022d2182db755550 Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Tue, 16 Jun 2020 21:37:07 +0200 Subject: [PATCH 2/5] Add comment Signed-off-by: Otto van der Schaaf --- source/client/options_impl.cc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/source/client/options_impl.cc b/source/client/options_impl.cc index cce221353..96e5d117e 100644 --- a/source/client/options_impl.cc +++ b/source/client/options_impl.cc @@ -671,6 +671,10 @@ CommandLineOptionsPtr OptionsImpl::toCommandLineOptionsInternal() const { request_options->mutable_request_body_size()->set_value(requestBodySize()); } } + + // Only set the tls context if it looks non-empty, to avoid a warning being logged about field + // deprecation. Ideally this would follow the way transport_socket uses absl::optional below. + // But as this field is about to get eliminated this minimal effort shortcut may be more suitable. if (!Envoy::MessageUtil()(tls_context_, envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext())) { *(command_line_options->mutable_tls_context()) = tls_context_; From 895ac4f8d4e28b371b1533f103c8ea4565b0f1fd Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Tue, 16 Jun 2020 21:39:58 +0200 Subject: [PATCH 3/5] Fix format Signed-off-by: Otto van der Schaaf --- source/client/options_impl.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/client/options_impl.cc b/source/client/options_impl.cc index 96e5d117e..abe16325d 100644 --- a/source/client/options_impl.cc +++ b/source/client/options_impl.cc @@ -671,12 +671,12 @@ CommandLineOptionsPtr OptionsImpl::toCommandLineOptionsInternal() const { request_options->mutable_request_body_size()->set_value(requestBodySize()); } } - + // Only set the tls context if it looks non-empty, to avoid a warning being logged about field // deprecation. Ideally this would follow the way transport_socket uses absl::optional below. // But as this field is about to get eliminated this minimal effort shortcut may be more suitable. if (!Envoy::MessageUtil()(tls_context_, - envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext())) { + envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext())) { *(command_line_options->mutable_tls_context()) = tls_context_; } if (transport_socket_.has_value()) { From 6b8a0889c10225a3f036c8c098571832f03b900c Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Thu, 18 Jun 2020 11:51:43 +0200 Subject: [PATCH 4/5] Review: clearer message comparison Signed-off-by: Otto van der Schaaf --- source/client/options_impl.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/source/client/options_impl.cc b/source/client/options_impl.cc index abe16325d..046c9428c 100644 --- a/source/client/options_impl.cc +++ b/source/client/options_impl.cc @@ -672,11 +672,11 @@ CommandLineOptionsPtr OptionsImpl::toCommandLineOptionsInternal() const { } } - // Only set the tls context if it looks non-empty, to avoid a warning being logged about field - // deprecation. Ideally this would follow the way transport_socket uses absl::optional below. + // Only set the tls context if needed, to avoid a warning being logged about field deprecation. + // Ideally this would follow the way transport_socket uses absl::optional below. // But as this field is about to get eliminated this minimal effort shortcut may be more suitable. - if (!Envoy::MessageUtil()(tls_context_, - envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext())) { + if (!Envoy::Protobuf::util::MessageDifferencer::Equivalent( + tls_context_, envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext())) { *(command_line_options->mutable_tls_context()) = tls_context_; } if (transport_socket_.has_value()) { From f467e86d64ee70f3bfaba391bfbb31ba5a5a16f6 Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Thu, 18 Jun 2020 23:53:23 +0200 Subject: [PATCH 5/5] Review feedback Signed-off-by: Otto van der Schaaf --- source/client/options_impl.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/source/client/options_impl.cc b/source/client/options_impl.cc index ae90c96d7..ac537f02a 100644 --- a/source/client/options_impl.cc +++ b/source/client/options_impl.cc @@ -697,8 +697,7 @@ CommandLineOptionsPtr OptionsImpl::toCommandLineOptionsInternal() const { // Only set the tls context if needed, to avoid a warning being logged about field deprecation. // Ideally this would follow the way transport_socket uses absl::optional below. // But as this field is about to get eliminated this minimal effort shortcut may be more suitable. - if (!Envoy::Protobuf::util::MessageDifferencer::Equivalent( - tls_context_, envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext())) { + if (tls_context_.ByteSizeLong() > 0) { *(command_line_options->mutable_tls_context()) = tls_context_; } if (transport_socket_.has_value()) {