Fix inbound server label ambiguity (#1947)

As the proxy discovers policies, it refers to the source of the policy
configuration. This comes in two flavors: "default" (synthetic)
resources and resources that actually exist in the cluster.

When the proxy reads routes and authorization, the control plane uses
protobuf types to differentiate these states; but server resources are
described by a generic label map. When the proxy reads this label map,
it doesn't properly detect when default/synthetic resources are used and
it _always_ indicates that the resource is real.

This can lead to duplicate metrics when, for instance, the proxy
initially uses a default server and then obtains a default configuration
by discovery.

This change fixes this situation in two ways:

1. We now properly detect and differentiate default server
   configurations.
2. We update the hash and comparisons functions for the `Meta` type so
   that an improperly construct resource matches a default (i.e. when
   inserted into a label map).

Fixes linkerd/linkerd2#9502

Signed-off-by: Oliver Gould <[email protected]>

Author: olix0r

Cargo.lock

@@ -1489,6 +1489,7 @@ dependencies = [
  "ipnet",
  "linkerd-http-route",
  "linkerd2-proxy-api",
+ "maplit",
  "prost-types",
  "quickcheck",
  "thiserror",

linkerd/server-policy/Cargo.toml

@@ -13,9 +13,14 @@ proto = ["linkerd-http-route/proto", "linkerd2-proxy-api", "prost-types"]
 ipnet = "2"
 http = "0.2"
 linkerd-http-route = { path = "../http-route" }
-linkerd2-proxy-api = { version = "0.7", features = ["inbound"], optional = true }
 prost-types = { version = "0.11", optional = true }
 thiserror = "1"
 
+[dependencies.linkerd2-proxy-api]
+version = "0.7"
+features = ["inbound"]
+optional = true
+
 [dev-dependencies]
+maplit = "1"
 quickcheck = { version = "1", default-features = false }

linkerd/server-policy/src/meta.rs

@@ -1,6 +1,6 @@
-use std::{borrow::Cow, hash::Hash, sync::Arc};
+use std::{borrow::Cow, sync::Arc};
 
-#[derive(Debug, PartialEq, Eq, Hash)]
+#[derive(Debug, Eq)]
 pub enum Meta {
     Default {
         name: Cow<'static, str>,
@@ -16,31 +16,47 @@ pub enum Meta {
 
 impl Meta {
     pub fn new_default(name: impl Into<Cow<'static, str>>) -> Arc<Self> {
-        Arc::new(Meta::Default { name: name.into() })
+        Arc::new(Self::Default { name: name.into() })
     }
 
     pub fn name(&self) -> &str {
         match self {
-            Meta::Default { name } => name,
-            Meta::Resource { name, .. } => name,
+            Self::Default { name } => name,
+            Self::Resource { name, .. } => name,
         }
     }
 
     pub fn kind(&self) -> &str {
         match self {
-            Meta::Default { .. } => "default",
-            Meta::Resource { kind, .. } => kind,
+            Self::Default { .. } => "default",
+            Self::Resource { kind, .. } => kind,
         }
     }
 
     pub fn group(&self) -> &str {
         match self {
-            Meta::Default { .. } => "",
-            Meta::Resource { group, .. } => group,
+            Self::Default { .. } => "",
+            Self::Resource { group, .. } => group,
         }
     }
 }
 
+impl std::cmp::PartialEq for Meta {
+    fn eq(&self, other: &Self) -> bool {
+        // Resources that look like Defaults are considered equal.
+        self.group() == other.group() && self.kind() == other.kind() && self.name() == other.name()
+    }
+}
+
+impl std::hash::Hash for Meta {
+    fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
+        // Resources that look like Defaults are considered the same.
+        self.group().hash(state);
+        self.kind().hash(state);
+        self.name().hash(state);
+    }
+}
+
 #[cfg(feature = "proto")]
 pub mod proto {
     use super::*;
@@ -76,19 +92,27 @@ pub mod proto {
                 .unwrap_or_else(|| default_group.to_string());
 
             if let Some(kind) = labels.remove("kind") {
-                return Ok(Arc::new(Meta::Resource { group, kind, name }));
+                // If the controller set a 'group' and 'kind', but it's just
+                // indicating that we're dealing with a default, then build a
+                // default.
+                if group.is_empty() && kind == "default" {
+                    return Ok(Self::new_default(name));
+                }
+
+                return Ok(Arc::new(Self::Resource { group, kind, name }));
             }
 
             // Older control plane versions don't set the kind label and, instead, may
             // encode kinds in the name like `default:deny`.
             let mut parts = name.splitn(2, ':');
             let meta = match (parts.next().unwrap(), parts.next()) {
-                (kind, Some(name)) => Meta::Resource {
+                ("default", Some(name)) => return Ok(Self::new_default(name.to_string())),
+                (kind, Some(name)) => Self::Resource {
                     group,
                     kind: kind.into(),
                     name: name.into(),
                 },
-                (name, None) => Meta::Resource {
+                (name, None) => Self::Resource {
                     group,
                     kind: default_kind.into(),
                     name: name.into(),
@@ -104,7 +128,7 @@ pub mod proto {
 
         fn try_from(pb: api::Metadata) -> Result<Self, Self::Error> {
             match pb.kind.ok_or(InvalidMeta::Missing)? {
-                api::metadata::Kind::Default(name) => Ok(Meta::Default { name: name.into() }),
+                api::metadata::Kind::Default(name) => Ok(Self::Default { name: name.into() }),
                 api::metadata::Kind::Resource(r) => r.try_into(),
             }
         }
@@ -114,11 +138,114 @@ pub mod proto {
         type Error = InvalidMeta;
 
         fn try_from(pb: api::Resource) -> Result<Self, Self::Error> {
-            Ok(Meta::Resource {
+            Ok(Self::Resource {
                 group: pb.group,
                 kind: pb.kind,
                 name: pb.name,
             })
         }
     }
+
+    #[cfg(test)]
+    #[test]
+    fn default_from_labels() {
+        let m = Meta::try_new_with_default(
+            maplit::hashmap! {
+                "group".into() => "".into(),
+                "kind".into() => "default".into(),
+                "name".into() => "foo".into(),
+            },
+            "foog",
+            "fook",
+        )
+        .unwrap();
+        assert!(matches!(&*m, Meta::Default { name } if name == "foo"));
+
+        let m = Meta::try_new_with_default(
+            maplit::hashmap! {
+                "name".into() => "default:foo".into(),
+            },
+            "foog",
+            "fook",
+        )
+        .unwrap();
+        assert!(matches!(&*m, Meta::Default { name } if name == "foo"));
+    }
+}
+
+#[cfg(test)]
+#[test]
+fn cmp() {
+    fn hash(m: &Meta) -> u64 {
+        use std::hash::{Hash, Hasher};
+        let mut h = std::collections::hash_map::DefaultHasher::new();
+        m.hash(&mut h);
+        h.finish()
+    }
+
+    for (left, right, equiv) in [
+        (
+            Meta::Default { name: "foo".into() },
+            Meta::Default { name: "foo".into() },
+            true,
+        ),
+        (
+            Meta::Default { name: "foo".into() },
+            Meta::Default { name: "bar".into() },
+            false,
+        ),
+        (
+            Meta::Default { name: "foo".into() },
+            Meta::Resource {
+                group: "".into(),
+                kind: "default".into(),
+                name: "foo".into(),
+            },
+            true,
+        ),
+        (
+            Meta::Default { name: "foo".into() },
+            Meta::Resource {
+                group: "".into(),
+                kind: "default".into(),
+                name: "bar".into(),
+            },
+            false,
+        ),
+        (
+            Meta::Resource {
+                group: "foog".into(),
+                kind: "fook".into(),
+                name: "foo".into(),
+            },
+            Meta::Resource {
+                group: "foog".into(),
+                kind: "fook".into(),
+                name: "foo".into(),
+            },
+            true,
+        ),
+        (
+            Meta::Resource {
+                group: "foog".into(),
+                kind: "fook".into(),
+                name: "foo".into(),
+            },
+            Meta::Resource {
+                group: "barg".into(),
+                kind: "fook".into(),
+                name: "foo".into(),
+            },
+            false,
+        ),
+    ] {
+        assert!(
+            (left == right) == equiv,
+            "expected {left:?} == {right:?} to be {equiv}",
+        );
+        assert!(
+            (hash(&left) == hash(&right)) == equiv,
+            "expected hash({left:?}) == hash({right:?}) to be {equiv}",
+        );
+    }
 }