config: dependency injection of message validation. #7072
Conversation
There was a problem hiding this comment.
Skimmed and I can't think of a better way of doing this if we want to get rid of the global, which I agree we should do anyway. Also, per your comment, I think the visitor pattern can hopefully also allow us to report/log/etc. unknown fields when the proto folks add that support so this seems good regardless. +1 to continue.
/wait
|
@mattklein123 OK, I'm continuing down this path. So far I've converted the |
This is the plumbing to support envoyproxy#6651 and envoyproxy#6818, where we remove the global control over message validation type and put it under the control over a new injectable interface. It's pretty horrible IMHO that there is this much churn; the closest major context object that we could associate with is Api, but it's not part of the filter API as such and we will want to later inject different validators at different points, depending on whether we are reading the static bootstrap or on xDS. No new behavior should be added in this PR. Risk level: Low. Testing: bazel test //test/... Signed-off-by: Harvey Tuch <htuch@google.com>
htuch
changed the title
Signed-off-by: Harvey Tuch <htuch@google.com>
Signed-off-by: Harvey Tuch <htuch@google.com>
|
@mattklein123 this is ready for review. I suggest doing a pass as is, without CI passing if there are still failures, as any failures probably are interesting to independently examine as they get fixed. |
Signed-off-by: Harvey Tuch <htuch@google.com>
| @@ -108,6 +111,8 @@ void RdsRouteConfigSubscription::onConfigUpdate( | |||
| if (config_update_info_->routeConfiguration().has_vhds()) { | |||
| ENVOY_LOG(debug, "rds: vhds configuration present, starting vhds: config_name={} hash={}", | |||
| route_config_name_, config_update_info_->configHash()); | |||
| // TODO(dmitri-d): It's unsafe to depend directly on factory context here, | |||
There was a problem hiding this comment.
@dmitri-d I tagged you here. The use of factory context outside of the constructor is probably unsafe, since the subscription might outlive the listener (at least that's what ASAN tells me).
Signed-off-by: Harvey Tuch <htuch@google.com>
| // TODO(htuch): clean this up when protobuf supports JSON/YAML unknown field | ||
| // detection directly. | ||
| options.ignore_unknown_fields = true; | ||
| const auto relaxed_status = Protobuf::util::JsonStringToMessage(json, &message, options); |
There was a problem hiding this comment.
do you think there are any perf concerns for doing this parse twice? I think not, but wanted to double check. If we care about perf you could probably short circuit by asking the visitor whether it wants to accept unknown fields or not which I think would be a very simple interface change. Not sure if that is worth it or not.
There was a problem hiding this comment.
I think this won't have any performance implications on the happy path, since we only do a single JSON -> protobuf conversion there. It's only when we fail that we double work, in which case we are rejecting config. Once we have a clean solution to protocolbuffers/protobuf#5967, even the unhappy path can be fast.
Signed-off-by: Harvey Tuch <htuch@google.com>
Signed-off-by: Harvey Tuch <htuch@google.com>
Fixes a typo from #7072. Risk Level: Low Signed-off-by: James Buckland <jbuckland@google.com>
This is the plumbing to support #6651 and #6818, where we remove the global control over message
validation type and put it under the control of a new injectable interface. It's pretty horrible
IMHO that there is this much churn; the closest major context object that we could associate with is
Api.
The main from the new dependency is that we can:
This PR introduces a visitor pattern for the validator and starts to do the threading needed for dependency injection here.
It's possible that we could extend this visitor for deprecated fields in the future to provide finer grained control over these alongside unknown, if there is a use case.
No new behavior should be added in this PR.
Risk level: Low.
Testing: bazel test //test/...
Signed-off-by: Harvey Tuch htuch@google.com