diff --git a/extensions/common/metadata_object.cc b/extensions/common/metadata_object.cc index 74e1e09424a..4c880d9a26e 100644 --- a/extensions/common/metadata_object.cc +++ b/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> 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(&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 +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; diff --git a/extensions/common/metadata_object.h b/extensions/common/metadata_object.h index 956d2e7edf2..f24f3bcd252 100644 --- a/extensions/common/metadata_object.h +++ b/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 field(absl::string_view field_name) const; void setLabels(std::vector> labels) { labels_ = labels; } std::vector> getLabels() const { return labels_; } std::string baggage() const; diff --git a/extensions/common/metadata_object_test.cc b/extensions/common/metadata_object_test.cc index 1d085952599..097e962dfd1 100644 --- a/extensions/common/metadata_object_test.cc +++ b/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(r->getField("service")), "foo-service"); + EXPECT_EQ(absl::get(r->getField("revision")), "v1alpha3"); + EXPECT_EQ(absl::get(r->getField("app")), "foo-service"); + EXPECT_EQ(absl::get(r->getField("version")), "v1alpha3"); + checkStructConversion(*r); + } + + { + const auto r = convertBaggageToWorkloadMetadata("app.name=foo-app,app.version=latest"); + EXPECT_EQ(absl::get(r->getField("service")), "foo-app"); + EXPECT_EQ(absl::get(r->getField("revision")), "latest"); + EXPECT_EQ(absl::get(r->getField("app")), "foo-app"); + EXPECT_EQ(absl::get(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(r->getField("service")), "foo-service"); + EXPECT_EQ(absl::get(r->getField("revision")), "v1alpha3"); + EXPECT_EQ(absl::get(r->getField("app")), "foo-app"); + EXPECT_EQ(absl::get(r->getField("version")), "latest"); + checkStructConversion(*r); + } + { const auto r = convertBaggageToWorkloadMetadata("k8s.namespace.name=default"); EXPECT_EQ(absl::get(r->getField("namespace")), "default"); diff --git a/source/extensions/filters/http/peer_metadata/filter_test.cc b/source/extensions/filters/http/peer_metadata/filter_test.cc index 6c0dd8b3b89..d1c00f45f20 100644 --- a/source/extensions/filters/http/peer_metadata/filter_test.cc +++ b/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")); diff --git a/test/envoye2e/inventory.go b/test/envoye2e/inventory.go index aed10e00316..e2c0f7216be 100644 --- a/test/envoye2e/inventory.go +++ b/test/envoye2e/inventory.go @@ -61,5 +61,6 @@ func init() { "TestAdditionalLabels", "TestTCPMXAdditionalLabels", "TestStatsClientSidecarCONNECT", + "TestStatsWithBaggageWaypointProxy", }...) } diff --git a/test/envoye2e/stats_plugin/stats_test.go b/test/envoye2e/stats_plugin/stats_test.go index 28691918f92..e5fb1cca81b 100644 --- a/test/envoye2e/stats_plugin/stats_test.go +++ b/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", diff --git a/testdata/cluster/internal_outbound.yaml.tmpl b/testdata/cluster/internal_outbound.yaml.tmpl index 7bca7e86a44..485a3ab4966 100644 --- a/testdata/cluster/internal_outbound.yaml.tmpl +++ b/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: diff --git a/testdata/filters/baggage_peer_metadata.yaml.tmpl b/testdata/filters/baggage_peer_metadata.yaml.tmpl new file mode 100644 index 00000000000..91d7f757ef5 --- /dev/null +++ b/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 diff --git a/testdata/filters/respond_with_baggage.yaml.tmpl b/testdata/filters/respond_with_baggage.yaml.tmpl new file mode 100644 index 00000000000..6b3a52c9108 --- /dev/null +++ b/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 diff --git a/testdata/filters/upstream_peer_metadata.yaml.tmpl b/testdata/filters/upstream_peer_metadata.yaml.tmpl new file mode 100644 index 00000000000..ddac6578fb8 --- /dev/null +++ b/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: {} diff --git a/testdata/listener/baggage_peer_metadata.yaml.tmpl b/testdata/listener/baggage_peer_metadata.yaml.tmpl new file mode 100644 index 00000000000..8eefcd99482 --- /dev/null +++ b/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 diff --git a/testdata/listener/terminate_connect.yaml.tmpl b/testdata/listener/terminate_connect.yaml.tmpl index cc930e5caba..0066d069900 100644 --- a/testdata/listener/terminate_connect.yaml.tmpl +++ b/testdata/listener/terminate_connect.yaml.tmpl @@ -60,6 +60,12 @@ filter_chains: text_format_source: inline_string: "%DOWNSTREAM_LOCAL_URI_SAN%" shared_with_upstream: ONCE + - object_key: io.istio.baggage + factory_key: envoy.string + format_string: + text_format_source: + inline_string: "%REQ(baggage)%" + shared_with_upstream: ONCE {{ end }} - name: peer_metadata typed_config: @@ -68,6 +74,8 @@ filter_chains: value: downstream_discovery: - workload_discovery: {} + downstream_propagation: + - baggage: {} shared_with_upstream: true - name: envoy.filters.http.router typed_config: diff --git a/testdata/metric/client_request_total_baggage.yaml.tmpl b/testdata/metric/client_request_total_baggage.yaml.tmpl new file mode 100644 index 00000000000..3bb2439b7a0 --- /dev/null +++ b/testdata/metric/client_request_total_baggage.yaml.tmpl @@ -0,0 +1,60 @@ +name: istio_requests_total +type: COUNTER +metric: +- counter: + value: {{ .Vars.RequestCount }} + label: + - name: reporter + value: waypoint + - name: source_workload + value: curl + - name: source_canonical_service + value: curl + - name: source_canonical_revision + value: v1 + - name: source_workload_namespace + value: default + - name: source_principal + value: unknown + - name: source_app + value: curl + - name: source_version + value: v1 + - name: source_cluster + value: curl-cluster + - name: destination_workload + value: ratings-v1 + - name: destination_workload_namespace + value: default + - name: destination_principal + value: spiffe://cluster.local/ns/default/sa/server + - name: destination_app + value: ratings + - name: destination_version + value: v1 + - name: destination_service + value: server.default.svc.cluster.local + - name: destination_canonical_service + value: ratings + - name: destination_canonical_revision + value: version-1 + - name: destination_service_name + value: server + - name: destination_service_namespace + value: default + - name: destination_cluster + value: server-cluster + - name: request_protocol + {{- if .Vars.GrpcResponseStatus }} + value: grpc + {{- else }} + value: http + {{- end }} + - name: response_code + value: "200" + - name: grpc_response_status + value: "{{ .Vars.GrpcResponseStatus }}" + - name: response_flags + value: "-" + - name: connection_security_policy + value: none