Skip to content

Commit

Permalink
Refactor FullyQualifiedAuthority::normalize to always return authority (
Browse files Browse the repository at this point in the history 
#476)

As requested by @briansmith in #415 (comment) and #415 (comment), I've refactored `FullyQualifiedAuthority::normalize` to _always_ return a `FullyQualifiedAuthority`, along with a boolean value indicating whether or not the Destination service should be used for that authority. 

This is in contrast to returning an `Option<FullyQualifiedAuthority>` where `None` indicated that the Destination service should not be used, which is what this function did previously.

This is required for further progress on #415.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
  • Loading branch information
@hawkw
hawkw authored Feb 28, 2018
1 parent 1e611c2 commit 41bef41
Show file tree
Hide file tree
Showing 3 changed files with 123 additions and 90 deletions.
5 changes: 3 additions & 2 deletions proxy/src/control/discovery.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,8 @@ where
Entry::Vacant(vac) => {
let req = Destination {
scheme: "k8s".into(),
path: vac.key().without_trailing_dot().into(),
path: vac.key().without_trailing_dot()
.as_str().into(),
};
// TODO: Can grpc::Request::new be removed?
let mut svc = DestinationSvc::new(client.lift_ref());
Expand Down Expand Up @@ -298,7 +299,7 @@ where
trace!("Destination.Get reconnect {:?}", auth);
let req = Destination {
scheme: "k8s".into(),
path: auth.without_trailing_dot().into(),
path: auth.without_trailing_dot().as_str().into(),
};
let mut svc = DestinationSvc::new(client.lift_ref());
let response = svc.get(grpc::Request::new(req));
Expand Down
197 changes: 113 additions & 84 deletions proxy/src/fully_qualified_authority.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,16 @@ use std::str::FromStr;

use http::uri::Authority;

/// A normalized `Authority`.
#[derive(Clone, Debug, Eq, Hash, PartialEq)]
pub struct FullyQualifiedAuthority(Authority);

#[derive(Clone, Debug, Eq, Hash, PartialEq)]
pub struct NamedAddress {
pub name: FullyQualifiedAuthority,
pub use_destination_service: bool
}

impl FullyQualifiedAuthority {
/// Normalizes the name according to Kubernetes service naming conventions.
/// Case folding is not done; that is done internally inside `Authority`.
Expand All @@ -16,11 +23,15 @@ impl FullyQualifiedAuthority {
///
/// Returns `None` is authority doesn't look like a local Kubernetes service.
pub fn normalize(authority: &Authority, default_namespace: Option<&str>,
default_zone: Option<&str>)
-> Option<FullyQualifiedAuthority> {
default_zone: Option<&str>)
-> NamedAddress
{
// Don't change IP-address-based authorities.
if IpAddr::from_str(authority.host()).is_ok() {
return None;
return NamedAddress {
name: FullyQualifiedAuthority(authority.clone()),
use_destination_service: false,
}
};

// TODO: `Authority` doesn't have a way to get the serialized form of the
Expand All @@ -38,7 +49,12 @@ impl FullyQualifiedAuthority {
if name.ends_with('.') {
let authority = authority.clone().into_bytes();
let normalized = authority.slice(0, authority.len() - 1);
return Some(FullyQualifiedAuthority(Authority::from_shared(normalized).unwrap()));
let authority = Authority::from_shared(normalized).unwrap();
let name = FullyQualifiedAuthority(authority);
return NamedAddress {
name,
use_destination_service: true,
}
}

// parts should have a maximum 4 of pieces (name, namespace, svc, zone)
Expand All @@ -59,15 +75,21 @@ impl FullyQualifiedAuthority {
let append_svc = if let Some(part) = parts.next() {
if !part.eq_ignore_ascii_case("svc") {
// if not "$name.$namespace.svc", treat as external
return None;
return NamedAddress {
name: FullyQualifiedAuthority(authority.clone()),
use_destination_service: false,
}
}

false
} else if has_explicit_namespace {
true
} else if namespace_to_append.is_none() {
// We can't append ".svc" without a namespace, so treat as external.
return None;
return NamedAddress {
name: FullyQualifiedAuthority(authority.clone()),
use_destination_service: false,
}
} else {
true
};
Expand All @@ -78,7 +100,10 @@ impl FullyQualifiedAuthority {
if !zone.eq_ignore_ascii_case(default_zone) {
// if "a.b.svc.foo" and zone is not "foo",
// treat as external
return None;
return NamedAddress {
name: FullyQualifiedAuthority(authority.clone()),
use_destination_service: false,
}
}
}
None
Expand All @@ -99,7 +124,10 @@ impl FullyQualifiedAuthority {

// If we're not going to change anything then don't allocate anything.
if additional_len == 0 {
return Some(FullyQualifiedAuthority(authority.clone()));
return NamedAddress {
name: FullyQualifiedAuthority(authority.clone()),
use_destination_service: true,
}
}

// `authority.as_str().len()` includes the length of `colon_port`.
Expand All @@ -119,154 +147,155 @@ impl FullyQualifiedAuthority {
}
normalized.extend_from_slice(colon_port.as_bytes());

Some(FullyQualifiedAuthority(Authority::from_shared(normalized.freeze())
.expect("syntactically-valid authority")))
let name = Authority::from_shared(normalized.freeze())
.expect("syntactically-valid authority");
let name = FullyQualifiedAuthority(name);
NamedAddress {
name,
use_destination_service: true,
}
}

pub fn without_trailing_dot(&self) -> &str {
self.0.as_str()
pub fn without_trailing_dot(&self) -> &Authority {
&self.0
}
}

#[cfg(test)]
mod tests {
#[test]
fn test_normalized_authority() {
fn f(input: &str, default_namespace: Option<&str>,
fn local(input: &str, default_namespace: Option<&str>,
default_zone: Option<&str>)
-> String {
use bytes::Bytes;
use http::uri::Authority;

let input = Authority::from_shared(Bytes::from(input.as_bytes())).unwrap();
let output = match super::FullyQualifiedAuthority::normalize(
&input, default_namespace, default_zone) {
Some(output) => output,
None => panic!(
"unexpected None for input={:?}, default_namespace={:?}, default_zone={:?}",
input,
default_namespace,
default_zone
),
};
output.without_trailing_dot().into()
let input = Authority::from_shared(Bytes::from(input.as_bytes()))
.unwrap();
let output = super::FullyQualifiedAuthority::normalize(
&input, default_namespace, default_zone);
assert_eq!(output.use_destination_service, true);
output.name.without_trailing_dot().as_str().into()
}

fn none(input: &str, default_namespace: Option<&str>,
fn external(input: &str, default_namespace: Option<&str>,
default_zone: Option<&str>) {
use bytes::Bytes;
use http::uri::Authority;

let input = Authority::from_shared(Bytes::from(input.as_bytes())).unwrap();
assert_eq!(None, super::FullyQualifiedAuthority::normalize(
&input, default_namespace, default_zone));
let output = super::FullyQualifiedAuthority::normalize(
&input, default_namespace, default_zone);
assert_eq!(output.use_destination_service, false);
assert_eq!(output.name.without_trailing_dot().as_str(), input);
}

none("name", None, None);
external("name", None, None);
assert_eq!("name.namespace.svc",
f("name.namespace", None, None));
local("name.namespace", None, None));
assert_eq!("name.namespace.svc",
f("name.namespace.svc", None, None));
local("name.namespace.svc", None, None));
assert_eq!("name.namespace.svc.cluster",
f("name.namespace.svc.cluster", None, None));
local("name.namespace.svc.cluster", None, None));
assert_eq!("name.namespace.svc.cluster.local",
f("name.namespace.svc.cluster.local", None, None));
local("name.namespace.svc.cluster.local", None, None));

assert_eq!("name.namespace.svc",
f("name", Some("namespace"), None));
local("name", Some("namespace"), None));
assert_eq!("name.namespace.svc",
f("name.namespace", Some("namespace"), None));
local("name.namespace", Some("namespace"), None));
assert_eq!("name.namespace.svc",
f("name.namespace.svc", Some("namespace"), None));
local("name.namespace.svc", Some("namespace"), None));
assert_eq!("name.namespace.svc.cluster",
f("name.namespace.svc.cluster", Some("namespace"), None));
local("name.namespace.svc.cluster", Some("namespace"), None));
assert_eq!("name.namespace.svc.cluster.local",
f("name.namespace.svc.cluster.local", Some("namespace"), None));
local("name.namespace.svc.cluster.local", Some("namespace"), None));

none("name", None, Some("cluster.local"));
external("name", None, Some("cluster.local"));
assert_eq!("name.namespace.svc.cluster.local",
f("name.namespace", None, Some("cluster.local")));
local("name.namespace", None, Some("cluster.local")));
assert_eq!("name.namespace.svc.cluster.local",
f("name.namespace.svc", None, Some("cluster.local")));
none("name.namespace.svc.cluster", None, Some("cluster.local"));
local("name.namespace.svc", None, Some("cluster.local")));
external("name.namespace.svc.cluster", None, Some("cluster.local"));
assert_eq!("name.namespace.svc.cluster.local",
f("name.namespace.svc.cluster.local", None, Some("cluster.local")));
local("name.namespace.svc.cluster.local", None, Some("cluster.local")));

assert_eq!("name.namespace.svc.cluster.local",
f("name", Some("namespace"), Some("cluster.local")));
local("name", Some("namespace"), Some("cluster.local")));
assert_eq!("name.namespace.svc.cluster.local",
f("name.namespace", Some("namespace"), Some("cluster.local")));
local("name.namespace", Some("namespace"), Some("cluster.local")));
assert_eq!("name.namespace.svc.cluster.local",
f("name.namespace.svc", Some("namespace"), Some("cluster.local")));
none("name.namespace.svc.cluster", Some("namespace"), Some("cluster.local"));
local("name.namespace.svc", Some("namespace"), Some("cluster.local")));
external("name.namespace.svc.cluster", Some("namespace"), Some("cluster.local"));
assert_eq!("name.namespace.svc.cluster.local",
f("name.namespace.svc.cluster.local", Some("namespace"), Some("cluster.local")));
local("name.namespace.svc.cluster.local", Some("namespace"), Some("cluster.local")));

// Fully-qualified names end with a dot and aren't modified except by removing the dot.
assert_eq!("name",
f("name.", None, None));
local("name.", None, None));
assert_eq!("name.namespace",
f("name.namespace.", None, None));
local("name.namespace.", None, None));
assert_eq!("name.namespace.svc",
f("name.namespace.svc.", None, None));
local("name.namespace.svc.", None, None));
assert_eq!("name.namespace.svc.cluster",
f("name.namespace.svc.cluster.", None, None));
local("name.namespace.svc.cluster.", None, None));
assert_eq!("name.namespace.svc.cluster.local",
f("name.namespace.svc.cluster.local.", None, None));
local("name.namespace.svc.cluster.local.", None, None));
assert_eq!("name",
f("name.", Some("namespace"), None));
local("name.", Some("namespace"), None));
assert_eq!("name.namespace",
f("name.namespace.", Some("namespace"), None));
local("name.namespace.", Some("namespace"), None));
assert_eq!("name.namespace.svc",
f("name.namespace.svc.", Some("namespace"), None));
local("name.namespace.svc.", Some("namespace"), None));
assert_eq!("name.namespace.svc.cluster",
f("name.namespace.svc.cluster.", Some("namespace"), None));
local("name.namespace.svc.cluster.", Some("namespace"), None));
assert_eq!("name.namespace.svc.cluster.local",
f("name.namespace.svc.cluster.local.", Some("namespace"), None));
local("name.namespace.svc.cluster.local.", Some("namespace"), None));
assert_eq!("name",
f("name.", Some("namespace"), Some("cluster.local")));
local("name.", Some("namespace"), Some("cluster.local")));
assert_eq!("name.namespace",
f("name.namespace.", Some("namespace"), Some("cluster.local")));
local("name.namespace.", Some("namespace"), Some("cluster.local")));
assert_eq!("name.namespace.svc",
f("name.namespace.svc.", Some("namespace"), Some("cluster.local")));
local("name.namespace.svc.", Some("namespace"), Some("cluster.local")));
assert_eq!("name.namespace.svc.cluster",
f("name.namespace.svc.cluster.", Some("namespace"), Some("cluster.local")));
local("name.namespace.svc.cluster.", Some("namespace"), Some("cluster.local")));
assert_eq!("name.namespace.svc.cluster.local",
f("name.namespace.svc.cluster.local.", Some("namespace"), Some("cluster.local")));
local("name.namespace.svc.cluster.local.", Some("namespace"), Some("cluster.local")));

// Ports are preserved.
assert_eq!("name.namespace.svc.cluster.local:1234",
f("name:1234", Some("namespace"), Some("cluster.local")));
local("name:1234", Some("namespace"), Some("cluster.local")));
assert_eq!("name.namespace.svc.cluster.local:1234",
f("name.namespace:1234", Some("namespace"), Some("cluster.local")));
local("name.namespace:1234", Some("namespace"), Some("cluster.local")));
assert_eq!("name.namespace.svc.cluster.local:1234",
f("name.namespace.svc:1234", Some("namespace"), Some("cluster.local")));
none("name.namespace.svc.cluster:1234", Some("namespace"), Some("cluster.local"));
local("name.namespace.svc:1234", Some("namespace"), Some("cluster.local")));
external("name.namespace.svc.cluster:1234", Some("namespace"), Some("cluster.local"));
assert_eq!("name.namespace.svc.cluster.local:1234",
f("name.namespace.svc.cluster.local:1234", Some("namespace"), Some("cluster.local")));
local("name.namespace.svc.cluster.local:1234", Some("namespace"), Some("cluster.local")));

// "SVC" is recognized as being equivalent to "svc"
assert_eq!("name.namespace.SVC.cluster.local",
f("name.namespace.SVC", Some("namespace"), Some("cluster.local")));
none("name.namespace.SVC.cluster", Some("namespace"), Some("cluster.local"));
local("name.namespace.SVC", Some("namespace"), Some("cluster.local")));
external("name.namespace.SVC.cluster", Some("namespace"), Some("cluster.local"));
assert_eq!("name.namespace.SVC.cluster.local",
f("name.namespace.SVC.cluster.local", Some("namespace"), Some("cluster.local")));
local("name.namespace.SVC.cluster.local", Some("namespace"), Some("cluster.local")));

// IPv4 addresses are left unchanged.
none("1.2.3.4", Some("namespace"), Some("cluster.local"));
none("1.2.3.4:1234", Some("namespace"), Some("cluster.local"));
none("127.0.0.1", Some("namespace"), Some("cluster.local"));
none("127.0.0.1:8080", Some("namespace"), Some("cluster.local"));
none("127.0.0.1", None, Some("cluster.local"));
none("127.0.0.1", Some("namespace"), None);
none("127.0.0.1", None, None);
none("127.0.0.1", Some("1"), Some("1"));
external("1.2.3.4", Some("namespace"), Some("cluster.local"));
external("1.2.3.4:1234", Some("namespace"), Some("cluster.local"));
external("127.0.0.1", Some("namespace"), Some("cluster.local"));
external("127.0.0.1:8080", Some("namespace"), Some("cluster.local"));
external("127.0.0.1", None, Some("cluster.local"));
external("127.0.0.1", Some("namespace"), None);
external("127.0.0.1", None, None);
external("127.0.0.1", Some("1"), Some("1"));

// IPv6 addresses are left unchanged.
none("[::1]", Some("namespace"), Some("cluster.local"));
none("[::1]:1234", Some("namespace"), Some("cluster.local"));
none("[::1]", None, Some("cluster.local"));
none("[::1]", Some("namespace"), None);
none("[::1]", None, None);
external("[::1]", Some("namespace"), Some("cluster.local"));
external("[::1]:1234", Some("namespace"), Some("cluster.local"));
external("[::1]", None, Some("cluster.local"));
external("[::1]", Some("namespace"), None);
external("[::1]", None, None);
}
}
11 changes: 7 additions & 4 deletions proxy/src/outbound.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use bind::{self, Bind, Protocol};
use control::{self, discovery};
use control::discovery::Bind as BindTrait;
use ctx;
use fully_qualified_authority::FullyQualifiedAuthority;
use fully_qualified_authority::{FullyQualifiedAuthority, NamedAddress};
use timeout::Timeout;

type BindProtocol<B> = bind::BindProtocol<Arc<ctx::Proxy>, B>;
Expand Down Expand Up @@ -72,7 +72,7 @@ where
>>>>;

fn recognize(&self, req: &Self::Request) -> Option<Self::Key> {
let local = req.uri().authority_part().and_then(|authority| {
let local = req.uri().authority_part().map(|authority| {
FullyQualifiedAuthority::normalize(
authority,
self.default_namespace.as_ref().map(|s| s.as_ref()),
Expand All @@ -87,8 +87,11 @@ where
// In practice, this shouldn't ever happen, since we expect the proxy
// to be run on Linux servers, with iptables setup, so there should
// always be an original destination.
let dest = if let Some(local) = local {
Destination::LocalSvc(local)
let dest = if let Some(NamedAddress {
name,
use_destination_service: true
}) = local {
Destination::LocalSvc(name)
} else {
let orig_dst = req.extensions()
.get::<Arc<ctx::transport::Server>>()
Expand Down

0 comments on commit 41bef41

Please sign in to comment.