-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Baggage discovery #6779
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Baggage discovery #6779
Changes from all commits
4c87a04
c429b9d
7d22875
72394b4
fba6e72
5d84f5a
08d5751
52467c8
0e0806f
efd48f5
7407537
dbf83ef
ac7c5ef
1a87c89
70f2b9d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,7 +24,10 @@ namespace Istio { | |
| namespace Common { | ||
|
|
||
| namespace { | ||
| static absl::flat_hash_map<absl::string_view, BaggageToken> ALL_BAGGAGE_TOKENS = { | ||
|
|
||
| // This maps field names into baggage tokens. We use it to decode field names | ||
| // when WorkloadMetadataObject content is accessed through the Envoy API. | ||
| static absl::flat_hash_map<absl::string_view, BaggageToken> ALL_METADATA_FIELDS = { | ||
| {NamespaceNameToken, BaggageToken::NamespaceName}, | ||
| {ClusterNameToken, BaggageToken::ClusterName}, | ||
| {ServiceNameToken, BaggageToken::ServiceName}, | ||
|
|
@@ -36,6 +39,22 @@ static absl::flat_hash_map<absl::string_view, BaggageToken> ALL_BAGGAGE_TOKENS = | |
| {InstanceNameToken, BaggageToken::InstanceName}, | ||
| }; | ||
|
|
||
| // This maps baggage keys into baggage tokens. We use it to decode baggage keys | ||
| // coming over the wire when building WorkloadMetadataObject. | ||
| static absl::flat_hash_map<absl::string_view, BaggageToken> ALL_BAGGAGE_TOKENS = { | ||
| {NamespaceNameBaggageToken, BaggageToken::NamespaceName}, | ||
| {ClusterNameBaggageToken, BaggageToken::ClusterName}, | ||
| {ServiceNameBaggageToken, BaggageToken::ServiceName}, | ||
| {ServiceVersionBaggageToken, BaggageToken::ServiceVersion}, | ||
| {AppNameBaggageToken, BaggageToken::AppName}, | ||
| {AppVersionBaggageToken, BaggageToken::AppVersion}, | ||
| {DeploymentNameBaggageToken, BaggageToken::WorkloadName}, | ||
| {PodNameBaggageToken, BaggageToken::WorkloadName}, | ||
| {CronjobNameBaggageToken, BaggageToken::WorkloadName}, | ||
| {JobNameBaggageToken, BaggageToken::WorkloadName}, | ||
| {InstanceNameBaggageToken, BaggageToken::InstanceName}, | ||
| }; | ||
|
|
||
| static absl::flat_hash_map<absl::string_view, WorkloadType> ALL_WORKLOAD_TOKENS = { | ||
| {PodSuffix, WorkloadType::Pod}, | ||
| {DeploymentSuffix, WorkloadType::Deployment}, | ||
|
|
@@ -69,13 +88,13 @@ std::string WorkloadMetadataObject::baggage() const { | |
| } | ||
| // 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, "k8s.namespace.name"}, | ||
| {Istio::Common::ClusterNameToken, "k8s.cluster.name"}, | ||
| {Istio::Common::ServiceNameToken, "service.name"}, | ||
| {Istio::Common::ServiceVersionToken, "service.version"}, | ||
| {Istio::Common::AppNameToken, "app.name"}, | ||
| {Istio::Common::AppVersionToken, "app.version"}, | ||
| {Istio::Common::InstanceNameToken, "k8s.instance.name"}, | ||
| {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}, | ||
| }; | ||
|
|
||
| for (const auto& [field_name, baggage_key] : field_to_baggage) { | ||
|
|
@@ -320,8 +339,8 @@ std::string serializeToStringDeterministic(const google::protobuf::Struct& metad | |
|
|
||
| WorkloadMetadataObject::FieldType | ||
| WorkloadMetadataObject::getField(absl::string_view field_name) const { | ||
| const auto it = ALL_BAGGAGE_TOKENS.find(field_name); | ||
| if (it != ALL_BAGGAGE_TOKENS.end()) { | ||
| const auto it = ALL_METADATA_FIELDS.find(field_name); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This feels like a contract change; even if the naming is clearer, we CANNOT change the contract. Please revert
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah I see what you're doing. Make sure you double check all of the other filters in istio-proxy to ensure they're not depending on the old naming. If possible, I'd prefer to split the rename into its own change later just so we and future reviewers can focus on the substantive changes
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO this change is reasonably safe. |
||
| if (it != ALL_METADATA_FIELDS.end()) { | ||
| switch (it->second) { | ||
| case BaggageToken::NamespaceName: | ||
| return namespace_name_; | ||
|
|
@@ -389,15 +408,19 @@ convertBaggageToWorkloadMetadata(absl::string_view data, absl::string_view ident | |
| case BaggageToken::AppVersion: | ||
| app_version = parts.second; | ||
| break; | ||
| case BaggageToken::WorkloadName: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why the change if this worked before?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is old cold, it worked with the previous baggage keys. Now that the workload type is embedded in baggage key we have to get the type and the name from the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note how it now comes from
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh good catch; yeah the ztunnel format and old pilot format are apparently different |
||
| case BaggageToken::WorkloadName: { | ||
| workload = parts.second; | ||
| std::vector<absl::string_view> splitWorkloadKey = absl::StrSplit(parts.first, "."); | ||
| if (splitWorkloadKey.size() >= 2 && splitWorkloadKey[0] == "k8s") { | ||
| workload_type = fromSuffix(splitWorkloadKey[1]); | ||
| } | ||
| break; | ||
| case BaggageToken::WorkloadType: | ||
| workload_type = fromSuffix(parts.second); | ||
| break; | ||
| } | ||
| case BaggageToken::InstanceName: | ||
| instance = parts.second; | ||
| break; | ||
| default: | ||
| break; | ||
| } | ||
| } | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's used in
getField()which is in-turn used to generatae baggageThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ALL_BAGGAGE_TOKENSis used to convert baggage into metadata, andALL_METADATA_FIELDSis used for creating baggage from metadata. At first I thought we hadgetFieldbeing used also in other parts of the code, but looks like we don't. I'll unify the mappings and change the tests accordingly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I'm afraid WorkloadMetadtaObject can be accessed from Envoy's API and we may need to maintain compatibility. So if there's any use out there to access data using the old telemetry labels, we probably don't want to break that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added some comments, I hope it makes things clearer.