proto: move extension-specific linking validation into extensions #6657
Conversation
Signed-off-by: Mike Schore <mike.schore@gmail.com>
|
I considered moving ratelimit as well, but we didn't actually run into issues with it. (Not sure if it's actually possible to fully compile out* ratelimit at this point.) Also an open question whether there are any other cases that should be included. Just wanted to mention in case anyone has insight/opinions. |
Signed-off-by: Mike Schore <mike.schore@gmail.com>
| srcs = ["grpc_access_log_proto_descriptors.cc"], | ||
| hdrs = ["grpc_access_log_proto_descriptors.h"], | ||
| deps = [ | ||
| "//source/common/config:protobuf_link_hacks", |
There was a problem hiding this comment.
I don't think the link hacks apply in this situation (see the comment in the respective file).
There was a problem hiding this comment.
Ah yep, makes sense - this is cruft left over from me copying the dependencies.
|
Not sure what we might be missing - but one thought I floated by Matt in an earlier conversation is that we could also just do dummy linking for everything instead of validation, until whatever needs to be fixed upstream for this all to work is fixed. |
Signed-off-by: Mike Schore <mike.schore@gmail.com>
|
@goaway I think the challenge is figuring out what "everything" is here. Do you have any thoughts on how that would be computed? |
|
Empirically, we encountered just the above two cases when building with only HttpConnMan and the Router as extensions, with a single public entrypoint. We could possibly induce further failures by building with only a single dummy filter. However, this really only captures the current state of the world, and further development/refactors could change things. (As an aside, the current validation setup in proto_descriptors.cc will fail fast, but requires a code change to see what actually failed, since it only returns a boolean. We could assert/panic inside that file - though perhaps the boolean return value is better for testing.) My thought though was merely to replace the validation of each proto currently checked with a dummy ref to ensure it gets compiled in. This won't capture cases we've missed, but it won't reduce our coverage either. If we really wanted to go all in; we could add a dummy message to any proto file we build in, and explicitly reference those messages whenever the generated header is included. |
|
And also, thanks @htuch ! |
|
@goaway yeah, I guess it would be a bit cleaner, but I'm also wondering if we should just figure out if the suggestion in protocolbuffers/protobuf#4221 makes sense. If we set |
|
@htuch I was under the possible misapprehension that |
|
Well, not sure if it does :) The last state in that Protobuf GH issue is that we needed to try it. |
* master: docs: add extension policy (envoyproxy#6678) ext_authz: added ability to detect partial request body data (envoyproxy#6583) version_history.rst: jwt_authn change missed 1.10.0 (envoyproxy#6684) docs: fix link in pull request template (envoyproxy#6679) Explicitly convert absl::string_view to std::string. (envoyproxy#6687) docs: improving watermark docs/comments (envoyproxy#6683) http filter: add CSRF filter (envoyproxy#6470) event: reintroduce dispatcher stats (envoyproxy#6659) security: postmortem for CVE-2019-990[01] (envoyproxy#6597) Improve build rules for (test only) library quic_port_utils. (envoyproxy#6672) spell check: skip unsupported extensions when called with a file (envoyproxy#6648) Changed TestHooks to ListenerHooks (envoyproxy#6642) proto: move extension-specific linking validation into extensions (envoyproxy#6657) stats: add/test heterogenous set of StatNameStorage objects. (envoyproxy#6504) docs: move xds protocol to rst (envoyproxy#6670) fix version history order (envoyproxy#6671) Signed-off-by: Michael Puncel <mpuncel@squareup.com>
|
I don't mind giving it a try. Can at least see if it works for the case we encountered. |
Signed-off-by: Mike Schore mike.schore@gmail.com
Description: A couple of the proto descriptors validated during startup are specific to extensions that may be compiled out. This change only validates their presence when the extensions are used. This fixes some spurious validation failures in certain minimal build configurations.
Risk Level: Low
Testing: Ran test suite, ran server.
Docs Changes: N/A
Release Notes: N/A