pgv: validation support for Bootstrap/xDS/filters. by htuch · Pull Request #2146 · envoyproxy/envoy

htuch · 2017-12-01T19:00:25Z

This patch lands checking via https://github.com/lyft/protoc-gen-validate in Envoy for bootstrap, xDS ingestion and during filter instantiation.

Risk Level: Medium (things might start blowing up on config ingestion that were slipping through before).
Testing: Mostly negative tests for bootstrap/xDS/filters, since the happy path is already validated through the existing test suite.

Signed-off-by: Harvey Tuch htuch@google.com

htuch · 2017-12-01T19:01:46Z

@akonradi @mattklein123 (also @rodaine FYI) if we can agree on this approach, I can flesh out the PR.

mattklein123 · 2017-12-01T19:11:02Z

I really wish there was a way to generically call Validate() on the base class. It would be so much cleaner. Barring that, I think this is the best we can do. LGTM.

akonradi · 2017-12-01T19:11:50Z

I'm okay with the general approach here but I'll defer to @mattklein123 and others' stylistic preferences.

htuch · 2017-12-01T19:17:16Z

@mattklein123 I will bring this up when we meet with the protobuf team. For 1.5, this is the way it has to be I believe.

rodaine · 2017-12-01T19:33:54Z

bazel/repository_locations.bzl

looks like this leaked in

* Bootstrap and xDS APIs implemented with negative tests. There is a known issue with recursive validation of repeated messages in PGV that needs to be fixed before we can enable ValidateFail in rds_impl_test. Positive tests not required, since this is exercised by all tests that aren't failing. * Added validation support of filters and negative tests (where applicable). Signed-off-by: Harvey Tuch <htuch@google.com>

htuch · 2017-12-01T23:01:27Z

This is ready for review.

mattklein123

Sweet. Few random comments and needs merge.

mattklein123 · 2017-12-02T01:08:11Z

source/common/protobuf/utility.h

+    try {
+      validate(message);
+    } catch (const EnvoyException&) {
+      message.Clear();


what's the reasoning behind clearing here? We don't do it in the other functions where we validate. Just because we also loaded here so we own the whole thing? Maybe comment?

Yeah, it had to do with some later code that depended on this being clear on failure in server init. TBH, that's an anti-pattern, I will fix that code instead.

mattklein123 · 2017-12-02T01:10:25Z

test/common/router/rds_impl_test.cc

-  Json::ObjectSharedPtr config = Json::Factory::loadFromString(config_json);
-  envoy::api::v2::filter::network::Rds rds;
-  Envoy::Config::Utility::translateRdsConfig(*config, rds);
+// Negative test for protoc-gen-validate constraints.


Worth leaving this code for now? Or just delete and add TODO up above? I'm fine either way. Maybe also link to issue tracking this?

Coverage is broken until some recent PGV fixups make their way across to data-plane-api. Need to merge https://github.com/lyft/protoc-gen-validate/pull/40 first though. So, will wait until that all happens, then this test should work for real.

mattklein123 · 2017-12-02T01:11:11Z

test/common/upstream/cds_api_impl_test.cc

+  Protobuf::RepeatedPtrField<envoy::api::v2::Cluster> clusters;
+  clusters.Add();
+
+  EXPECT_THROW(dynamic_cast<CdsApiImpl*>(cds_.get())->onConfigUpdate(clusters),


EXPECT_THROW_WITH_MESSAGE? Maybe not convenient because exception contains entire debug string? Same elsewhere?

It will get a bit fragile, since the current errors thrown by PGV are not very pretty, so will probably be evolved. This is why I added ProtoValidationException in this PR, so we can at least sure that these are PGV errors.

OK sounds good.

Signed-off-by: Harvey Tuch <htuch@google.com>

htuch · 2017-12-04T22:36:28Z

@mattklein123 @akonradi ready for final review, PGV works really well (thanks @akonradi @rodaine!).

mattklein123

This is super amazing!!! Thanks @htuch @akonradi @rodaine! One small nit.

mattklein123 · 2017-12-04T22:39:10Z

source/common/protobuf/utility.cc

+                                                   const Protobuf::Message& message)
+    : EnvoyException(fmt::format("Proto constraint validation failed ({}): {}", validation_error,
+                                 message.DebugString())) {
+  ENVOY_LOG_MISC(debug, "Throwing {}", what());


nit: kind of a strange debug message: "Proto validation error. Throwing ..." ?

Signed-off-by: Harvey Tuch <htuch@google.com>

* API sha for proxy * API sha for proxy

Updates to the latest version of Envoy upstream Fixes a flaky stats test that got worse with the bump Forces the registration of the new client channel factory extension that is part of core Implements a new method for the synthetic address Signed-off-by: Snow Pettersen <snowp@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

rodaine reviewed Dec 1, 2017

View reviewed changes

bazel/repository_locations.bzl Outdated

Copy link

Member

rodaine Dec 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like this leaked in

htuch force-pushed the pgv-bootstrap-xds branch from a399eb7 to c01919e Compare December 1, 2017 22:56

htuch changed the title ~~[RFC] pgv: validation support for Bootstrap/xDS/filters.~~ pgv: validation support for Bootstrap/xDS/filters. Dec 1, 2017

mattklein123 reviewed Dec 2, 2017

View reviewed changes

htuch added 7 commits December 4, 2017 15:02

Merge remote-tracking branch 'upstream/master' into pgv-bootstrap-xds

4dc1d71

Signed-off-by: Harvey Tuch <htuch@google.com>

Bump to PGV with C++ code gen bug fixes.

39ccb52

Signed-off-by: Harvey Tuch <htuch@google.com>

Enable RDS negative test now that PGV bugs are fixed.

1f33f01

Signed-off-by: Harvey Tuch <htuch@google.com>

Avoid clearing proto when validation fails.

c66c676

Signed-off-by: Harvey Tuch <htuch@google.com>

More network/http/stats filter tests enabled with PGV.

3505a29

Signed-off-by: Harvey Tuch <htuch@google.com>

Bump data-plane-api again to pick UDP support for statsd.

10ad461

Signed-off-by: Harvey Tuch <htuch@google.com>

Merge remote-tracking branch 'upstream/master' into pgv-bootstrap-xds

bdae4f4

mattklein123 reviewed Dec 4, 2017

View reviewed changes

Fix ProtoValidationException debug log.

d93b4d4

Signed-off-by: Harvey Tuch <htuch@google.com>

mattklein123 approved these changes Dec 4, 2017

View reviewed changes

htuch merged commit 9ed6292 into envoyproxy:master Dec 4, 2017

htuch deleted the pgv-bootstrap-xds branch December 4, 2017 23:24

Shikugawa pushed a commit to Shikugawa/envoy that referenced this pull request Mar 28, 2020

Rc3. new API sha for proxy. (envoyproxy#2146)

9d0bda2

* API sha for proxy * API sha for proxy

Conversation

htuch commented Dec 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

htuch commented Dec 1, 2017

Uh oh!

mattklein123 commented Dec 1, 2017

Uh oh!

akonradi commented Dec 1, 2017

Uh oh!

htuch commented Dec 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch commented Dec 1, 2017

Uh oh!

mattklein123 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!

htuch commented Dec 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

htuch commented Dec 1, 2017 •

edited

Loading

htuch commented Dec 4, 2017 •

edited

Loading