Add e2e test for proxy baggage-based metadata discovery + fixes

extensions/common/metadata_object.cc

@@ -90,25 +90,42 @@ std::string WorkloadMetadataObject::baggage() const {
   if (!workload_name_.empty()) {
     parts.push_back("k8s." + std::string(workload_type) + ".name=" + std::string(workload_name_));
   }
+
+  const auto appName = field(Istio::Common::AppNameToken).value_or("");
+  const auto serviceName = field(Istio::Common::ServiceNameToken).value_or(appName);
+
+  if (!serviceName.empty()) {
+    parts.push_back(absl::StrCat(Istio::Common::ServiceNameBaggageToken, "=", serviceName));
+  }
+
+  if (!appName.empty() && appName != serviceName) {
+    parts.push_back(absl::StrCat(Istio::Common::AppNameBaggageToken, "=", appName));
+  }
+
+  const auto appVersion = field(Istio::Common::AppVersionToken).value_or("");
+  const auto serviceVersion = field(Istio::Common::ServiceVersionToken).value_or(appVersion);
+
+  if (!serviceVersion.empty()) {
+    parts.push_back(absl::StrCat(Istio::Common::ServiceVersionBaggageToken, "=", serviceVersion));
+  }
+
+  if (!appVersion.empty() && appVersion != serviceVersion) {
+    parts.push_back(absl::StrCat(Istio::Common::AppVersionBaggageToken, "=", appVersion));
+  }
+
   // Map the workload metadata fields to baggage tokens
   const std::vector<std::pair<absl::string_view, absl::string_view>> field_to_baggage = {
       {Istio::Common::NamespaceNameToken, Istio::Common::NamespaceNameBaggageToken},
       {Istio::Common::ClusterNameToken, Istio::Common::ClusterNameBaggageToken},
-      {Istio::Common::ServiceNameToken, Istio::Common::ServiceNameBaggageToken},
-      {Istio::Common::ServiceVersionToken, Istio::Common::ServiceVersionBaggageToken},
-      {Istio::Common::AppNameToken, Istio::Common::AppNameBaggageToken},
-      {Istio::Common::AppVersionToken, Istio::Common::AppVersionBaggageToken},
       {Istio::Common::InstanceNameToken, Istio::Common::InstanceNameBaggageToken},
       {Istio::Common::RegionToken, Istio::Common::LocalityRegionBaggageToken},
       {Istio::Common::ZoneToken, Istio::Common::LocalityZoneBaggageToken},
   };
 
   for (const auto& [field_name, baggage_key] : field_to_baggage) {
-    const auto field_result = getField(field_name);
-    if (auto field_value = std::get_if<absl::string_view>(&field_result)) {
-      if (!field_value->empty()) {
-        parts.push_back(absl::StrCat(baggage_key, "=", *field_value));
-      }
+    const auto value = field(field_name);
+    if (value && !value->empty()) {
+      parts.push_back(absl::StrCat(baggage_key, "=", *value));
     }
   }
   return absl::StrJoin(parts, ",");
@@ -333,9 +350,13 @@ convertStructToWorkloadMetadata(const google::protobuf::Struct& metadata,
           canonical_name = labels_it.second.string_value();
         } else if (labels_it.first == CanonicalRevisionLabel) {
           canonical_revision = labels_it.second.string_value();
-        } else if (labels_it.first == AppNameLabel) {
+        } else if (labels_it.first == AppNameQualifiedLabel) {
+          app_name = labels_it.second.string_value();
+        } else if (labels_it.first == AppNameLabel && app_name.empty()) {
           app_name = labels_it.second.string_value();
-        } else if (labels_it.first == AppVersionLabel) {
+        } else if (labels_it.first == AppVersionQualifiedLabel) {
+          app_version = labels_it.second.string_value();
+        } else if (labels_it.first == AppVersionLabel && app_version.empty()) {
           app_version = labels_it.second.string_value();
         } else if (!additional_labels.empty() &&
                    additional_labels.contains(std::string(labels_it.first))) {
@@ -386,8 +407,8 @@ std::string serializeToStringDeterministic(const google::protobuf::Struct& metad
   return out;
 }
 
-WorkloadMetadataObject::FieldType
-WorkloadMetadataObject::getField(absl::string_view field_name) const {
+absl::optional<absl::string_view>
+WorkloadMetadataObject::field(absl::string_view field_name) const {
   const auto it = ALL_METADATA_FIELDS.find(field_name);
   if (it != ALL_METADATA_FIELDS.end()) {
     switch (it->second) {
@@ -418,6 +439,15 @@ WorkloadMetadataObject::getField(absl::string_view field_name) const {
       return locality_zone_;
     }
   }
+  return absl::nullopt;
+}
+
+WorkloadMetadataObject::FieldType
+WorkloadMetadataObject::getField(absl::string_view field_name) const {
+  const auto value = field(field_name);
+  if (value) {
+    return *value;
+  }
   return {};
 }
 
@@ -454,18 +484,28 @@ convertBaggageToWorkloadMetadata(absl::string_view data, absl::string_view ident
       case BaggageToken::ServiceName:
         // canonical name and app name are always the same
         canonical_name = parts.second;
-        app_name = parts.second;
+        if (app_name.empty()) {
+          app_name = parts.second;
+        }
         break;
       case BaggageToken::ServiceVersion:
         // canonical revision and app version are always the same
         canonical_revision = parts.second;
-        app_version = parts.second;
+        if (app_version.empty()) {
+          app_version = parts.second;
+        }
         break;
       case BaggageToken::AppName:
         app_name = parts.second;
+        if (canonical_name.empty()) {
+          canonical_name = parts.second;
+        }
         break;
       case BaggageToken::AppVersion:
         app_version = parts.second;
+        if (canonical_revision.empty()) {
+          canonical_revision = parts.second;
+        }
         break;
       case BaggageToken::WorkloadName: {
         workload = parts.second;

extensions/common/metadata_object.h

@@ -43,7 +43,9 @@ constexpr absl::string_view NoPeer = "peer_not_found";
 // Special labels used in the peer metadata.
 constexpr absl::string_view CanonicalNameLabel = "service.istio.io/canonical-name";
 constexpr absl::string_view CanonicalRevisionLabel = "service.istio.io/canonical-revision";
+constexpr absl::string_view AppNameQualifiedLabel = "app.kubernetes.io/name";
 constexpr absl::string_view AppNameLabel = "app";
+constexpr absl::string_view AppVersionQualifiedLabel = "app.kubernetes.io/version";
 constexpr absl::string_view AppVersionLabel = "version";
 
 enum class WorkloadType {
@@ -141,6 +143,7 @@ class WorkloadMetadataObject : public Envoy::StreamInfo::FilterState::Object,
   bool hasFieldSupport() const override { return true; }
   using Envoy::StreamInfo::FilterState::Object::FieldType;
   FieldType getField(absl::string_view) const override;
+  absl::optional<absl::string_view> field(absl::string_view field_name) const;
   void setLabels(std::vector<std::pair<std::string, std::string>> labels) { labels_ = labels; }
   std::vector<std::pair<std::string, std::string>> getLabels() const { return labels_; }
   std::string baggage() const;

extensions/common/metadata_object_test.cc

@@ -25,7 +25,7 @@ namespace Common {
 using Envoy::Protobuf::util::MessageDifferencer;
 using ::testing::NiceMock;
 
-TEST(WorkloadMetadataObjectTest, Baggage) {
+TEST(WorkloadMetadataObjectTest, SerializeAsString) {
   constexpr absl::string_view identity = "spiffe://cluster.local/ns/default/sa/default";
   WorkloadMetadataObject deploy("pod-foo-1234", "my-cluster", "default", "foo", "foo-service",
                                 "v1alpha3", "", "", WorkloadType::Deployment, identity, "", "");
@@ -166,6 +166,35 @@ TEST(WorkloadMetadataObjectTest, Conversion) {
     checkStructConversion(*r);
   }
 
+  {
+    const auto r =
+        convertBaggageToWorkloadMetadata("service.name=foo-service,service.version=v1alpha3");
+    EXPECT_EQ(absl::get<absl::string_view>(r->getField("service")), "foo-service");
+    EXPECT_EQ(absl::get<absl::string_view>(r->getField("revision")), "v1alpha3");
+    EXPECT_EQ(absl::get<absl::string_view>(r->getField("app")), "foo-service");
+    EXPECT_EQ(absl::get<absl::string_view>(r->getField("version")), "v1alpha3");
+    checkStructConversion(*r);
+  }
+
+  {
+    const auto r = convertBaggageToWorkloadMetadata("app.name=foo-app,app.version=latest");
+    EXPECT_EQ(absl::get<absl::string_view>(r->getField("service")), "foo-app");
+    EXPECT_EQ(absl::get<absl::string_view>(r->getField("revision")), "latest");
+    EXPECT_EQ(absl::get<absl::string_view>(r->getField("app")), "foo-app");
+    EXPECT_EQ(absl::get<absl::string_view>(r->getField("version")), "latest");
+    checkStructConversion(*r);
+  }
+
+  {
+    const auto r = convertBaggageToWorkloadMetadata(
+        "service.name=foo-service,service.version=v1alpha3,app.name=foo-app,app.version=latest");
+    EXPECT_EQ(absl::get<absl::string_view>(r->getField("service")), "foo-service");
+    EXPECT_EQ(absl::get<absl::string_view>(r->getField("revision")), "v1alpha3");
+    EXPECT_EQ(absl::get<absl::string_view>(r->getField("app")), "foo-app");
+    EXPECT_EQ(absl::get<absl::string_view>(r->getField("version")), "latest");
+    checkStructConversion(*r);
+  }
+
   {
     const auto r = convertBaggageToWorkloadMetadata("k8s.namespace.name=default");
     EXPECT_EQ(absl::get<absl::string_view>(r->getField("namespace")), "default");

source/extensions/filters/http/peer_metadata/filter_test.cc

@@ -702,8 +702,6 @@ TEST_F(PeerMetadataTest, BaggagePropagationWithNodeMetadata) {
   // Verify baggage contains expected key-value pairs
   EXPECT_TRUE(absl::StrContains(baggage_value, "k8s.namespace.name=production"));
   EXPECT_TRUE(absl::StrContains(baggage_value, "k8s.cluster.name=test-cluster"));
-  EXPECT_TRUE(absl::StrContains(baggage_value, "app.name=test-app"));
-  EXPECT_TRUE(absl::StrContains(baggage_value, "app.version=v1.0"));
   EXPECT_TRUE(absl::StrContains(baggage_value, "service.name=test-service"));
   EXPECT_TRUE(absl::StrContains(baggage_value, "service.version=main"));
   EXPECT_TRUE(absl::StrContains(baggage_value, "k8s.pod.name=test-workload"));

test/envoye2e/inventory.go

@@ -61,5 +61,6 @@ func init() {
 		"TestAdditionalLabels",
 		"TestTCPMXAdditionalLabels",
 		"TestStatsClientSidecarCONNECT",
+		"TestStatsWithBaggageWaypointProxy",
 	}...)
 }

test/envoye2e/stats_plugin/stats_test.go

@@ -662,6 +662,110 @@ func TestStatsServerWaypointProxy(t *testing.T) {
 	}
 }
 
+// TestStatsWithBaggageWaypointProxy verifies that baggage-based metadata discovery works correctly.
+//
+// The test setup is somewhat simplified version of the configuration that pilot generates. The major differences
+// relevant to understanding the test below are as follows:
+//
+//  1. We use just 2 Envoys - client and server, that's what most of the tests here use and we just build
+//     on top of that
+//     - server plays a role of a destination workload ztunnel - it terminates HBONE connection
+//     - client plays a role of a waypoint
+//  2. For connection from the test to the client we don't use HBONE:
+//     - downstream peer metadata discovery works with or without HBONE as long as baggage is provided
+//     - for connection from client to server we have to use HBONE, because upstream metadata discovery
+//     relies on network filters in the upstream internal listener.
+func TestStatsWithBaggageWaypointProxy(t *testing.T) {
+	params := driver.NewTestParams(t, map[string]string{
+		// We are testing baggage-based metadata discovery, so no need for xDS-based metadata discovery
+		"EnableMetadataDiscovery": "false",
+		"RequestCount":            "10",
+		// This makes internal_outbound cluster populate endpoint metadata with original destination details
+		"EnableTunnelEndpointMetadata": "true",
+		// This overrides the server port from ServerPort to ServerTunnelPort, and the listener that can
+		// terminate HBONE listens on the ServerTunnelPort, so that's what we need.
+		"EnableOriginalDstPortOverride": "true",
+	}, envoye2e.ProxyE2ETests)
+
+	params.Vars["ClientMetadata"] = driver.LoadTestData("testdata/client_node_metadata.json.tmpl")
+	params.Vars["ServerMetadata"] = driver.LoadTestData("testdata/server_node_metadata.json.tmpl")
+	params.Vars["ServerClusterName"] = "internal_outbound"
+	params.Vars["ServerInternalAddress"] = "internal_inbound"
+	params.Vars["StatsFilterClientConfig"] = "disable_host_header_fallback: true\nreporter: SERVER_GATEWAY"
+	params.Vars["ClientHTTPFilters"] = driver.LoadTestData("testdata/filters/baggage_peer_metadata.yaml.tmpl") +
+		"\n" + driver.LoadTestData("testdata/filters/stats_outbound.yaml.tmpl")
+	// This filter is what modifies the default framework HTTP response to include the baggage field from
+	// the request as one of the headers in the response
+	params.Vars["ServerHTTPFilters"] = driver.LoadTestData("testdata/filters/respond_with_baggage.yaml.tmpl")
+	params.Vars["UpstreamNetworkFilters"] = driver.LoadTestData("testdata/filters/upstream_peer_metadata.yaml.tmpl")
+
+	clientBaggage := "k8s.deployment.name=productpage-v1,service.name=productpage-v1,app.name=productpage," +
+		"service.version=version-1,app.version=v1,k8s.namespace.name=default,k8s.cluster.name=client-cluster," +
+		"k8s.instance.name=productpage-v1-84975bc778-pxz2w"
+	testBaggage := "k8s.deployment.name=curl,service.name=curl,service.version=v1,k8s.namespace.name=default," +
+		"k8s.cluster.name=curl-cluster,k8s.instance.name=curl-xxxxxxxxxx-xxxxx"
+
+	if err := (&driver.Scenario{
+		Steps: []driver.Step{
+			&driver.XDS{},
+			&driver.Update{
+				Node: "client", Version: "0",
+				Clusters: []string{
+					params.LoadTestData("testdata/cluster/internal_outbound.yaml.tmpl"),
+					params.LoadTestData("testdata/cluster/original_dst.yaml.tmpl"),
+				},
+				Listeners: []string{
+					params.LoadTestData("testdata/listener/client.yaml.tmpl"),
+					params.LoadTestData("testdata/listener/baggage_peer_metadata.yaml.tmpl"),
+				},
+				Secrets: []string{
+					params.LoadTestData("testdata/secret/client.yaml.tmpl"),
+				},
+			},
+			&driver.Update{
+				Node: "server", Version: "0",
+				Clusters: []string{
+					params.LoadTestData("testdata/cluster/internal_inbound.yaml.tmpl"),
+				},
+				Listeners: []string{
+					params.LoadTestData("testdata/listener/terminate_connect.yaml.tmpl"),
+					params.LoadTestData("testdata/listener/server.yaml.tmpl"),
+				},
+				Secrets: []string{
+					params.LoadTestData("testdata/secret/server.yaml.tmpl"),
+				},
+			},
+			&driver.Envoy{Bootstrap: params.LoadTestData("testdata/bootstrap/client.yaml.tmpl")},
+			&driver.Envoy{Bootstrap: params.LoadTestData("testdata/bootstrap/server.yaml.tmpl")},
+			&driver.Sleep{Duration: 1 * time.Second},
+			&driver.Repeat{
+				N: 10,
+				Step: &driver.HTTPCall{
+					Port: params.Ports.ClientPort,
+					RequestHeaders: map[string]string{
+						"baggage": testBaggage,
+					},
+					ResponseCode: 200,
+					ResponseHeaders: map[string]string{
+						// This is what we got from the client
+						"baggage": clientBaggage,
+						// This is what the server got from the client
+						"request-baggage": clientBaggage,
+					},
+				},
+			},
+			&driver.Stats{
+				AdminPort: params.Ports.ClientAdmin,
+				Matchers: map[string]driver.StatMatcher{
+					"istio_requests_total": &driver.ExactStat{Metric: "testdata/metric/client_request_total_baggage.yaml.tmpl"},
+				},
+			},
+		},
+	}).Run(params); err != nil {
+		t.Fatal(err)
+	}
+}
+
 func TestStatsClientSidecarCONNECT(t *testing.T) {
 	params := driver.NewTestParams(t, map[string]string{
 		"RequestCount":            "10",

testdata/cluster/internal_outbound.yaml.tmpl

@@ -22,6 +22,10 @@ load_assignment:
           istio:
             workload: ratings-v1;default;ratings;version-1;server-cluster
 {{- end }}
+{{- if .Vars.UpstreamNetworkFilters }}
+filters:
+{{ .Vars.UpstreamNetworkFilters | fill }}
+{{- end }}
 transport_socket:
   name: envoy.transport_sockets.internal_upstream
   typed_config:

testdata/filters/baggage_peer_metadata.yaml.tmpl

@@ -0,0 +1,12 @@
+- name: waypoint_peer_metadata
+  typed_config:
+    "@type": type.googleapis.com/udpa.type.v1.TypedStruct
+    type_url: type.googleapis.com/io.istio.http.peer_metadata.Config
+    value:
+      downstream_discovery:
+      - baggage: {}
+      downstream_propagation:
+      - baggage: {}
+      upstream_discovery:
+      - upstream_filter_state:
+          peer_metadata_key: upstream_peer

testdata/filters/respond_with_baggage.yaml.tmpl

@@ -0,0 +1,12 @@
+- name: envoy.filters.http.header_mutation
+  typed_config:
+    "@type": type.googleapis.com/udpa.type.v1.TypedStruct
+    type_url: type.googleapis.com/envoy.extensions.filters.http.header_mutation.v3.HeaderMutation
+    value:
+      mutations:
+        response_mutations:
+        - append:
+            header:
+              key: "request-baggage"
+              value: "%FILTER_STATE(io.istio.baggage:PLAIN)%"
+            append_action: OVERWRITE_IF_EXISTS_OR_ADD

testdata/filters/upstream_peer_metadata.yaml.tmpl

@@ -0,0 +1,5 @@
+- name: upstream_peer_metadata
+  typed_config:
+    "@type": type.googleapis.com/udpa.type.v1.TypedStruct
+    type_url: type.googleapis.com/envoy.extensions.network_filters.peer_metadata.UpstreamConfig
+    value: {}

testdata/listener/baggage_peer_metadata.yaml.tmpl

@@ -0,0 +1,29 @@
+name: internal_outbound
+use_original_dst: false
+internal_listener: {}
+listener_filters:
+- name: set_dst_address
+  typed_config:
+    "@type": type.googleapis.com/udpa.type.v1.TypedStruct
+    type_url: type.googleapis.com/envoy.extensions.filters.listener.original_dst.v3.OriginalDst
+filter_chains:
+- filters:
+  - name: envoy.filters.network.peer_metadata
+    typed_config:
+      "@type": type.googleapis.com/udpa.type.v1.TypedStruct
+      type_url: type.googleapis.com/envoy.extensions.network_filters.peer_metadata.Config
+      value:
+        baggage_key: "io.istio.baggage"
+  - name: connect_originate
+    typed_config:
+      "@type": type.googleapis.com/envoy.extensions.filters.network.tcp_proxy.v3.TcpProxy
+      stat_prefix: connect_originate
+      cluster: original_dst
+      tunneling_config:
+        hostname: "%DOWNSTREAM_LOCAL_ADDRESS%"
+        propagate_response_headers: true
+        headers_to_add:
+        - header:
+            key: "baggage"
+            value: "%FILTER_STATE(io.istio.baggage:PLAIN)%"
+          append_action: OVERWRITE_IF_EXISTS_OR_ADD