Skip to content
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

refactor(policy): generalize route types in outbound index #12664

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
2c08f77
refactor: preliminary name changes to make route types generic
the-wondersmith May 30, 2024
b6dbafc
style: cargo fmt
the-wondersmith May 30, 2024
c053346
revert: revert lock changes
the-wondersmith May 30, 2024
b397fe8
refactor: generalize outbound route types
the-wondersmith May 30, 2024
ce8a72e
Merge branch 'main' into policy-refactor-index-outbound-generalize-ro…
the-wondersmith May 31, 2024
be33011
revert(deps): revert lockfile changes
the-wondersmith Jun 3, 2024
acf6102
revert(deps): revert dep changes
the-wondersmith Jun 3, 2024
229d892
chore(deps): fix bad lockfile update
the-wondersmith Jun 3, 2024
124d390
Merge remote-tracking branch 'refs/remotes/origin/main' into policy-r…
the-wondersmith Jun 4, 2024
584fd1b
style: consolidate imports from same crate
the-wondersmith Jun 4, 2024
c90c5fd
chore: remove resolved todo
the-wondersmith Jun 4, 2024
fb040c9
style: revert `for_each` calls back to explicit for loops
the-wondersmith Jun 4, 2024
ac269f0
refactor: add Empty variant to OutboundRouteType
the-wondersmith Jun 4, 2024
61faf69
Merge branch 'main' into policy-refactor-index-outbound-generalize-ro…
the-wondersmith Jun 4, 2024
153e350
perf: improve implementation of `insert` and `remove`
the-wondersmith Jun 4, 2024
ad46e78
perf: remove unnecessary emptiness check
the-wondersmith Jun 4, 2024
817907c
style: revert change to filter vs if in ServiceRoutes::apply
the-wondersmith Jun 4, 2024
92d4ccb
Merge branch 'main' into policy-refactor-index-outbound-generalize-ro…
the-wondersmith Jun 5, 2024
259fc2c
bump
the-wondersmith Jun 5, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.lock
Original file line number Diff line number Diff line change
Expand Up @@ -1221,6 +1221,7 @@ dependencies = [
"futures",
"http",
"hyper",
"itertools",
"linkerd-policy-controller-core",
"linkerd2-proxy-api",
"maplit",
Expand Down Expand Up @@ -1258,7 +1259,6 @@ dependencies = [
"chrono",
"futures",
"http",
"k8s-gateway-api",
"k8s-openapi",
"kube",
"kubert",
Expand Down
78 changes: 70 additions & 8 deletions policy-controller/core/src/outbound.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use crate::routes::{
GroupKindNamespaceName, HeaderModifierFilter, HostMatch, HttpRouteMatch, RequestRedirectFilter,
FailureInjectorFilter, GroupKindNamespaceName, HeaderModifierFilter, HostMatch, HttpRouteMatch,
RequestRedirectFilter,
};
use ahash::AHashMap as HashMap;
use anyhow::Result;
Expand All @@ -26,9 +27,21 @@ pub struct OutboundDiscoverTarget {
pub source_namespace: String,
}

#[derive(Clone, Debug, PartialEq, Eq)]
pub enum TypedOutboundRoute {
Http(OutboundRoute<HttpRouteMatch>),
}

#[derive(Clone, Debug, Default, PartialEq, Eq)]
pub enum OutboundRouteCollection {
#[default]
Empty,
Http(HashMap<GroupKindNamespaceName, OutboundRoute<HttpRouteMatch>>),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of adding an Empty variant to this enum and then making routes an OutboundRouteCollection instead of an Option<OutboundRouteCollection>. (we would use OutboundRouteCollection::Empty instead of None)

The advantage of this is it lets this type internalize more of the state logic. E.g. calling insert with an HttpRoute while in the Empty state transitions to the OutboundRouteCollection::Http state. Calling insert with a GrpcRoute while in the Http state returns an error. Calling remove which causes the HashMap to become empty transitions it to the Empty state. etc. etc.

Internalizing this state logic lets us get rid of functions like default_collection_for.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with all of that. I went the Option route to reduce the diff, the amount of additional/future code maintenance introduced, etc...

If there's buy in for it, I think your suggestion would for sure be a more polished way to go

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adleong implemented and pushed the changes to make this happen. Looking forward to your thoughts

}

#[derive(Clone, Debug, PartialEq)]
pub struct OutboundPolicy {
pub http_routes: HashMap<GroupKindNamespaceName, HttpRoute>,
pub routes: OutboundRouteCollection,
pub authority: String,
pub name: String,
pub namespace: String,
Expand All @@ -38,18 +51,18 @@ pub struct OutboundPolicy {
}

#[derive(Clone, Debug, PartialEq, Eq)]
pub struct HttpRoute {
pub struct OutboundRoute<MatchType> {
pub hostnames: Vec<HostMatch>,
pub rules: Vec<HttpRouteRule>,
pub rules: Vec<OutboundRouteRule<MatchType>>,

/// This is required for ordering returned `HttpRoute`s by their creation
/// timestamp.
/// This is required for ordering returned routes
/// by their creation timestamp.
pub creation_timestamp: Option<DateTime<Utc>>,
}

#[derive(Clone, Debug, PartialEq, Eq)]
pub struct HttpRouteRule {
pub matches: Vec<HttpRouteMatch>,
pub struct OutboundRouteRule<MatchType> {
pub matches: Vec<MatchType>,
pub backends: Vec<Backend>,
pub request_timeout: Option<time::Duration>,
pub backend_request_timeout: Option<time::Duration>,
Expand Down Expand Up @@ -98,4 +111,53 @@ pub enum Filter {
RequestHeaderModifier(HeaderModifierFilter),
ResponseHeaderModifier(HeaderModifierFilter),
RequestRedirect(RequestRedirectFilter),
FailureInjector(FailureInjectorFilter),
}

// === impl TypedOutboundRoute ===

impl From<OutboundRoute<HttpRouteMatch>> for TypedOutboundRoute {
fn from(route: OutboundRoute<HttpRouteMatch>) -> Self {
Self::Http(route)
}
}

// === impl OutboundRouteCollection ===

impl OutboundRouteCollection {
pub fn is_empty(&self) -> bool {
matches!(self, Self::Empty)
}

pub fn remove(&mut self, key: &GroupKindNamespaceName) {
match self {
Self::Empty => {}
Self::Http(routes) => {
routes.remove(key);
if routes.is_empty() {
*self = Self::Empty;
}
}
}
}

pub fn insert<Route: Into<TypedOutboundRoute>>(
&mut self,
key: GroupKindNamespaceName,
route: Route,
) -> Result<Option<TypedOutboundRoute>> {
let route = route.into();

match (self, route) {
(this @ Self::Empty, TypedOutboundRoute::Http(route)) => {
let mut routes = HashMap::default();
let inserted = routes.insert(key, route).map(Into::into);
*this = Self::Http(routes);
Ok(inserted)
}
(Self::Http(routes), TypedOutboundRoute::Http(route)) => {
Ok(routes.insert(key, route).map(Into::into))
}
}
}
}
15 changes: 14 additions & 1 deletion policy-controller/core/src/routes.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use anyhow::Result;

pub use http::{
header::{HeaderName, HeaderValue},
uri::Scheme,
Expand All @@ -8,7 +9,6 @@ use regex::Regex;
use std::{borrow::Cow, num::NonZeroU16};

#[derive(Clone, Debug, Hash, PartialEq, Eq)]

pub struct GroupKindName {
pub group: Cow<'static, str>,
pub kind: Cow<'static, str>,
Expand Down Expand Up @@ -126,6 +126,19 @@ impl GroupKindName {
}
}

// === impl HttpRouteMatch ===

impl Default for HttpRouteMatch {
fn default() -> Self {
Self {
method: None,
headers: vec![],
query_params: vec![],
path: Some(PathMatch::Prefix("/".to_string())),
}
}
}

// === impl PathMatch ===

impl PartialEq for PathMatch {
Expand Down
3 changes: 2 additions & 1 deletion policy-controller/grpc/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,9 @@ async-stream = "0.3"
async-trait = "0.1"
http = "0.2"
drain = "0.1"
hyper = { version = "0.14", features = ["http2", "server", "tcp"] }
futures = { version = "0.3", default-features = false }
hyper = { version = "0.14", features = ["http2", "server", "tcp"] }
itertools = "0.12"
linkerd-policy-controller-core = { path = "../core" }
maplit = "1"
prost-types = "0.12.6"
Expand Down
Loading