config: Implement both versions of onConfigUpdate() everywhere #6879

fredlas · 2019-05-09T19:47:01Z

Adding support for the new delta-style onConfigUpdate everywhere. Going by the comments, we had planned to have just this onConfigUpdate, rather than both it and the state-of-the-world style one. However, when I investigated unifying them it didn't make sense; I think both are needed. Part of #4991.

Risk Level: low
Testing: added tests for the delta style onConfigUpdate in CDS and EDS

Signed-off-by: Fred Douglas <fredlas@google.com>

fredlas · 2019-05-09T19:47:20Z

/assign htuch

Signed-off-by: Fred Douglas <fredlas@google.com>

htuch

Can you elaborate on why having both onConfigUpdate methods is necessary long term? I thought it was OK to keep them during the migration, but we could have only a single one as end state.

Here's how I reckon it could work. You write generically, in the SOTW generic code, a stack tracking library. This translates all SOTW updates into a delta update for the purposes of onConfigUpdate. This comes down to filling in the list of resources to remove based on the knowledge of the resources present in the previous update and the current one. Would that work?

fredlas · 2019-05-13T20:09:11Z

I think the issue is the name field in the DeltaDiscoveryResponse::Resource proto. DiscoveryResponse just has the plain Any blobs (the thing that would be the resource field of a Resource). For the APIs where name makes sense, they themselves can know to reach in and grab it - because at that point you can convert from an Any to e.g. Cluster - but that isn't something that you can do generically in DeltaSubscriptionImpl or wherever. (Since, I believe what you're describing would also need the concept of "name" guaranteed to be a part of all xDS resources. Some APIs also have deduplication logic that relies on the name).

I think the name field got added to the proto later on, after the comments about the double onConfigUpdate being a hack had gone in. So, I think the issue of names just hadn't occurred to anyone yet.

htuch · 2019-05-13T21:20:46Z

@fredlas good point; we used to (sort of) have a method that did this mapping, making use of the fact that we could downcast to a known proto and it had that business logic for extracting the name out of the known resource types. I think you cleaned this up (or somebody did).

Would there be value in a utility method that each individual xDS can call from its SOTW onConfigUpdate() to do the delta onConfigUpdate() call with a naming lambda?

fredlas · 2019-05-14T15:54:01Z

That sounded like a good approach, so I started looking into it, but hit another issue: the question of what is being diffed against. The answer is "state retrieved from some part of the rest of Envoy," like cm_.clusters() for CDS or listener_manager_.listeners() for LDS. I guess you could also have that utility function take a list of existing names as another base argument.

But, this is all seeming a little messy. This PR as it is now will accomplish the same goals, just with ~10 lines of code reused once. Going the duplicate onConfigUpdate route will also not make it any harder to introduce this generic approach later. So, let's proceed this way so that aggregated delta can get done.

htuch · 2019-05-14T21:51:27Z

@fredlas I'll continue the review, but would be appreciated if you could track this improvement as an issue or a TODO.

htuch

Cool, nice to see this all coming together.
/wait

htuch · 2019-05-14T21:58:39Z

source/common/router/rds_impl.cc

+  }
+  Protobuf::RepeatedPtrField<ProtobufWkt::Any> unwrapped_resource;
+  *unwrapped_resource.Add() = resources[0].resource();
+  onConfigUpdate(unwrapped_resource, resources[0].version());


So, the way it works today is that we only get an RdsRouteConfigSubscription::onConfigUpdate update if the resource is present in the RDS response. If it's absent, no change is made to the configuration. Initially it's empty, and we warm listeners on first response. We may have an empty config though if the update failed.

In the delta world, we can add resources as they arrive. If we get a removal request, what do we do? Should we be resetting the configuration back to empty? The semantics are a bit different here to SOTW, since with SOTW, we are cool with missing config implying latching, here we have an explicit removal operation.

My guess would be to have a removal request reset the configuration, since RDS is one of the types that does have a "name" to it. If the server wanted to update, it should just send an updated version. If it's sending an update explicitly naming the resource as being gone, it makes sense to get rid of that resource. Although, it sounds like when the RDS client gets started, it already has a name in mind, and only that name is valid. So removing the config should be more like shutting down.

Do you plan to address the code changes for this in this PR?

Done. I ended up logging an error and ignoring the remove request. It seems like it basically doesn't make sense for the server to even try a removal here.

Yeah, I think this becomes more interesting with on-demand, where you have the possibility the management server decides to remove a resource and have the client lazily fetch (and maybe recreate it on that path). Since we don't support that, I think it's fine to error out for now, but I'd like to make this a TODO and link to an on-demand tracking issue (do we have one?) so that folks know this is subject to change in terms of behavior.

Ah, I see, yes. Left a TODO. Looks like there is #2500.

source/common/router/rds_impl.cc

source/common/upstream/cds_api_impl.cc

test/common/upstream/eds_test.cc

source/server/lds_api.cc

Signed-off-by: Fred Douglas <fredlas@google.com>

htuch · 2019-05-15T22:34:40Z

test/common/router/rds_impl_test.cc

+
+  std::string config_json2 = R"EOF(
+    {
+      "api_type": "REST",


How does a REST RDS configuration figure into delta xDS testing?

Won't accept DELTA_GRPC. Maybe I should just delete this test.

test/common/secret/sds_api_test.cc

htuch · 2019-05-15T22:40:07Z

test/common/router/rds_impl_test.cc

+
+  EXPECT_EQ(0UL,
+            route_config_provider_manager_->dumpRouteConfigs()->dynamic_route_configs().size());
+}


It's a bit unclear to me which bits of this are testing generic route config provider logic and which are the delta xDS specifics. Is it possible to add more comments or reduce the route config provider testing that is already covered elsewhere?

Signed-off-by: Fred Douglas <fredlas@google.com>

htuch

/wait

test/common/secret/sds_api_test.cc

htuch · 2019-05-22T02:36:56Z

test/common/secret/sds_api_test.cc

+};
+
+// Basic test of delta's passthrough call to the state-of-the-world variant, to
+// increase coverage.


It's a bit more than just coverage, since we have some new code and want to ensure it's doing the right thing. I think especially because this is otherwise unused by anyone today, it's valuable to have this.

The new code is the same validateUpdateSize() call that the SotW onConfigUpdate has, followed by calling the SotW onConfigUpdate on the 0th element of the array. It's not literally onConfigUpdate(x, y) { onConfigUpdate(x) }, but it's close.

htuch · 2019-05-22T02:37:16Z

test/common/secret/sds_api_test.cc

@@ -115,6 +115,92 @@ TEST_F(SdsApiTest, DynamicTlsCertificateUpdateSuccess) {
  handle->remove();
 }

+class PartialMockSds : public SdsApi {


test/common/secret/sds_api_test.cc

fredlas · 2019-05-22T14:29:53Z

/retest

repokitteh-read-only · 2019-05-22T14:29:59Z

🔨 rebuilding ci/circleci: release (failed build)

🐱

Caused by: a #6879 (comment) was created by @fredlas.

see: more, trace.

Signed-off-by: Fred Douglas <fredlas@google.com>

htuch

LGTM, thanks for the patience on this one.

* master: test: Add coverage for IsolatedStoreImpl::find* (envoyproxy#7043) server: add ProcessContext (envoyproxy#7018) config: Implement both versions of onConfigUpdate() everywhere (envoyproxy#6879) gzip: add test for various compression strategy and level (envoyproxy#7055) Fix typo in comment for rds.RouteConfiguration.validate_clusters (envoyproxy#7056) mysql_filter: add handling for partial messages (envoyproxy#6885) migrate from v2alpha to v2 (envoyproxy#7044) tests: fix tsan test flake (envoyproxy#7052) upstream: fix HostUtility::healthFlagsToString (envoyproxy#7051) tech debt: eliminate absl::make_unique (envoyproxy#7034) router: add a route name field in route.Route list (envoyproxy#6776) ext_authz: configurable HTTP status code for network errors. (envoyproxy#6669) stats: remove const-cast for symbol-table in edcs_filter_test.cc (envoyproxy#7045) build: bump libevent to 3b1864b. (envoyproxy#7012) stats: improve test-coverage for a few stats-related functions. (envoyproxy#7038) docs: fix csrf filter source origin note (envoyproxy#7041) Fix common typo: grcp -> grpc (envoyproxy#7040) snapshot (envoyproxy#7036) Signed-off-by: Michael Puncel <mpuncel@squareup.com>

fredlas added 3 commits May 9, 2019 15:11

snapshot

7c16e12

Signed-off-by: Fred Douglas <fredlas@google.com>

Merge remote-tracking branch 'upstream/master' into ONE_onConfigUpdate

53bc44b

Signed-off-by: Fred Douglas <fredlas@google.com>

add unit tests

8be158a

Signed-off-by: Fred Douglas <fredlas@google.com>

repokitteh-read-only bot assigned htuch May 9, 2019

remove TODO

fe65b32

Signed-off-by: Fred Douglas <fredlas@google.com>

htuch suggested changes May 13, 2019

View reviewed changes

htuch added the waiting:any label May 13, 2019

repokitteh-read-only bot removed the waiting:any label May 13, 2019

htuch suggested changes May 14, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label May 14, 2019

fredlas mentioned this pull request May 15, 2019

router: scoped rds (2c): implement scoped rds API #6932

Merged

add RDS and SDS tests

cd22fd2

Signed-off-by: Fred Douglas <fredlas@google.com>

repokitteh-read-only bot removed the waiting label May 15, 2019

htuch reviewed May 15, 2019

View reviewed changes

htuch added the waiting:any label May 15, 2019

fredlas added 2 commits May 16, 2019 13:53

handle removal attempt in RDS

2e9f5f2

Signed-off-by: Fred Douglas <fredlas@google.com>

Merge remote-tracking branch 'upstream/master' into ONE_onConfigUpdate

1e4af0d

Signed-off-by: Fred Douglas <fredlas@google.com>

repokitteh-read-only bot removed the waiting:any label May 16, 2019

fredlas added 3 commits May 16, 2019 13:57

cant change RDS test to DELTA_GRPC, removing

8aa5f95

Signed-off-by: Fred Douglas <fredlas@google.com>

add RDS removal TODO

94529c2

Signed-off-by: Fred Douglas <fredlas@google.com>

Merge remote-tracking branch 'upstream/master' into ONE_onConfigUpdate

22d8df6

Signed-off-by: Fred Douglas <fredlas@google.com>

htuch added the waiting:any label May 20, 2019

repokitteh-read-only bot removed waiting:any labels May 20, 2019

snapshot

3c65309

Signed-off-by: Fred Douglas <fredlas@google.com>

Merge remote-tracking branch 'upstream/master' into ONE_onConfigUpdate

0b0b3ea

Signed-off-by: Fred Douglas <fredlas@google.com>

htuch suggested changes May 22, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label May 22, 2019

fredlas added 2 commits May 22, 2019 11:27

further verification in SDS delta test

b0afa6a

Signed-off-by: Fred Douglas <fredlas@google.com>

Merge remote-tracking branch 'upstream/master' into ONE_onConfigUpdate

d340ca8

Signed-off-by: Fred Douglas <fredlas@google.com>

repokitteh-read-only bot removed the waiting label May 22, 2019

htuch added the waiting label May 22, 2019

fredlas added 2 commits May 22, 2019 17:14

fix hidden onConfigUpdate

99e78ac

Signed-off-by: Fred Douglas <fredlas@google.com>

Merge remote-tracking branch 'upstream/master' into ONE_onConfigUpdate

9fa75e2

Signed-off-by: Fred Douglas <fredlas@google.com>

repokitteh-read-only bot removed the waiting label May 23, 2019

htuch approved these changes May 23, 2019

View reviewed changes

htuch merged commit 73c46c9 into envoyproxy:master May 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

config: Implement both versions of onConfigUpdate() everywhere #6879

config: Implement both versions of onConfigUpdate() everywhere #6879

fredlas commented May 9, 2019 •

edited

Loading

fredlas commented May 9, 2019

htuch left a comment

fredlas commented May 13, 2019

htuch commented May 13, 2019

fredlas commented May 14, 2019

htuch commented May 14, 2019

htuch left a comment

htuch May 14, 2019

fredlas May 15, 2019

htuch May 15, 2019

fredlas May 16, 2019

htuch May 16, 2019

fredlas May 17, 2019

htuch May 15, 2019

fredlas May 16, 2019

htuch May 15, 2019

htuch left a comment

htuch May 22, 2019

fredlas May 22, 2019

htuch May 22, 2019

fredlas commented May 22, 2019

repokitteh-read-only bot commented May 22, 2019

htuch left a comment

config: Implement both versions of onConfigUpdate() everywhere #6879

config: Implement both versions of onConfigUpdate() everywhere #6879

Conversation

fredlas commented May 9, 2019 • edited Loading

fredlas commented May 9, 2019

htuch left a comment

Choose a reason for hiding this comment

fredlas commented May 13, 2019

htuch commented May 13, 2019

fredlas commented May 14, 2019

htuch commented May 14, 2019

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredlas commented May 22, 2019

repokitteh-read-only bot commented May 22, 2019

htuch left a comment

Choose a reason for hiding this comment

fredlas commented May 9, 2019 •

edited

Loading