srds: permit dynamic SRDS resources to contain inline RDS configuration

changelogs/current.yaml

@@ -18,6 +18,10 @@ minor_behavior_changes:
   change: |
     The NaN and Infinity values of float will be serialized to ``null`` and ``"inf"`` respectively in the
     metadata (``DYNAMIC_METADATA``, ``CLUSTER_METADATA``, etc.) formatter.
+- area: scoped_rds
+  change: |
+    The :ref:`route_configuration <envoy_v3_api_field_config.route.v3.ScopedRouteConfiguration.route_configuration>` field
+    is supported when the `ScopedRouteConfiguration` resource is delivered via SRDS.
 
 bug_fixes:
 # *Changes expected to improve the state of the world and are unlikely to have negative effects*

source/common/router/scoped_rds.cc

@@ -269,38 +269,59 @@ absl::StatusOr<bool> ScopedRdsConfigSubscription::addOrUpdateScopes(
   envoy::extensions::filters::network::http_connection_manager::v3::Rds rds;
   rds.mutable_config_source()->MergeFrom(rds_config_source_);
   std::vector<ScopedRouteInfoConstSharedPtr> updated_scopes;
+  std::list<RdsRouteConfigProviderHelperPtr> to_be_removed_rds_providers;
   for (const auto& resource : resources) {
     // Explicit copy so that we can std::move later.
     envoy::config::route::v3::ScopedRouteConfiguration scoped_route_config =
         dynamic_cast<const envoy::config::route::v3::ScopedRouteConfiguration&>(
             resource.get().resource());
-    if (scoped_route_config.route_configuration_name().empty()) {
-      return absl::InvalidArgumentError("route_configuration_name is empty.");
-    }
     const std::string scope_name = scoped_route_config.name();
-    if (const auto& scope_info_iter = scoped_route_map_.find(scope_name);
-        scope_info_iter != scoped_route_map_.end() &&
-        scope_info_iter->second->configHash() == MessageUtil::hash(scoped_route_config)) {
-      continue;
-    }
-    rds.set_route_config_name(scoped_route_config.route_configuration_name());
-    std::unique_ptr<RdsRouteConfigProviderHelper> rds_config_provider_helper;
     std::shared_ptr<ScopedRouteInfo> scoped_route_info = nullptr;
-    if (scoped_route_config.on_demand() == false) {
-      // For default scopes, create a rds helper with rds provider initialized.
-      rds_config_provider_helper =
-          std::make_unique<RdsRouteConfigProviderHelper>(*this, scope_name, rds, init_manager);
-      scoped_route_info = std::make_shared<ScopedRouteInfo>(
-          std::move(scoped_route_config), rds_config_provider_helper->routeConfig());
+    if (scoped_route_config.has_route_configuration()) {
+      RouteConfigProviderPtr route_config_provider =
+          route_config_provider_manager_.createStaticRouteConfigProvider(
+              scoped_route_config.route_configuration(), factory_context_,
+              factory_context_.messageValidationContext().staticValidationVisitor());
+      scoped_route_info = std::make_shared<ScopedRouteInfo>(std::move(scoped_route_config),
+                                                            route_config_provider->configCast());
+      // If this is an update from a scoped route configuration specifying route_configuration_name
+      // to one specifying route_configuration, then the RDS subscription is no longer needed. We
+      // can remove the RDS config provider, but hold on to them until exiting the loop in case the
+      // subscription is reused by another scope still to be added.
+      auto rds_config_provider_helper_iter = route_provider_by_scope_.find(scope_name);
+      if (rds_config_provider_helper_iter != route_provider_by_scope_.end()) {
+        to_be_removed_rds_providers.emplace_back(
+            std::move(rds_config_provider_helper_iter->second));
+        route_provider_by_scope_.erase(rds_config_provider_helper_iter);
+      }
     } else {
-      // For on demand scopes, create a rds helper with rds provider uninitialized.
-      rds_config_provider_helper =
-          std::make_unique<RdsRouteConfigProviderHelper>(*this, scope_name);
-      // scope_route_info->routeConfig() will be nullptr, because RouteConfiguration is not loaded.
-      scoped_route_info =
-          std::make_shared<ScopedRouteInfo>(std::move(scoped_route_config), nullptr);
+      if (scoped_route_config.route_configuration_name().empty()) {
+        return absl::InvalidArgumentError("route_configuration_name is empty.");
+      }
+      if (const auto& scope_info_iter = scoped_route_map_.find(scope_name);
+          scope_info_iter != scoped_route_map_.end() &&
+          scope_info_iter->second->configHash() == MessageUtil::hash(scoped_route_config)) {
+        continue;
+      }
+      rds.set_route_config_name(scoped_route_config.route_configuration_name());
+      std::unique_ptr<RdsRouteConfigProviderHelper> rds_config_provider_helper;
+      if (scoped_route_config.on_demand() == false) {
+        // For default scopes, create a rds helper with rds provider initialized.
+        rds_config_provider_helper =
+            std::make_unique<RdsRouteConfigProviderHelper>(*this, scope_name, rds, init_manager);
+        scoped_route_info = std::make_shared<ScopedRouteInfo>(
+            std::move(scoped_route_config), rds_config_provider_helper->routeConfig());
+      } else {
+        // For on demand scopes, create a rds helper with rds provider uninitialized.
+        rds_config_provider_helper =
+            std::make_unique<RdsRouteConfigProviderHelper>(*this, scope_name);
+        // scope_route_info->routeConfig() will be nullptr, because RouteConfiguration is not
+        // loaded.
+        scoped_route_info =
+            std::make_shared<ScopedRouteInfo>(std::move(scoped_route_config), nullptr);
+      }
+      route_provider_by_scope_[scope_name] = std::move(rds_config_provider_helper);
     }
-    route_provider_by_scope_[scope_name] = std::move(rds_config_provider_helper);
     scope_name_by_hash_[scoped_route_info->scopeKey().hash()] = scoped_route_info->scopeName();
     scoped_route_map_[scoped_route_info->scopeName()] = scoped_route_info;
     updated_scopes.push_back(scoped_route_info);

test/common/router/scoped_rds_test.cc

@@ -740,6 +740,49 @@ route_configuration_name: foo_routes
             "foo_routes");
 }
 
+TEST_F(ScopedRdsTest, ScopedRdsSupportsInlineRds) {
+  server_factory_context_.cluster_manager_.initializeClusters({"baz"}, {});
+  setup();
+
+  const std::string config_yaml = R"EOF(
+name: foo_scope
+route_configuration:
+  name: route_config
+  virtual_hosts:
+    - name: virtual_host
+      domains: ["*"]
+      routes:
+        - match: { prefix: "/" }
+          route: { cluster: baz }
+key:
+  fragments:
+    - string_key: x-foo-key
+)EOF";
+  const auto resource = parseScopedRouteConfigurationFromYaml(config_yaml);
+
+  init_watcher_.expectReady(); // Only the SRDS parent_init_target_.
+  context_init_manager_.initialize(init_watcher_);
+  const auto decoded_resources = TestUtility::decodeResources({resource});
+  EXPECT_TRUE(srds_subscription_->onConfigUpdate(decoded_resources.refvec_, "1").ok());
+  EXPECT_EQ(1UL,
+            server_factory_context_.store_.counter("foo.scoped_rds.foo_scoped_routes.config_reload")
+                .value());
+  EXPECT_EQ(1UL, all_scopes_.value());
+  // Inline RDS doesn't count as an active scope.
+  EXPECT_EQ(0UL, active_scopes_.value());
+
+  // Verify the config is a ScopedConfigImpl instance, scope points to route_config already as there
+  // is no need to wait for RDS.
+  ASSERT_THAT(getScopedRdsProvider(), Not(IsNull()));
+  ASSERT_THAT(getScopedRdsProvider()->config<ScopedConfigImpl>(), Not(IsNull()));
+  EXPECT_EQ(getScopedRdsProvider()
+                ->config<ScopedConfigImpl>()
+                ->getRouteConfig(scope_key_builder_->computeScopeKey(
+                    TestRequestHeaderMapImpl{{"Addr", "x-foo-key;x-foo-key"}}))
+                ->name(),
+            "route_config");
+}
+
 // Tests that conflict resources in the same push are detected.
 TEST_F(ScopedRdsTest, MultipleResourcesWithKeyConflictSotW) {
   setup();

test/integration/scoped_rds_integration_test.cc

@@ -326,6 +326,110 @@ route_configuration_name: {}
       /*cluster_1 */ 1);
 }
 
+TEST_P(ScopedRdsIntegrationTest, SrdsWithInlineRouteConfiguration) {
+  constexpr absl::string_view scoped_route_with_inline_route_config_tmpl = R"EOF(
+name: {}
+route_configuration:
+  name: route_config
+  virtual_hosts:
+    - name: integration
+      domains: ["*"]
+      routes:
+        - match: {{ prefix: "/" }}
+          route: {{ cluster: {} }}
+key:
+  fragments:
+    - string_key: {}
+)EOF";
+  const std::string scoped_route1 = fmt::format(scoped_route_with_inline_route_config_tmpl,
+                                                "scoped_route_0", "cluster_0", "foo-route");
+  const std::string scoped_route2 = fmt::format(scoped_route_with_inline_route_config_tmpl,
+                                                "scoped_route_0", "cluster_1", "bar-route");
+  const std::string scoped_route3 = fmt::format(scoped_route_with_inline_route_config_tmpl,
+                                                "scoped_route_1", "cluster_0", "foo-route");
+
+  const std::string scoped_route_with_rds_subscription = R"EOF(
+name: scoped_route_0
+route_configuration_name: route_config
+key:
+  fragments:
+    - string_key: foo-route
+)EOF";
+  const std::string route_config = R"EOF(
+      name: route_config
+      virtual_hosts:
+      - name: integration
+        domains: ["*"]
+        routes:
+        - match: { prefix: "/" }
+          route: { cluster: cluster_1 }
+)EOF";
+
+  on_server_init_function_ = [&]() {
+    createScopedRdsStream();
+    sendSrdsResponse({scoped_route1}, {scoped_route1}, {}, "1");
+  };
+  initialize();
+  registerTestServerPorts({"http"});
+
+  sendRequestAndVerifyResponse(
+      Http::TestRequestHeaderMapImpl{{":method", "GET"},
+                                     {":path", "/"},
+                                     {":authority", "host"},
+                                     {":scheme", "http"},
+                                     {"Addr", "x-foo-key=foo-route"}},
+      /*request_size=*/0, Http::TestResponseHeaderMapImpl{{":status", "200"}, {"service", "foo"}},
+      /*response_size=*/0,
+      /*backend_idx=*/0);
+
+  // Replace foo-scope with a scoped route using an RDS subscription.
+  sendSrdsResponse({scoped_route_with_rds_subscription}, {scoped_route_with_rds_subscription}, {},
+                   "2");
+  test_server_->waitForCounterGe("http.config_test.scoped_rds.foo-scoped-routes.update_success", 2);
+  createRdsStream("route_config");
+  test_server_->waitForCounterGe("http.config_test.rds.route_config.update_attempt", 1);
+  sendRdsResponse(route_config, "1");
+  test_server_->waitForCounterGe("http.config_test.rds.route_config.update_success", 1);
+
+  // foo-route now goes to cluster_1.
+  sendRequestAndVerifyResponse(
+      Http::TestRequestHeaderMapImpl{{":method", "GET"},
+                                     {":path", "/"},
+                                     {":authority", "host"},
+                                     {":scheme", "http"},
+                                     {"Addr", "x-foo-key=foo-route"}},
+      /*request_size=*/0, Http::TestResponseHeaderMapImpl{{":status", "200"}, {"service", "foo"}},
+      /*response_size=*/0,
+      /*backend_idx=*/1);
+
+  // Replace foo-scope with bar-scope, which uses an inlined RouteConfiguration.
+  sendSrdsResponse({scoped_route2}, {scoped_route2}, {}, "3");
+  test_server_->waitForCounterGe("http.config_test.scoped_rds.foo-scoped-routes.update_success", 3);
+
+  // foo-route now returns 404.
+  codec_client_ = makeHttpConnection(lookupPort("http"));
+  auto response = codec_client_->makeHeaderOnlyRequest(
+      Http::TestRequestHeaderMapImpl{{":method", "GET"},
+                                     {":path", "/"},
+                                     {":authority", "host"},
+                                     {":scheme", "http"},
+                                     {"Addr", "x-foo-key=foo-route"}});
+  ASSERT_TRUE(response->waitForEndStream());
+  verifyResponse(std::move(response), "404", Http::TestResponseHeaderMapImpl{}, "");
+  cleanupUpstreamAndDownstream();
+
+  // bar-route now returns 200 and is routed to cluster_1.
+  sendRequestAndVerifyResponse(
+      Http::TestRequestHeaderMapImpl{{":method", "GET"},
+                                     {":path", "/"},
+                                     {":authority", "host"},
+                                     {":scheme", "http"},
+                                     {"Addr", "x-foo-key=bar-route"}},
+      /*request_size=*/0, Http::TestResponseHeaderMapImpl{{":status", "200"}, {"service", "foo"}},
+      /*response_size=*/0,
+      /*backend_idx=*/1);
+}
+
 // Test that a bad config update updates the corresponding stats.
 TEST_P(ScopedRdsIntegrationTest, ConfigUpdateFailure) {
   // 'name' will fail to validate due to empty string.