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.

Sign up for GitHub

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

feat(policy): check Service port in admission controller #13325

Merged
merged 10 commits into from
Nov 16, 2024
Merged

feat(policy): check Service port in admission controller #13325

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
7a8034d
feat(policy): Check Service port in admission controller
olix0r Nov 14, 2024
750084d
fix test
olix0r Nov 14, 2024
002015e
improve error message
olix0r Nov 14, 2024
a3c00f6
fix(policy): correct service matching
olix0r Nov 14, 2024
fd3d8e5
remove info used for debugging
olix0r Nov 14, 2024
c73ecdd
simplify
olix0r Nov 15, 2024
66dbb67
simplify
olix0r Nov 15, 2024
19cc228
fix typo
olix0r Nov 15, 2024
6f927e6
simplify
olix0r Nov 15, 2024
eb9f5df
Merge branch 'main' into ver/http-port-admission
olix0r Nov 15, 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
36 changes: 33 additions & 3 deletions policy-controller/src/admission.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use crate::k8s::policy::{
NetworkAuthentication, NetworkAuthenticationSpec, RateLimitPolicySpec, Server,
ServerAuthorization, ServerAuthorizationSpec, ServerSpec,
};
use anyhow::{anyhow, bail, ensure, Result};
use anyhow::{anyhow, bail, ensure, Context, Result};
use futures::future;
use hyper::{body::Buf, http, Body, Request, Response};
use k8s_openapi::api::core::v1::{Namespace, ServiceAccount};
Expand Down Expand Up @@ -650,6 +650,20 @@ impl Validate<HttpRouteSpec> for Admission {
}
}

fn validate_backend_if_service(br: &k8s_gateway_api::BackendObjectReference) -> Result<()> {
let is_service = matches!(br.group.as_deref(), Some("core") | Some("") | None)
&& matches!(br.kind.as_deref(), Some("Service") | None);

// If the backend reference is a Service, it must have a port. If it is not
// a Service, then we have to admit it for interoperability with other
// controllers.
if is_service && matches!(br.port, None | Some(0)) {
bail!("cannot reference a Service without a port");
}

Ok(())
}

#[async_trait::async_trait]
impl Validate<k8s_gateway_api::HttpRouteSpec> for Admission {
async fn validate(
Expand Down Expand Up @@ -697,7 +711,9 @@ impl Validate<k8s_gateway_api::HttpRouteSpec> for Admission {
// from `HttpRouteSpec` to `InboundRouteBinding`, except that we don't
// actually allocate stuff in order to return an `InboundRouteBinding`.
for k8s_gateway_api::HttpRouteRule {
filters, matches, ..
filters,
matches,
backend_refs,
} in spec.rules.into_iter().flatten()
{
for m in matches.into_iter().flatten() {
Expand All @@ -707,6 +723,14 @@ impl Validate<k8s_gateway_api::HttpRouteSpec> for Admission {
for f in filters.into_iter().flatten() {
validate_filter(f)?;
}

for br in backend_refs
.iter()
.flatten()
.filter_map(|br| br.backend_ref.as_ref())
{
validate_backend_if_service(&br.inner).context("invalid backendRef")?;
}
}

Ok(())
Expand Down Expand Up @@ -784,7 +808,9 @@ impl Validate<k8s_gateway_api::GrpcRouteSpec> for Admission {
// a `GrpcMethodMatch` rule where neither `method.method`
// nor `method.service` actually contain a value)
for k8s_gateway_api::GrpcRouteRule {
filters, matches, ..
filters,
matches,
backend_refs,
} in spec.rules.into_iter().flatten()
{
for rule in matches.into_iter().flatten() {
Expand All @@ -794,6 +820,10 @@ impl Validate<k8s_gateway_api::GrpcRouteSpec> for Admission {
for filter in filters.into_iter().flatten() {
validate_filter(filter)?;
}

for br in backend_refs.iter().flatten() {
validate_backend_if_service(&br.inner).context("invalid backendRef")?;
}
}

Ok(())
Expand Down
185 changes: 185 additions & 0 deletions policy-test/tests/admit_http_route_gateway.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,117 @@ async fn accepts_not_implemented_extensionref() {
.await;
}

#[tokio::test(flavor = "current_thread")]
async fn accepts_backend_unknown_kind() {
admission::accepts(|ns| HttpRoute {
metadata: meta(&ns),
spec: HttpRouteSpec {
inner: CommonRouteSpec {
parent_refs: Some(vec![server_parent_ref(ns)]),
},
hostnames: None,
rules: Some(vec![HttpRouteRule {
matches: Some(vec![HttpRouteMatch {
path: Some(HttpPathMatch::Exact {
value: "/foo".to_string(),
}),
..HttpRouteMatch::default()
}]),
filters: None,
backend_refs: Some(vec![k8s_gateway_api::HttpBackendRef {
backend_ref: Some(k8s_gateway_api::BackendRef {
weight: None,
inner: BackendObjectReference {
group: Some("alien.example.com".to_string()),
kind: Some("ExoService".to_string()),
namespace: Some("foo".to_string()),
name: "foo".to_string(),
port: None,
},
}),
filters: None,
}]),
}]),
},
status: None,
})
.await;
}

#[tokio::test(flavor = "current_thread")]
async fn accepts_backend_service_with_port() {
admission::accepts(|ns| HttpRoute {
metadata: meta(&ns),
spec: HttpRouteSpec {
inner: CommonRouteSpec {
parent_refs: Some(vec![server_parent_ref(ns)]),
},
hostnames: None,
rules: Some(vec![HttpRouteRule {
matches: Some(vec![HttpRouteMatch {
path: Some(HttpPathMatch::Exact {
value: "/foo".to_string(),
}),
..HttpRouteMatch::default()
}]),
filters: None,
backend_refs: Some(vec![k8s_gateway_api::HttpBackendRef {
backend_ref: Some(k8s_gateway_api::BackendRef {
weight: None,
inner: BackendObjectReference {
group: Some("core".to_string()),
kind: Some("Service".to_string()),
namespace: Some("foo".to_string()),
name: "foo".to_string(),
port: Some(8080),
},
}),
filters: None,
}]),
}]),
},
status: None,
})
.await;
}

#[tokio::test(flavor = "current_thread")]
async fn accepts_backend_service_implicit_with_port() {
admission::accepts(|ns| HttpRoute {
metadata: meta(&ns),
spec: HttpRouteSpec {
inner: CommonRouteSpec {
parent_refs: Some(vec![server_parent_ref(ns)]),
},
hostnames: None,
rules: Some(vec![HttpRouteRule {
matches: Some(vec![HttpRouteMatch {
path: Some(HttpPathMatch::Exact {
value: "/foo".to_string(),
}),
..HttpRouteMatch::default()
}]),
filters: None,
backend_refs: Some(vec![k8s_gateway_api::HttpBackendRef {
backend_ref: Some(k8s_gateway_api::BackendRef {
weight: None,
inner: BackendObjectReference {
group: None,
kind: None,
namespace: Some("foo".to_string()),
name: "foo".to_string(),
port: Some(8080),
},
}),
filters: None,
}]),
}]),
},
status: None,
})
.await;
}

#[tokio::test(flavor = "current_thread")]
async fn rejects_relative_path_match() {
admission::rejects(|ns| HttpRoute {
Expand Down Expand Up @@ -197,6 +308,80 @@ async fn rejects_relative_redirect_path() {
.await;
}

#[tokio::test(flavor = "current_thread")]
async fn rejects_backend_service_without_port() {
admission::rejects(|ns| HttpRoute {
metadata: meta(&ns),
spec: HttpRouteSpec {
inner: CommonRouteSpec {
parent_refs: Some(vec![server_parent_ref(ns)]),
},
hostnames: None,
rules: Some(vec![HttpRouteRule {
matches: Some(vec![HttpRouteMatch {
path: Some(HttpPathMatch::Exact {
value: "/foo".to_string(),
}),
..HttpRouteMatch::default()
}]),
filters: None,
backend_refs: Some(vec![k8s_gateway_api::HttpBackendRef {
backend_ref: Some(k8s_gateway_api::BackendRef {
weight: None,
inner: BackendObjectReference {
group: Some("core".to_string()),
kind: Some("Service".to_string()),
namespace: Some("foo".to_string()),
name: "foo".to_string(),
port: None,
},
}),
filters: None,
}]),
}]),
},
status: None,
})
.await;
}

#[tokio::test(flavor = "current_thread")]
async fn rejects_backend_service_implicit_without_port() {
admission::rejects(|ns| HttpRoute {
metadata: meta(&ns),
spec: HttpRouteSpec {
inner: CommonRouteSpec {
parent_refs: Some(vec![server_parent_ref(ns)]),
},
hostnames: None,
rules: Some(vec![HttpRouteRule {
matches: Some(vec![HttpRouteMatch {
path: Some(HttpPathMatch::Exact {
value: "/foo".to_string(),
}),
..HttpRouteMatch::default()
}]),
filters: None,
backend_refs: Some(vec![k8s_gateway_api::HttpBackendRef {
backend_ref: Some(k8s_gateway_api::BackendRef {
weight: None,
inner: BackendObjectReference {
group: None,
kind: None,
namespace: Some("foo".to_string()),
name: "foo".to_string(),
port: None,
},
}),
filters: None,
}]),
}]),
},
status: None,
})
.await;
}

fn server_parent_ref(ns: impl ToString) -> ParentReference {
ParentReference {
group: Some("policy.linkerd.io".to_string()),
Expand Down
Loading