odcds: convert ODCDS over ADS to pre-cluster-single-subscription by adisuissa · Pull Request #41174 · envoyproxy/envoy

adisuissa · 2025-09-22T15:46:09Z

Commit Message: odcds: convert ODCDS over ADS to pre-cluster-single-subscription
Additional Description:
Prior to this PR there were issues when using OD-CDS without cds_config.
This PR converts OD-CDS over ADS to use the new XdstpOdCdsApiImpl (that is also used for xDS-federation based OD-CDS subscriptions).

Risk Level: low - only impacts OD-CDS usage
Testing: Added many integration tests. Unit tests were already added for the original OD-CDS implementations.

Docs Changes: N/A
Release Notes: Added.
Platform Specific Features: N/A
Runtime guard: This change is guarded by envoy.reloadable_features.odcds_over_ads_fix to ensure that there's an escape hatch if some unintentional change occurs.

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

repokitteh-read-only · 2025-09-22T15:46:15Z

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: #41174 was opened by adisuissa.

see: more, trace.

repokitteh-read-only · 2025-09-22T15:46:17Z

CC @envoyproxy/runtime-guard-changes: FYI only for changes made to (source/common/runtime/runtime_features.cc).

🐱

Caused by: #41174 was opened by adisuissa.

see: more, trace.

adisuissa · 2025-09-22T17:34:55Z

This is a continuation of #40988 and uses the component introduced there to fix a bug where OD-CDS isn't working when cds_config isn't defined.
/assign @KBaichoo @yanavlasov @nezdolik

yanavlasov

LGTM from me. Will wait for @KBaichoo or @nezdolik review as well.

/wait-any

nezdolik · 2025-09-22T21:01:59Z

source/extensions/filters/http/on_demand/on_demand_update.cc

+                                        Upstream::OdCdsApiHandlePtr);
+        }
+      }
+      if (odcds == nullptr) {


should this be an else block instead? E.g.:

if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.odcds_over_ads_fix")) { ... } else { odcds = THROW_OR_RETURN_VALUE(cm.allocateOdCdsApi(&Upstream::OdCdsApiImpl::create, odcds_config->source(), absl::nullopt, validation_visitor), Upstream::OdCdsApiHandlePtr); }

Or is it written as intentional fallback in case when flag is enabled and on_demand_cds fails to initialize for some reason?

It should be the else of the 2 conditions above, so I could rewrite it as follows:

if ((Runtime::runtimeFeatureEnabled("envoy.reloadable_features.odcds_over_ads_fix")) && (odcds_config->source().config_source_specifier_case() == envoy::config::core::v3::ConfigSource::ConfigSourceSpecifierCase::kAds)) { odcds = THROW_OR_RETURN_VALUE(cm.allocateOdCdsApi(&Upstream::XdstpOdCdsApiImpl::create, odcds_config->source(), absl::nullopt, validation_visitor), Upstream::OdCdsApiHandlePtr); } else { ...

I'm not against it, and just thought that the first case is more readable, but I'm happy to change this if you think otherwise (I'm really good with both options).

sounds good, i missed that it was else for 2 conditions above

nezdolik · 2025-09-22T21:17:53Z

test/integration/ads_integration.h


-class AdsIntegrationTest : public AdsDeltaSotwIntegrationSubStateParamTest,
-                           public HttpIntegrationTest {
+class AdsIntegrationTestBase : public Grpc::BaseGrpcClientIntegrationParamTest,


do we expect multiple parallel subclasses of AdsIntegrationTestBase class in future? If not, could just keep is simple with one base class

Good question.
There are already multiple sub-classes of AdsIntegrationTest, but they all use the same TestWithParam parameters (although some might not need to). I think the others will need to be refactored (less parameters) to decrease the tests runtime.

KBaichoo

Thank you for working on this. LGTM besides small comments.

KBaichoo · 2025-09-23T00:21:34Z

changelogs/current.yaml

    was not being properly extracted in the final Prometheus stat name.
+- area: odcds
+  change: |
+    Fixed a bug where using OD-CDS without cds_config would not workin some


s/workin/ work in/

Oops, thanks, fixed.

KBaichoo · 2025-09-23T00:24:16Z

source/common/upstream/od_cds_api_impl.h


  // A singleton through which all subscriptions will be processed.
  XdstpOdcdsSubscriptionsManagerSharedPtr subscriptions_manager_;
+  bool old_ads_;


KBaichoo · 2025-09-23T00:26:40Z

source/common/upstream/od_cds_api_impl.cc

+  // TODO(adisuissa): convert the config_source to optional.
+  bool old_ads = false;
+  if (config_source.config_source_specifier_case() ==
+      envoy::config::core::v3::ConfigSource::ConfigSourceSpecifierCase::kAds) {


why is old_ads not just equal to the test that sets it to true?

e.g.

const bool old_ads = config_source.config_source_specifier_case() == envoy::config::core::v3::ConfigSource::ConfigSourceSpecifierCase::kAds;

Good idea, fixed.

KBaichoo · 2025-09-23T00:29:44Z

/wait

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

Commit Message: tcp_proxy: fix OD-CDS over ADS Additional Description: This PR is similar to #41174 (and uses the same component to achieve the same goal). The fix allows proper use of tcp_proxy on-demand cluster discovery when used over ADS. Risk Level: low - only impacts OD-CDS over tcp_proxy Testing: Added integration tests. Docs Changes: N/A Release Notes: Added Platform Specific Features: N/A Runtime guard: Added `envoy.reloadable_features.tcp_proxy_odcds_over_ads_fix` to disable the behavior. --------- Signed-off-by: Adi Suissa-Peleg <adip@google.com>

Commit Message: tcp_proxy: fix OD-CDS over ADS Additional Description: This PR is similar to envoyproxy#41174 (and uses the same component to achieve the same goal). The fix allows proper use of tcp_proxy on-demand cluster discovery when used over ADS. Risk Level: low - only impacts OD-CDS over tcp_proxy Testing: Added integration tests. Docs Changes: N/A Release Notes: Added Platform Specific Features: N/A Runtime guard: Added `envoy.reloadable_features.tcp_proxy_odcds_over_ads_fix` to disable the behavior. --------- Signed-off-by: Adi Suissa-Peleg <adip@google.com> Signed-off-by: bjmask <11672696+bjmask@users.noreply.github.com>

Commit Message: tcp_proxy: fix OD-CDS over ADS Additional Description: This PR is similar to envoyproxy#41174 (and uses the same component to achieve the same goal). The fix allows proper use of tcp_proxy on-demand cluster discovery when used over ADS. Risk Level: low - only impacts OD-CDS over tcp_proxy Testing: Added integration tests. Docs Changes: N/A Release Notes: Added Platform Specific Features: N/A Runtime guard: Added `envoy.reloadable_features.tcp_proxy_odcds_over_ads_fix` to disable the behavior. --------- Signed-off-by: Adi Suissa-Peleg <adip@google.com>

odcds: convert ODCDS over ADS to pre-cluster-single-subscription

63d57fb

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

adisuissa marked this pull request as ready for review September 22, 2025 17:33

adisuissa requested review from kyessenov and yanavlasov as code owners September 22, 2025 17:33

repokitteh-read-only bot assigned KBaichoo, nezdolik and yanavlasov Sep 22, 2025

yanavlasov reviewed Sep 22, 2025

View reviewed changes

repokitteh-read-only bot added the waiting:any label Sep 22, 2025

nezdolik reviewed Sep 22, 2025

View reviewed changes

repokitteh-read-only bot removed waiting:any labels Sep 22, 2025

KBaichoo previously approved these changes Sep 23, 2025

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 23, 2025

addressing comments

6a00c0a

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

adisuissa dismissed KBaichoo’s stale review via 6a00c0a September 23, 2025 04:04

repokitteh-read-only bot removed the waiting label Sep 23, 2025

Merge remote-tracking branch 'upstream/main' into odcds_over_ads_fix

af0bfce

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

nezdolik approved these changes Sep 23, 2025

View reviewed changes

KBaichoo merged commit d5c7a06 into envoyproxy:main Sep 23, 2025
25 checks passed

adisuissa mentioned this pull request Feb 18, 2026

tcp_proxy: fix OD-CDS over ADS #43529

Merged

Conversation

adisuissa commented Sep 22, 2025

Uh oh!

repokitteh-read-only bot commented Sep 22, 2025

Uh oh!

repokitteh-read-only bot commented Sep 22, 2025

Uh oh!

adisuissa commented Sep 22, 2025

Uh oh!

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

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!

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

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!

KBaichoo commented Sep 23, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants