From 7a8034dbc12711baca34fe1a92073fb0dc98969b Mon Sep 17 00:00:00 2001 From: Oliver Gould Date: Thu, 14 Nov 2024 18:47:28 +0000 Subject: [PATCH 1/9] feat(policy): Check Service port in admission controller Our Gateway API bindings say: Port specifies the destination port number to use for this resource. Port is required when the referent is a Kubernetes Service. For other resources, destination port might be derived from the referent resource or this field. Therefore, when the port is omitted on a Service, we cause all requests to the backend to fail. This is a jarring user experience. To improve on this, we can check the port's validity in the admission controller. This change adds a validation for Gateway API HTTPRoute resources that checks that a valid port is specified when a backend is a Service. --- policy-controller/src/admission.rs | 39 +++- policy-test/tests/admit_http_route_gateway.rs | 185 ++++++++++++++++++ 2 files changed, 222 insertions(+), 2 deletions(-) diff --git a/policy-controller/src/admission.rs b/policy-controller/src/admission.rs index 10902aad82f86..039c3c43ce08b 100644 --- a/policy-controller/src/admission.rs +++ b/policy-controller/src/admission.rs @@ -9,7 +9,7 @@ use crate::k8s::policy::{ use anyhow::{anyhow, bail, ensure, Result}; use futures::future; use hyper::{body::Buf, http, Body, Request, Response}; -use k8s_openapi::api::core::v1::{Namespace, ServiceAccount}; +use k8s_openapi::api::core::v1::{self as corev1, Namespace, ServiceAccount}; use kube::{core::DynamicObject, Resource, ResourceExt}; use linkerd_policy_controller_core as core; use linkerd_policy_controller_k8s_api::gateway::{self as k8s_gateway_api, GrpcRoute}; @@ -692,12 +692,39 @@ impl Validate for Admission { } } + fn validate_backend(br: &k8s_gateway_api::BackendRef) -> Result<()> { + // If the backend points at something Linkerd doesn't know about it, + // let status resolution determine the handling of the route. + if let Some(ref brg) = br.inner.group { + if *brg != corev1::Service::group(&()) { + return Ok(()); + } + } + if let Some(ref brk) = br.inner.kind { + if *brk != corev1::Service::kind(&()) { + return Ok(()); + } + } + + // But if a service is specified, it must have a port. + let Some(port) = br.inner.port else { + bail!("cannot target a Service without specifying a port"); + }; + if port == 0 { + bail!("cannot target a Service with port 0"); + } + + Ok(()) + } + // Validate the rules in this spec. // This is essentially equivalent to the indexer's conversion function // 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() { @@ -707,6 +734,14 @@ impl Validate 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(br)?; + } } Ok(()) diff --git a/policy-test/tests/admit_http_route_gateway.rs b/policy-test/tests/admit_http_route_gateway.rs index dd49854dbc324..33c384d5f86d3 100644 --- a/policy-test/tests/admit_http_route_gateway.rs +++ b/policy-test/tests/admit_http_route_gateway.rs @@ -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 { @@ -197,6 +308,80 @@ async fn rejects_relative_redirect_path() { .await; } +#[tokio::test(flavor = "current_thread")] +async fn rejects_backend_service_without_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: None, + }, + }), + filters: None, + }]), + }]), + }, + status: None, + }) + .await; +} + +#[tokio::test(flavor = "current_thread")] +async fn rejects_backend_service_implicit_without_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: None, + }, + }), + filters: None, + }]), + }]), + }, + status: None, + }) + .await; +} + fn server_parent_ref(ns: impl ToString) -> ParentReference { ParentReference { group: Some("policy.linkerd.io".to_string()), From 750084dbb5b279d9c024c3293d3a90c1d943b302 Mon Sep 17 00:00:00 2001 From: Oliver Gould Date: Thu, 14 Nov 2024 19:13:57 +0000 Subject: [PATCH 2/9] fix test --- policy-test/tests/admit_http_route_gateway.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/policy-test/tests/admit_http_route_gateway.rs b/policy-test/tests/admit_http_route_gateway.rs index 33c384d5f86d3..881f5d44e0fd3 100644 --- a/policy-test/tests/admit_http_route_gateway.rs +++ b/policy-test/tests/admit_http_route_gateway.rs @@ -310,7 +310,7 @@ async fn rejects_relative_redirect_path() { #[tokio::test(flavor = "current_thread")] async fn rejects_backend_service_without_port() { - admission::accepts(|ns| HttpRoute { + admission::rejects(|ns| HttpRoute { metadata: meta(&ns), spec: HttpRouteSpec { inner: CommonRouteSpec { @@ -347,7 +347,7 @@ async fn rejects_backend_service_without_port() { #[tokio::test(flavor = "current_thread")] async fn rejects_backend_service_implicit_without_port() { - admission::accepts(|ns| HttpRoute { + admission::rejects(|ns| HttpRoute { metadata: meta(&ns), spec: HttpRouteSpec { inner: CommonRouteSpec { From 002015e3f7f17947081d622d20713845d6787023 Mon Sep 17 00:00:00 2001 From: Oliver Gould Date: Thu, 14 Nov 2024 19:16:13 +0000 Subject: [PATCH 3/9] improve error message --- policy-controller/src/admission.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/policy-controller/src/admission.rs b/policy-controller/src/admission.rs index 039c3c43ce08b..be8dcef272123 100644 --- a/policy-controller/src/admission.rs +++ b/policy-controller/src/admission.rs @@ -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::{self as corev1, Namespace, ServiceAccount}; @@ -740,7 +740,7 @@ impl Validate for Admission { .flatten() .filter_map(|br| br.backend_ref.as_ref()) { - validate_backend(br)?; + validate_backend(br).context("invalid backendRef")?; } } From a3c00f638dee770668867e1cdd471550f59c3d33 Mon Sep 17 00:00:00 2001 From: Oliver Gould Date: Thu, 14 Nov 2024 19:47:31 +0000 Subject: [PATCH 4/9] fix(policy): correct service matching --- policy-controller/src/admission.rs | 61 ++++++++++++++++-------------- 1 file changed, 33 insertions(+), 28 deletions(-) diff --git a/policy-controller/src/admission.rs b/policy-controller/src/admission.rs index be8dcef272123..b4ac3c94402ff 100644 --- a/policy-controller/src/admission.rs +++ b/policy-controller/src/admission.rs @@ -9,7 +9,7 @@ use crate::k8s::policy::{ use anyhow::{anyhow, bail, ensure, Context, Result}; use futures::future; use hyper::{body::Buf, http, Body, Request, Response}; -use k8s_openapi::api::core::v1::{self as corev1, Namespace, ServiceAccount}; +use k8s_openapi::api::core::v1::{Namespace, ServiceAccount}; use kube::{core::DynamicObject, Resource, ResourceExt}; use linkerd_policy_controller_core as core; use linkerd_policy_controller_k8s_api::gateway::{self as k8s_gateway_api, GrpcRoute}; @@ -650,6 +650,29 @@ impl Validate for Admission { } } +fn validate_backend_if_service(br: &k8s_gateway_api::BackendObjectReference) -> Result<()> { + fn is_service(br: &k8s_gateway_api::BackendObjectReference) -> bool { + matches!(br.group.as_deref(), Some("core") | Some("") | None) + && matches!(br.kind.as_deref(), Some("Service") | None) + } + + // If the backend references something Linkerd doesn't know about it, let + // status resolution determine the handling of the route. + if !is_service(br) { + return Ok(()); + } + + // But if a service is specified, it must have a port. + let Some(port) = br.port else { + bail!("cannot reference a Service without specifying a port"); + }; + if port == 0 { + bail!("cannot reference a Service with port 0"); + } + + Ok(()) +} + #[async_trait::async_trait] impl Validate for Admission { async fn validate( @@ -692,31 +715,6 @@ impl Validate for Admission { } } - fn validate_backend(br: &k8s_gateway_api::BackendRef) -> Result<()> { - // If the backend points at something Linkerd doesn't know about it, - // let status resolution determine the handling of the route. - if let Some(ref brg) = br.inner.group { - if *brg != corev1::Service::group(&()) { - return Ok(()); - } - } - if let Some(ref brk) = br.inner.kind { - if *brk != corev1::Service::kind(&()) { - return Ok(()); - } - } - - // But if a service is specified, it must have a port. - let Some(port) = br.inner.port else { - bail!("cannot target a Service without specifying a port"); - }; - if port == 0 { - bail!("cannot target a Service with port 0"); - } - - Ok(()) - } - // Validate the rules in this spec. // This is essentially equivalent to the indexer's conversion function // from `HttpRouteSpec` to `InboundRouteBinding`, except that we don't @@ -740,7 +738,8 @@ impl Validate for Admission { .flatten() .filter_map(|br| br.backend_ref.as_ref()) { - validate_backend(br).context("invalid backendRef")?; + tracing::info!("validating backendRef: {:?}", br); + validate_backend_if_service(&br.inner).context("invalid backendRef")?; } } @@ -819,7 +818,9 @@ impl Validate 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() { @@ -829,6 +830,10 @@ impl Validate 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(()) From fd3d8e5bb4bf3f92aab310f0f2a6e0795db506a9 Mon Sep 17 00:00:00 2001 From: Oliver Gould Date: Thu, 14 Nov 2024 19:50:47 +0000 Subject: [PATCH 5/9] remove info used for debugging --- policy-controller/src/admission.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/policy-controller/src/admission.rs b/policy-controller/src/admission.rs index b4ac3c94402ff..f5b415fe373a9 100644 --- a/policy-controller/src/admission.rs +++ b/policy-controller/src/admission.rs @@ -738,7 +738,6 @@ impl Validate for Admission { .flatten() .filter_map(|br| br.backend_ref.as_ref()) { - tracing::info!("validating backendRef: {:?}", br); validate_backend_if_service(&br.inner).context("invalid backendRef")?; } } From c73ecdd7a4a9f394821cd801ba622a9a8de3af29 Mon Sep 17 00:00:00 2001 From: Oliver Gould Date: Fri, 15 Nov 2024 16:21:20 +0000 Subject: [PATCH 6/9] simplify --- policy-controller/src/admission.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/policy-controller/src/admission.rs b/policy-controller/src/admission.rs index f5b415fe373a9..e15a84c3aa797 100644 --- a/policy-controller/src/admission.rs +++ b/policy-controller/src/admission.rs @@ -663,11 +663,8 @@ fn validate_backend_if_service(br: &k8s_gateway_api::BackendObjectReference) -> } // But if a service is specified, it must have a port. - let Some(port) = br.port else { - bail!("cannot reference a Service without specifying a port"); - }; - if port == 0 { - bail!("cannot reference a Service with port 0"); + if matches!(br.port, None | Some(0)) { + bail!("cannot reference a Service without a port"); } Ok(()) From 66dbb67414702058808ab00583f4f04ab0916bc6 Mon Sep 17 00:00:00 2001 From: Oliver Gould Date: Fri, 15 Nov 2024 16:23:18 +0000 Subject: [PATCH 7/9] simplify --- policy-controller/src/admission.rs | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/policy-controller/src/admission.rs b/policy-controller/src/admission.rs index e15a84c3aa797..6eebdb37ad7ce 100644 --- a/policy-controller/src/admission.rs +++ b/policy-controller/src/admission.rs @@ -656,14 +656,10 @@ fn validate_backend_if_service(br: &k8s_gateway_api::BackendObjectReference) -> && matches!(br.kind.as_deref(), Some("Service") | None) } - // If the backend references something Linkerd doesn't know about it, let - // status resolution determine the handling of the route. - if !is_service(br) { - return Ok(()); - } - - // But if a service is specified, it must have a port. - if matches!(br.port, None | Some(0)) { + // If the backend references 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(br) && matches!(br.port, None | Some(0)) { bail!("cannot reference a Service without a port"); } From 19cc22822b668b202c3307cb03c8ff03db60a63d Mon Sep 17 00:00:00 2001 From: Oliver Gould Date: Fri, 15 Nov 2024 08:31:21 -0800 Subject: [PATCH 8/9] fix typo --- policy-controller/src/admission.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/policy-controller/src/admission.rs b/policy-controller/src/admission.rs index 6eebdb37ad7ce..f4f82ba1e40c0 100644 --- a/policy-controller/src/admission.rs +++ b/policy-controller/src/admission.rs @@ -656,7 +656,7 @@ fn validate_backend_if_service(br: &k8s_gateway_api::BackendObjectReference) -> && matches!(br.kind.as_deref(), Some("Service") | None) } - // If the backend references is a Service, it must have a port. If it is not + // 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(br) && matches!(br.port, None | Some(0)) { From 6f927e6c669b23a8bed104ba9c642bb3b2c7312c Mon Sep 17 00:00:00 2001 From: Oliver Gould Date: Fri, 15 Nov 2024 16:32:43 +0000 Subject: [PATCH 9/9] simplify --- policy-controller/src/admission.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/policy-controller/src/admission.rs b/policy-controller/src/admission.rs index f4f82ba1e40c0..903bd40b69e8c 100644 --- a/policy-controller/src/admission.rs +++ b/policy-controller/src/admission.rs @@ -651,15 +651,13 @@ impl Validate for Admission { } fn validate_backend_if_service(br: &k8s_gateway_api::BackendObjectReference) -> Result<()> { - fn is_service(br: &k8s_gateway_api::BackendObjectReference) -> bool { - matches!(br.group.as_deref(), Some("core") | Some("") | None) - && matches!(br.kind.as_deref(), Some("Service") | None) - } + 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(br) && matches!(br.port, None | Some(0)) { + if is_service && matches!(br.port, None | Some(0)) { bail!("cannot reference a Service without a port"); }