From c2fe98798627527145d047fa285bdcff87a5b704 Mon Sep 17 00:00:00 2001 From: Mikhail Krinkin Date: Tue, 27 Jan 2026 01:08:59 +0000 Subject: [PATCH 1/2] Drop app labels from baggage and propagate principal I think I confused folks a bit when I mentioned that app field is missing from the baggage - it wasn't. In fact, canonical name of the workload and app in ambient are the same thing, that's why baggage does not actually need an app label - it already has service.name that encodes what we need. I updated the design document, but it happened after I mentioned here and there that we need to add a missing field to the baggage. This change corrects implementation and that makes istio stats populate the app label correctly. The other field that has not been populated is principal. WorkloadMetadataObject contained that identity field that contained principle in principle, but the methods used to conver WorkloadMetadataObject to a protobuf Struct and back ignored that field and never populated it, so it got lost and istio stats never used it. We haven't noticed that before because in ambient we used xDS-based peer metadata discovery by default and it triggers a different code path that does not rely on the methods that convert protobuf Struct to WorkloadMetadataObject, and the code path used there didn't have the same issue. Signed-off-by: Mikhail Krinkin --- extensions/common/metadata_object.cc | 24 ++++++++++++----------- extensions/common/metadata_object.h | 4 ++-- extensions/common/metadata_object_test.cc | 18 +++++++++++------ 3 files changed, 27 insertions(+), 19 deletions(-) diff --git a/extensions/common/metadata_object.cc b/extensions/common/metadata_object.cc index c39c79431a7..aef6f50ed46 100644 --- a/extensions/common/metadata_object.cc +++ b/extensions/common/metadata_object.cc @@ -49,8 +49,6 @@ static absl::flat_hash_map ALL_BAGGAGE_TOKENS = {ClusterNameBaggageToken, BaggageToken::ClusterName}, {ServiceNameBaggageToken, BaggageToken::ServiceName}, {ServiceVersionBaggageToken, BaggageToken::ServiceVersion}, - {AppNameBaggageToken, BaggageToken::AppName}, - {AppVersionBaggageToken, BaggageToken::AppVersion}, {DeploymentNameBaggageToken, BaggageToken::WorkloadName}, {PodNameBaggageToken, BaggageToken::WorkloadName}, {CronjobNameBaggageToken, BaggageToken::WorkloadName}, @@ -96,8 +94,6 @@ std::string WorkloadMetadataObject::baggage() const { {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}, @@ -226,6 +222,8 @@ absl::optional WorkloadMetadataObject::owner() const { return {}; } +std::string WorkloadMetadataObject::identity() const { return identity_; } + WorkloadType fromSuffix(absl::string_view suffix) { const auto it = ALL_WORKLOAD_TOKENS.find(suffix); if (it != ALL_WORKLOAD_TOKENS.end()) { @@ -260,6 +258,9 @@ google::protobuf::Struct convertWorkloadMetadataToStruct(const WorkloadMetadataO if (!obj.cluster_name_.empty()) { (*metadata.mutable_fields())[ClusterMetadataField].set_string_value(obj.cluster_name_); } + if (!obj.identity_.empty()) { + (*metadata.mutable_fields())[IdentityMetadataField].set_string_value(obj.identity_); + } auto* labels = (*metadata.mutable_fields())[LabelsMetadataField].mutable_struct_value(); if (!obj.canonical_name_.empty()) { (*labels->mutable_fields())[CanonicalNameLabel].set_string_value(obj.canonical_name_); @@ -306,7 +307,7 @@ std::unique_ptr convertStructToWorkloadMetadata(const google::protobuf::Struct& metadata, const absl::flat_hash_set& additional_labels, const absl::optional locality) { - absl::string_view instance, namespace_name, owner, workload, cluster, canonical_name, + absl::string_view instance, namespace_name, owner, workload, cluster, identity, canonical_name, canonical_revision, app_name, app_version, region, zone; std::vector> labels; for (const auto& it : metadata.fields()) { @@ -320,6 +321,8 @@ convertStructToWorkloadMetadata(const google::protobuf::Struct& metadata, workload = it.second.string_value(); } else if (it.first == ClusterMetadataField) { cluster = it.second.string_value(); + } else if (it.first == IdentityMetadataField) { + identity = it.second.string_value(); } else if (it.first == LabelsMetadataField) { for (const auto& labels_it : it.second.struct_value().fields()) { if (labels_it.first == CanonicalNameLabel) { @@ -350,7 +353,7 @@ convertStructToWorkloadMetadata(const google::protobuf::Struct& metadata, } auto obj = std::make_unique( instance, cluster, namespace_name, workload, canonical_name, canonical_revision, app_name, - app_version, parseOwner(owner, workload), "", locality_region, locality_zone); + app_version, parseOwner(owner, workload), identity, locality_region, locality_zone); obj->setLabels(labels); return obj; } @@ -445,15 +448,14 @@ convertBaggageToWorkloadMetadata(absl::string_view data, absl::string_view ident cluster = parts.second; break; case BaggageToken::ServiceName: + // canonical name and app name are always the same canonical_name = parts.second; + app_name = parts.second; break; case BaggageToken::ServiceVersion: + // canonical revision and app version are always the same + canonical_name = parts.second; canonical_revision = parts.second; - break; - case BaggageToken::AppName: - app_name = parts.second; - break; - case BaggageToken::AppVersion: app_version = parts.second; break; case BaggageToken::WorkloadName: { diff --git a/extensions/common/metadata_object.h b/extensions/common/metadata_object.h index 4356aba838f..11c011c9deb 100644 --- a/extensions/common/metadata_object.h +++ b/extensions/common/metadata_object.h @@ -91,8 +91,6 @@ constexpr absl::string_view NamespaceNameBaggageToken = "k8s.namespace.name"; constexpr absl::string_view ClusterNameBaggageToken = "k8s.cluster.name"; constexpr absl::string_view ServiceNameBaggageToken = "service.name"; constexpr absl::string_view ServiceVersionBaggageToken = "service.version"; -constexpr absl::string_view AppNameBaggageToken = "app.name"; -constexpr absl::string_view AppVersionBaggageToken = "app.version"; constexpr absl::string_view DeploymentNameBaggageToken = "k8s.deployment.name"; constexpr absl::string_view PodNameBaggageToken = "k8s.pod.name"; constexpr absl::string_view CronjobNameBaggageToken = "k8s.cronjob.name"; @@ -104,6 +102,7 @@ constexpr absl::string_view LocalityZoneBaggageToken = "cloud.availability_zone" constexpr absl::string_view InstanceMetadataField = "NAME"; constexpr absl::string_view NamespaceMetadataField = "NAMESPACE"; constexpr absl::string_view ClusterMetadataField = "CLUSTER_ID"; +constexpr absl::string_view IdentityMetadataField = "IDENTITY"; constexpr absl::string_view OwnerMetadataField = "OWNER"; constexpr absl::string_view WorkloadMetadataField = "WORKLOAD_NAME"; constexpr absl::string_view LabelsMetadataField = "LABELS"; @@ -131,6 +130,7 @@ class WorkloadMetadataObject : public Envoy::StreamInfo::FilterState::Object, std::vector> serializeAsPairs() const; absl::optional serializeAsString() const override; absl::optional owner() const; + std::string identity() const; bool hasFieldSupport() const override { return true; } using Envoy::StreamInfo::FilterState::Object::FieldType; FieldType getField(absl::string_view) const override; diff --git a/extensions/common/metadata_object_test.cc b/extensions/common/metadata_object_test.cc index 82542a0d597..81a77878fa6 100644 --- a/extensions/common/metadata_object_test.cc +++ b/extensions/common/metadata_object_test.cc @@ -26,17 +26,19 @@ using Envoy::Protobuf::util::MessageDifferencer; using ::testing::NiceMock; TEST(WorkloadMetadataObjectTest, Baggage) { + 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, "", "", ""); + "v1alpha3", "", "", WorkloadType::Deployment, identity, "", ""); WorkloadMetadataObject pod("pod-foo-1234", "my-cluster", "default", "foo", "foo-service", - "v1alpha3", "", "", WorkloadType::Pod, "", "", ""); + "v1alpha3", "", "", WorkloadType::Pod, identity, "", ""); WorkloadMetadataObject cronjob("pod-foo-1234", "my-cluster", "default", "foo", "foo-service", - "v1alpha3", "foo-app", "v1", WorkloadType::CronJob, "", "", ""); + "v1alpha3", "foo-app", "v1", WorkloadType::CronJob, identity, "", + ""); WorkloadMetadataObject job("pod-foo-1234", "my-cluster", "default", "foo", "foo-service", - "v1alpha3", "", "", WorkloadType::Job, "", "", ""); + "v1alpha3", "", "", WorkloadType::Job, identity, "", ""); EXPECT_EQ(deploy.serializeAsString(), absl::StrCat("type=deployment,workload=foo,name=pod-foo-1234,cluster=my-cluster,", @@ -66,8 +68,9 @@ void checkStructConversion(const Envoy::StreamInfo::FilterState::Object& data) { } TEST(WorkloadMetadataObjectTest, ConversionWithLabels) { + 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, "", "", ""); + "v1alpha3", "", "", WorkloadType::Deployment, identity, "", ""); deploy.setLabels({{"label1", "value1"}, {"label2", "value2"}}); auto pb = convertWorkloadMetadataToStruct(deploy); auto obj1 = convertStructToWorkloadMetadata(pb, {"label1", "label2"}); @@ -81,10 +84,12 @@ TEST(WorkloadMetadataObjectTest, ConversionWithLabels) { TEST(WorkloadMetadataObjectTest, Conversion) { { + constexpr absl::string_view identity = "spiffe://cluster.local/ns/default/sa/default"; const auto r = convertBaggageToWorkloadMetadata( "k8s.deployment.name=foo,k8s.cluster.name=my-cluster," "k8s.namespace.name=default,service.name=foo-service,service.version=v1alpha3,app.name=foo-" - "app,app.version=latest"); + "app,app.version=latest", + identity); EXPECT_EQ(absl::get(r->getField("service")), "foo-service"); EXPECT_EQ(absl::get(r->getField("revision")), "v1alpha3"); EXPECT_EQ(absl::get(r->getField("type")), DeploymentSuffix); @@ -94,6 +99,7 @@ TEST(WorkloadMetadataObjectTest, Conversion) { EXPECT_EQ(absl::get(r->getField("cluster")), "my-cluster"); EXPECT_EQ(absl::get(r->getField("app")), "foo-app"); EXPECT_EQ(absl::get(r->getField("version")), "latest"); + EXPECT_EQ(r->identity(), identity); checkStructConversion(*r); } From b085ff26164a44b47e7ae23686571f71b32388c3 Mon Sep 17 00:00:00 2001 From: Keith Mattix II Date: Tue, 27 Jan 2026 02:01:37 +0000 Subject: [PATCH 2/2] Keep backwards compatibility for app.service and app.version baggage fields Signed-off-by: Keith Mattix II --- extensions/common/metadata_object.cc | 11 ++++++++++- extensions/common/metadata_object.h | 2 ++ extensions/common/metadata_object_test.cc | 10 ++++++++-- 3 files changed, 20 insertions(+), 3 deletions(-) diff --git a/extensions/common/metadata_object.cc b/extensions/common/metadata_object.cc index aef6f50ed46..74e1e09424a 100644 --- a/extensions/common/metadata_object.cc +++ b/extensions/common/metadata_object.cc @@ -49,6 +49,8 @@ static absl::flat_hash_map ALL_BAGGAGE_TOKENS = {ClusterNameBaggageToken, BaggageToken::ClusterName}, {ServiceNameBaggageToken, BaggageToken::ServiceName}, {ServiceVersionBaggageToken, BaggageToken::ServiceVersion}, + {AppNameBaggageToken, BaggageToken::AppName}, + {AppVersionBaggageToken, BaggageToken::AppVersion}, {DeploymentNameBaggageToken, BaggageToken::WorkloadName}, {PodNameBaggageToken, BaggageToken::WorkloadName}, {CronjobNameBaggageToken, BaggageToken::WorkloadName}, @@ -94,6 +96,8 @@ std::string WorkloadMetadataObject::baggage() const { {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}, @@ -454,10 +458,15 @@ convertBaggageToWorkloadMetadata(absl::string_view data, absl::string_view ident break; case BaggageToken::ServiceVersion: // canonical revision and app version are always the same - canonical_name = parts.second; canonical_revision = parts.second; app_version = parts.second; break; + case BaggageToken::AppName: + app_name = parts.second; + break; + case BaggageToken::AppVersion: + app_version = parts.second; + break; case BaggageToken::WorkloadName: { workload = parts.second; std::vector splitWorkloadKey = absl::StrSplit(parts.first, "."); diff --git a/extensions/common/metadata_object.h b/extensions/common/metadata_object.h index 11c011c9deb..13e1460f17f 100644 --- a/extensions/common/metadata_object.h +++ b/extensions/common/metadata_object.h @@ -91,6 +91,8 @@ constexpr absl::string_view NamespaceNameBaggageToken = "k8s.namespace.name"; constexpr absl::string_view ClusterNameBaggageToken = "k8s.cluster.name"; constexpr absl::string_view ServiceNameBaggageToken = "service.name"; constexpr absl::string_view ServiceVersionBaggageToken = "service.version"; +constexpr absl::string_view AppNameBaggageToken = "app.name"; +constexpr absl::string_view AppVersionBaggageToken = "app.version"; constexpr absl::string_view DeploymentNameBaggageToken = "k8s.deployment.name"; constexpr absl::string_view PodNameBaggageToken = "k8s.pod.name"; constexpr absl::string_view CronjobNameBaggageToken = "k8s.cronjob.name"; diff --git a/extensions/common/metadata_object_test.cc b/extensions/common/metadata_object_test.cc index 81a77878fa6..1d085952599 100644 --- a/extensions/common/metadata_object_test.cc +++ b/extensions/common/metadata_object_test.cc @@ -114,8 +114,8 @@ TEST(WorkloadMetadataObjectTest, Conversion) { EXPECT_EQ(absl::get(r->getField("name")), "foo-instance-435"); EXPECT_EQ(absl::get(r->getField("namespace")), "test"); EXPECT_EQ(absl::get(r->getField("cluster")), "my-cluster"); - EXPECT_EQ(absl::get(r->getField("app")), ""); - EXPECT_EQ(absl::get(r->getField("version")), ""); + EXPECT_EQ(absl::get(r->getField("app")), "foo-service"); + EXPECT_EQ(absl::get(r->getField("version")), "v1beta2"); checkStructConversion(*r); } @@ -130,6 +130,8 @@ TEST(WorkloadMetadataObjectTest, Conversion) { EXPECT_EQ(absl::get(r->getField("name")), "foo-instance-435"); EXPECT_EQ(absl::get(r->getField("namespace")), "test"); EXPECT_EQ(absl::get(r->getField("cluster")), "my-cluster"); + EXPECT_EQ(absl::get(r->getField("app")), "foo-service"); + EXPECT_EQ(absl::get(r->getField("version")), "v1beta4"); checkStructConversion(*r); } @@ -144,6 +146,8 @@ TEST(WorkloadMetadataObjectTest, Conversion) { EXPECT_EQ(absl::get(r->getField("name")), ""); EXPECT_EQ(absl::get(r->getField("namespace")), "test"); EXPECT_EQ(absl::get(r->getField("cluster")), "my-cluster"); + EXPECT_EQ(absl::get(r->getField("app")), "foo-service"); + EXPECT_EQ(absl::get(r->getField("version")), "v1beta4"); checkStructConversion(*r); } @@ -157,6 +161,8 @@ TEST(WorkloadMetadataObjectTest, Conversion) { EXPECT_EQ(absl::get(r->getField("workload")), "foo"); EXPECT_EQ(absl::get(r->getField("namespace")), "default"); EXPECT_EQ(absl::get(r->getField("cluster")), ""); + EXPECT_EQ(absl::get(r->getField("app")), "foo-service"); + EXPECT_EQ(absl::get(r->getField("version")), "v1alpha3"); checkStructConversion(*r); }