dynamic forward proxy: add factory declarations + linking validation#7889
Merged
mattklein123 merged 4 commits intoenvoyproxy:masterfrom Aug 11, 2019
Merged
dynamic forward proxy: add factory declarations + linking validation#7889mattklein123 merged 4 commits intoenvoyproxy:masterfrom
mattklein123 merged 4 commits intoenvoyproxy:masterfrom
Conversation
This was referenced Aug 9, 2019
mattklein123
requested changes
Aug 9, 2019
Member
mattklein123
left a comment
There was a problem hiding this comment.
Thanks, LGTM with 1 question.
/wait
| // [#protodoc-title: Dynamic forward proxy cluster configuration] | ||
|
|
||
| // [#not-implemented-hide:] Not configuration. Workaround for C++ issue with stripping proto files. | ||
| message ClusterDummy { |
Member
There was a problem hiding this comment.
I might be wrong but I don't think you need any of this as there is no service here. The protos will be compiled in when the code is used
Contributor
Author
There was a problem hiding this comment.
Hm yea looks like you're right. Removed the dummy proto but kept the validation assert around because that seems useful. LMK if you think we should remove/change it
Signed-off-by: Michael Rebello <me@michaelrebello.com>
mattklein123
requested changes
Aug 10, 2019
Member
mattklein123
left a comment
There was a problem hiding this comment.
LGTM but I would remove the verification code. Thank you!
/wait
|
|
||
| // This function validates that the message descriptors that | ||
| // are referenced in Any messages are available in the descriptor pool. | ||
| bool validateProtoDescriptors(); |
Member
There was a problem hiding this comment.
Yeah I would just kill all of this. The filter code won't compile if it's missing so there is no chance of it getting elided from the build.
Signed-off-by: Michael Rebello <me@michaelrebello.com>
rebello95
added a commit
to envoyproxy/envoy-mobile
that referenced
this pull request
Aug 13, 2019
We'll be using the [dynamic forward proxy configuration](https://www.envoyproxy.io/docs/envoy/latest/configuration/http_filters/dynamic_forward_proxy_filter#config-http-filters-dynamic-forward-proxy) as the defacto configuration that will ship with Envoy Mobile. This will allow us to avoid making consumers register the DNS/domains they need to support when starting up the Envoy instance. This PR: - Pulls in the upstream fixes to support for this in envoyproxy/envoy#7889 - Moves our example config to use the dynamic forward proxy - Updates our main interfaces to force register the necessary extensions Future PRs will: - Move this into a resource compiled with the library - Convert it to a template - Provide typed configurations for wrapping the template - Enable TLS: #322 Part of #169. Signed-off-by: Michael Rebello <me@michaelrebello.com>
jpsim
pushed a commit
that referenced
this pull request
Nov 28, 2022
We'll be using the [dynamic forward proxy configuration](https://www.envoyproxy.io/docs/envoy/latest/configuration/http_filters/dynamic_forward_proxy_filter#config-http-filters-dynamic-forward-proxy) as the defacto configuration that will ship with Envoy Mobile. This will allow us to avoid making consumers register the DNS/domains they need to support when starting up the Envoy instance. This PR: - Pulls in the upstream fixes to support for this in #7889 - Moves our example config to use the dynamic forward proxy - Updates our main interfaces to force register the necessary extensions Future PRs will: - Move this into a resource compiled with the library - Convert it to a template - Provide typed configurations for wrapping the template - Enable TLS: envoyproxy/envoy-mobile#322 Part of envoyproxy/envoy-mobile#169. Signed-off-by: Michael Rebello <me@michaelrebello.com> Signed-off-by: JP Simard <jp@jpsim.com>
jpsim
pushed a commit
that referenced
this pull request
Nov 29, 2022
We'll be using the [dynamic forward proxy configuration](https://www.envoyproxy.io/docs/envoy/latest/configuration/http_filters/dynamic_forward_proxy_filter#config-http-filters-dynamic-forward-proxy) as the defacto configuration that will ship with Envoy Mobile. This will allow us to avoid making consumers register the DNS/domains they need to support when starting up the Envoy instance. This PR: - Pulls in the upstream fixes to support for this in #7889 - Moves our example config to use the dynamic forward proxy - Updates our main interfaces to force register the necessary extensions Future PRs will: - Move this into a resource compiled with the library - Convert it to a template - Provide typed configurations for wrapping the template - Enable TLS: envoyproxy/envoy-mobile#322 Part of envoyproxy/envoy-mobile#169. Signed-off-by: Michael Rebello <me@michaelrebello.com> Signed-off-by: JP Simard <jp@jpsim.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
2 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.
In Envoy Mobile, we'll be using the dynamic forward proxy configuration as the defacto template for starting up Envoy. In the process of integrating this, we found that when we include the extension, the proto file for configuration gets stripped out and thus fails at runtime when starting up the Envoy instance. This PR fixes the issue by allowing us to force-register the necessary extensions and ensure the proto file is not stripped.
DECLARE_FACTORYmacroAdds a dummy proto which is then referenced in the implementation of the extension to force the proto file to be included with the extension (rather than being stripped)Not necessary because this proto isn't aserviceValidates that the proto descriptor for the cluster config is present using an assertionA similar change for validating proto descriptors are present was checked in here, and an example of how we've added dummy proto messages for this purpose in the past can be found here.
Risk Level: Low
Testing: Done locally, and CI
Docs Changes: None
Release Notes: None
Signed-off-by: Michael Rebello me@michaelrebello.com