udpa: filesystem list collection support for inline entries. by htuch · Pull Request #13028 · envoyproxy/envoy

htuch · 2020-09-09T15:16:40Z

This is the first PR towards filesystem support for file:/// URLs
compatible with #11264. Currently it plumbs in only LDS filesystem
support for list collections with only inline entries.

Risk level: Low (opt in)
Testing: Unit and integration tests added.

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

This is the first PR towards filesystem support for file:/// URLs compatible with envoyproxy#11264. Currently it plumbs in only LDS filesystem support for list collections with only inline entries. Risk level: Low (opt in) Testing: WiP, will finalize PR when these are ready. Signed-off-by: Harvey Tuch <htuch@google.com>

repokitteh-read-only · 2020-09-09T15:16:46Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #13028 was opened by htuch.

see: more, trace.

htuch · 2020-09-09T15:17:12Z

@chaoqin-li1123 FYI re: common plumbing from LDS to subscription factory.

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

stevenzzzz · 2020-09-09T20:16:54Z

/cc stevenzzzz

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

…ions

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

htuch · 2020-09-10T18:50:53Z

The file:// encoding for Windows is a bit broken, since it needs to be file:///c:/foo/bar. If we do this, when we do generic URI decoding, we get /c:/foo/bar. @wrowe @sunjayBhatia do you folks know how this is usually handled on Windows? Is there a need to special case URI decoding, etc.

mattklein123

Nice to see this coming together. Some small comments but generally LGTM. Thank you!

/wait

source/common/config/decoded_resource_impl.h

source/common/config/filesystem_subscription_impl.cc

mattklein123 · 2020-09-10T22:15:50Z

source/common/config/subscription_factory_impl.cc

+    return std::make_unique<Config::FilesystemCollectionSubscriptionImpl>(
+        dispatcher_, path, callbacks, resource_decoder, stats, validation_visitor_, api_);
+  }
+  default:


Is this checked by schema? If not, maybe better to not use default and specify all of the options so we get a compile error if something is added?

This is an enum, so you have weird values like ResourceLocator_Scheme_ResourceLocator_Scheme_INT_MIN_SENTINEL_DO_NOT_USE_

source/server/lds_api.cc

mattklein123 · 2020-09-10T22:18:13Z

test/common/config/filesystem_subscription_impl_test.cc


+// TODO(htuch): Add generic test harness support for collection subscriptions so that we can test
+// gRPC/HTTP transports similar to below.
+class FilesystemCollectionSubscriptionImplTest : public testing::Test,


I'm wondering, per my comment above in the prof code about shared code, could the tests here actually be a TEST_P that somehow creates a collection or not and just runs all the same tests for the most part to verify the same stats, etc.?

I plan on a followup PR to address this as per the TODO.

OK, seems like a lot of duplication but up to you.

The more time I spent looking at these tests and what coverage they provide, the more I realized they don't actually do a whole lot for some of the unhappy path cases that I've been adding in this PR. I definitely want to reform this entire set of tests before moving forward with any further UDPA PRs, since they are key to being able to validate implementations for both filesystem and gRPC. This is probably going to be a series of PRs that don't have a ton to do with the filesystem list collection support per se, but they would benefit from having it landed as a forcing function.

Sure sounds fine to do as followup cleanups if you are tracking.

test/config/integration/server_xds.lds.udpa.list_collection.yaml

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

mattklein123

Thanks!

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

…code * upstream/master: lint: add more linters for using absl:: over std:: (envoyproxy#13043) udpa: filesystem list collection support for inline entries. (envoyproxy#13028) filter: http: jwt: implement matching for HTTP CONNECT (envoyproxy#13064) [fuzz] split http filter logic into a fuzzing class (envoyproxy#13016) xds: allow empty delta update (envoyproxy#12699) CacheFilter: parses the allowed_vary_headers from the cache config. (envoyproxy#12928) router: extend HTTP CONNECT route matching criteria (envoyproxy#13056) docs: clarify use of Extended CONNECT for h/2 (envoyproxy#13051) build: shellcheck tools/ (envoyproxy#13007) [fuzz] Refactored Health Checker Impl Tests (envoyproxy#13017) Signed-off-by: Lihao Luo <lihao@squareup.com>

repokitteh-read-only bot added the api label Sep 9, 2020

Filesystem xDS integration test for UDPA list collections.

a118b30

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

Unit tests for FilesystemCollectionSubscriptionImpl.

a02aa21

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

htuch changed the title ~~[WiP] udpa: filesystem list collection support for inline entries.~~ udpa: filesystem list collection support for inline entries. Sep 9, 2020

htuch assigned mattklein123 Sep 9, 2020

htuch marked this pull request as ready for review September 9, 2020 20:33

htuch added 2 commits September 10, 2020 11:51

Merge remote-tracking branch 'upstream/master' into udpa-file-collect…

740d9a5

…ions

Build/test fixes.

3db3656

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

mattklein123 requested changes Sep 10, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 10, 2020

htuch added 7 commits September 11, 2020 08:30

Fix file:/// path issue for Windows.

f4499bd

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

clang-tidy fix

fd6b75c

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

filesystem_subscription_impl refactor

cf4dd6f

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

Avoid ByteSize() == 0.

d940e4d

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

Upgrade integration configs to v3.

2f2bc4f

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

Fix tests

2b31fc1

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

fix format

7c92ee8

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

repokitteh-read-only bot removed the waiting label Sep 11, 2020

mattklein123 previously approved these changes Sep 11, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Sep 11, 2020

Fix typo.

2c199f6

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

htuch dismissed mattklein123’s stale review via 2c199f6 September 11, 2020 19:55

repokitteh-read-only bot added the api label Sep 11, 2020

mattklein123 approved these changes Sep 11, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Sep 11, 2020

htuch merged commit 108d2bc into envoyproxy:master Sep 11, 2020

htuch deleted the udpa-file-collections branch September 11, 2020 22:48

Conversation

htuch commented Sep 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Sep 9, 2020

Uh oh!

htuch commented Sep 9, 2020

Uh oh!

stevenzzzz commented Sep 9, 2020

Uh oh!

htuch commented Sep 10, 2020

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattklein123 Sep 10, 2020

Choose a reason for hiding this comment

Uh oh!

htuch Sep 11, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattklein123 Sep 10, 2020

Choose a reason for hiding this comment

Uh oh!

htuch Sep 10, 2020

Choose a reason for hiding this comment

Uh oh!

mattklein123 Sep 10, 2020

Choose a reason for hiding this comment

Uh oh!

htuch Sep 11, 2020

Choose a reason for hiding this comment

Uh oh!

mattklein123 Sep 11, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

htuch commented Sep 9, 2020 •

edited

Loading