config: add path_config_source and watched_directory config by mattklein123 · Pull Request #19974 · envoyproxy/envoy

mattklein123 · 2022-02-15T20:19:42Z

For xDS over the file system, sometimes more control is required over
what directory/file is watched for symbolic link swaps. Specifically,
in order to deliver xDS over a Kubernetes ConfigMap, this extra
configuration is required.

Fixes #10979

Risk Level: Low, new feature
Testing: Integration test
Docs Changes: Added
Release Notes: Added
Platform Specific Features: N/A
Deprecated: Yes, details added

For xDS over the file system, sometimes more control is required over what directory/file is watched for symbolic link swaps. Specifically, in order to deliver xDS over a Kubernetes ConfigMap, this extra configuration is required. Fixes #10979 Signed-off-by: Matt Klein <mklein@lyft.com>

repokitteh-read-only · 2022-02-15T20:19:48Z

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

🐱

Caused by: #19974 was opened by mattklein123.

see: more, trace.

Signed-off-by: Matt Klein <mklein@lyft.com>

htuch

API/implementation LGTM modulo minor comments, will look at tests tomorow.

api/envoy/config/core/v3/config_source.proto

envoy/router/router.h

htuch · 2022-02-16T06:08:14Z

source/common/config/filesystem_subscription_impl.cc

+      stats_(stats), api_(api), validation_visitor_(validation_visitor) {
+  if (!path_config_source.has_watched_directory()) {
+    file_watcher_ = dispatcher.createFilesystemWatcher();
+    file_watcher_->addWatch(path_, Filesystem::Watcher::Events::MovedTo, [this](uint32_t) {


Can you map the legacy case to WatchedDirectory to dedupe this logic?

I thought about this but I'm not sure how to do this without duplicating a bunch of code that lives inside the watcher (watching a specific file, etc.). Do you have any suggestions? Happy to collapse if there is a good way. FWIW the SDS code has similar bifurcation.

Not sure if it's worth the hassle, but I think the callback can be refactored, so it's created before the if statement and moved to the file/directory watcher (will also require adding a setCallback(uint32_t) to WatchedDirectory and some refactoring there).

Yeah, I tried to do this, but rolled it back when I encountered said refactoring. Let's see what @htuch has in mind and then I'm happy to make some changes if it's worthwhile (I'm not sure it is).

Probably not worth it for these marginal deprecated paths, so let's skip.

mattklein123 · 2022-02-16T15:50:08Z

test/config/integration/server.yaml

  typed_config:
    "@type": type.googleapis.com/envoy.config.metrics.v3.StatsdSink
    tcp_cluster_name: statsd
-watchdog: {}


This change and the other one like it are unrelated to this change, however when I built with disabling deprecations locally, the hot restart tests failed (we don't compile hot restart in the CI build which tests this), so I just fixed this at the same time. The empty deprecated watchdog config seems a historical legacy and not relevant to hot restart.

snowp

Thanks, just one small suggestion

test/integration/rtds_integration_test.cc

Signed-off-by: Matt Klein <mklein@lyft.com>

adisuissa

API LGTM, modulo comment from @htuch.

adisuissa · 2022-02-16T16:53:34Z

source/common/config/filesystem_subscription_impl.cc

+      stats_(stats), api_(api), validation_visitor_(validation_visitor) {
+  if (!path_config_source.has_watched_directory()) {
+    file_watcher_ = dispatcher.createFilesystemWatcher();
+    file_watcher_->addWatch(path_, Filesystem::Watcher::Events::MovedTo, [this](uint32_t) {


Not sure if it's worth the hassle, but I think the callback can be refactored, so it's created before the if statement and moved to the file/directory watcher (will also require adding a setCallback(uint32_t) to WatchedDirectory and some refactoring there).

Signed-off-by: Matt Klein <mklein@lyft.com>

mattklein123 · 2022-02-16T23:03:37Z

/retest

repokitteh-read-only · 2022-02-16T23:03:41Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #19974 (comment) was created by @mattklein123.

see: more, trace.

htuch

LGTM, thanks!

htuch · 2022-02-17T05:52:05Z

test/integration/rtds_integration_test.cc

+TEST_P(RtdsIntegrationTest, FileRtdsReload) {
+  // Create an initial setup that looks similar to a K8s ConfigMap deployment. This is a file
+  // contained in a directory and referenced via an intermediate symlink on the directory.
+  const std::string temp_path{TestEnvironment::temporaryDirectory() + "/rtds_test"};


Nit: we have temporaryPath() for this.

htuch · 2022-02-17T05:55:12Z

source/common/config/filesystem_subscription_impl.cc

+      stats_(stats), api_(api), validation_visitor_(validation_visitor) {
+  if (!path_config_source.has_watched_directory()) {
+    file_watcher_ = dispatcher.createFilesystemWatcher();
+    file_watcher_->addWatch(path_, Filesystem::Watcher::Events::MovedTo, [this](uint32_t) {


Probably not worth it for these marginal deprecated paths, so let's skip.

adisuissa

/lgtm api
LGTM code changes.

repokitteh-read-only bot added the api label Feb 15, 2022

repokitteh-read-only bot assigned adisuissa Feb 15, 2022

mattklein123 assigned snowp and htuch and unassigned adisuissa Feb 15, 2022

mattklein123 added 2 commits February 16, 2022 03:13

Merge remote-tracking branch 'origin/main' into path_xds

2e86465

Signed-off-by: Matt Klein <mklein@lyft.com>

fix

712aade

Signed-off-by: Matt Klein <mklein@lyft.com>

mattklein123 requested review from alyssawilk and lizan as code owners February 16, 2022 05:42

htuch reviewed Feb 16, 2022

View reviewed changes

mattklein123 commented Feb 16, 2022

View reviewed changes

snowp reviewed Feb 16, 2022

View reviewed changes

test/integration/rtds_integration_test.cc Show resolved Hide resolved

Merge remote-tracking branch 'origin/main' into path_xds

9df0f6b

Signed-off-by: Matt Klein <mklein@lyft.com>

adisuissa reviewed Feb 16, 2022

View reviewed changes

mattklein123 added 2 commits February 16, 2022 17:06

comments

ec094c2

Signed-off-by: Matt Klein <mklein@lyft.com>

fix

01476c0

Signed-off-by: Matt Klein <mklein@lyft.com>

htuch approved these changes Feb 17, 2022

View reviewed changes

repokitteh-read-only bot removed the api label Feb 17, 2022

adisuissa approved these changes Feb 17, 2022

View reviewed changes

mattklein123 merged commit 8670309 into main Feb 17, 2022

mattklein123 deleted the path_xds branch February 17, 2022 15:50

Conversation

mattklein123 commented Feb 15, 2022

Uh oh!

repokitteh-read-only bot commented Feb 15, 2022

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Feb 16, 2022

Uh oh!

repokitteh-read-only bot commented Feb 16, 2022

Uh oh!

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

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants