ecds: add dynamic config provider usage in downstream network filters #28477

ohadvano · 2023-07-19T07:37:25Z

Additional Description: Follow up to #28407, this PR adds dynamic config provider usage in downstream network filters, with relevant integration tests. A follow up PR will add support for upstream network filters. Link: #14696.
Risk Level: low (guarded by config)
Testing: Integration tests
Docs Changes: ECDS API doc, network filter, ECDS config doc
Release Notes: None
Platform Specific Features: None

…ration tests Signed-off-by: ohadvano <[email protected]>

Signed-off-by: ohadvano <[email protected]>

repokitteh-read-only · 2023-07-19T07:37:36Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @lizan
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #28477 was opened by ohadvano.

see: more, trace.

Signed-off-by: ohadvano <[email protected]>

ohadvano · 2023-07-19T10:43:54Z

/assign @kyessenov
I'll appreciate your review on this as a follow-up. The actual source change is very small, this is mostly integration tests.

Signed-off-by: ohadvano <[email protected]>

ohadvano · 2023-07-20T19:32:30Z

/unassign @kyessenov
Seems like Kuat is out for vacation.
/assign @htuch
Could you please review this as a follow up to the previous PR?
Thanks

htuch · 2023-07-21T02:31:51Z

I'm also out for the next 1.5 weeks. @adisuissa @mattklein123 do you think you folks could take this one? Thanks.

ohadvano · 2023-07-21T06:22:55Z

/unassign @lizan
/unassign @htuch
/assign @adisuissa
/assign @mattklein123
Assigning to you if you could hopefully review this. Thanks

Signed-off-by: ohadvano <[email protected]>

mattklein123 · 2023-07-26T22:51:21Z

Sorry I don't have time to review this in detail so will wait for @adisuissa to do a first pass, thanks.

adisuissa

Thanks for working on this! Looks very good!
Mostly nits and some high-level comment about further testing.

cc @yanjunxiang-google as a person who worked on this before.

api/envoy/service/extension/v3/config_discovery.proto

test/integration/network_extension_discovery_integration_test.cc

docs/root/configuration/overview/extension.rst

test/integration/network_extension_discovery_integration_test.cc

adisuissa · 2023-07-27T13:49:59Z

test/integration/network_extension_discovery_integration_test.cc

+  EXPECT_EQ(4, network_filter_config.bytes_to_drain());
+}
+
+} // namespace


2 high-level questions:

Are there planned integration tests that test the interaction between listener updates (LDS) and network-filter updates (ECDS)?

Can the HCM be updated? If so I think more tests are needed in that space because of the lifetime of some objects.

Please add tests that have ongoing connections during an ECDS update and show that the "old" config is still used for these filters.

I added LDS integration and a test that verifies that old config is used for connections that were established before ECDS update. Due to (3), I believe the behavior here would be the same, connections will get the HCM config that is updated at the time of connection establishment. Did you have another behavior in mind which I missed?

Co-authored-by: Adi (Suissa) Peleg <[email protected]> Signed-off-by: ohadvano <[email protected]>

Signed-off-by: ohadvano <[email protected]>

ohadvano · 2023-07-28T12:30:22Z

@adisuissa thanks for the thorough review, I addressed your comment and will appreciate another pass

adisuissa

Thanks, LGTM.
/assign-from @envoyproxy/senior-maintainers

test/integration/network_extension_discovery_integration_test.cc

repokitteh-read-only · 2023-07-28T13:35:16Z

@envoyproxy/senior-maintainers assignee is @mattklein123

🐱

Caused by: a #28477 (review) was submitted by @adisuissa.

see: more, trace.

yanjunxiang-google · 2023-07-28T14:56:19Z

test/integration/network_extension_discovery_integration_test.cc

+  // The two filters drain 4 + 5  bytes.
+  sendDataVerifyResults(9);
+}
+


I think it is valuable to add integration test cases for pure static filters as this:

envoy/test/integration/listener_extension_discovery_integration_test.cc

Line 496 in 06e3e7d

TEST_P(ListenerExtensionDiscoveryIntegrationTest, TwoStaticFilters) {

@yanjunxiang-google can you please elaborate on the value that you see? Is it not just like not using ECDS at all (which I believe is covered in regular network filter tests)?

test/integration/network_extension_discovery_integration_test.cc

Signed-off-by: ohadvano <[email protected]>

ohadvano · 2023-07-28T21:10:40Z

@mattklein123 seems lke Adi approved, could you please take a look?

ohadvano · 2023-07-29T15:12:38Z

@mattklein123 can you merge the PR please?

…ig missing (#28742) Follow up to #28477, increasing a metric in cases where a downstream connection is missing a network filter configuration. Risk Level: low Testing: Unit test Docs Changes: Changelog, ECDS docs, listener docs Signed-off-by: ohadvano <[email protected]>

ohadvano added 2 commits July 19, 2023 10:13

use dynamic config provider for downstream network filters with integ…

d51f659

…ration tests Signed-off-by: ohadvano <[email protected]>

remove not implemented

1751df4

Signed-off-by: ohadvano <[email protected]>

repokitteh-read-only bot added the api label Jul 19, 2023

repokitteh-read-only bot assigned lizan Jul 19, 2023

ohadvano added 2 commits July 19, 2023 11:32

update docs

ddc8227

Signed-off-by: ohadvano <[email protected]>

change test size

9c668dc

Signed-off-by: ohadvano <[email protected]>

repokitteh-read-only bot assigned kyessenov Jul 19, 2023

update changelog

c53dca0

Signed-off-by: ohadvano <[email protected]>

ohadvano mentioned this pull request Jul 20, 2023

unhide config discovery option for network filters #28507

Closed

repokitteh-read-only bot assigned htuch and unassigned kyessenov Jul 20, 2023

repokitteh-read-only bot assigned adisuissa and mattklein123 and unassigned lizan and htuch Jul 21, 2023

ohadvano and others added 4 commits July 24, 2023 22:07

Merge branch 'main' into ecds_downstream_network

0c4a025

Signed-off-by: ohadvano <[email protected]>

merge

ab3f82e

Signed-off-by: ohadvano <[email protected]>

Merge branch 'envoyproxy:main' into ecds_downstream_network

0fbbbc0

changelog

c76d7ae

Signed-off-by: ohadvano <[email protected]>

adisuissa reviewed Jul 27, 2023

View reviewed changes

ohadvano and others added 4 commits July 27, 2023 17:01

Update test/integration/network_extension_discovery_integration_test.cc

4382666

Co-authored-by: Adi (Suissa) Peleg <[email protected]> Signed-off-by: ohadvano <[email protected]>

Update test/integration/network_extension_discovery_integration_test.cc

3f5c43e

Co-authored-by: Adi (Suissa) Peleg <[email protected]> Signed-off-by: ohadvano <[email protected]>

Update test/integration/network_extension_discovery_integration_test.cc

79f673a

Co-authored-by: Adi (Suissa) Peleg <[email protected]> Signed-off-by: ohadvano <[email protected]>

update doc, format

5f6911d

Signed-off-by: ohadvano <[email protected]>

lds integration and update during connection test

5340a60

Signed-off-by: ohadvano <[email protected]>

ohadvano requested a review from adisuissa July 28, 2023 12:19

Merge branch 'envoyproxy:main' into ecds_downstream_network

54930eb

adisuissa previously approved these changes Jul 28, 2023

View reviewed changes

test/integration/network_extension_discovery_integration_test.cc Show resolved Hide resolved

test/integration/network_extension_discovery_integration_test.cc Show resolved Hide resolved

repokitteh-read-only bot removed the api label Jul 28, 2023

yanjunxiang-google reviewed Jul 28, 2023

View reviewed changes

test/integration/network_extension_discovery_integration_test.cc Show resolved Hide resolved

add admin config dump handler test

0292112

Signed-off-by: ohadvano <[email protected]>

ohadvano dismissed adisuissa’s stale review via 0292112 July 28, 2023 15:35

repokitteh-read-only bot added the api label Jul 28, 2023

Merge branch 'envoyproxy:main' into ecds_downstream_network

27a6369

ohadvano requested a review from yanjunxiang-google July 28, 2023 15:37

Merge branch 'envoyproxy:main' into ecds_downstream_network

1872e92

mattklein123 approved these changes Jul 28, 2023

View reviewed changes

repokitteh-read-only bot removed the api label Jul 28, 2023

ohadvano added 2 commits July 29, 2023 01:04

Merge branch 'envoyproxy:main' into ecds_downstream_network

2c0dffc

Merge branch 'envoyproxy:main' into ecds_downstream_network

7217c00

mattklein123 merged commit 1b01f90 into envoyproxy:main Jul 30, 2023

ohadvano deleted the ecds_downstream_network branch July 31, 2023 05:08

ohadvano mentioned this pull request Aug 1, 2023

listener: add metric for connection closed due to network filter config missing #28742

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ecds: add dynamic config provider usage in downstream network filters #28477

ecds: add dynamic config provider usage in downstream network filters #28477

ohadvano commented Jul 19, 2023 •

edited

Loading

repokitteh-read-only bot commented Jul 19, 2023

ohadvano commented Jul 19, 2023 •

edited

Loading

ohadvano commented Jul 20, 2023

htuch commented Jul 21, 2023

ohadvano commented Jul 21, 2023 •

edited

Loading

mattklein123 commented Jul 26, 2023

adisuissa left a comment

adisuissa Jul 27, 2023

ohadvano Jul 28, 2023

ohadvano commented Jul 28, 2023

adisuissa left a comment

repokitteh-read-only bot commented Jul 28, 2023

yanjunxiang-google Jul 28, 2023

ohadvano Jul 28, 2023

ohadvano commented Jul 28, 2023

ohadvano commented Jul 29, 2023

ecds: add dynamic config provider usage in downstream network filters #28477

ecds: add dynamic config provider usage in downstream network filters #28477

Conversation

ohadvano commented Jul 19, 2023 • edited Loading

repokitteh-read-only bot commented Jul 19, 2023

ohadvano commented Jul 19, 2023 • edited Loading

ohadvano commented Jul 20, 2023

htuch commented Jul 21, 2023

ohadvano commented Jul 21, 2023 • edited Loading

mattklein123 commented Jul 26, 2023

adisuissa left a comment

Choose a reason for hiding this comment

adisuissa Jul 27, 2023

Choose a reason for hiding this comment

ohadvano Jul 28, 2023

Choose a reason for hiding this comment

ohadvano commented Jul 28, 2023

adisuissa left a comment

Choose a reason for hiding this comment

repokitteh-read-only bot commented Jul 28, 2023

yanjunxiang-google Jul 28, 2023

Choose a reason for hiding this comment

ohadvano Jul 28, 2023

Choose a reason for hiding this comment

ohadvano commented Jul 28, 2023

ohadvano commented Jul 29, 2023

ohadvano commented Jul 19, 2023 •

edited

Loading

ohadvano commented Jul 19, 2023 •

edited

Loading

ohadvano commented Jul 21, 2023 •

edited

Loading