From 76919dc699969e3e1904023aaf302b3542652f98 Mon Sep 17 00:00:00 2001
From: "Mark D. Roth" <roth@google.com>
Date: Wed, 14 Jun 2023 13:53:16 -0700
Subject: [PATCH] [xDS] require EDS resource name in CDS resources with xdstp
 names (#33425)

Implements the change described in
https://github.com/grpc/proposal/pull/377.
---
 src/core/ext/xds/xds_cluster.cc               | 42 ++++++++++++-------
 .../xds/xds_cluster_resource_type_test.cc     | 22 ++++++++++
 2 files changed, 48 insertions(+), 16 deletions(-)

diff --git a/src/core/ext/xds/xds_cluster.cc b/src/core/ext/xds/xds_cluster.cc
index ad3773e6106..b0694cc5b1d 100644
--- a/src/core/ext/xds/xds_cluster.cc
+++ b/src/core/ext/xds/xds_cluster.cc
@@ -24,6 +24,7 @@
 
 #include "absl/status/status.h"
 #include "absl/status/statusor.h"
+#include "absl/strings/match.h"
 #include "absl/strings/str_cat.h"
 #include "absl/strings/str_join.h"
 #include "absl/strings/strip.h"
@@ -187,23 +188,32 @@ XdsClusterResource::Eds EdsConfigParse(
   if (eds_cluster_config == nullptr) {
     errors->AddError("field not present");
   } else {
-    ValidationErrors::ScopedField field(errors, ".eds_config");
-    const envoy_config_core_v3_ConfigSource* eds_config =
-        envoy_config_cluster_v3_Cluster_EdsClusterConfig_eds_config(
-            eds_cluster_config);
-    if (eds_config == nullptr) {
-      errors->AddError("field not present");
-    } else {
-      if (!envoy_config_core_v3_ConfigSource_has_ads(eds_config) &&
-          !envoy_config_core_v3_ConfigSource_has_self(eds_config)) {
-        errors->AddError("ConfigSource is not ads or self");
-      }
-      // Record EDS service_name (if any).
-      upb_StringView service_name =
-          envoy_config_cluster_v3_Cluster_EdsClusterConfig_service_name(
+    // Validate ConfigSource.
+    {
+      ValidationErrors::ScopedField field(errors, ".eds_config");
+      const envoy_config_core_v3_ConfigSource* eds_config =
+          envoy_config_cluster_v3_Cluster_EdsClusterConfig_eds_config(
               eds_cluster_config);
-      if (service_name.size != 0) {
-        eds.eds_service_name = UpbStringToStdString(service_name);
+      if (eds_config == nullptr) {
+        errors->AddError("field not present");
+      } else {
+        if (!envoy_config_core_v3_ConfigSource_has_ads(eds_config) &&
+            !envoy_config_core_v3_ConfigSource_has_self(eds_config)) {
+          errors->AddError("ConfigSource is not ads or self");
+        }
+      }
+    }
+    // Record EDS service_name (if any).
+    // This field is required if the CDS resource has an xdstp name.
+    eds.eds_service_name = UpbStringToStdString(
+        envoy_config_cluster_v3_Cluster_EdsClusterConfig_service_name(
+            eds_cluster_config));
+    if (eds.eds_service_name.empty()) {
+      absl::string_view cluster_name =
+          UpbStringToAbsl(envoy_config_cluster_v3_Cluster_name(cluster));
+      if (absl::StartsWith(cluster_name, "xdstp:")) {
+        ValidationErrors::ScopedField field(errors, ".service_name");
+        errors->AddError("must be set if Cluster resource has an xdstp name");
       }
     }
   }
diff --git a/test/core/xds/xds_cluster_resource_type_test.cc b/test/core/xds/xds_cluster_resource_type_test.cc
index 2995ac8c1c7..e7ec0548e5c 100644
--- a/test/core/xds/xds_cluster_resource_type_test.cc
+++ b/test/core/xds/xds_cluster_resource_type_test.cc
@@ -217,6 +217,28 @@ TEST_F(ClusterTypeTest, EdsServiceName) {
   EXPECT_EQ(eds->eds_service_name, "bar");
 }
 
+TEST_F(ClusterTypeTest, EdsServiceNameAbsentWithXdstpName) {
+  Cluster cluster;
+  cluster.set_name("xdstp:foo");
+  cluster.set_type(cluster.EDS);
+  auto* eds_cluster_config = cluster.mutable_eds_cluster_config();
+  eds_cluster_config->mutable_eds_config()->mutable_self();
+  std::string serialized_resource;
+  ASSERT_TRUE(cluster.SerializeToString(&serialized_resource));
+  auto* resource_type = XdsClusterResourceType::Get();
+  auto decode_result =
+      resource_type->Decode(decode_context_, serialized_resource);
+  ASSERT_TRUE(decode_result.name.has_value());
+  EXPECT_EQ(*decode_result.name, "xdstp:foo");
+  EXPECT_EQ(decode_result.resource.status().code(),
+            absl::StatusCode::kInvalidArgument);
+  EXPECT_EQ(decode_result.resource.status().message(),
+            "errors validating Cluster resource: ["
+            "field:eds_cluster_config.service_name "
+            "error:must be set if Cluster resource has an xdstp name]")
+      << decode_result.resource.status();
+}
+
 TEST_F(ClusterTypeTest, DiscoveryTypeNotPresent) {
   Cluster cluster;
   cluster.set_name("foo");