changing xDS APIs to take abs::Satus #29439

alyssawilk · 2023-09-05T21:00:47Z

Unlike many of the throw->status PRs I'm doing xDS in two parts - one is a pure API change, and the follow-up(s) will change the throw over to returning non-Ok stauts. This PR is large enough already I didn't want to do them all at once.

Risk Level: low (but will cause downstream churn, sorry)
Testing: updated tests
Docs Changes: n/a
Release Notes: n/a
envoyproxy/envoy-mobile#176

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

repokitteh-read-only · 2023-09-05T21:00:52Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #29439 was opened by alyssawilk.

see: more, trace.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

adisuissa

Thanks for cleaning up the exceptions usage!
Overall LGTM, left a few small nits.

adisuissa · 2023-09-06T17:49:01Z

envoy/rds/route_config_provider.h

@@ -51,7 +51,7 @@ class RouteConfigProvider {
  /**
   * Callback used to notify RouteConfigProvider about configuration changes.
   */
-  virtual void onConfigUpdate() PURE;
+  virtual absl::Status onConfigUpdate() PURE;


nit: update the doc-string to include returned value.

adisuissa · 2023-09-06T17:51:49Z

source/common/filter/config_discovery_impl.cc

@@ -215,7 +225,7 @@ void FilterConfigProviderManagerImplBase::applyLastOrDefaultConfig(
  bool last_config_valid = false;
  if (subscription->lastConfig()) {
    TRY_ASSERT_MAIN_THREAD {
-      provider.validateTypeUrl(subscription->lastTypeUrl());
+      THROW_IF_NOT_OK(provider.validateTypeUrl(subscription->lastTypeUrl()))


OOC why doesn't the validateMessage() in the line below needs the same THROW_IF_NOT_OK wrapper?

Looking further in this PR, I guess this is going to be a future change.

adisuissa · 2023-09-06T17:53:18Z

source/common/filter/config_discovery_impl.cc

+      auto status = provider.onConfigUpdate(*subscription->lastConfig(),
+                                            subscription->lastVersionInfo(), nullptr);
+      if (!status.ok()) {
+        throw EnvoyException(std::string(status.message()));
+      }


nit: can THROW_IF_NOT_OK be used here?

adisuissa · 2023-09-06T17:54:04Z

source/common/filter/config_discovery_impl.h

+      auto status = onConfigUpdate(*default_configuration_, "", nullptr);
+      if (!status.ok()) {
+        throw EnvoyException(std::string(status.message()));
+      }


nit: can THROW_IF_NOT_OK be used here?

adisuissa · 2023-09-06T17:55:00Z

source/common/rds/rds_route_config_subscription.cc

+      auto status = route_config_provider_->onConfigUpdate();
+      if (!status.ok()) {
+        throw EnvoyException(std::string(status.message()));
+      }


nit: can THROW_IF_NOT_OK be used here?

adisuissa · 2023-09-06T17:58:19Z

source/common/router/scoped_rds.cc

-  ConfigSubscriptionCommonBase::onConfigUpdate();
-  if (any_applied) {
+  bool any_applied = false;
+  absl::Status scopes = addOrUpdateScopes(


nit: rename scopes to scopes_status

BTW: why not use StatusOr for the return value? just to clarify that any_applied is a return val)?

adisuissa · 2023-09-06T18:03:01Z

source/common/router/scoped_rds.cc

    const std::vector<Envoy::Config::DecodedResourceRef>& resources, Init::Manager& init_manager,
-    const std::string& version_info) {
-  bool any_applied = false;


I think setting to false is still needed, in case the passed value is true, but nothing will be applied

adisuissa · 2023-09-06T18:04:12Z

source/common/router/vhds.cc

+      auto status = route_config_provider_->onConfigUpdate();
+
+      if (!status.ok()) {
+        throw EnvoyException(std::string(status.message()));
+      }


nit: can THROW_IF_NOT_OK be used here?

adisuissa · 2023-09-06T18:11:35Z

envoy/common/exception.h

+  absl::Status status = status_fn;                                                                 \
+  if (!status.ok()) {                                                                              \
+    throw EnvoyException(std::string(status.message()));                                           \
+  }


Consider adding this inside an inner scope, to allow reuse of status variable name in the same block.

adisuissa · 2023-09-06T18:16:07Z

test/common/filter/config_discovery_impl_test.cc

    return;
  }

  EXPECT_THROW_WITH_MESSAGE(
-      config_discovery_test.callbacks_->onConfigUpdate(decoded_resources.refvec_,
-                                                       response.version_info()),
+      EXPECT_TRUE(config_discovery_test.callbacks_


A bit confusing (expect throw, while expecting a successful return value), but I guess this is just to satisfy the linter.

yeah and they're mostly all changing in the follow-up PR.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

mattklein123

LGTM at a high level. I live now in the land of ? so looking at all of this is pretty painful. 😉

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

adisuissa

Thanks, LGTM!
Left a couple of small const nits, that I think can be done in a future PR.

adisuissa · 2023-09-07T13:08:35Z

source/common/router/scoped_rds.cc

+  if (!status_or_applied.status().ok()) {
+    return status_or_applied.status();
+  }
+  bool any_applied = status_or_applied.value();


nit: const here and places above and below (defer for future PR)

adisuissa · 2023-09-07T13:09:37Z

envoy/common/exception.h

@@ -12,6 +12,14 @@ class EnvoyException : public std::runtime_error {
  EnvoyException(const std::string& message) : std::runtime_error(message) {}
 };

+#define THROW_IF_NOT_OK(status_fn)                                                                 \
+  {                                                                                                \
+    absl::Status status = status_fn;                                                               \


nit: const (defer to future PR)

alyssawilk · 2023-09-07T13:31:39Z

Thanks for the fast turnaround! I'll address the last two nits in the follow-up (removing throws) which I have staged to go

follow up to #29439 using the new status return code Risk Level: medium Testing: updated tests Docs Changes: n/a Release Notes: n/a envoyproxy/envoy-mobile#176 Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Relates: envoyproxy/envoy#29439 Signed-off-by: Tam Mach <tam.mach@cilium.io>

follow up to envoyproxy#29439 using the new status return code Risk Level: medium Testing: updated tests Docs Changes: n/a Release Notes: n/a envoyproxy/envoy-mobile#176 Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Relates: envoyproxy/envoy#29439 Signed-off-by: Tam Mach <tam.mach@cilium.io>

alyssawilk added 2 commits August 21, 2023 08:52

WIP

fad0f95

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

integration tests pass

fb94be7

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk force-pushed the ds branch from 8c16e30 to eeb3209 Compare September 6, 2023 12:38

changing onConfigUpdate to take absl::Status

945c60a

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk force-pushed the ds branch from eeb3209 to 945c60a Compare September 6, 2023 13:38

alyssawilk assigned adisuissa and mattklein123 Sep 6, 2023

alyssawilk marked this pull request as ready for review September 6, 2023 13:54

alyssawilk requested review from wbpcode, soulxu, htuch, zuercher and lizan as code owners September 6, 2023 13:54

adisuissa reviewed Sep 6, 2023

View reviewed changes

comments

02c6106

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

mattklein123 approved these changes Sep 7, 2023

View reviewed changes

Merge branch 'main' into ds

94348b2

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

adisuissa approved these changes Sep 7, 2023

View reviewed changes

alyssawilk enabled auto-merge (squash) September 7, 2023 13:30

alyssawilk merged commit e6ee1cf into envoyproxy:main Sep 7, 2023

alyssawilk mentioned this pull request Sep 7, 2023

xds: returning absl::Status #29497

Merged

zirain mentioned this pull request Sep 8, 2023

Automator: update envoy@ in istio/proxy@master istio/proxy#4942

Closed

sayboras added a commit to sayboras/proxy that referenced this pull request Oct 22, 2023

cilium: Adjust onConfigUpdate signature

135d04f

Relates: envoyproxy/envoy#29439 Signed-off-by: Tam Mach <tam.mach@cilium.io>

sayboras added a commit to sayboras/proxy that referenced this pull request Oct 22, 2023

cilium: Adjust onConfigUpdate signature

a4b49af

Relates: envoyproxy/envoy#29439 Signed-off-by: Tam Mach <tam.mach@cilium.io>

sayboras added a commit to sayboras/proxy that referenced this pull request Oct 22, 2023

cilium: Adjust onConfigUpdate signature

3428269

Relates: envoyproxy/envoy#29439 Signed-off-by: Tam Mach <tam.mach@cilium.io>

sayboras added a commit to sayboras/proxy that referenced this pull request Oct 22, 2023

cilium: Adjust onConfigUpdate signature

55304d2

Relates: envoyproxy/envoy#29439 Signed-off-by: Tam Mach <tam.mach@cilium.io>

sayboras added a commit to sayboras/proxy that referenced this pull request Nov 11, 2023

cilium: Adjust onConfigUpdate signature

a96fcd4

Relates: envoyproxy/envoy#29439 Signed-off-by: Tam Mach <tam.mach@cilium.io>

sayboras added a commit to sayboras/proxy that referenced this pull request Dec 2, 2023

cilium: Adjust onConfigUpdate signature

693f4c0

Relates: envoyproxy/envoy#29439 Signed-off-by: Tam Mach <tam.mach@cilium.io>

sayboras added a commit to sayboras/proxy that referenced this pull request Dec 6, 2023

cilium: Adjust onConfigUpdate signature

31d99cc

Relates: envoyproxy/envoy#29439 Signed-off-by: Tam Mach <tam.mach@cilium.io>

sayboras added a commit to sayboras/proxy that referenced this pull request Dec 10, 2023

cilium: Adjust onConfigUpdate signature

7479604

Relates: envoyproxy/envoy#29439 Signed-off-by: Tam Mach <tam.mach@cilium.io>

sayboras added a commit to sayboras/proxy that referenced this pull request Dec 12, 2023

cilium: Adjust onConfigUpdate signature

a69bd07

Relates: envoyproxy/envoy#29439 Signed-off-by: Tam Mach <tam.mach@cilium.io>

sayboras added a commit to sayboras/proxy that referenced this pull request Dec 27, 2023

cilium: Adjust onConfigUpdate signature

70939e2

Relates: envoyproxy/envoy#29439 Signed-off-by: Tam Mach <tam.mach@cilium.io>

sayboras added a commit to sayboras/proxy that referenced this pull request Jan 23, 2024

cilium: Adjust onConfigUpdate signature

ccbb228

Relates: envoyproxy/envoy#29439 Signed-off-by: Tam Mach <tam.mach@cilium.io>

sayboras added a commit to sayboras/proxy that referenced this pull request Jan 29, 2024

cilium: Adjust onConfigUpdate signature

3bc63ab

Relates: envoyproxy/envoy#29439 Signed-off-by: Tam Mach <tam.mach@cilium.io>

sayboras added a commit to sayboras/proxy that referenced this pull request Jan 29, 2024

cilium: Adjust onConfigUpdate signature

65a3de6

Relates: envoyproxy/envoy#29439 Signed-off-by: Tam Mach <tam.mach@cilium.io>

sayboras added a commit to cilium/proxy that referenced this pull request Jan 29, 2024

cilium: Adjust onConfigUpdate signature

dd881a3

Relates: envoyproxy/envoy#29439 Signed-off-by: Tam Mach <tam.mach@cilium.io>

sayboras added a commit to sayboras/proxy that referenced this pull request Jan 29, 2024

cilium: Adjust onConfigUpdate signature

13e3cd2

Relates: envoyproxy/envoy#29439 Signed-off-by: Tam Mach <tam.mach@cilium.io>

github-merge-queue bot pushed a commit to cilium/proxy that referenced this pull request Jan 30, 2024

cilium: Adjust onConfigUpdate signature

6c0b5f3

Relates: envoyproxy/envoy#29439 Signed-off-by: Tam Mach <tam.mach@cilium.io>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

changing xDS APIs to take abs::Satus #29439

changing xDS APIs to take abs::Satus #29439

alyssawilk commented Sep 5, 2023 •

edited

Loading

repokitteh-read-only bot commented Sep 5, 2023

adisuissa left a comment

adisuissa Sep 6, 2023

adisuissa Sep 6, 2023

adisuissa Sep 6, 2023

adisuissa Sep 6, 2023

adisuissa Sep 6, 2023

adisuissa Sep 6, 2023

adisuissa Sep 6, 2023

adisuissa Sep 6, 2023

adisuissa Sep 6, 2023

adisuissa Sep 6, 2023

adisuissa Sep 6, 2023

alyssawilk Sep 6, 2023

mattklein123 left a comment

adisuissa left a comment

adisuissa Sep 7, 2023

adisuissa Sep 7, 2023

alyssawilk commented Sep 7, 2023

changing xDS APIs to take abs::Satus #29439

changing xDS APIs to take abs::Satus #29439

Conversation

alyssawilk commented Sep 5, 2023 • edited Loading

repokitteh-read-only bot commented Sep 5, 2023

adisuissa 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

adisuissa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alyssawilk commented Sep 7, 2023

alyssawilk commented Sep 5, 2023 •

edited

Loading