move boringssl dependencies to extensions by bdecoste · Pull Request #5543 · envoyproxy/envoy

bdecoste · 2019-01-08T19:20:20Z

Signed-off-by: William DeCoste bdecoste@gmail.com

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Description: As part of the work to support openssl this PR moves all boringssl dependencies to extensions. This is one step that will enable the linking and testing of openssl with a new transport socket for openssl. Those files with boringssl dependencies were moved from common/ssl to extensions/transport_sockets/ssl with similar changes for the tests. Please see #5524 for additional context.
Risk Level: Low
Testing: Standard tests passed
Docs Changes: None
Release Notes: None
[Optional Fixes #Issue]
[Optional Deprecated:]

PiotrSikora · 2019-01-08T19:49:45Z

/assign @lizan @PiotrSikora

lizan · 2019-01-08T21:16:01Z

/retest

repokitteh-read-only · 2019-01-08T21:16:05Z

🔨 rebuilding ci/circleci: release (failed build)

🐱

Caused by: a #5543 (comment) was created by @lizan.

see: more, trace.

lizan · 2019-01-08T21:17:47Z

At a glance this looks good, @bdecoste can you open an issue to track the overall work to make BoringSSL can be compiled out? i.e. what is the next step of this PR etc, based on the conversation in KubeCon last month.

bdecoste · 2019-01-08T21:41:55Z

#5545

lizan · 2019-01-08T22:32:20Z

source/extensions/transport_sockets/ssl/context_config_impl.cc

Ideally I think we want the namespaces to be consistent with the path, do you want to do that in this PR or in next PR? I'm fine either.

PiotrSikora

Could you move it to extensions/transport_sockets/tls instead, so that it matches naming we use in the API? Thanks!

Also, changing namespaces would be great.

bdecoste · 2019-01-09T00:54:21Z

Thanks you. Just to confirm: You want the directory changed to extensions/transport_sockets/tls and the corresponding namespace to be Envoy::Extensions::TransportSockets::Tls?

Thanks -Bill

PiotrSikora · 2019-01-09T01:30:47Z

@bdecoste yes, if that's not too much work (there is already config in extensions/transport_sockets/ssl, which you'd need to move as well). Thanks!

lizan · 2019-01-09T01:31:51Z

SGTM, the transport socket is already named as tls. Thank you!

bdecoste · 2019-01-09T13:52:41Z

Great, thank you. I will make the changes and update the PR asap. The transport socket namespace in the extensions/transport_socket/ssl directory was Envoy::Extensions::TransportSockets::SslTransport so I will move and rename.

bdecoste · 2019-01-09T14:12:39Z

Do you also want me to change include/envoy/ssl and Envoy::Ssl to include/envoy/tls and Envoy::Tls? Thanks

PiotrSikora · 2019-01-09T14:29:22Z

@bdecoste I don't particularly care if it's done as part of this PR or another, but it should be renamed before OpenSSL extension starts including it.

bdecoste · 2019-01-09T15:49:39Z

OK, thanks. I will update the include as the next PR. This one is big enough already :-)

lizan

LGTM, sorry for the delay.

bdecoste · 2019-01-11T13:16:03Z

Thanks Lizan. As soon as this is merged, I'll put in another PR for moving the remaining file/namespaces from ssl to tls. I have this all ready and tested locally.

PiotrSikora · 2019-01-09T23:35:28Z

source/server/config_validation/server.cc

Nit: I think you can omit Envoy:: here (and everywhere else), Extensions::TransportSockets::Tls:: should also work, though I'm not sure if there are any style guidelines or preferences for that.

Ya, I find having the full namespace is easier to read but if you want me to reduce them please let me know

No, it's fine. Thanks.

In general we do try to reduce the namespace length, so I have a mild preference to remove Envoy:: here if you don't mind. If you don't feel like it it's fine to merge.

OK, I'll remove the Envoy::
Thanks

repokitteh-read-only · 2019-01-11T21:02:54Z

🙀 Error while processing event:

evaluation error
error: finished: cannot load github.com/repokitteh/modules/lib/utils.star: error from server: {module load error GET https://api.github.com/repos/repokitteh/modules/contents/lib/utils.star: 401 Bad credentials [] map[]}

🐱

Caused by: #5543 was synchronize by bdecoste.

see: more, trace.

PiotrSikora

@bdecoste It looks that you've missed one file, looks good, otherwise. Thanks!

Also, please don't force push in the future, since it makes reviewing much more demanding (basically, requiring review from scratch), and GitHub doesn't send notifications for the force push, so no one knows that you've made any changes. Instead, please push multiple commits (including merge with master, if your base drifted too much). All commits are squashed during merge, so it's a single commit once merged into master.

source/extensions/transport_sockets/tls/context_manager_impl.h

bdecoste · 2019-01-15T15:41:29Z

Thanks. Will not squash and force push in the future. We've (RH) have always squashed to a single commit for our other projects.

Signed-off-by: William DeCoste <bdecoste@gmail.com>

bdecoste · 2019-01-15T18:06:51Z

Resolved merge conflicts but had to force push to resolve.

Signed-off-by: William DeCoste <bdecoste@gmail.com>

PiotrSikora

Thanks!

mattklein123 · 2019-01-16T01:47:48Z

source/extensions/transport_sockets/tls/context_config_impl.h

  // TODO(htuch): This needs to be made const again and/or zero copy and/or callers fixed.
-  std::vector<std::reference_wrapper<const TlsCertificateConfig>> tlsCertificates() const override {
-    std::vector<std::reference_wrapper<const TlsCertificateConfig>> configs;
+  std::vector<std::reference_wrapper<const Envoy::Ssl::TlsCertificateConfig>>


I still see a bunch of Envoy::Ssl in here. Is that needed for a namespace conflict? Doesn't seem like there should be?

Interfaces in include/envoy still use Envoy:Ssl, @bdecoste is going to rename those in a follow-up PR... Unless you prefer to do it in this one?

My point was I don't think the Envoy:: is required, but if this is going to be changed in a follow up we can go ahead and just merge it.

Ah, that. I focused on Ssl vs Tls and completely missed it. Yes, it probably wasn't needed.

When @bdecoste renames Envoy::Ssl to Envoy::Tls this will conflict with Envoy::Extensions::TransportSockets::Tls, so let's just leave this as Envoy::Ssl.

Thanks for the reviews and merge. I should have the next PR to complete the directory and namespace change from ssl to tls in today or tomorrow.

To confirm... do you want me to leave include/envoy/ssl alone (i.e. Envoy::Ssl) or make the change to include/envoy/tls and Envoy::Tls and use fully qualified namespaces to avoid conflict?If we are sticking with ssl then I will put in a PR to remove the extraneous Envoy::

Thanks

*Description*: As part of the work to support openssl this PR moves all boringssl dependencies to extensions. This is one step that will enable the linking and testing of openssl with a new transport socket for openssl. Those files with boringssl dependencies were moved from common/ssl to extensions/transport_sockets/ssl with similar changes for the tests. Please see envoyproxy#5524 for additional context. *Risk Level*: Low *Testing*: Standard tests passed *Docs Changes*: None *Release Notes*: None Signed-off-by: William DeCoste <bdecoste@gmail.com> Signed-off-by: Fred Douglas <fredlas@google.com>

repokitteh-read-only bot assigned lizan and PiotrSikora Jan 8, 2019

bdecoste mentioned this pull request Jan 8, 2019

Make SSL impl (i.e. boringssl vs openssl) pluggable #5545

Closed

lizan reviewed Jan 8, 2019

View reviewed changes

PiotrSikora reviewed Jan 8, 2019

View reviewed changes

lizan previously approved these changes Jan 11, 2019

View reviewed changes

PiotrSikora previously approved these changes Jan 11, 2019

View reviewed changes

bdecoste dismissed stale reviews from PiotrSikora and lizan via 08d62cc January 11, 2019 20:39

PiotrSikora suggested changes Jan 13, 2019

View reviewed changes

source/extensions/transport_sockets/tls/context_manager_impl.h Outdated Show resolved Hide resolved

lizan mentioned this pull request Jan 14, 2019

Add AWS http request signer #5580

Merged

bdecoste added 2 commits January 15, 2019 08:11

move boringssl dependencies to extensions

e75ea0f

Signed-off-by: William DeCoste <bdecoste@gmail.com>

rm old source/common/ssl/context_manager_impl.h

b22dbbb

Signed-off-by: William DeCoste <bdecoste@gmail.com>

mattklein123 self-assigned this Jan 15, 2019

format

6a5254c

Signed-off-by: William DeCoste <bdecoste@gmail.com>

PiotrSikora approved these changes Jan 15, 2019

View reviewed changes

lizan approved these changes Jan 15, 2019

View reviewed changes

mattklein123 reviewed Jan 16, 2019

View reviewed changes

mattklein123 approved these changes Jan 16, 2019

View reviewed changes

lizan merged commit 568b257 into envoyproxy:master Jan 16, 2019

Conversation

bdecoste commented Jan 8, 2019

Uh oh!

PiotrSikora commented Jan 8, 2019

Uh oh!

lizan commented Jan 8, 2019

Uh oh!

repokitteh-read-only bot commented Jan 8, 2019

Uh oh!

lizan commented Jan 8, 2019

Uh oh!

bdecoste commented Jan 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PiotrSikora left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bdecoste commented Jan 9, 2019

Uh oh!

PiotrSikora commented Jan 9, 2019

Uh oh!

lizan commented Jan 9, 2019

Uh oh!

bdecoste commented Jan 9, 2019

Uh oh!

bdecoste commented Jan 9, 2019

Uh oh!

PiotrSikora commented Jan 9, 2019

Uh oh!

bdecoste commented Jan 9, 2019

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

bdecoste commented Jan 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bdecoste Jan 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

repokitteh-read-only bot commented Jan 11, 2019

Uh oh!

PiotrSikora left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bdecoste commented Jan 15, 2019

Uh oh!

bdecoste commented Jan 15, 2019

Uh oh!

PiotrSikora left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

PiotrSikora left a comment •

edited

Loading

bdecoste Jan 11, 2019 •

edited

Loading