From ad49cd116fc944603d1cbf87d7c12c7ccf9979cd Mon Sep 17 00:00:00 2001 From: Alex Leong Date: Fri, 10 May 2024 09:22:26 -0700 Subject: [PATCH] Set backend_not_found route status when any backends are not found (#12565) The policy controller sets a status on HttpRoute resources depending on the state of the backends. If all the backends can be resolved, it should set a resolved_ref status and if any of the backends cannot be resolved, it should set a backend_not_found status. We update the logic to reflect this. Additionally, this resolves the edge case where an empty list of backends was resulting in a backend_not_found status instead of a resolved_refs status. Signed-off-by: Alex Leong --- policy-controller/k8s/status/src/index.rs | 2 +- .../k8s/status/src/tests/http_routes.rs | 345 +++++++++++++++++- 2 files changed, 333 insertions(+), 14 deletions(-) diff --git a/policy-controller/k8s/status/src/index.rs b/policy-controller/k8s/status/src/index.rs index 8b45d5c050553..8892c269403a7 100644 --- a/policy-controller/k8s/status/src/index.rs +++ b/policy-controller/k8s/status/src/index.rs @@ -421,7 +421,7 @@ impl Index { // If all references have been resolved (i.e exist in our services cache), // return positive status, otherwise, one of them does not exist - if backend_refs.iter().any(|backend_ref| match backend_ref { + if backend_refs.iter().all(|backend_ref| match backend_ref { BackendReference::Service(service) => self.services.contains_key(service), _ => false, }) { diff --git a/policy-controller/k8s/status/src/tests/http_routes.rs b/policy-controller/k8s/status/src/tests/http_routes.rs index 543b350a0d1c9..19fde10a18d19 100644 --- a/policy-controller/k8s/status/src/tests/http_routes.rs +++ b/policy-controller/k8s/status/src/tests/http_routes.rs @@ -3,7 +3,8 @@ use crate::{ resource_id::NamespaceGroupKindName, Index, IndexMetrics, }; -use k8s::Resource; +use gateway::{BackendObjectReference, BackendRef, HttpBackendRef, ParentReference}; +use k8s::{Resource, ResourceExt}; use kubert::index::IndexNamespacedResource; use linkerd_policy_controller_core::{http_route::GroupKindName, POLICY_CONTROLLER_NAME}; use linkerd_policy_controller_k8s_api::{self as k8s, gateway, policy::server::Port}; @@ -27,7 +28,15 @@ fn http_route_accepted_after_server_create() { ); // Apply the route. - let http_route = make_route("ns-0", "route-foo", "srv-8080"); + let parent = ParentReference { + group: Some(POLICY_API_GROUP.to_string()), + kind: Some("Server".to_string()), + namespace: None, + name: "srv-8080".to_string(), + section_name: None, + port: None, + }; + let http_route = make_route("ns-0", "route-foo", parent, None); index.write().apply(http_route); // Create the expected update. @@ -111,7 +120,15 @@ fn http_route_rejected_after_server_delete() { // There should be no update since there are no HTTPRoutes yet. assert!(updates_rx.try_recv().is_err()); - let http_route = make_route("ns-0", "route-foo", "srv-8080"); + let parent = ParentReference { + group: Some(POLICY_API_GROUP.to_string()), + kind: Some("Server".to_string()), + namespace: None, + name: "srv-8080".to_string(), + section_name: None, + port: None, + }; + let http_route = make_route("ns-0", "route-foo", parent, None); index.write().apply(http_route); // Create the expected update. @@ -164,6 +181,298 @@ fn http_route_rejected_after_server_delete() { assert!(updates_rx.try_recv().is_err()); } +#[test] +fn http_route_with_invalid_backend() { + let hostname = "test"; + let claim = kubert::lease::Claim { + holder: "test".to_string(), + expiry: chrono::DateTime::::MAX_UTC, + }; + let (_claims_tx, claims_rx) = watch::channel(Arc::new(claim)); + let (updates_tx, mut updates_rx) = mpsc::channel(10000); + let index = Index::shared( + hostname, + claims_rx, + updates_tx, + IndexMetrics::register(&mut Default::default()), + ); + + // Apply the parent service + let parent = make_service("ns-0", "svc"); + index.write().apply(parent.clone()); + + // Apply one backend service + let backend = make_service("ns-0", "backend-1"); + index.write().apply(backend.clone()); + + // Apply the route. + let parent = ParentReference { + group: Some("core".to_string()), + kind: Some("Service".to_string()), + namespace: parent.namespace(), + name: parent.name_unchecked(), + section_name: None, + port: Some(8080), + }; + let http_route = make_route( + "ns-0", + "route-foo", + parent.clone(), + Some(vec![ + HttpBackendRef { + backend_ref: Some(BackendRef { + weight: None, + inner: BackendObjectReference { + group: Some("core".to_string()), + kind: Some("Service".to_string()), + name: backend.name_unchecked(), + namespace: backend.namespace(), + port: Some(8080), + }, + }), + filters: None, + }, + HttpBackendRef { + backend_ref: Some(BackendRef { + weight: None, + inner: BackendObjectReference { + group: Some("core".to_string()), + kind: Some("Service".to_string()), + name: "nonexistant-backend".to_string(), + namespace: backend.namespace(), + port: Some(8080), + }, + }), + filters: None, + }, + ]), + ); + index.write().apply(http_route); + + // Create the expected update. + let id = NamespaceGroupKindName { + namespace: "ns-0".to_string(), + gkn: GroupKindName { + group: k8s::policy::HttpRoute::group(&()), + kind: k8s::policy::HttpRoute::kind(&()), + name: "route-foo".into(), + }, + }; + let accepted_condition = k8s::Condition { + last_transition_time: k8s::Time(chrono::DateTime::::MIN_UTC), + message: "".to_string(), + observed_generation: None, + reason: "Accepted".to_string(), + status: "True".to_string(), + type_: "Accepted".to_string(), + }; + // One of the backends does not exist so the status should be BackendNotFound. + let backend_condition = k8s::Condition { + last_transition_time: k8s::Time(chrono::DateTime::::MIN_UTC), + message: "".to_string(), + observed_generation: None, + reason: "BackendNotFound".to_string(), + status: "False".to_string(), + type_: "ResolvedRefs".to_string(), + }; + let parent_status = gateway::RouteParentStatus { + parent_ref: parent, + controller_name: POLICY_CONTROLLER_NAME.to_string(), + conditions: vec![accepted_condition, backend_condition], + }; + let status = make_status(vec![parent_status]); + let patch = index::make_patch("route-foo", status); + + let update = updates_rx.try_recv().unwrap(); + assert_eq!(id, update.id); + assert_eq!(patch, update.patch); + assert!(updates_rx.try_recv().is_err()) +} + +#[test] +fn http_route_with_no_backends() { + let hostname = "test"; + let claim = kubert::lease::Claim { + holder: "test".to_string(), + expiry: chrono::DateTime::::MAX_UTC, + }; + let (_claims_tx, claims_rx) = watch::channel(Arc::new(claim)); + let (updates_tx, mut updates_rx) = mpsc::channel(10000); + let index = Index::shared( + hostname, + claims_rx, + updates_tx, + IndexMetrics::register(&mut Default::default()), + ); + + // Apply the parent service + let parent = make_service("ns-0", "svc"); + index.write().apply(parent.clone()); + + // Apply the route. + let parent = ParentReference { + group: Some("core".to_string()), + kind: Some("Service".to_string()), + namespace: parent.namespace(), + name: parent.name_unchecked(), + section_name: None, + port: Some(8080), + }; + let http_route = make_route("ns-0", "route-foo", parent.clone(), None); + index.write().apply(http_route); + + // Create the expected update. + let id = NamespaceGroupKindName { + namespace: "ns-0".to_string(), + gkn: GroupKindName { + group: k8s::policy::HttpRoute::group(&()), + kind: k8s::policy::HttpRoute::kind(&()), + name: "route-foo".into(), + }, + }; + let accepted_condition = k8s::Condition { + last_transition_time: k8s::Time(chrono::DateTime::::MIN_UTC), + message: "".to_string(), + observed_generation: None, + reason: "Accepted".to_string(), + status: "True".to_string(), + type_: "Accepted".to_string(), + }; + // No backends were specified, so we have vacuously have resolved them all. + let backend_condition = k8s::Condition { + last_transition_time: k8s::Time(chrono::DateTime::::MIN_UTC), + message: "".to_string(), + observed_generation: None, + reason: "ResolvedRefs".to_string(), + status: "True".to_string(), + type_: "ResolvedRefs".to_string(), + }; + let parent_status = gateway::RouteParentStatus { + parent_ref: parent, + controller_name: POLICY_CONTROLLER_NAME.to_string(), + conditions: vec![accepted_condition, backend_condition], + }; + let status = make_status(vec![parent_status]); + let patch = index::make_patch("route-foo", status); + + let update = updates_rx.try_recv().unwrap(); + assert_eq!(id, update.id); + assert_eq!(patch, update.patch); + assert!(updates_rx.try_recv().is_err()) +} + +#[test] +fn http_route_with_valid_backends() { + let hostname = "test"; + let claim = kubert::lease::Claim { + holder: "test".to_string(), + expiry: chrono::DateTime::::MAX_UTC, + }; + let (_claims_tx, claims_rx) = watch::channel(Arc::new(claim)); + let (updates_tx, mut updates_rx) = mpsc::channel(10000); + let index = Index::shared( + hostname, + claims_rx, + updates_tx, + IndexMetrics::register(&mut Default::default()), + ); + + // Apply the parent service + let parent = make_service("ns-0", "svc"); + index.write().apply(parent.clone()); + + // Apply one backend service + let backend1 = make_service("ns-0", "backend-1"); + index.write().apply(backend1.clone()); + + // Apply one backend service + let backend2 = make_service("ns-0", "backend-2"); + index.write().apply(backend2.clone()); + + // Apply the route. + let parent = ParentReference { + group: Some("core".to_string()), + kind: Some("Service".to_string()), + namespace: parent.namespace(), + name: parent.name_unchecked(), + section_name: None, + port: Some(8080), + }; + let http_route = make_route( + "ns-0", + "route-foo", + parent.clone(), + Some(vec![ + HttpBackendRef { + backend_ref: Some(BackendRef { + weight: None, + inner: BackendObjectReference { + group: Some("core".to_string()), + kind: Some("Service".to_string()), + name: backend1.name_unchecked(), + namespace: backend1.namespace(), + port: Some(8080), + }, + }), + filters: None, + }, + HttpBackendRef { + backend_ref: Some(BackendRef { + weight: None, + inner: BackendObjectReference { + group: Some("core".to_string()), + kind: Some("Service".to_string()), + name: backend2.name_unchecked(), + namespace: backend2.namespace(), + port: Some(8080), + }, + }), + filters: None, + }, + ]), + ); + index.write().apply(http_route); + + // Create the expected update. + let id = NamespaceGroupKindName { + namespace: "ns-0".to_string(), + gkn: GroupKindName { + group: k8s::policy::HttpRoute::group(&()), + kind: k8s::policy::HttpRoute::kind(&()), + name: "route-foo".into(), + }, + }; + let accepted_condition = k8s::Condition { + last_transition_time: k8s::Time(chrono::DateTime::::MIN_UTC), + message: "".to_string(), + observed_generation: None, + reason: "Accepted".to_string(), + status: "True".to_string(), + type_: "Accepted".to_string(), + }; + // All backends exist and can be resolved. + let backend_condition = k8s::Condition { + last_transition_time: k8s::Time(chrono::DateTime::::MIN_UTC), + message: "".to_string(), + observed_generation: None, + reason: "ResolvedRefs".to_string(), + status: "True".to_string(), + type_: "ResolvedRefs".to_string(), + }; + let parent_status = gateway::RouteParentStatus { + parent_ref: parent, + controller_name: POLICY_CONTROLLER_NAME.to_string(), + conditions: vec![accepted_condition, backend_condition], + }; + let status = make_status(vec![parent_status]); + let patch = index::make_patch("route-foo", status); + + let update = updates_rx.try_recv().unwrap(); + assert_eq!(id, update.id); + assert_eq!(patch, update.patch); + assert!(updates_rx.try_recv().is_err()) +} + fn make_server( namespace: impl ToString, name: impl ToString, @@ -192,10 +501,27 @@ fn make_server( } } +fn make_service(namespace: impl ToString, name: impl ToString) -> k8s::api::core::v1::Service { + k8s::api::core::v1::Service { + metadata: k8s::ObjectMeta { + namespace: Some(namespace.to_string()), + name: Some(name.to_string()), + ..Default::default() + }, + spec: Some(k8s::ServiceSpec { + cluster_ip: Some("1.2.3.4".to_string()), + cluster_ips: Some(vec!["1.2.3.4".to_string()]), + ..Default::default() + }), + status: None, + } +} + fn make_route( namespace: impl ToString, name: impl ToString, - server: impl ToString, + parent: ParentReference, + backends: Option>, ) -> k8s::policy::HttpRoute { use chrono::Utc; use k8s::{policy::httproute::*, Time}; @@ -209,14 +535,7 @@ fn make_route( }, spec: HttpRouteSpec { inner: CommonRouteSpec { - parent_refs: Some(vec![ParentReference { - group: Some(POLICY_API_GROUP.to_string()), - kind: Some("Server".to_string()), - namespace: None, - name: server.to_string(), - section_name: None, - port: None, - }]), + parent_refs: Some(vec![parent]), }, hostnames: None, rules: Some(vec![HttpRouteRule { @@ -229,7 +548,7 @@ fn make_route( method: Some("GET".to_string()), }]), filters: None, - backend_refs: None, + backend_refs: backends, timeouts: None, }]), },