-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Mixer Client uses Node metadata to populate Mixer attributes #1961
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
Changes from 24 commits
e05a9e5
bda84ae
acfcccc
1bd4e3a
95dc720
8bcdb0c
70139cc
06eba60
f2210ab
f53f6e5
e75a35f
977b7bf
7ed5742
bf0840d
2b2a748
84223fc
a142f1b
f2f9067
37ee3a7
fe42b37
86bb3ee
cf0467c
e32821a
9c245a7
c0b19f2
b83c060
cfd30bf
8d06af7
d9669fc
6817646
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 |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| /* Copyright 2018 Istio Authors. All Rights Reserved. | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| #ifndef ISTIO_UTILS_LOCAL_ATTRIBUTES_H | ||
| #define ISTIO_UTILS_LOCAL_ATTRIBUTES_H | ||
|
|
||
| #include "mixer/v1/attributes.pb.h" | ||
|
|
||
| using ::istio::mixer::v1::Attributes; | ||
|
mandarjog marked this conversation as resolved.
Outdated
|
||
|
|
||
| namespace istio { | ||
| namespace utils { | ||
|
|
||
| struct LocalAttributes { | ||
| LocalAttributes(const Attributes& inbound, const Attributes& outbound, | ||
| const Attributes& forward) | ||
| : inbound(inbound), outbound(outbound), forward(forward) {} | ||
|
|
||
| // local inbound attributes | ||
| const Attributes inbound; | ||
|
|
||
| // local outbound attributes | ||
| const Attributes outbound; | ||
|
|
||
| // local forward attributes | ||
| const Attributes forward; | ||
| }; | ||
|
|
||
| // LocalNode are used to extract information from envoy Node. | ||
| struct LocalNode { | ||
|
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. There should be nothing envoy specific under istio directory. Maybe get rid of this struct and just create LocalAttributes from envoy node in utils there?
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. LocalNode is a very portable concept. It has the few pieces of information that we need. |
||
| std::string ns; | ||
| std::string ip; | ||
| std::string uid; | ||
| }; | ||
|
|
||
| std::unique_ptr<const LocalAttributes> CreateLocalAttributes( | ||
| const LocalNode& local); | ||
|
|
||
| } // namespace utils | ||
| } // namespace istio | ||
|
|
||
| #endif // ISTIO_UTILS_LOCAL_ATTRIBUTES_H | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,9 +17,18 @@ | |
| #include "src/envoy/utils/grpc_transport.h" | ||
|
|
||
| using ::istio::mixerclient::Statistics; | ||
| using ::istio::utils::AttributeName; | ||
| using ::istio::utils::LocalAttributes; | ||
| using ::istio::utils::LocalNode; | ||
|
|
||
| namespace Envoy { | ||
| namespace Utils { | ||
|
|
||
| const char NodeKey::kName[] = "NODE_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. Can we just have UID and namespace? We don't know fully qualified UID yet, not until multi-cluster support lands in Mixer. Until then, we should treat UID as opaque, and not assume it include kubernetes:// and only has two parts.
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. I removed string processing from code. |
||
| const char NodeKey::kNamespace[] = "NODE_NAMESPACE"; | ||
| const char NodeKey::kIp[] = "NODE_IP"; | ||
| const char NodeKey::kRegistry[] = "NODE_REGISTRY"; | ||
|
|
||
| namespace { | ||
|
|
||
| // A class to wrap envoy timer for mixer client timer. | ||
|
|
@@ -100,5 +109,70 @@ Grpc::AsyncClientFactoryPtr GrpcClientFactoryForCluster( | |
| return std::make_unique<EnvoyGrpcAsyncClientFactory>(cm, service); | ||
| } | ||
|
|
||
| // This function is for compatibility with existing node ids. | ||
| // "sidecar~10.36.0.15~fortioclient-84469dc8d7-jbbxt.service-graph~service-graph.svc.cluster.local" | ||
| // --> {proxy_type}~{ip}~{node_name}.{node_ns}~{node_domain} | ||
| bool ExtractInfoCompat(const std::string &nodeid, LocalNode *args) { | ||
| auto parts = StringUtil::splitToken(nodeid, "~"); | ||
| if (parts.size() < 3) { | ||
| GOOGLE_LOG(ERROR) << "ExtractInfoCompat error len(nodeid.split(~))<3: " | ||
| << nodeid; | ||
| return false; | ||
| } | ||
|
|
||
| auto ip = std::string(parts[1].begin(), parts[1].end()); | ||
| auto longname = std::string(parts[2].begin(), parts[2].end()); | ||
| auto names = StringUtil::splitToken(longname, "."); | ||
| if (names.size() < 2) { | ||
| GOOGLE_LOG(ERROR) | ||
| << "ExtractInfoCompat error len(split(longname, '.')) < 3: " | ||
| << longname; | ||
| return false; | ||
| } | ||
| auto ns = std::string(names[1].begin(), names[1].end()); | ||
|
|
||
| args->ip = ip; | ||
| args->ns = ns; | ||
| args->uid = "kubernetes://" + longname; | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| // ExtractInfo depends on NODE_NAME, NODE_NAMESPACE, NODE_IP and optional | ||
| // NODE_REG If work cannot be done, returns false. | ||
| bool ExtractInfo(const envoy::api::v2::core::Node &node, LocalNode *args) { | ||
| const auto meta = node.metadata().fields(); | ||
|
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. check if node.has_metadata() first or if node.metadata().fields().empty()
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. done. |
||
| std::string name; | ||
| if (!ReadMap(meta, NodeKey::kName, &name)) { | ||
| GOOGLE_LOG(ERROR) << "ExtractInfo metadata missing " << NodeKey::kName | ||
| << " " << node.metadata().DebugString(); | ||
| return false; | ||
| } | ||
| std::string ns; | ||
| ReadMap(meta, NodeKey::kNamespace, &ns); | ||
|
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. check return value
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. done. |
||
|
|
||
| std::string ip; | ||
| ReadMap(meta, NodeKey::kIp, &ip); | ||
|
|
||
| std::string reg("kubernetes"); | ||
| ReadMap(meta, NodeKey::kRegistry, ®); | ||
|
|
||
| args->ip = ip; | ||
| args->ns = ns; | ||
| args->uid = reg + "://" + name + "." + ns; | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| bool Extract(const envoy::api::v2::core::Node &node, LocalNode *args) { | ||
| if (ExtractInfo(node, args)) { | ||
| return true; | ||
| } | ||
| if (ExtractInfoCompat(node.id(), args)) { | ||
| return true; | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| } // namespace Utils | ||
| } // namespace Envoy | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,12 +16,18 @@ | |
| #pragma once | ||
|
|
||
| #include "envoy/event/dispatcher.h" | ||
| #include "envoy/local_info/local_info.h" | ||
| #include "envoy/runtime/runtime.h" | ||
| #include "envoy/upstream/cluster_manager.h" | ||
| #include "include/istio/mixerclient/client.h" | ||
| #include "include/istio/utils/attribute_names.h" | ||
| #include "include/istio/utils/local_attributes.h" | ||
| #include "src/envoy/utils/config.h" | ||
|
|
||
| using ::istio::mixer::v1::Attributes; | ||
| using ::istio::mixer::v1::Attributes_AttributeValue; | ||
| using ::istio::utils::LocalAttributes; | ||
| using ::istio::utils::LocalNode; | ||
|
|
||
| namespace Envoy { | ||
| namespace Utils { | ||
|
|
@@ -42,5 +48,40 @@ Grpc::AsyncClientFactoryPtr GrpcClientFactoryForCluster( | |
| const std::string &cluster_name, Upstream::ClusterManager &cm, | ||
| Stats::Scope &scope); | ||
|
|
||
| std::unique_ptr<const LocalAttributes> CreateLocalAttributes( | ||
|
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. Do you expect these functions to be used by multiple places? if no, just one place, please just move code to that place.
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 left there erroneously. Removed. |
||
| const LocalNode &local); | ||
|
|
||
| bool Extract(const envoy::api::v2::core::Node &node, LocalNode *args); | ||
|
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. ExtractNodeInfo?
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. done. |
||
|
|
||
| inline bool ReadMap( | ||
|
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. I don't think this is a correct use of metadata in envoy. It assumes that the key is some filter ID, and the list of values is inside the struct value. Take a look at
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. node metadata is actually
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. are you suggesting that this should be nested inside Mixer ?
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. OK, yes, I assumed node proto is filter-aware. This is fine.
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. ReadProtoMap
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. done. |
||
| const google::protobuf::Map<std::string, google::protobuf::Value> &meta, | ||
| const std::string &key, std::string *val) { | ||
| const auto it = meta.find(key); | ||
| if (it != meta.end()) { | ||
| *val = it->second.string_value(); | ||
| return true; | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| inline bool ReadMap( | ||
|
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. ReadAttributeMap
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. done. |
||
| const google::protobuf::Map<std::string, Attributes_AttributeValue> &meta, | ||
| const std::string &key, std::string *val) { | ||
| const auto it = meta.find(key); | ||
| if (it != meta.end()) { | ||
| *val = it->second.string_value(); | ||
| return true; | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| // NodeKey are node matadata keys that are expected to be set. | ||
| struct NodeKey { | ||
|
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 should be a proto that is extracted from metadata for mixer filter (filter ID "mixer").
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. I suspect that these names are not shared by many places. If so, just defined inside .cc file and inside anonymous namespace
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. moved it to .cc file. |
||
| static const char kName[]; | ||
| static const char kNamespace[]; | ||
| static const char kIp[]; | ||
| static const char kRegistry[]; | ||
| }; | ||
|
|
||
| } // namespace Utils | ||
| } // namespace Envoy | ||
Uh oh!
There was an error while loading. Please reload this page.