From 6f303ffbc5c4ad54fa513dd88b9ecd520cc11be4 Mon Sep 17 00:00:00 2001 From: Maxi Wittich Date: Mon, 10 Aug 2026 11:59:38 +0200 Subject: [PATCH 01/22] feat: Add workloadKind, internalTrafficPolicy and PDB config to servers.roleConfig Replaces the EmptyRoleConfig parameter of OpaRoleType with a product-specific OpaRoleConfig, as the extension point for deploying the servers role as either a DaemonSet or a Deployment (#525). The CRD half only - no builder, PDB or orphan-cleanup changes. Both internalTrafficPolicy and podDisruptionBudget.enabled are Options with a null default, because their effective default depends on workloadKind and an OpenAPI default cannot express that. The operator derives them instead: workloadKind | internalTrafficPolicy | podDisruptionBudget.enabled DaemonSet | Local | false Deployment | Cluster | true A hard-coded schema default would let the apiserver stamp a value in before the operator sees the object, so "user chose Local" and "user said nothing" would be indistinguishable. It would also create a PodDisruptionBudget on every existing DaemonSet installation at upgrade time, protecting nothing. Decision: https://github.com/stackabletech/decisions/issues/91 Co-Authored-By: Claude Opus 5 (1M context) --- extra/crds.yaml | 138 ++++++++++++++++- rust/operator-binary/src/crd/mod.rs | 203 ++++++++++++++++++++++++- rust/resource-info-fetcher/src/api.rs | 2 +- rust/resource-info-fetcher/src/main.rs | 2 +- 4 files changed, 333 insertions(+), 12 deletions(-) diff --git a/extra/crds.yaml b/extra/crds.yaml index 17d51505..2cd70f5f 100644 --- a/extra/crds.yaml +++ b/extra/crds.yaml @@ -1490,10 +1490,71 @@ spec: type: object x-kubernetes-preserve-unknown-fields: true roleConfig: - default: {} - description: |- - This is a product-agnostic RoleConfig, with nothing in it. It is used e.g. by products that have - nothing configurable at role level. + default: + podDisruptionBudget: + enabled: null + maxUnavailable: null + workloadKind: DaemonSet + description: Role-level configuration for the OPA servers. + properties: + internalTrafficPolicy: + description: |- + The `internalTrafficPolicy` of the role Service. + + * `Local`: Only route to OPA Pods on the same node as the client. This avoids + cross-node latency, but requests will fail if there is no OPA Pod on the node. + + * `Cluster`: Route to any OPA Pod of the role. + + Defaults to `Local` when `workloadKind` is a `DaemonSet` and to `Cluster` when it is a + `Deployment`. + enum: + - Local + - Cluster + - null + nullable: true + type: string + podDisruptionBudget: + default: + enabled: null + maxUnavailable: null + description: |- + This struct is used to configure: + + 1. If PodDisruptionBudgets are created by the operator + 2. The allowed number of Pods to be unavailable (`maxUnavailable`) + + Documentation: + [allowed Pod disruptions documentation](https://docs.stackable.tech/home/nightly/concepts/operations/pod_disruptions). + properties: + enabled: + description: |- + Whether a PodDisruptionBudget should be written out for this role. + + Defaults to `true` when `workloadKind` is `Deployment` and to `false` when it is + `DaemonSet`, since a PodDisruptionBudget doesn't make sense for a DaemonSet. + nullable: true + type: boolean + maxUnavailable: + description: The number of Pods that are allowed to be down simultaneous. + format: uint16 + maximum: 65535.0 + minimum: 0.0 + nullable: true + type: integer + type: object + workloadKind: + default: DaemonSet + description: |- + The Kubernetes workload the OPA servers run as. + + * `DaemonSet`: one Pod per node. `replicas` is ignored. + + * `Deployment`: fixed number of Pods, configured by `replicas`. + enum: + - DaemonSet + - Deployment + type: string type: object roleGroups: additionalProperties: @@ -3747,10 +3808,71 @@ spec: type: object x-kubernetes-preserve-unknown-fields: true roleConfig: - default: {} - description: |- - This is a product-agnostic RoleConfig, with nothing in it. It is used e.g. by products that have - nothing configurable at role level. + default: + podDisruptionBudget: + enabled: null + maxUnavailable: null + workloadKind: DaemonSet + description: Role-level configuration for the OPA servers. + properties: + internalTrafficPolicy: + description: |- + The `internalTrafficPolicy` of the role Service. + + * `Local`: Only route to OPA Pods on the same node as the client. This avoids + cross-node latency, but requests will fail if there is no OPA Pod on the node. + + * `Cluster`: Route to any OPA Pod of the role. + + Defaults to `Local` when `workloadKind` is a `DaemonSet` and to `Cluster` when it is a + `Deployment`. + enum: + - Local + - Cluster + - null + nullable: true + type: string + podDisruptionBudget: + default: + enabled: null + maxUnavailable: null + description: |- + This struct is used to configure: + + 1. If PodDisruptionBudgets are created by the operator + 2. The allowed number of Pods to be unavailable (`maxUnavailable`) + + Documentation: + [allowed Pod disruptions documentation](https://docs.stackable.tech/home/nightly/concepts/operations/pod_disruptions). + properties: + enabled: + description: |- + Whether a PodDisruptionBudget should be written out for this role. + + Defaults to `true` when `workloadKind` is `Deployment` and to `false` when it is + `DaemonSet`, since a PodDisruptionBudget doesn't make sense for a DaemonSet. + nullable: true + type: boolean + maxUnavailable: + description: The number of Pods that are allowed to be down simultaneous. + format: uint16 + maximum: 65535.0 + minimum: 0.0 + nullable: true + type: integer + type: object + workloadKind: + default: DaemonSet + description: |- + The Kubernetes workload the OPA servers run as. + + * `DaemonSet`: one Pod per node. `replicas` is ignored. + + * `Deployment`: fixed number of Pods, configured by `replicas`. + enum: + - DaemonSet + - Deployment + type: string type: object roleGroups: additionalProperties: diff --git a/rust/operator-binary/src/crd/mod.rs b/rust/operator-binary/src/crd/mod.rs index ed3f32f2..2c189d43 100644 --- a/rust/operator-binary/src/crd/mod.rs +++ b/rust/operator-binary/src/crd/mod.rs @@ -16,7 +16,7 @@ use stackable_operator::{ k8s_openapi::apimachinery::pkg::api::resource::Quantity, kube::CustomResource, product_logging::{self, spec::Logging}, - role_utils::{EmptyRoleConfig, Role}, + role_utils::Role, schemars::{self, JsonSchema}, shared::time::Duration, status::condition::{ClusterCondition, HasStatusCondition}, @@ -45,7 +45,7 @@ pub const DEFAULT_SERVER_GRACEFUL_SHUTDOWN_TIMEOUT: Duration = Duration::from_mi pub const SERVER_GRACEFUL_SHUTDOWN_SAFETY_OVERHEAD: Duration = Duration::from_secs(5); pub type OpaRoleType = - Role; + Role; #[versioned( version(name = "v1alpha1"), @@ -142,6 +142,85 @@ pub mod versioned { pub tls: Option, } + /// Role-level configuration for the OPA servers. + #[derive(Clone, Debug, Default, Deserialize, Eq, JsonSchema, PartialEq, Serialize)] + #[serde(rename_all = "camelCase")] + pub struct OpaRoleConfig { + /// The Kubernetes workload the OPA servers run as. + /// + /// * `DaemonSet`: one Pod per node. `replicas` is ignored. + /// + /// * `Deployment`: fixed number of Pods, configured by `replicas`. + #[serde(default)] + pub workload_kind: WorkloadKind, + + /// The `internalTrafficPolicy` of the role Service. + /// + /// * `Local`: Only route to OPA Pods on the same node as the client. This avoids + /// cross-node latency, but requests will fail if there is no OPA Pod on the node. + /// + /// * `Cluster`: Route to any OPA Pod of the role. + /// + /// Defaults to `Local` when `workloadKind` is a `DaemonSet` and to `Cluster` when it is a + /// `Deployment`. + // `skip_serializing_if` keeps the `null` out of the schema `default` that is generated for + // the enclosing `roleConfig`. The apiserver validates that default against this property's + // schema, and rejects a `null` against an `enum`. + #[serde(default, skip_serializing_if = "Option::is_none")] + #[versioned(hint(option))] + pub internal_traffic_policy: Option, + + // We can not #[serde(flatten)] a `GenericRoleConfig` here, as we need a PodDisruptionBudget + // default that depends on `workloadKind`. + #[serde(default)] + pub pod_disruption_budget: OpaPdbConfig, + } + + /// The Kubernetes Kind currently supported. + #[derive(Clone, Debug, Default, Deserialize, Eq, JsonSchema, PartialEq, Serialize)] + #[serde(rename_all = "PascalCase")] + pub enum WorkloadKind { + #[default] + DaemonSet, + Deployment, + } + + /// The `internalTrafficPolicy` of a Kubernetes Service. + /// + /// The variants are spelled as Kubernetes spells them, so the value can be passed through to + /// `Service.spec.internalTrafficPolicy` unchanged. + #[derive(Clone, Debug, Deserialize, Display, Eq, JsonSchema, PartialEq, Serialize)] + #[serde(rename_all = "PascalCase")] + pub enum InternalTrafficPolicy { + Local, + Cluster, + } + + // A copy of `PdbConfig` from stackable-operator, but with `enabled` as an `Option`. The + // default depends on `workloadKind` and can therefore not be hard-coded. + // + /// This struct is used to configure: + /// + /// 1. If PodDisruptionBudgets are created by the operator + /// 2. The allowed number of Pods to be unavailable (`maxUnavailable`) + /// + /// Documentation: + /// [allowed Pod disruptions documentation](DOCS_BASE_URL_PLACEHOLDER/concepts/operations/pod_disruptions). + #[derive(Clone, Debug, Default, Deserialize, Eq, JsonSchema, PartialEq, Serialize)] + #[serde(rename_all = "camelCase")] + pub struct OpaPdbConfig { + /// Whether a PodDisruptionBudget should be written out for this role. + /// + /// Defaults to `true` when `workloadKind` is `Deployment` and to `false` when it is + /// `DaemonSet`, since a PodDisruptionBudget doesn't make sense for a DaemonSet. + #[serde(default)] + pub enabled: Option, + + /// The number of Pods that are allowed to be down simultaneous. + #[serde(default)] + pub max_unavailable: Option, + } + #[derive(Clone, Debug, Deserialize, Eq, JsonSchema, PartialEq, Serialize)] #[serde(rename_all = "camelCase")] pub struct OpaTls { @@ -302,6 +381,34 @@ impl v1alpha2::CurrentlySupportedListenerClasses { } } +// TODO: Remove the `allow` once the Deployment and PodDisruptionBudget builders call these. +// This change is the CRD half of https://github.com/stackabletech/opa-operator/issues/525. +#[allow(dead_code)] +impl v1alpha2::OpaRoleConfig { + /// The `internalTrafficPolicy` to write into the role Service. + /// + /// Falls back to the [`v1alpha2::WorkloadKind`] default when unset: `Local` for a DaemonSet, + /// which covers every node, and `Cluster` for a Deployment, whose Pods do not. + pub fn internal_traffic_policy(&self) -> v1alpha2::InternalTrafficPolicy { + self.internal_traffic_policy + .clone() + .unwrap_or(match self.workload_kind { + v1alpha2::WorkloadKind::DaemonSet => v1alpha2::InternalTrafficPolicy::Local, + v1alpha2::WorkloadKind::Deployment => v1alpha2::InternalTrafficPolicy::Cluster, + }) + } + + /// Whether a PodDisruptionBudget should be written out for this role. + /// + /// Falls back to `true` for a Deployment only: `kubectl drain` requires `--ignore-daemonsets` + /// and then leaves those Pods alone, so a PDB would protect nothing in DaemonSet mode. + pub fn pod_disruption_budget_enabled(&self) -> bool { + self.pod_disruption_budget + .enabled + .unwrap_or(self.workload_kind == v1alpha2::WorkloadKind::Deployment) + } +} + impl OpaConfig { pub fn default_config() -> OpaConfigFragment { OpaConfigFragment { @@ -346,10 +453,102 @@ impl HasStatusCondition for v1alpha2::OpaCluster { #[cfg(test)] mod tests { use indoc::formatdoc; + use serde_json::json; use stackable_operator::versioned::test_utils::RoundtripTestData; use super::{v1alpha1, v1alpha2}; + /// The defaults the operator derives from `workloadKind`, which an OpenAPI schema default + /// cannot express. Locks the table in the CRD docs of the two fields. + #[test] + fn role_config_defaults_follow_workload_kind() { + let role_config = |workload_kind| v1alpha2::OpaRoleConfig { + workload_kind, + ..v1alpha2::OpaRoleConfig::default() + }; + + let daemon_set = role_config(v1alpha2::WorkloadKind::DaemonSet); + assert_eq!( + daemon_set.internal_traffic_policy(), + v1alpha2::InternalTrafficPolicy::Local + ); + // `kubectl drain` skips DaemonSet Pods, so a PDB would protect nothing. + assert!(!daemon_set.pod_disruption_budget_enabled()); + + let deployment = role_config(v1alpha2::WorkloadKind::Deployment); + assert_eq!( + deployment.internal_traffic_policy(), + v1alpha2::InternalTrafficPolicy::Cluster + ); + assert!(deployment.pod_disruption_budget_enabled()); + } + + /// An explicitly configured value wins over the `workloadKind`-derived default, which is the + /// point of exposing the two fields at all (DaemonSet with `Cluster` is used in the field). + #[test] + fn explicit_role_config_overrides_the_derived_defaults() { + let role_config = v1alpha2::OpaRoleConfig { + workload_kind: v1alpha2::WorkloadKind::DaemonSet, + internal_traffic_policy: Some(v1alpha2::InternalTrafficPolicy::Cluster), + pod_disruption_budget: v1alpha2::OpaPdbConfig { + enabled: Some(true), + max_unavailable: None, + }, + }; + + assert_eq!( + role_config.internal_traffic_policy(), + v1alpha2::InternalTrafficPolicy::Cluster + ); + assert!(role_config.pod_disruption_budget_enabled()); + } + + /// Leaving the two fields out and writing them as an explicit `null` must resolve to the same + /// unset state, as the derived defaults are applied by the operator rather than by the schema. + /// + /// Only covers what serde does; substituting the `roleConfig` default for an entirely absent + /// `roleConfig` is the apiserver's job and is not exercised here. + #[test] + fn unset_role_config_fields_deserialise_to_none() { + let unset = v1alpha2::OpaRoleConfig::default(); + + for value in [ + json!({}), + json!({ "workloadKind": "DaemonSet" }), + json!({ + "workloadKind": "DaemonSet", + "internalTrafficPolicy": null, + "podDisruptionBudget": { "enabled": null, "maxUnavailable": null }, + }), + ] { + let role_config: v1alpha2::OpaRoleConfig = + serde_json::from_value(value.clone()).expect("a valid role config"); + assert_eq!(role_config, unset, "unexpected role config for {value}"); + } + } + + /// The two enums must serialise the way Kubernetes spells them: `workloadKind` names the + /// workload API kinds, and `internalTrafficPolicy` is passed through to `Service.spec`. + #[test] + fn enums_use_the_kubernetes_spelling() { + assert_eq!( + serde_json::to_value(v1alpha2::WorkloadKind::DaemonSet).unwrap(), + json!("DaemonSet") + ); + assert_eq!( + serde_json::to_value(v1alpha2::WorkloadKind::Deployment).unwrap(), + json!("Deployment") + ); + assert_eq!( + serde_json::to_value(v1alpha2::InternalTrafficPolicy::Local).unwrap(), + json!("Local") + ); + assert_eq!( + serde_json::to_value(v1alpha2::InternalTrafficPolicy::Cluster).unwrap(), + json!("Cluster") + ); + } + impl RoundtripTestData for v1alpha1::OpaClusterSpec { fn roundtrip_test_data() -> Vec { let user_info_fetcher_sections = vec![ diff --git a/rust/resource-info-fetcher/src/api.rs b/rust/resource-info-fetcher/src/api.rs index b270c2ed..159fd68b 100644 --- a/rust/resource-info-fetcher/src/api.rs +++ b/rust/resource-info-fetcher/src/api.rs @@ -91,7 +91,7 @@ pub struct RawIdentifier { } /// Generates the trivial `From for ResourceInfoRequest` conversions, so each HTTP handler -/// can turn its deserialized query parameters into a [`ResourceInfoRequest`] via `.into()`. Adding a +/// can turn its deserialized query parameters into a [`ResourceInfoRequest`] via `.from()`. Adding a /// resource type means adding its struct above and one entry here — no hand-written conversion. macro_rules! impl_into_resource_info_request { ($($variant:ident),+ $(,)?) => { diff --git a/rust/resource-info-fetcher/src/main.rs b/rust/resource-info-fetcher/src/main.rs index 6bbf79d8..0218918e 100644 --- a/rust/resource-info-fetcher/src/main.rs +++ b/rust/resource-info-fetcher/src/main.rs @@ -49,7 +49,7 @@ pub struct Args { #[derive(Clone)] struct AppState { backend: Arc, - // Note: Although we might not talk JSON to the underlying backend, we always return JSON as a + // Note: Although we might not talk JSON to the underlying backend, we always return JSON as a // result to the caller, so we can cache that. resource_info_cache: Cache, } From 5fbe662e238506be20e5a9357eae91ea02db0949 Mon Sep 17 00:00:00 2001 From: Maxi Wittich Date: Fri, 31 Jul 2026 15:42:10 +0200 Subject: [PATCH 02/22] refactor before adding deployment.rs as parallel mode --- rust/operator-binary/src/controller/build.rs | 4 +- .../src/controller/build/resource/mod.rs | 2 +- .../build/resource/workload/daemonset.rs | 475 ++++++++++++++++++ .../resource/{daemonset => workload}/mod.rs | 473 +---------------- .../resource_info_fetcher.rs | 2 +- .../user_info_fetcher.rs | 2 +- 6 files changed, 498 insertions(+), 460 deletions(-) create mode 100644 rust/operator-binary/src/controller/build/resource/workload/daemonset.rs rename rust/operator-binary/src/controller/build/resource/{daemonset => workload}/mod.rs (60%) rename rust/operator-binary/src/controller/build/resource/{daemonset => workload}/resource_info_fetcher.rs (99%) rename rust/operator-binary/src/controller/build/resource/{daemonset => workload}/user_info_fetcher.rs (99%) diff --git a/rust/operator-binary/src/controller/build.rs b/rust/operator-binary/src/controller/build.rs index d97844f8..4d166868 100644 --- a/rust/operator-binary/src/controller/build.rs +++ b/rust/operator-binary/src/controller/build.rs @@ -14,13 +14,13 @@ use crate::controller::{ KubernetesResources, RoleGroupName, ValidatedCluster, build::resource::{ config_map::build_rolegroup_config_map, - daemonset::build_server_rolegroup_daemonset, discovery::build_discovery_config_map, rbac::{build_role_binding, build_service_account}, service::{ build_rolegroup_headless_service, build_rolegroup_metrics_service, build_server_role_service, }, + workload::daemonset::build_server_rolegroup_daemonset, }, }; @@ -37,7 +37,7 @@ pub enum Error { #[snafu(display("failed to build DaemonSet for role group {role_group}"))] DaemonSet { - source: resource::daemonset::Error, + source: resource::workload::Error, role_group: RoleGroupName, }, diff --git a/rust/operator-binary/src/controller/build/resource/mod.rs b/rust/operator-binary/src/controller/build/resource/mod.rs index a921b543..51581d51 100644 --- a/rust/operator-binary/src/controller/build/resource/mod.rs +++ b/rust/operator-binary/src/controller/build/resource/mod.rs @@ -2,7 +2,7 @@ //! Kubernetes resources, one module per resource kind. pub mod config_map; -pub mod daemonset; pub mod discovery; pub mod rbac; pub mod service; +pub mod workload; diff --git a/rust/operator-binary/src/controller/build/resource/workload/daemonset.rs b/rust/operator-binary/src/controller/build/resource/workload/daemonset.rs new file mode 100644 index 00000000..8f6de324 --- /dev/null +++ b/rust/operator-binary/src/controller/build/resource/workload/daemonset.rs @@ -0,0 +1,475 @@ +//! Builds the rolegroup [`DaemonSet`] that runs OPA on every node. + +use stackable_operator::k8s_openapi::{ + api::apps::v1::{DaemonSet, DaemonSetSpec, DaemonSetUpdateStrategy, RollingUpdateDaemonSet}, + apimachinery::pkg::apis::meta::v1::LabelSelector, +}; + +use super::*; + +/// The rolegroup [`DaemonSet`] runs the rolegroup, as configured by the administrator. +/// +/// The [`Pod`](`stackable_operator::k8s_openapi::api::core::v1::Pod`)s are accessible through the +/// corresponding [`Service`](`stackable_operator::k8s_openapi::api::core::v1::Service`) (from +/// [`build_server_role_service`](super::super::service::build_server_role_service)). +/// +/// We run an OPA on each node, because we want to avoid requiring network roundtrips for services making +/// policy queries (which are often chained in serial, and block other tasks in the products). +#[allow(clippy::too_many_arguments)] +pub fn build_server_rolegroup_daemonset( + cluster: &ValidatedCluster, + role_group_name: &RoleGroupName, + role_group: &OpaRoleGroupConfig, + opa_bundle_builder_image: &str, + user_info_fetcher_image: &str, + resource_info_fetcher_image: &str, + cluster_info: &KubernetesClusterInfo, +) -> Result { + let pod_template = build_server_rolegroup_pod_template( + cluster, + role_group_name, + role_group, + opa_bundle_builder_image, + user_info_fetcher_image, + resource_info_fetcher_image, + cluster_info, + )?; + + let metadata = build::object_meta( + cluster, + cluster + .role_group_resource_names(role_group_name) + .daemon_set_name() + .to_string(), + role_group_name, + ) + .build(); + + let daemonset_spec = DaemonSetSpec { + selector: LabelSelector { + match_labels: Some(cluster.role_group_selector(role_group_name).into()), + ..LabelSelector::default() + }, + template: pod_template, + update_strategy: Some(DaemonSetUpdateStrategy { + type_: Some("RollingUpdate".to_string()), + rolling_update: Some(RollingUpdateDaemonSet { + max_surge: Some(IntOrString::Int(1)), + max_unavailable: Some(IntOrString::Int(0)), + }), + }), + ..DaemonSetSpec::default() + }; + + Ok(DaemonSet { + metadata, + spec: Some(daemonset_spec), + status: None, + }) +} + +#[cfg(test)] +mod tests { + use serde_json::json; + use stackable_operator::{ + commons::networking::DomainName, k8s_openapi::api::core::v1::Container, + }; + + use stackable_opa_operator::crd::OpaRole; + + use super::*; + use crate::controller::build::properties::test_support::validated_cluster_from_spec; + + fn cluster_info() -> KubernetesClusterInfo { + KubernetesClusterInfo { + cluster_domain: DomainName::try_from("cluster.local").unwrap(), + } + } + + fn build(cluster: &ValidatedCluster) -> DaemonSet { + let (role_group_name, role_group) = cluster.role_group_configs[&OpaRole::Server] + .iter() + .next() + .expect("the default role group should exist"); + build_server_rolegroup_daemonset( + cluster, + role_group_name, + role_group, + "bundle-builder-image", + "user-info-fetcher-image", + "resource-info-fetcher-image", + &cluster_info(), + ) + .expect("the daemonset should build") + } + + fn container_names(ds: &DaemonSet) -> Vec { + ds.spec + .as_ref() + .unwrap() + .template + .spec + .as_ref() + .unwrap() + .containers + .iter() + .map(|c| c.name.clone()) + .collect() + } + + fn volume_names(ds: &DaemonSet) -> Vec { + ds.spec + .as_ref() + .unwrap() + .template + .spec + .as_ref() + .unwrap() + .volumes + .as_ref() + .unwrap() + .iter() + .map(|v| v.name.clone()) + .collect() + } + + #[test] + fn daemonset_has_expected_name_and_rolling_update_strategy() { + let ds = build(&validated_cluster_from_spec(json!({ + "image": { "productVersion": "1.2.3" }, + "servers": { "roleGroups": { "default": {} } }, + }))); + + assert_eq!(ds.metadata.name.as_deref(), Some("test-opa-server-default")); + let strategy = ds.spec.as_ref().unwrap().update_strategy.as_ref().unwrap(); + assert_eq!(strategy.type_.as_deref(), Some("RollingUpdate")); + let rolling_update = strategy.rolling_update.as_ref().unwrap(); + // A DaemonSet must never take an OPA pod down before the replacement is ready. + assert_eq!(rolling_update.max_unavailable, Some(IntOrString::Int(0))); + } + + #[test] + fn daemonset_runs_opa_and_bundle_builder_with_prepare_init_container() { + let ds = build(&validated_cluster_from_spec(json!({ + "image": { "productVersion": "1.2.3" }, + "servers": { "roleGroups": { "default": {} } }, + }))); + + let containers = container_names(&ds); + assert!(containers.contains(&"opa".to_owned())); + assert!(containers.contains(&"bundle-builder".to_owned())); + // No sidecars without the corresponding cluster config. + assert!(!containers.contains(&"user-info-fetcher".to_owned())); + assert!(!containers.contains(&"vector".to_owned())); + + let pod_spec = ds.spec.as_ref().unwrap().template.spec.as_ref().unwrap(); + let init_containers: Vec<_> = pod_spec + .init_containers + .as_ref() + .unwrap() + .iter() + .map(|c| c.name.clone()) + .collect(); + assert_eq!(init_containers, vec!["prepare".to_owned()]); + + // The standard volumes are always present; the TLS volume is not (no TLS configured). + let volumes = volume_names(&ds); + for expected in ["config", "bundles", "log"] { + assert!( + volumes.contains(&expected.to_owned()), + "missing volume {expected}" + ); + } + assert!(!volumes.contains(&"tls".to_owned())); + } + + #[test] + fn daemonset_adds_vector_container_when_agent_enabled() { + let ds = build(&validated_cluster_from_spec(json!({ + "image": { "productVersion": "1.2.3" }, + "clusterConfig": { "vectorAggregatorConfigMapName": "vector-aggregator-discovery" }, + "servers": { + "config": { "logging": { "enableVectorAgent": true } }, + "roleGroups": { "default": {} }, + }, + }))); + + assert!(container_names(&ds).contains(&"vector".to_owned())); + } + + #[test] + fn daemonset_adds_user_info_fetcher_container_when_configured() { + let ds = build(&validated_cluster_from_spec(json!({ + "image": { "productVersion": "1.2.3" }, + "clusterConfig": { + "userInfo": { + "backend": { + "experimentalXfscAas": { + "hostname": "aas.default.svc.cluster.local", + "port": 5000, + } + } + } + }, + "servers": { "roleGroups": { "default": {} } }, + }))); + + assert!(container_names(&ds).contains(&"user-info-fetcher".to_owned())); + } + + #[test] + fn opa_probes_root_and_bundle_builder_probes_status() { + let ds = build(&validated_cluster_from_spec(json!({ + "image": { "productVersion": "1.2.3" }, + "servers": { "roleGroups": { "default": {} } }, + }))); + let pod_spec = ds.spec.as_ref().unwrap().template.spec.as_ref().unwrap(); + let liveness_path = |container: &str| -> String { + pod_spec + .containers + .iter() + .find(|c| c.name == container) + .unwrap_or_else(|| panic!("container {container} should exist")) + .liveness_probe + .as_ref() + .unwrap() + .http_get + .as_ref() + .unwrap() + .path + .clone() + .unwrap() + }; + // OPA's HTTP server answers `/`; only the bundle-builder exposes `/status`. A wrong path + // here makes the liveness probe fail and the OPA container CrashLoop. + assert_eq!(liveness_path("opa"), "/"); + assert_eq!(liveness_path("bundle-builder"), "/status"); + } + + #[test] + fn daemonset_adds_tls_volume_when_tls_enabled() { + let ds = build(&validated_cluster_from_spec(json!({ + "image": { "productVersion": "1.2.3" }, + "clusterConfig": { "tls": { "serverSecretClass": "tls" } }, + "servers": { "roleGroups": { "default": {} } }, + }))); + + assert!(volume_names(&ds).contains(&"tls".to_owned())); + } + + #[test] + fn opa_container_serves_https_when_tls_enabled() { + let ds = build(&validated_cluster_from_spec(json!({ + "image": { "productVersion": "1.2.3" }, + "clusterConfig": { "tls": { "serverSecretClass": "tls" } }, + "servers": { "roleGroups": { "default": {} } }, + }))); + let pod_spec = ds.spec.as_ref().unwrap().template.spec.as_ref().unwrap(); + let opa = pod_spec + .containers + .iter() + .find(|c| c.name == "opa") + .expect("opa container should exist"); + + // The single container port is the HTTPS data port. + let ports = opa.ports.as_ref().unwrap(); + assert_eq!(ports.len(), 1); + assert_eq!(ports[0].name.as_deref(), Some("https")); + assert_eq!(ports[0].container_port, 8443); + + // The probe must speak HTTPS, otherwise it would fail against the TLS-only server. + let scheme = opa + .liveness_probe + .as_ref() + .unwrap() + .http_get + .as_ref() + .unwrap() + .scheme + .clone(); + assert_eq!(scheme.as_deref(), Some("HTTPS")); + + // The start command binds the HTTPS port and passes the TLS cert/key flags. + let args = opa.args.as_ref().unwrap(); + assert!(args[0].contains("-a 0.0.0.0:8443")); + assert!(args[0].contains("--tls-cert-file")); + } + + #[test] + fn bundle_builder_start_command_silences_console_only_when_none() { + let role_group_config = |spec: serde_json::Value| { + let cluster = validated_cluster_from_spec(spec); + cluster.role_group_configs[&OpaRole::Server] + .values() + .next() + .expect("the default role group should exist") + .config + .clone() + }; + + // Console level NONE redirects bundle-builder output to /dev/null (no `tee`). + let silenced = role_group_config(json!({ + "image": { "productVersion": "1.2.3" }, + "servers": { + "config": { "logging": { "containers": { + "bundle-builder": { "console": { "level": "NONE" } } + } } }, + "roleGroups": { "default": {} }, + }, + })); + // The redirect is appended directly after the bundle-builder invocation. (`/dev/null` also + // appears in the shared bash trap helpers, so match the specific redirect.) + assert!( + build_bundle_builder_start_command(&silenced, "bundle-builder") + .contains("stackable-opa-bundle-builder > /dev/null") + ); + + // With a console level above NONE, output is not discarded. + let logging = role_group_config(json!({ + "image": { "productVersion": "1.2.3" }, + "servers": { + "config": { "logging": { "containers": { + "bundle-builder": { "console": { "level": "INFO" } } + } } }, + "roleGroups": { "default": {} }, + }, + })); + assert!( + build_bundle_builder_start_command(&logging, "bundle-builder") + .contains("stackable-opa-bundle-builder &") + ); + } + + fn uif_container(ds: &DaemonSet) -> Container { + ds.spec + .as_ref() + .unwrap() + .template + .spec + .as_ref() + .unwrap() + .containers + .iter() + .find(|c| c.name == "user-info-fetcher") + .expect("the user-info-fetcher container should exist") + .clone() + } + + fn env_var(container: &Container, name: &str) -> String { + container + .env + .as_ref() + .expect("the container should have env vars") + .iter() + .find(|e| e.name == name) + .unwrap_or_else(|| panic!("env var {name} should be set")) + .value + .clone() + .unwrap_or_else(|| panic!("env var {name} should have a literal value")) + } + + fn mount_path(container: &Container, volume_name: &str) -> String { + container + .volume_mounts + .as_ref() + .expect("the container should have volume mounts") + .iter() + .find(|m| m.name == volume_name) + .unwrap_or_else(|| panic!("volume mount {volume_name} should exist")) + .mount_path + .clone() + } + + #[test] + fn user_info_fetcher_container_has_expected_command_and_config_wiring() { + let ds = build(&validated_cluster_from_spec(json!({ + "image": { "productVersion": "1.2.3" }, + "clusterConfig": { + "userInfo": { + "backend": { + "experimentalXfscAas": { + "hostname": "aas.default.svc.cluster.local", + "port": 5000, + } + } + } + }, + "servers": { "roleGroups": { "default": {} } }, + }))); + + let uif = uif_container(&ds); + assert_eq!( + uif.command, + Some(vec!["stackable-opa-user-info-fetcher".to_owned()]) + ); + // The sidecar reads its config from the shared config volume, and looks for backend + // credentials in a fixed directory (populated by the backend-specific arms below). + assert_eq!( + env_var(&uif, "CONFIG"), + "/stackable/config/user-info-fetcher.json" + ); + assert_eq!(env_var(&uif, "CREDENTIALS_DIR"), "/stackable/credentials"); + assert_eq!(mount_path(&uif, "config"), "/stackable/config"); + } + + #[test] + fn user_info_fetcher_active_directory_backend_mounts_kerberos_and_sets_krb5_env() { + let ds = build(&validated_cluster_from_spec(json!({ + "image": { "productVersion": "1.2.3" }, + "clusterConfig": { + "userInfo": { + "backend": { + "experimentalActiveDirectory": { + "ldapServer": "ad.example.com", + "baseDistinguishedName": "dc=example,dc=com", + "kerberosSecretClassName": "kerberos", + } + } + } + }, + "servers": { "roleGroups": { "default": {} } }, + }))); + + // A Kerberos secret volume is provisioned and mounted for the sidecar. + assert!(volume_names(&ds).contains(&"kerberos".to_owned())); + let uif = uif_container(&ds); + assert_eq!(mount_path(&uif, "kerberos"), "/stackable/kerberos"); + // The krb5 client must find the config and keytab, and keep tickets in memory only. + assert_eq!( + env_var(&uif, "KRB5_CONFIG"), + "/stackable/kerberos/krb5.conf" + ); + assert_eq!( + env_var(&uif, "KRB5_CLIENT_KTNAME"), + "/stackable/kerberos/keytab" + ); + assert_eq!(env_var(&uif, "KRB5CCNAME"), "MEMORY:"); + } + + #[test] + fn user_info_fetcher_keycloak_backend_mounts_client_credentials_secret() { + let ds = build(&validated_cluster_from_spec(json!({ + "image": { "productVersion": "1.2.3" }, + "clusterConfig": { + "userInfo": { + "backend": { + "keycloak": { + "hostname": "keycloak.example.com", + "clientCredentialsSecret": "keycloak-credentials", + "adminRealm": "master", + "userRealm": "my-realm", + } + } + } + }, + "servers": { "roleGroups": { "default": {} } }, + }))); + + // The client credentials secret is projected into the sidecar's credentials dir. + assert!(volume_names(&ds).contains(&"user-info-fetcher-credentials".to_owned())); + assert_eq!( + mount_path(&uif_container(&ds), "user-info-fetcher-credentials"), + "/stackable/credentials" + ); + } +} diff --git a/rust/operator-binary/src/controller/build/resource/daemonset/mod.rs b/rust/operator-binary/src/controller/build/resource/workload/mod.rs similarity index 60% rename from rust/operator-binary/src/controller/build/resource/daemonset/mod.rs rename to rust/operator-binary/src/controller/build/resource/workload/mod.rs index e6ab8c44..840225c8 100644 --- a/rust/operator-binary/src/controller/build/resource/daemonset/mod.rs +++ b/rust/operator-binary/src/controller/build/resource/workload/mod.rs @@ -1,5 +1,8 @@ -//! Builds the rolegroup [`DaemonSet`] that runs OPA (plus its bundle-builder, optional -//! user-info-fetcher, and Vector sidecars) on every node. +//! Building blocks shared by the rolegroup workload objects that run OPA (plus its +//! bundle-builder, optional user-info-fetcher, and Vector sidecars). +//! +//! The Pod template is identical regardless of how the rolegroup is deployed, so it is built here +//! and wrapped by the workload-specific submodules ([`daemonset`]). use std::{collections::BTreeMap, str::FromStr}; @@ -21,14 +24,11 @@ use stackable_operator::{ commons::secret_class::SecretClassVolumeProvisionParts, k8s_openapi::{ DeepMerge, - api::{ - apps::v1::{DaemonSet, DaemonSetSpec, DaemonSetUpdateStrategy, RollingUpdateDaemonSet}, - core::v1::{ - EmptyDirVolumeSource, EnvVarSource, HTTPGetAction, ObjectFieldSelector, Probe, - ResourceRequirements, - }, + api::core::v1::{ + EmptyDirVolumeSource, EnvVarSource, HTTPGetAction, ObjectFieldSelector, + PodTemplateSpec, Probe, ResourceRequirements, }, - apimachinery::pkg::{apis::meta::v1::LabelSelector, util::intstr::IntOrString}, + apimachinery::pkg::util::intstr::IntOrString, }, memory::{BinaryMultiple, MemoryQuantity}, product_logging::{ @@ -52,7 +52,7 @@ use crate::{ OpaRoleGroupConfig, RoleGroupName, ValidatedCluster, ValidatedOpaConfig, build::{ self, - resource::daemonset::{ + resource::workload::{ resource_info_fetcher::add_resource_info_fetcher_sidecar, user_info_fetcher::add_user_info_fetcher_sidecar, }, @@ -61,6 +61,7 @@ use crate::{ operations::graceful_shutdown::add_graceful_shutdown_config, }; +pub mod daemonset; mod resource_info_fetcher; mod user_info_fetcher; @@ -221,16 +222,13 @@ fn http_liveness_probe(path: &str, port: IntOrString, scheme: Option) -> } } -/// The rolegroup [`DaemonSet`] runs the rolegroup, as configured by the administrator. -/// -/// The [`Pod`](`stackable_operator::k8s_openapi::api::core::v1::Pod`)s are accessible through the -/// corresponding [`Service`](`stackable_operator::k8s_openapi::api::core::v1::Service`) (from -/// [`build_server_role_service`](super::service::build_server_role_service)). +/// Builds the [`PodTemplateSpec`] for a rolegroup, shared by every deployment mode. /// -/// We run an OPA on each node, because we want to avoid requiring network roundtrips for services making -/// policy queries (which are often chained in serial, and block other tasks in the products). +/// The template carries the `prepare` init container, the OPA and bundle-builder containers, the +/// optional user-info-fetcher and Vector sidecars, and all volumes they mount. Callers wrap it in +/// the workload object of their choice; see [`daemonset::build_server_rolegroup_daemonset`]. #[allow(clippy::too_many_arguments)] -pub fn build_server_rolegroup_daemonset( +pub(crate) fn build_server_rolegroup_pod_template( cluster: &ValidatedCluster, role_group_name: &RoleGroupName, role_group: &OpaRoleGroupConfig, @@ -238,7 +236,7 @@ pub fn build_server_rolegroup_daemonset( user_info_fetcher_image: &str, resource_info_fetcher_image: &str, cluster_info: &KubernetesClusterInfo, -) -> Result { +) -> Result { let resolved_product_image = &cluster.image; let rolegroup_config = role_group; // All overrides were already merged (role group over role over defaults) in the validate step. @@ -474,37 +472,7 @@ pub fn build_server_rolegroup_daemonset( let mut pod_template = pb.build_template(); pod_template.merge_from(rolegroup_config.pod_overrides.clone()); - let metadata = build::object_meta( - cluster, - cluster - .role_group_resource_names(role_group_name) - .daemon_set_name() - .to_string(), - role_group_name, - ) - .build(); - - let daemonset_spec = DaemonSetSpec { - selector: LabelSelector { - match_labels: Some(cluster.role_group_selector(role_group_name).into()), - ..LabelSelector::default() - }, - template: pod_template, - update_strategy: Some(DaemonSetUpdateStrategy { - type_: Some("RollingUpdate".to_string()), - rolling_update: Some(RollingUpdateDaemonSet { - max_surge: Some(IntOrString::Int(1)), - max_unavailable: Some(IntOrString::Int(0)), - }), - }), - ..DaemonSetSpec::default() - }; - - Ok(DaemonSet { - metadata, - spec: Some(daemonset_spec), - status: None, - }) + Ok(pod_template) } /// Env variables that are need to run stackable Rust binaries, such as @@ -736,408 +704,3 @@ fn build_prepare_start_command( prepare_container_args } - -#[cfg(test)] -mod tests { - use serde_json::json; - use stackable_opa_operator::crd::OpaRole; - use stackable_operator::{ - commons::networking::DomainName, k8s_openapi::api::core::v1::Container, - }; - - use super::*; - use crate::controller::build::properties::test_support::validated_cluster_from_spec; - - fn cluster_info() -> KubernetesClusterInfo { - KubernetesClusterInfo { - cluster_domain: DomainName::try_from("cluster.local").unwrap(), - } - } - - fn build(cluster: &ValidatedCluster) -> DaemonSet { - let (role_group_name, role_group) = cluster.role_group_configs[&OpaRole::Server] - .iter() - .next() - .expect("the default role group should exist"); - build_server_rolegroup_daemonset( - cluster, - role_group_name, - role_group, - "bundle-builder-image", - "user-info-fetcher-image", - "resource-info-fetcher-image", - &cluster_info(), - ) - .expect("the daemonset should build") - } - - fn container_names(ds: &DaemonSet) -> Vec { - ds.spec - .as_ref() - .unwrap() - .template - .spec - .as_ref() - .unwrap() - .containers - .iter() - .map(|c| c.name.clone()) - .collect() - } - - fn volume_names(ds: &DaemonSet) -> Vec { - ds.spec - .as_ref() - .unwrap() - .template - .spec - .as_ref() - .unwrap() - .volumes - .as_ref() - .unwrap() - .iter() - .map(|v| v.name.clone()) - .collect() - } - - #[test] - fn daemonset_has_expected_name_and_rolling_update_strategy() { - let ds = build(&validated_cluster_from_spec(json!({ - "image": { "productVersion": "1.2.3" }, - "servers": { "roleGroups": { "default": {} } }, - }))); - - assert_eq!(ds.metadata.name.as_deref(), Some("test-opa-server-default")); - let strategy = ds.spec.as_ref().unwrap().update_strategy.as_ref().unwrap(); - assert_eq!(strategy.type_.as_deref(), Some("RollingUpdate")); - let rolling_update = strategy.rolling_update.as_ref().unwrap(); - // A DaemonSet must never take an OPA pod down before the replacement is ready. - assert_eq!(rolling_update.max_unavailable, Some(IntOrString::Int(0))); - } - - #[test] - fn daemonset_runs_opa_and_bundle_builder_with_prepare_init_container() { - let ds = build(&validated_cluster_from_spec(json!({ - "image": { "productVersion": "1.2.3" }, - "servers": { "roleGroups": { "default": {} } }, - }))); - - let containers = container_names(&ds); - assert!(containers.contains(&"opa".to_owned())); - assert!(containers.contains(&"bundle-builder".to_owned())); - // No sidecars without the corresponding cluster config. - assert!(!containers.contains(&"user-info-fetcher".to_owned())); - assert!(!containers.contains(&"vector".to_owned())); - - let pod_spec = ds.spec.as_ref().unwrap().template.spec.as_ref().unwrap(); - let init_containers: Vec<_> = pod_spec - .init_containers - .as_ref() - .unwrap() - .iter() - .map(|c| c.name.clone()) - .collect(); - assert_eq!(init_containers, vec!["prepare".to_owned()]); - - // The standard volumes are always present; the TLS volume is not (no TLS configured). - let volumes = volume_names(&ds); - for expected in ["config", "bundles", "log"] { - assert!( - volumes.contains(&expected.to_owned()), - "missing volume {expected}" - ); - } - assert!(!volumes.contains(&"tls".to_owned())); - } - - #[test] - fn daemonset_adds_vector_container_when_agent_enabled() { - let ds = build(&validated_cluster_from_spec(json!({ - "image": { "productVersion": "1.2.3" }, - "clusterConfig": { "vectorAggregatorConfigMapName": "vector-aggregator-discovery" }, - "servers": { - "config": { "logging": { "enableVectorAgent": true } }, - "roleGroups": { "default": {} }, - }, - }))); - - assert!(container_names(&ds).contains(&"vector".to_owned())); - } - - #[test] - fn daemonset_adds_user_info_fetcher_container_when_configured() { - let ds = build(&validated_cluster_from_spec(json!({ - "image": { "productVersion": "1.2.3" }, - "clusterConfig": { - "userInfo": { - "backend": { - "experimentalXfscAas": { - "hostname": "aas.default.svc.cluster.local", - "port": 5000, - } - } - } - }, - "servers": { "roleGroups": { "default": {} } }, - }))); - - assert!(container_names(&ds).contains(&"user-info-fetcher".to_owned())); - } - - #[test] - fn opa_probes_root_and_bundle_builder_probes_status() { - let ds = build(&validated_cluster_from_spec(json!({ - "image": { "productVersion": "1.2.3" }, - "servers": { "roleGroups": { "default": {} } }, - }))); - let pod_spec = ds.spec.as_ref().unwrap().template.spec.as_ref().unwrap(); - let liveness_path = |container: &str| -> String { - pod_spec - .containers - .iter() - .find(|c| c.name == container) - .unwrap_or_else(|| panic!("container {container} should exist")) - .liveness_probe - .as_ref() - .unwrap() - .http_get - .as_ref() - .unwrap() - .path - .clone() - .unwrap() - }; - // OPA's HTTP server answers `/`; only the bundle-builder exposes `/status`. A wrong path - // here makes the liveness probe fail and the OPA container CrashLoop. - assert_eq!(liveness_path("opa"), "/"); - assert_eq!(liveness_path("bundle-builder"), "/status"); - } - - #[test] - fn daemonset_adds_tls_volume_when_tls_enabled() { - let ds = build(&validated_cluster_from_spec(json!({ - "image": { "productVersion": "1.2.3" }, - "clusterConfig": { "tls": { "serverSecretClass": "tls" } }, - "servers": { "roleGroups": { "default": {} } }, - }))); - - assert!(volume_names(&ds).contains(&"tls".to_owned())); - } - - #[test] - fn opa_container_serves_https_when_tls_enabled() { - let ds = build(&validated_cluster_from_spec(json!({ - "image": { "productVersion": "1.2.3" }, - "clusterConfig": { "tls": { "serverSecretClass": "tls" } }, - "servers": { "roleGroups": { "default": {} } }, - }))); - let pod_spec = ds.spec.as_ref().unwrap().template.spec.as_ref().unwrap(); - let opa = pod_spec - .containers - .iter() - .find(|c| c.name == "opa") - .expect("opa container should exist"); - - // The single container port is the HTTPS data port. - let ports = opa.ports.as_ref().unwrap(); - assert_eq!(ports.len(), 1); - assert_eq!(ports[0].name.as_deref(), Some("https")); - assert_eq!(ports[0].container_port, 8443); - - // The probe must speak HTTPS, otherwise it would fail against the TLS-only server. - let scheme = opa - .liveness_probe - .as_ref() - .unwrap() - .http_get - .as_ref() - .unwrap() - .scheme - .clone(); - assert_eq!(scheme.as_deref(), Some("HTTPS")); - - // The start command binds the HTTPS port and passes the TLS cert/key flags. - let args = opa.args.as_ref().unwrap(); - assert!(args[0].contains("-a 0.0.0.0:8443")); - assert!(args[0].contains("--tls-cert-file")); - } - - #[test] - fn bundle_builder_start_command_silences_console_only_when_none() { - let role_group_config = |spec: serde_json::Value| { - let cluster = validated_cluster_from_spec(spec); - cluster.role_group_configs[&OpaRole::Server] - .values() - .next() - .expect("the default role group should exist") - .config - .clone() - }; - - // Console level NONE redirects bundle-builder output to /dev/null (no `tee`). - let silenced = role_group_config(json!({ - "image": { "productVersion": "1.2.3" }, - "servers": { - "config": { "logging": { "containers": { - "bundle-builder": { "console": { "level": "NONE" } } - } } }, - "roleGroups": { "default": {} }, - }, - })); - // The redirect is appended directly after the bundle-builder invocation. (`/dev/null` also - // appears in the shared bash trap helpers, so match the specific redirect.) - assert!( - build_bundle_builder_start_command(&silenced, "bundle-builder") - .contains("stackable-opa-bundle-builder > /dev/null") - ); - - // With a console level above NONE, output is not discarded. - let logging = role_group_config(json!({ - "image": { "productVersion": "1.2.3" }, - "servers": { - "config": { "logging": { "containers": { - "bundle-builder": { "console": { "level": "INFO" } } - } } }, - "roleGroups": { "default": {} }, - }, - })); - assert!( - build_bundle_builder_start_command(&logging, "bundle-builder") - .contains("stackable-opa-bundle-builder &") - ); - } - - fn uif_container(ds: &DaemonSet) -> Container { - ds.spec - .as_ref() - .unwrap() - .template - .spec - .as_ref() - .unwrap() - .containers - .iter() - .find(|c| c.name == "user-info-fetcher") - .expect("the user-info-fetcher container should exist") - .clone() - } - - fn env_var(container: &Container, name: &str) -> String { - container - .env - .as_ref() - .expect("the container should have env vars") - .iter() - .find(|e| e.name == name) - .unwrap_or_else(|| panic!("env var {name} should be set")) - .value - .clone() - .unwrap_or_else(|| panic!("env var {name} should have a literal value")) - } - - fn mount_path(container: &Container, volume_name: &str) -> String { - container - .volume_mounts - .as_ref() - .expect("the container should have volume mounts") - .iter() - .find(|m| m.name == volume_name) - .unwrap_or_else(|| panic!("volume mount {volume_name} should exist")) - .mount_path - .clone() - } - - #[test] - fn user_info_fetcher_container_has_expected_command_and_config_wiring() { - let ds = build(&validated_cluster_from_spec(json!({ - "image": { "productVersion": "1.2.3" }, - "clusterConfig": { - "userInfo": { - "backend": { - "experimentalXfscAas": { - "hostname": "aas.default.svc.cluster.local", - "port": 5000, - } - } - } - }, - "servers": { "roleGroups": { "default": {} } }, - }))); - - let uif = uif_container(&ds); - assert_eq!( - uif.command, - Some(vec!["stackable-opa-user-info-fetcher".to_owned()]) - ); - // The sidecar reads its config from the shared config volume, and looks for backend - // credentials in a fixed directory (populated by the backend-specific arms below). - assert_eq!( - env_var(&uif, "CONFIG"), - "/stackable/config/user-info-fetcher.json" - ); - assert_eq!(env_var(&uif, "CREDENTIALS_DIR"), "/stackable/credentials"); - assert_eq!(mount_path(&uif, "config"), "/stackable/config"); - } - - #[test] - fn user_info_fetcher_active_directory_backend_mounts_kerberos_and_sets_krb5_env() { - let ds = build(&validated_cluster_from_spec(json!({ - "image": { "productVersion": "1.2.3" }, - "clusterConfig": { - "userInfo": { - "backend": { - "experimentalActiveDirectory": { - "ldapServer": "ad.example.com", - "baseDistinguishedName": "dc=example,dc=com", - "kerberosSecretClassName": "kerberos", - } - } - } - }, - "servers": { "roleGroups": { "default": {} } }, - }))); - - // A Kerberos secret volume is provisioned and mounted for the sidecar. - assert!(volume_names(&ds).contains(&"kerberos".to_owned())); - let uif = uif_container(&ds); - assert_eq!(mount_path(&uif, "kerberos"), "/stackable/kerberos"); - // The krb5 client must find the config and keytab, and keep tickets in memory only. - assert_eq!( - env_var(&uif, "KRB5_CONFIG"), - "/stackable/kerberos/krb5.conf" - ); - assert_eq!( - env_var(&uif, "KRB5_CLIENT_KTNAME"), - "/stackable/kerberos/keytab" - ); - assert_eq!(env_var(&uif, "KRB5CCNAME"), "MEMORY:"); - } - - #[test] - fn user_info_fetcher_keycloak_backend_mounts_client_credentials_secret() { - let ds = build(&validated_cluster_from_spec(json!({ - "image": { "productVersion": "1.2.3" }, - "clusterConfig": { - "userInfo": { - "backend": { - "keycloak": { - "hostname": "keycloak.example.com", - "clientCredentialsSecret": "keycloak-credentials", - "adminRealm": "master", - "userRealm": "my-realm", - } - } - } - }, - "servers": { "roleGroups": { "default": {} } }, - }))); - - // The client credentials secret is projected into the sidecar's credentials dir. - assert!(volume_names(&ds).contains(&"user-info-fetcher-credentials".to_owned())); - assert_eq!( - mount_path(&uif_container(&ds), "user-info-fetcher-credentials"), - "/stackable/credentials" - ); - } -} diff --git a/rust/operator-binary/src/controller/build/resource/daemonset/resource_info_fetcher.rs b/rust/operator-binary/src/controller/build/resource/workload/resource_info_fetcher.rs similarity index 99% rename from rust/operator-binary/src/controller/build/resource/daemonset/resource_info_fetcher.rs rename to rust/operator-binary/src/controller/build/resource/workload/resource_info_fetcher.rs index 21f64222..508500c1 100644 --- a/rust/operator-binary/src/controller/build/resource/daemonset/resource_info_fetcher.rs +++ b/rust/operator-binary/src/controller/build/resource/workload/resource_info_fetcher.rs @@ -15,7 +15,7 @@ use crate::controller::{ ValidatedCluster, ValidatedOpaConfig, build::{ self, - resource::daemonset::{ + resource::workload::{ CONFIG_DIR, CONFIG_VOLUME_NAME, RESOURCE_INFO_FETCHER_CREDENTIALS_DIR, RESOURCE_INFO_FETCHER_CREDENTIALS_VOLUME_NAME, add_stackable_rust_cli_env_vars, container_name, sidecar_container_log_level, sidecar_resource_requirements, diff --git a/rust/operator-binary/src/controller/build/resource/daemonset/user_info_fetcher.rs b/rust/operator-binary/src/controller/build/resource/workload/user_info_fetcher.rs similarity index 99% rename from rust/operator-binary/src/controller/build/resource/daemonset/user_info_fetcher.rs rename to rust/operator-binary/src/controller/build/resource/workload/user_info_fetcher.rs index 31b4f542..45efe36b 100644 --- a/rust/operator-binary/src/controller/build/resource/daemonset/user_info_fetcher.rs +++ b/rust/operator-binary/src/controller/build/resource/workload/user_info_fetcher.rs @@ -21,7 +21,7 @@ use crate::controller::{ ValidatedCluster, ValidatedOpaConfig, build::{ self, - resource::daemonset::{ + resource::workload::{ CONFIG_DIR, CONFIG_VOLUME_NAME, USER_INFO_FETCHER_CREDENTIALS_DIR, USER_INFO_FETCHER_CREDENTIALS_VOLUME_NAME, USER_INFO_FETCHER_KERBEROS_DIR, USER_INFO_FETCHER_KERBEROS_VOLUME_NAME, add_stackable_rust_cli_env_vars, From 2aeb81617358528df92cd4f15cff3d2782dcef89 Mon Sep 17 00:00:00 2001 From: Maxi Wittich Date: Mon, 10 Aug 2026 15:18:30 +0200 Subject: [PATCH 03/22] Remove internalTrafficPolicy for now, waiting on the decision --- extra/crds.yaml | 34 ---------------- rust/operator-binary/src/crd/mod.rs | 60 +++++++++++++---------------- 2 files changed, 26 insertions(+), 68 deletions(-) diff --git a/extra/crds.yaml b/extra/crds.yaml index 2cd70f5f..2276f537 100644 --- a/extra/crds.yaml +++ b/extra/crds.yaml @@ -1497,23 +1497,6 @@ spec: workloadKind: DaemonSet description: Role-level configuration for the OPA servers. properties: - internalTrafficPolicy: - description: |- - The `internalTrafficPolicy` of the role Service. - - * `Local`: Only route to OPA Pods on the same node as the client. This avoids - cross-node latency, but requests will fail if there is no OPA Pod on the node. - - * `Cluster`: Route to any OPA Pod of the role. - - Defaults to `Local` when `workloadKind` is a `DaemonSet` and to `Cluster` when it is a - `Deployment`. - enum: - - Local - - Cluster - - null - nullable: true - type: string podDisruptionBudget: default: enabled: null @@ -3815,23 +3798,6 @@ spec: workloadKind: DaemonSet description: Role-level configuration for the OPA servers. properties: - internalTrafficPolicy: - description: |- - The `internalTrafficPolicy` of the role Service. - - * `Local`: Only route to OPA Pods on the same node as the client. This avoids - cross-node latency, but requests will fail if there is no OPA Pod on the node. - - * `Cluster`: Route to any OPA Pod of the role. - - Defaults to `Local` when `workloadKind` is a `DaemonSet` and to `Cluster` when it is a - `Deployment`. - enum: - - Local - - Cluster - - null - nullable: true - type: string podDisruptionBudget: default: enabled: null diff --git a/rust/operator-binary/src/crd/mod.rs b/rust/operator-binary/src/crd/mod.rs index 2c189d43..cabed1b8 100644 --- a/rust/operator-binary/src/crd/mod.rs +++ b/rust/operator-binary/src/crd/mod.rs @@ -154,22 +154,11 @@ pub mod versioned { #[serde(default)] pub workload_kind: WorkloadKind, - /// The `internalTrafficPolicy` of the role Service. - /// - /// * `Local`: Only route to OPA Pods on the same node as the client. This avoids - /// cross-node latency, but requests will fail if there is no OPA Pod on the node. - /// - /// * `Cluster`: Route to any OPA Pod of the role. - /// - /// Defaults to `Local` when `workloadKind` is a `DaemonSet` and to `Cluster` when it is a - /// `Deployment`. - // `skip_serializing_if` keeps the `null` out of the schema `default` that is generated for - // the enclosing `roleConfig`. The apiserver validates that default against this property's - // schema, and rejects a `null` against an `enum`. - #[serde(default, skip_serializing_if = "Option::is_none")] - #[versioned(hint(option))] - pub internal_traffic_policy: Option, - + // `internalTrafficPolicy` is deliberately not a field here: the operator derives it from + // `workloadKind` in `OpaRoleConfig::internal_traffic_policy`. Exposing it as a user + // override means adding an `Option` field back and falling back to + // that helper's `match`. + // // We can not #[serde(flatten)] a `GenericRoleConfig` here, as we need a PodDisruptionBudget // default that depends on `workloadKind`. #[serde(default)] @@ -189,6 +178,8 @@ pub mod versioned { /// /// The variants are spelled as Kubernetes spells them, so the value can be passed through to /// `Service.spec.internalTrafficPolicy` unchanged. + /// + /// TODO: Not yet part of the CRD: the operator derives the policy from [`WorkloadKind`]. #[derive(Clone, Debug, Deserialize, Display, Eq, JsonSchema, PartialEq, Serialize)] #[serde(rename_all = "PascalCase")] pub enum InternalTrafficPolicy { @@ -387,15 +378,16 @@ impl v1alpha2::CurrentlySupportedListenerClasses { impl v1alpha2::OpaRoleConfig { /// The `internalTrafficPolicy` to write into the role Service. /// - /// Falls back to the [`v1alpha2::WorkloadKind`] default when unset: `Local` for a DaemonSet, - /// which covers every node, and `Cluster` for a Deployment, whose Pods do not. + /// Derived from the [`v1alpha2::WorkloadKind`]: `Local` for a DaemonSet, which covers every + /// node, and `Cluster` for a Deployment, whose Pods do not. + /// + /// This is the single place the policy is decided, so exposing a user override later means + /// adding the CRD field back and wrapping this `match` in an `unwrap_or`; no call site changes. pub fn internal_traffic_policy(&self) -> v1alpha2::InternalTrafficPolicy { - self.internal_traffic_policy - .clone() - .unwrap_or(match self.workload_kind { - v1alpha2::WorkloadKind::DaemonSet => v1alpha2::InternalTrafficPolicy::Local, - v1alpha2::WorkloadKind::Deployment => v1alpha2::InternalTrafficPolicy::Cluster, - }) + match self.workload_kind { + v1alpha2::WorkloadKind::DaemonSet => v1alpha2::InternalTrafficPolicy::Local, + v1alpha2::WorkloadKind::Deployment => v1alpha2::InternalTrafficPolicy::Cluster, + } } /// Whether a PodDisruptionBudget should be written out for this role. @@ -458,8 +450,9 @@ mod tests { use super::{v1alpha1, v1alpha2}; - /// The defaults the operator derives from `workloadKind`, which an OpenAPI schema default - /// cannot express. Locks the table in the CRD docs of the two fields. + /// The values the operator derives from `workloadKind`, which an OpenAPI schema default cannot + /// express. `internalTrafficPolicy` is derived outright; `podDisruptionBudget.enabled` is a + /// default the user can override. #[test] fn role_config_defaults_follow_workload_kind() { let role_config = |workload_kind| v1alpha2::OpaRoleConfig { @@ -483,28 +476,28 @@ mod tests { assert!(deployment.pod_disruption_budget_enabled()); } - /// An explicitly configured value wins over the `workloadKind`-derived default, which is the - /// point of exposing the two fields at all (DaemonSet with `Cluster` is used in the field). + /// An explicitly configured `podDisruptionBudget.enabled` wins over the `workloadKind`-derived + /// default, which is the point of exposing the field as an `Option` at all. #[test] fn explicit_role_config_overrides_the_derived_defaults() { let role_config = v1alpha2::OpaRoleConfig { workload_kind: v1alpha2::WorkloadKind::DaemonSet, - internal_traffic_policy: Some(v1alpha2::InternalTrafficPolicy::Cluster), pod_disruption_budget: v1alpha2::OpaPdbConfig { enabled: Some(true), max_unavailable: None, }, }; + assert!(role_config.pod_disruption_budget_enabled()); + // `internalTrafficPolicy` is not yet user-configurable, so it stays at the DaemonSet default. assert_eq!( role_config.internal_traffic_policy(), - v1alpha2::InternalTrafficPolicy::Cluster + v1alpha2::InternalTrafficPolicy::Local ); - assert!(role_config.pod_disruption_budget_enabled()); } - /// Leaving the two fields out and writing them as an explicit `null` must resolve to the same - /// unset state, as the derived defaults are applied by the operator rather than by the schema. + /// Leaving the PDB fields out and writing them as an explicit `null` must resolve to the same + /// unset state, as the derived default is applied by the operator rather than by the schema. /// /// Only covers what serde does; substituting the `roleConfig` default for an entirely absent /// `roleConfig` is the apiserver's job and is not exercised here. @@ -517,7 +510,6 @@ mod tests { json!({ "workloadKind": "DaemonSet" }), json!({ "workloadKind": "DaemonSet", - "internalTrafficPolicy": null, "podDisruptionBudget": { "enabled": null, "maxUnavailable": null }, }), ] { From 0a7e60ee5ce096d89c9ce5ee7d5830bef8c7e16d Mon Sep 17 00:00:00 2001 From: Maxi Wittich Date: Mon, 10 Aug 2026 15:19:13 +0200 Subject: [PATCH 04/22] Adding validation for RoleConfig --- rust/operator-binary/src/controller/mod.rs | 20 +++++++ .../src/controller/validate.rs | 53 +++++++++++++++++++ 2 files changed, 73 insertions(+) diff --git a/rust/operator-binary/src/controller/mod.rs b/rust/operator-binary/src/controller/mod.rs index 1aa2769e..0dfe41b6 100644 --- a/rust/operator-binary/src/controller/mod.rs +++ b/rust/operator-binary/src/controller/mod.rs @@ -59,6 +59,11 @@ pub struct ValidatedCluster { pub product_version: ProductVersion, pub image: ResolvedProductImage, pub cluster_config: ValidatedClusterConfig, + /// The role-level configuration of every role, keyed the same way as `role_group_configs`. + /// + /// Role-level rather than role-group-level, because `workloadKind` decides the shape of the + /// role Service, which selects across all of a role's role groups. + pub role_configs: BTreeMap, pub role_group_configs: BTreeMap>, } @@ -69,6 +74,7 @@ impl ValidatedCluster { uid: Uid, image: ResolvedProductImage, cluster_config: ValidatedClusterConfig, + role_configs: BTreeMap, role_group_configs: BTreeMap>, ) -> Self { let product_version = ProductVersion::from_str(&image.app_version_label_value) @@ -88,10 +94,24 @@ impl ValidatedCluster { product_version, image, cluster_config, + role_configs, role_group_configs, } } + /// The role-level configuration of `role`. + /// + /// The validate step inserts an entry for every [`OpaRole`], falling back to the + /// `OpaRoleConfig` default for roles the user did not configure. + // TODO: Remove the `allow` once the workload dispatch and the Service and PodDisruptionBudget + // builders call this. Part of https://github.com/stackabletech/opa-operator/issues/525. + #[allow(dead_code)] + pub fn role_config(&self, role: &OpaRole) -> &v1alpha2::OpaRoleConfig { + self.role_configs + .get(role) + .expect("the validate step inserts a role config for every role") + } + /// Whether the cluster serves HTTPS, derived from the validated cluster config. pub fn is_tls_enabled(&self) -> bool { self.cluster_config.tls.is_some() diff --git a/rust/operator-binary/src/controller/validate.rs b/rust/operator-binary/src/controller/validate.rs index 954abb19..f79ba2d6 100644 --- a/rust/operator-binary/src/controller/validate.rs +++ b/rust/operator-binary/src/controller/validate.rs @@ -165,10 +165,16 @@ pub fn validate( .vector_aggregator_config_map_name .clone(); + let mut role_configs = BTreeMap::new(); let mut role_group_configs = BTreeMap::new(); for opa_role in OpaRole::iter() { let role = opa.role(&opa_role); + // Carried per role rather than per cluster, so a second role could pick its own + // `workloadKind`. `serde(default)` on `Role::role_config` means this is the + // `OpaRoleConfig` default when the user configured no `roleConfig` at all. + role_configs.insert(opa_role.clone(), role.role_config.clone()); + let mut group_configs = BTreeMap::new(); for (role_group_name, role_group) in &role.role_groups { // Merge default <- role <- role group and validate the config fragment, plus merge all @@ -235,6 +241,7 @@ pub fn validate( tls: opa.spec.cluster_config.tls.clone(), listener_class: opa.spec.cluster_config.listener_class.clone(), }, + role_configs, role_group_configs, )) } @@ -305,6 +312,12 @@ mod tests { v1alpha2::CurrentlySupportedListenerClasses::ClusterInternal ); + // The fixture sets no `roleConfig`, so the role falls back to the `OpaRoleConfig` default. + assert_eq!( + cluster.role_config(&OpaRole::Server), + &v1alpha2::OpaRoleConfig::default() + ); + // A single `server` role with the single `default` role group; the Vector agent is off. assert_eq!(cluster.role_group_configs.len(), 1); let role_groups = &cluster.role_group_configs[&OpaRole::Server]; @@ -317,6 +330,46 @@ mod tests { assert_eq!(role_group.config.logging.vector_container, None); } + /// A configured `roleConfig` reaches the build step, and every role gets an entry so + /// `ValidatedCluster::role_config` cannot panic. + #[test] + fn validate_carries_the_role_config_of_every_role() { + let opa: v1alpha2::OpaCluster = serde_json::from_value(json!({ + "apiVersion": "opa.stackable.tech/v1alpha2", + "kind": "OpaCluster", + "metadata": { + "name": "test-opa", + "namespace": "default", + "uid": "c27b3971-ca72-42c1-80a4-abdfc1db0ddd", + }, + "spec": { + "image": { "productVersion": "1.2.3" }, + "servers": { + "roleConfig": { "workloadKind": "Deployment" }, + "roleGroups": { "default": {} }, + }, + }, + })) + .expect("valid test input"); + let operator_environment = OperatorEnvironmentOptions { + operator_namespace: "stackable-operators".to_string(), + operator_service_name: "opa-operator".to_string(), + image_repository: "oci.example.org".to_string(), + }; + + let cluster = validate(&opa, &operator_environment).expect("the fixture validates"); + + assert_eq!( + cluster.role_config(&OpaRole::Server).workload_kind, + v1alpha2::WorkloadKind::Deployment + ); + // Every role is present, whether or not the user configured it. + assert_eq!(cluster.role_configs.len(), OpaRole::iter().count()); + for opa_role in OpaRole::iter() { + cluster.role_config(&opa_role); + } + } + /// A [`Logging`] with an automatic log config for every container, as the (defaulted) merged /// config provides at runtime. `validate_logging` validates all containers, so all must be /// present. From f2ba0c88422b162cf43bba7dd2a2ccbc9d2370e4 Mon Sep 17 00:00:00 2001 From: Maxi Wittich Date: Mon, 10 Aug 2026 15:41:51 +0200 Subject: [PATCH 05/22] Adding deployment.rs module to handle deployments of opa --- .../build/resource/workload/deployment.rs | 189 ++++++++++++++++++ .../controller/build/resource/workload/mod.rs | 6 +- rust/resource-info-fetcher/src/main.rs | 2 +- 3 files changed, 194 insertions(+), 3 deletions(-) create mode 100644 rust/operator-binary/src/controller/build/resource/workload/deployment.rs diff --git a/rust/operator-binary/src/controller/build/resource/workload/deployment.rs b/rust/operator-binary/src/controller/build/resource/workload/deployment.rs new file mode 100644 index 00000000..99080178 --- /dev/null +++ b/rust/operator-binary/src/controller/build/resource/workload/deployment.rs @@ -0,0 +1,189 @@ +//! Builds the rolegroup [`Deployment`] that runs a fixed number of OPA replicas. + +use stackable_operator::k8s_openapi::{ + api::apps::v1::{Deployment, DeploymentSpec, DeploymentStrategy, RollingUpdateDeployment}, + apimachinery::pkg::apis::meta::v1::LabelSelector, +}; + +use super::*; + +/// Runs a fixed number of replicas, unlike [`daemonset`](super::daemonset), which covers every +/// node. The Pods therefore do not cover every node and the role Service has to route to any of +/// them rather than to a node-local one. +// TODO: Remove the `allow` once the workload dispatch calls this. +// Part of https://github.com/stackabletech/opa-operator/issues/525. +#[allow(dead_code)] +#[allow(clippy::too_many_arguments)] +pub fn build_server_rolegroup_deployment( + cluster: &ValidatedCluster, + role_group_name: &RoleGroupName, + role_group: &OpaRoleGroupConfig, + opa_bundle_builder_image: &str, + user_info_fetcher_image: &str, + resource_info_fetcher_image: &str, + cluster_info: &KubernetesClusterInfo, +) -> Result { + let pod_template = build_server_rolegroup_pod_template( + cluster, + role_group_name, + role_group, + opa_bundle_builder_image, + user_info_fetcher_image, + resource_info_fetcher_image, + cluster_info, + )?; + + let metadata = build::object_meta( + cluster, + cluster + .role_group_resource_names(role_group_name) + .deployment_name() + .to_string(), + role_group_name, + ) + .build(); + + let deployment_spec = DeploymentSpec { + // Left unset so Kubernetes applies its default of one, rather than the operator inventing + // a replica count. + replicas: role_group.replicas.map(i32::from), + selector: LabelSelector { + match_labels: Some(cluster.role_group_selector(role_group_name).into()), + ..LabelSelector::default() + }, + template: pod_template, + strategy: Some(DeploymentStrategy { + type_: Some("RollingUpdate".to_string()), + rolling_update: Some(RollingUpdateDeployment { + max_surge: Some(IntOrString::Int(1)), + max_unavailable: Some(IntOrString::Int(0)), + }), + }), + ..DeploymentSpec::default() + }; + + Ok(Deployment { + metadata, + spec: Some(deployment_spec), + status: None, + }) +} + +#[cfg(test)] +mod tests { + use serde_json::json; + use stackable_operator::commons::networking::DomainName; + + use stackable_opa_operator::crd::OpaRole; + + use super::*; + use crate::controller::build::properties::test_support::validated_cluster_from_spec; + + fn cluster_info() -> KubernetesClusterInfo { + KubernetesClusterInfo { + cluster_domain: DomainName::try_from("cluster.local").unwrap(), + } + } + + fn build(cluster: &ValidatedCluster) -> Deployment { + let (role_group_name, role_group) = cluster.role_group_configs[&OpaRole::Server] + .iter() + .next() + .expect("the default role group should exist"); + build_server_rolegroup_deployment( + cluster, + role_group_name, + role_group, + "bundle-builder-image", + "user-info-fetcher-image", + "resource-info-fetcher-image", + &cluster_info(), + ) + .expect("the deployment should build") + } + + /// Named like the DaemonSet it replaces, so switching `workloadKind` swaps like for like. + #[test] + fn deployment_has_expected_name_and_rolling_update_strategy() { + let deployment = build(&validated_cluster_from_spec(json!({ + "image": { "productVersion": "1.2.3" }, + "servers": { + "roleConfig": { "workloadKind": "Deployment" }, + "roleGroups": { "default": {} }, + }, + }))); + + assert_eq!( + deployment.metadata.name.as_deref(), + Some("test-opa-server-default") + ); + let strategy = deployment.spec.as_ref().unwrap().strategy.as_ref().unwrap(); + assert_eq!(strategy.type_.as_deref(), Some("RollingUpdate")); + let rolling_update = strategy.rolling_update.as_ref().unwrap(); + // OPA sits in the products' hot path, so a rollout must never reduce the ready Pod count. + assert_eq!(rolling_update.max_unavailable, Some(IntOrString::Int(0))); + assert_eq!(rolling_update.max_surge, Some(IntOrString::Int(1))); + } + + /// `replicas` is what a Deployment adds over a DaemonSet, so it has to reach the spec. + #[test] + fn deployment_takes_the_replicas_of_its_role_group() { + let deployment = build(&validated_cluster_from_spec(json!({ + "image": { "productVersion": "1.2.3" }, + "servers": { + "roleConfig": { "workloadKind": "Deployment" }, + "roleGroups": { "default": { "replicas": 3 } }, + }, + }))); + + assert_eq!(deployment.spec.as_ref().unwrap().replicas, Some(3)); + } + + /// An unset `replicas` stays unset, leaving the Kubernetes default of one in place. + #[test] + fn deployment_without_replicas_leaves_them_unset() { + let deployment = build(&validated_cluster_from_spec(json!({ + "image": { "productVersion": "1.2.3" }, + "servers": { + "roleConfig": { "workloadKind": "Deployment" }, + "roleGroups": { "default": {} }, + }, + }))); + + assert_eq!(deployment.spec.as_ref().unwrap().replicas, None); + } + + /// The Pod template is shared with the DaemonSet and covered by its tests; this only checks that + /// it is wrapped and selected the same way. + #[test] + fn deployment_wraps_the_shared_pod_template() { + let deployment = build(&validated_cluster_from_spec(json!({ + "image": { "productVersion": "1.2.3" }, + "servers": { + "roleConfig": { "workloadKind": "Deployment" }, + "roleGroups": { "default": {} }, + }, + }))); + + let spec = deployment.spec.as_ref().unwrap(); + let containers: Vec<&str> = spec + .template + .spec + .as_ref() + .unwrap() + .containers + .iter() + .map(|container| container.name.as_str()) + .collect(); + assert!(containers.contains(&"opa")); + assert!(containers.contains(&"bundle-builder")); + + let match_labels = spec.selector.match_labels.as_ref().unwrap(); + assert_eq!( + match_labels + .get("app.kubernetes.io/role-group") + .map(String::as_str), + Some("default") + ); + } +} diff --git a/rust/operator-binary/src/controller/build/resource/workload/mod.rs b/rust/operator-binary/src/controller/build/resource/workload/mod.rs index 840225c8..6bbc4735 100644 --- a/rust/operator-binary/src/controller/build/resource/workload/mod.rs +++ b/rust/operator-binary/src/controller/build/resource/workload/mod.rs @@ -2,7 +2,7 @@ //! bundle-builder, optional user-info-fetcher, and Vector sidecars). //! //! The Pod template is identical regardless of how the rolegroup is deployed, so it is built here -//! and wrapped by the workload-specific submodules ([`daemonset`]). +//! and wrapped by the workload-specific submodules ([`daemonset`], [`deployment`]). use std::{collections::BTreeMap, str::FromStr}; @@ -62,6 +62,7 @@ use crate::{ }; pub mod daemonset; +pub mod deployment; mod resource_info_fetcher; mod user_info_fetcher; @@ -226,7 +227,8 @@ fn http_liveness_probe(path: &str, port: IntOrString, scheme: Option) -> /// /// The template carries the `prepare` init container, the OPA and bundle-builder containers, the /// optional user-info-fetcher and Vector sidecars, and all volumes they mount. Callers wrap it in -/// the workload object of their choice; see [`daemonset::build_server_rolegroup_daemonset`]. +/// the workload object of their choice; see [`daemonset::build_server_rolegroup_daemonset`] and +/// [`deployment::build_server_rolegroup_deployment`]. #[allow(clippy::too_many_arguments)] pub(crate) fn build_server_rolegroup_pod_template( cluster: &ValidatedCluster, diff --git a/rust/resource-info-fetcher/src/main.rs b/rust/resource-info-fetcher/src/main.rs index 0218918e..6bbf79d8 100644 --- a/rust/resource-info-fetcher/src/main.rs +++ b/rust/resource-info-fetcher/src/main.rs @@ -49,7 +49,7 @@ pub struct Args { #[derive(Clone)] struct AppState { backend: Arc, - // Note: Although we might not talk JSON to the underlying backend, we always return JSON as a + // Note: Although we might not talk JSON to the underlying backend, we always return JSON as a // result to the caller, so we can cache that. resource_info_cache: Cache, } From 3cf4fe1fb54ed002b7ea49cdbbabfe734390e421 Mon Sep 17 00:00:00 2001 From: Maxi Wittich Date: Tue, 11 Aug 2026 10:10:21 +0200 Subject: [PATCH 06/22] Adds opa as deployment as well as localTrafficPolicy as evaluation of workload kind --- CHANGELOG.md | 5 + .../templates/clusterrole-operator.yaml | 5 +- rust/operator-binary/src/controller/build.rs | 110 +++++++++++++++--- .../src/controller/build/resource/service.rs | 38 +++++- .../build/resource/workload/deployment.rs | 3 - rust/operator-binary/src/controller/mod.rs | 15 ++- .../src/controller/validate.rs | 5 +- rust/operator-binary/src/crd/mod.rs | 13 ++- rust/operator-binary/src/main.rs | 9 +- rust/operator-binary/src/opa_controller.rs | 25 +++- 10 files changed, 184 insertions(+), 44 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9e5c6889..59e823b2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,10 @@ All notable changes to this project will be documented in this file. Also, a rego-rule library has been added to make it easier to call resource-info-fetcher from within OPA. The API (especially the response) might change in the future once more data catalogs are supported ([#863]). - Allow specifying the maximum number of cached entries in the user-info-fetcher ([#863]). +- The `servers` role can now run as a `Deployment` instead of a `DaemonSet`, selected via + `spec.servers.roleConfig.workloadKind`. `DaemonSet` stays the default, so existing installations are + unchanged. In `Deployment` mode the role group's `replicas` is respected, and the role Service uses + `internalTrafficPolicy: Cluster` instead of `Local`, because the Pods no longer cover every node ([#525]). ### Changed @@ -27,6 +31,7 @@ All notable changes to this project will be documented in this file. which could cause problems with GitOps tools (e.g. ArgoCD) reporting a diff in the custom resources. See [our internal issue](https://github.com/stackabletech/hdfs-operator/issues/626) and [the fix](https://github.com/kube-rs/kube/pull/2042) for details ([#871]). +[#525]: https://github.com/stackabletech/opa-operator/issues/525 [#852]: https://github.com/stackabletech/opa-operator/pull/852 [#861]: https://github.com/stackabletech/opa-operator/pull/861 [#863]: https://github.com/stackabletech/opa-operator/pull/863 diff --git a/deploy/helm/opa-operator/templates/clusterrole-operator.yaml b/deploy/helm/opa-operator/templates/clusterrole-operator.yaml index 5805576d..fd7dc604 100644 --- a/deploy/helm/opa-operator/templates/clusterrole-operator.yaml +++ b/deploy/helm/opa-operator/templates/clusterrole-operator.yaml @@ -63,12 +63,13 @@ rules: - bind resourceNames: - {{ include "operator.name" . }}-clusterrole - # DaemonSet created per role group. Applied via SSA, tracked for orphan cleanup, and - # owned by the controller. + # DaemonSet or Deployment created per role group, depending on the role's `workloadKind`. + # Applied via SSA, tracked for orphan cleanup, and owned by the controller. - apiGroups: - apps resources: - daemonsets + - deployments verbs: - create - delete diff --git a/rust/operator-binary/src/controller/build.rs b/rust/operator-binary/src/controller/build.rs index 4d166868..fc470952 100644 --- a/rust/operator-binary/src/controller/build.rs +++ b/rust/operator-binary/src/controller/build.rs @@ -4,6 +4,7 @@ use std::str::FromStr; use snafu::{ResultExt, Snafu}; +use stackable_opa_operator::crd::v1alpha2; use stackable_operator::{ builder::meta::ObjectMetaBuilder, utils::cluster_info::KubernetesClusterInfo, @@ -20,7 +21,10 @@ use crate::controller::{ build_rolegroup_headless_service, build_rolegroup_metrics_service, build_server_role_service, }, - workload::daemonset::build_server_rolegroup_daemonset, + workload::{ + daemonset::build_server_rolegroup_daemonset, + deployment::build_server_rolegroup_deployment, + }, }, }; @@ -41,6 +45,12 @@ pub enum Error { role_group: RoleGroupName, }, + #[snafu(display("failed to build Deployment for role group {role_group}"))] + Deployment { + source: resource::workload::Error, + role_group: RoleGroupName, + }, + #[snafu(display("failed to build the discovery ConfigMap"))] Discovery { source: resource::discovery::Error }, } @@ -62,13 +72,17 @@ pub fn build( cluster_info: &KubernetesClusterInfo, ) -> Result { let mut daemon_sets = vec![]; + let mut deployments = vec![]; let mut services = vec![]; let mut config_maps = vec![]; // The role-level load-balanced Service, which is not bound to a single role group. services.push(build_server_role_service(cluster)); - for role_group_configs in cluster.role_group_configs.values() { + // Iterating with the role key, because the workload kind is configured per role. + for (opa_role, role_group_configs) in &cluster.role_group_configs { + let workload_kind = &cluster.role_config(opa_role).workload_kind; + for (role_group_name, role_group) in role_group_configs { config_maps.push( build_rolegroup_config_map(cluster, role_group_name, role_group).context( @@ -79,20 +93,37 @@ pub fn build( ); services.push(build_rolegroup_headless_service(cluster, role_group_name)); services.push(build_rolegroup_metrics_service(cluster, role_group_name)); - daemon_sets.push( - build_server_rolegroup_daemonset( - cluster, - role_group_name, - role_group, - opa_bundle_builder_image, - user_info_fetcher_image, - resource_info_fetcher_image, - cluster_info, - ) - .context(DaemonSetSnafu { - role_group: role_group_name.clone(), - })?, - ); + // Exactly one workload object per role group, of the kind its role asks for. + match workload_kind { + v1alpha2::WorkloadKind::DaemonSet => daemon_sets.push( + build_server_rolegroup_daemonset( + cluster, + role_group_name, + role_group, + opa_bundle_builder_image, + user_info_fetcher_image, + resource_info_fetcher_image, + cluster_info, + ) + .context(DaemonSetSnafu { + role_group: role_group_name.clone(), + })?, + ), + v1alpha2::WorkloadKind::Deployment => deployments.push( + build_server_rolegroup_deployment( + cluster, + role_group_name, + role_group, + opa_bundle_builder_image, + user_info_fetcher_image, + resource_info_fetcher_image, + cluster_info, + ) + .context(DeploymentSnafu { + role_group: role_group_name.clone(), + })?, + ), + } } } @@ -101,6 +132,7 @@ pub fn build( Ok(KubernetesResources { daemon_sets, + deployments, services, config_maps, service_accounts: vec![build_service_account(cluster)], @@ -185,11 +217,12 @@ mod tests { ) .expect("build succeeds"); - // One DaemonSet per role group. + // One DaemonSet per role group, as `workloadKind` defaults to `DaemonSet`. assert_eq!( sorted_names(&resources.daemon_sets), ["test-opa-server-default"] ); + assert!(resources.deployments.is_empty()); // The role-level Service plus a headless and a metrics Service per role group. assert_eq!( sorted_names(&resources.services), @@ -214,4 +247,47 @@ mod tests { ["test-opa-rolebinding"] ); } + + /// `workloadKind` decides which workload object a role group gets. Exactly one kind is built, so + /// the other list stays empty and `ClusterResources` sweeps the workload that is no longer + /// wanted when the administrator switches modes. + #[test] + fn build_dispatches_on_workload_kind() { + let build_with = |workload_kind| { + build( + &validated_cluster_from_spec(json!({ + "image": { "productVersion": "1.2.3" }, + "servers": { + "roleConfig": { "workloadKind": workload_kind }, + "roleGroups": { "default": {} }, + }, + })), + "bundle-builder-image", + "user-info-fetcher-image", + "resource-info-fetcher-image", + &cluster_info(), + ) + .expect("build succeeds") + }; + + let daemon_set_mode = build_with("DaemonSet"); + assert_eq!( + sorted_names(&daemon_set_mode.daemon_sets), + ["test-opa-server-default"] + ); + assert!(daemon_set_mode.deployments.is_empty()); + + let deployment_mode = build_with("Deployment"); + assert_eq!( + sorted_names(&deployment_mode.deployments), + ["test-opa-server-default"] + ); + assert!(deployment_mode.daemon_sets.is_empty()); + + // Products consume the discovery ConfigMap, so it must not depend on the workload kind. + assert_eq!( + sorted_names(&daemon_set_mode.config_maps), + sorted_names(&deployment_mode.config_maps) + ); + } } diff --git a/rust/operator-binary/src/controller/build/resource/service.rs b/rust/operator-binary/src/controller/build/resource/service.rs index bc49acef..1dfc5f29 100644 --- a/rust/operator-binary/src/controller/build/resource/service.rs +++ b/rust/operator-binary/src/controller/build/resource/service.rs @@ -1,5 +1,6 @@ use std::collections::BTreeMap; +use stackable_opa_operator::crd::OpaRole; use stackable_operator::{ k8s_openapi::api::core::v1::{Service, ServicePort, ServiceSpec}, v2::{ @@ -33,13 +34,24 @@ pub(crate) fn build_server_role_service(cluster: &ValidatedCluster) -> Service { type_: Some(cluster.cluster_config.listener_class.k8s_service_type()), ports: Some(data_service_ports(cluster.is_tls_enabled())), selector: Some(cluster.role_selector().into()), - // This ensures that products (e.g. Trino) on a node always talk to the OPA pod on the - // same node, avoiding cross-node latency. The downside is that if the local OPA pod is - // unavailable, requests fail instead of falling back to another node. - // TODO: Once our minimum supported Kubernetes version is 1.35, use + // Derived from the role's `workloadKind`: + // + // * `Local` for a DaemonSet, so that products (e.g. Trino) on a node always talk to the OPA + // Pod on the same node, avoiding cross-node latency. The downside is that if the local OPA + // Pod is unavailable, requests fail instead of falling back to another node. + // + // * `Cluster` for a Deployment, whose Pods do not cover every node, so node-local routing + // would leave products on Pod-less nodes unable to reach OPA at all. + // + // TODO: In the DaemonSet case, once our minimum supported Kubernetes version is 1.35, use // `trafficDistribution: PreferSameNode` instead, which prefers the local node but // gracefully falls back to other nodes if the local pod is unavailable. - internal_traffic_policy: Some("Local".to_string()), + internal_traffic_policy: Some( + cluster + .role_config(&OpaRole::Server) + .internal_traffic_policy() + .to_string(), + ), ..ServiceSpec::default() }; @@ -223,6 +235,22 @@ mod tests { assert!(!spec.selector.unwrap().contains_key(ROLE_GROUP_LABEL)); } + /// In `Deployment` mode the Pods do not cover every node, so node-local routing would strand + /// products running on Pod-less nodes. The policy has to follow `workloadKind`. + #[test] + fn role_service_traffic_policy_follows_workload_kind() { + let deployment_mode = validated_cluster_from_spec(json!({ + "image": { "productVersion": "1.2.3" }, + "servers": { + "roleConfig": { "workloadKind": "Deployment" }, + "roleGroups": { "default": {} }, + }, + })); + + let spec = build_server_role_service(&deployment_mode).spec.unwrap(); + assert_eq!(spec.internal_traffic_policy.as_deref(), Some("Cluster")); + } + #[test] fn role_service_port_follows_tls() { assert_eq!( diff --git a/rust/operator-binary/src/controller/build/resource/workload/deployment.rs b/rust/operator-binary/src/controller/build/resource/workload/deployment.rs index 99080178..29395e88 100644 --- a/rust/operator-binary/src/controller/build/resource/workload/deployment.rs +++ b/rust/operator-binary/src/controller/build/resource/workload/deployment.rs @@ -10,9 +10,6 @@ use super::*; /// Runs a fixed number of replicas, unlike [`daemonset`](super::daemonset), which covers every /// node. The Pods therefore do not cover every node and the role Service has to route to any of /// them rather than to a node-local one. -// TODO: Remove the `allow` once the workload dispatch calls this. -// Part of https://github.com/stackabletech/opa-operator/issues/525. -#[allow(dead_code)] #[allow(clippy::too_many_arguments)] pub fn build_server_rolegroup_deployment( cluster: &ValidatedCluster, diff --git a/rust/operator-binary/src/controller/mod.rs b/rust/operator-binary/src/controller/mod.rs index 0dfe41b6..bfb19c0e 100644 --- a/rust/operator-binary/src/controller/mod.rs +++ b/rust/operator-binary/src/controller/mod.rs @@ -16,7 +16,7 @@ use stackable_operator::{ resources::{NoRuntimeLimits, Resources}, }, k8s_openapi::api::{ - apps::v1::DaemonSet, + apps::v1::{DaemonSet, Deployment}, core::v1::{ConfigMap, Service, ServiceAccount}, rbac::v1::RoleBinding, }, @@ -103,9 +103,6 @@ impl ValidatedCluster { /// /// The validate step inserts an entry for every [`OpaRole`], falling back to the /// `OpaRoleConfig` default for roles the user did not configure. - // TODO: Remove the `allow` once the workload dispatch and the Service and PodDisruptionBudget - // builders call this. Part of https://github.com/stackabletech/opa-operator/issues/525. - #[allow(dead_code)] pub fn role_config(&self, role: &OpaRole) -> &v1alpha2::OpaRoleConfig { self.role_configs .get(role) @@ -255,12 +252,14 @@ impl KubeResource for ValidatedCluster { /// Every Kubernetes resource produced by the [`build`](build::build) step. /// -/// OPA runs as a `DaemonSet` (one Pod per node), so there are no `StatefulSet`s, PDBs or -/// `Listener`s. `services` holds the role-level `Service` and the per-role-group headless and -/// metrics `Service`s; `config_maps` holds the per-role-group `ConfigMap`s and the cluster-level -/// discovery `ConfigMap`. +/// Each role group might run as either a `DaemonSet` or a `Deployment`, depending on its role's +/// `workloadKind`, so exactly one of `daemon_sets` and `deployments` holds an entry for it. There +/// are no `StatefulSet`s or `Listener`s. `services` holds the role-level `Service` and the +/// per-role-group headless and metrics `Service`s; `config_maps` holds the per-role-group +/// `ConfigMap`s and the cluster-level discovery `ConfigMap`. pub struct KubernetesResources { pub daemon_sets: Vec, + pub deployments: Vec, pub services: Vec, pub config_maps: Vec, pub service_accounts: Vec, diff --git a/rust/operator-binary/src/controller/validate.rs b/rust/operator-binary/src/controller/validate.rs index f79ba2d6..4218515d 100644 --- a/rust/operator-binary/src/controller/validate.rs +++ b/rust/operator-binary/src/controller/validate.rs @@ -172,7 +172,7 @@ pub fn validate( // Carried per role rather than per cluster, so a second role could pick its own // `workloadKind`. `serde(default)` on `Role::role_config` means this is the - // `OpaRoleConfig` default when the user configured no `roleConfig` at all. + // `OpaRoleConfig` default. role_configs.insert(opa_role.clone(), role.role_config.clone()); let mut group_configs = BTreeMap::new(); @@ -215,7 +215,8 @@ pub fn validate( group_configs.insert( role_group_name, OpaRoleGroupConfig { - // Unused for a DaemonSet, but the `RoleGroupConfig` type requires it. + // Only used in `Deployment` mode; a DaemonSet derives its Pod count from the + // number of nodes. replicas: merged.replicas, config: ValidatedOpaConfig::from_merged(merged.config.config, logging), config_overrides: merged.config.config_overrides, diff --git a/rust/operator-binary/src/crd/mod.rs b/rust/operator-binary/src/crd/mod.rs index cabed1b8..d2df343b 100644 --- a/rust/operator-binary/src/crd/mod.rs +++ b/rust/operator-binary/src/crd/mod.rs @@ -372,8 +372,8 @@ impl v1alpha2::CurrentlySupportedListenerClasses { } } -// TODO: Remove the `allow` once the Deployment and PodDisruptionBudget builders call these. -// This change is the CRD half of https://github.com/stackabletech/opa-operator/issues/525. +// TODO: Remove the `allow` once the PodDisruptionBudget builder calls +// `pod_disruption_budget_enabled`. Part of https://github.com/stackabletech/opa-operator/issues/525. #[allow(dead_code)] impl v1alpha2::OpaRoleConfig { /// The `internalTrafficPolicy` to write into the role Service. @@ -539,6 +539,15 @@ mod tests { serde_json::to_value(v1alpha2::InternalTrafficPolicy::Cluster).unwrap(), json!("Cluster") ); + + // The Service builder writes the policy via `Display`, which is derived by strum and does + // not honour `#[serde(rename_all)]`. Asserted separately, so renaming a variant cannot + // leave serde green while the Service gets a value Kubernetes rejects. + assert_eq!(v1alpha2::InternalTrafficPolicy::Local.to_string(), "Local"); + assert_eq!( + v1alpha2::InternalTrafficPolicy::Cluster.to_string(), + "Cluster" + ); } impl RoundtripTestData for v1alpha1::OpaClusterSpec { diff --git a/rust/operator-binary/src/main.rs b/rust/operator-binary/src/main.rs index f15de158..e1469214 100644 --- a/rust/operator-binary/src/main.rs +++ b/rust/operator-binary/src/main.rs @@ -13,7 +13,7 @@ use stackable_operator::{ client, eos::EndOfSupportChecker, k8s_openapi::api::{ - apps::v1::DaemonSet, + apps::v1::{DaemonSet, Deployment}, core::v1::{ConfigMap, Service}, }, kube::{ @@ -147,6 +147,13 @@ async fn main() -> anyhow::Result<()> { watch_namespace.get_api::>(&client), watcher::Config::default(), ) + // Watched alongside DaemonSets, because a role group runs as either kind. Without + // this the cluster's `Available` condition would not follow a Deployment's Pods + // becoming ready or unready. + .owns( + watch_namespace.get_api::>(&client), + watcher::Config::default(), + ) .owns( watch_namespace.get_api::>(&client), watcher::Config::default(), diff --git a/rust/operator-binary/src/opa_controller.rs b/rust/operator-binary/src/opa_controller.rs index 374ad100..22abebca 100644 --- a/rust/operator-binary/src/opa_controller.rs +++ b/rust/operator-binary/src/opa_controller.rs @@ -16,7 +16,7 @@ use stackable_operator::{ shared::time::Duration, status::condition::{ compute_conditions, daemonset::DaemonSetConditionBuilder, - operations::ClusterOperationsConditionBuilder, + deployment::DeploymentConditionBuilder, operations::ClusterOperationsConditionBuilder, }, utils::cluster_info::KubernetesClusterInfo, v2::cluster_resources::cluster_resources_new, @@ -123,9 +123,10 @@ pub async fn reconcile_opa( .context(BuildResourcesSnafu)?; let mut ds_cond_builder = DaemonSetConditionBuilder::default(); + let mut deployment_cond_builder = DeploymentConditionBuilder::default(); - // Apply order: DaemonSets last, so a changed mounted ConfigMap already exists before the Pods - // (that would otherwise restart) are updated (commons-operator#111). + // Apply order: the workload objects last, so a changed mounted ConfigMap already exists before + // the Pods (that would otherwise restart) are updated (commons-operator#111). for service_account in resources.service_accounts { cluster_resources .add(client, service_account) @@ -182,11 +183,27 @@ pub async fn reconcile_opa( })?; } + for deployment in resources.deployments { + deployment_cond_builder.add( + cluster_resources + .add(client, deployment) + .await + .context(ApplyResourceSnafu)?, + ); + } + let cluster_operation_cond_builder = ClusterOperationsConditionBuilder::new(&opa.spec.cluster_operation); let status = OpaClusterStatus { - conditions: compute_conditions(opa, &[&ds_cond_builder, &cluster_operation_cond_builder]), + conditions: compute_conditions( + opa, + &[ + &ds_cond_builder, + &deployment_cond_builder, + &cluster_operation_cond_builder, + ], + ), }; client From a12b42a43615badd9221840c9a4f7c174a93d9e7 Mon Sep 17 00:00:00 2001 From: Maxi Wittich Date: Tue, 11 Aug 2026 11:24:23 +0200 Subject: [PATCH 07/22] Better changelog.md for now --- CHANGELOG.md | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 59e823b2..ecd08b2b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,9 +12,7 @@ All notable changes to this project will be documented in this file. The API (especially the response) might change in the future once more data catalogs are supported ([#863]). - Allow specifying the maximum number of cached entries in the user-info-fetcher ([#863]). - The `servers` role can now run as a `Deployment` instead of a `DaemonSet`, selected via - `spec.servers.roleConfig.workloadKind`. `DaemonSet` stays the default, so existing installations are - unchanged. In `Deployment` mode the role group's `replicas` is respected, and the role Service uses - `internalTrafficPolicy: Cluster` instead of `Local`, because the Pods no longer cover every node ([#525]). + `spec.servers.roleConfig.workloadKind`. ([#525]). ### Changed From e5d5a969594e8ca21c723ab46074a4ca754efa9e Mon Sep 17 00:00:00 2001 From: Maxi Wittich Date: Tue, 11 Aug 2026 11:26:44 +0200 Subject: [PATCH 08/22] Correct PR reference in changelog.md --- CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ecd08b2b..d533f4fd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,7 @@ All notable changes to this project will be documented in this file. The API (especially the response) might change in the future once more data catalogs are supported ([#863]). - Allow specifying the maximum number of cached entries in the user-info-fetcher ([#863]). - The `servers` role can now run as a `Deployment` instead of a `DaemonSet`, selected via - `spec.servers.roleConfig.workloadKind`. ([#525]). + `spec.servers.roleConfig.workloadKind`. ([#873]). ### Changed @@ -29,7 +29,7 @@ All notable changes to this project will be documented in this file. which could cause problems with GitOps tools (e.g. ArgoCD) reporting a diff in the custom resources. See [our internal issue](https://github.com/stackabletech/hdfs-operator/issues/626) and [the fix](https://github.com/kube-rs/kube/pull/2042) for details ([#871]). -[#525]: https://github.com/stackabletech/opa-operator/issues/525 +[#873]: https://github.com/stackabletech/opa-operator/pull/873 [#852]: https://github.com/stackabletech/opa-operator/pull/852 [#861]: https://github.com/stackabletech/opa-operator/pull/861 [#863]: https://github.com/stackabletech/opa-operator/pull/863 From fefe5520c3db24f040eed9c00a251d61979d2981 Mon Sep 17 00:00:00 2001 From: Maxi Wittich Date: Tue, 11 Aug 2026 12:21:28 +0200 Subject: [PATCH 09/22] Adding PDBs if deployment is chosen --- CHANGELOG.md | 2 + .../templates/clusterrole-operator.yaml | 13 ++ rust/operator-binary/src/controller/build.rs | 23 ++- .../src/controller/build/resource/mod.rs | 1 + .../src/controller/build/resource/pdb.rs | 170 ++++++++++++++++++ rust/operator-binary/src/controller/mod.rs | 5 + rust/operator-binary/src/crd/mod.rs | 3 - rust/operator-binary/src/opa_controller.rs | 6 + 8 files changed, 219 insertions(+), 4 deletions(-) create mode 100644 rust/operator-binary/src/controller/build/resource/pdb.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index d533f4fd..ba679757 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,8 @@ All notable changes to this project will be documented in this file. - Allow specifying the maximum number of cached entries in the user-info-fetcher ([#863]). - The `servers` role can now run as a `Deployment` instead of a `DaemonSet`, selected via `spec.servers.roleConfig.workloadKind`. ([#873]). +- A `PodDisruptionBudget` is now written out for the `servers` role when it runs as a `Deployment`, + with `maxUnavailable: 1`. Configurable via `spec.servers.roleConfig.podDisruptionBudget` ([#873]). ### Changed diff --git a/deploy/helm/opa-operator/templates/clusterrole-operator.yaml b/deploy/helm/opa-operator/templates/clusterrole-operator.yaml index fd7dc604..f44eea59 100644 --- a/deploy/helm/opa-operator/templates/clusterrole-operator.yaml +++ b/deploy/helm/opa-operator/templates/clusterrole-operator.yaml @@ -77,6 +77,19 @@ rules: - list - patch - watch + # PodDisruptionBudget created per role, when the role has it enabled. Also needs `delete`, because + # disabling it (or switching to a DaemonSet) must clean the existing budget up. + - apiGroups: + - policy + resources: + - poddisruptionbudgets + verbs: + - create + - delete + - get + - list + - patch + - watch # Required for maintaining the CRDs within the operator (including the conversion webhook info). # Also for the startup condition check before the controller can run. - apiGroups: diff --git a/rust/operator-binary/src/controller/build.rs b/rust/operator-binary/src/controller/build.rs index fc470952..ebe8791a 100644 --- a/rust/operator-binary/src/controller/build.rs +++ b/rust/operator-binary/src/controller/build.rs @@ -16,6 +16,7 @@ use crate::controller::{ build::resource::{ config_map::build_rolegroup_config_map, discovery::build_discovery_config_map, + pdb::build_role_pod_disruption_budget, rbac::{build_role_binding, build_service_account}, service::{ build_rolegroup_headless_service, build_rolegroup_metrics_service, @@ -75,13 +76,21 @@ pub fn build( let mut deployments = vec![]; let mut services = vec![]; let mut config_maps = vec![]; + let mut pod_disruption_budgets = vec![]; // The role-level load-balanced Service, which is not bound to a single role group. services.push(build_server_role_service(cluster)); // Iterating with the role key, because the workload kind is configured per role. for (opa_role, role_group_configs) in &cluster.role_group_configs { - let workload_kind = &cluster.role_config(opa_role).workload_kind; + let role_config = cluster.role_config(opa_role); + let workload_kind = &role_config.workload_kind; + + pod_disruption_budgets.extend(build_role_pod_disruption_budget( + cluster, + opa_role, + role_config, + )); for (role_group_name, role_group) in role_group_configs { config_maps.push( @@ -137,6 +146,7 @@ pub fn build( config_maps, service_accounts: vec![build_service_account(cluster)], role_bindings: vec![build_role_binding(cluster)], + pod_disruption_budgets, }) } @@ -246,6 +256,9 @@ mod tests { sorted_names(&resources.role_bindings), ["test-opa-rolebinding"] ); + // The default `DaemonSet` gets no PodDisruptionBudget, so existing installations gain no + // new object on upgrade. + assert!(resources.pod_disruption_budgets.is_empty()); } /// `workloadKind` decides which workload object a role group gets. Exactly one kind is built, so @@ -284,6 +297,14 @@ mod tests { ); assert!(deployment_mode.daemon_sets.is_empty()); + // One role-level PodDisruptionBudget for a Deployment, none for a DaemonSet whose Pods + // `kubectl drain` skips anyway. + assert!(daemon_set_mode.pod_disruption_budgets.is_empty()); + assert_eq!( + sorted_names(&deployment_mode.pod_disruption_budgets), + ["test-opa-server"] + ); + // Products consume the discovery ConfigMap, so it must not depend on the workload kind. assert_eq!( sorted_names(&daemon_set_mode.config_maps), diff --git a/rust/operator-binary/src/controller/build/resource/mod.rs b/rust/operator-binary/src/controller/build/resource/mod.rs index 51581d51..baefdf6e 100644 --- a/rust/operator-binary/src/controller/build/resource/mod.rs +++ b/rust/operator-binary/src/controller/build/resource/mod.rs @@ -3,6 +3,7 @@ pub mod config_map; pub mod discovery; +pub mod pdb; pub mod rbac; pub mod service; pub mod workload; diff --git a/rust/operator-binary/src/controller/build/resource/pdb.rs b/rust/operator-binary/src/controller/build/resource/pdb.rs new file mode 100644 index 00000000..fbeb5b72 --- /dev/null +++ b/rust/operator-binary/src/controller/build/resource/pdb.rs @@ -0,0 +1,170 @@ +//! Builds the [`PodDisruptionBudget`] that limits how many OPA Pods of a role a voluntary +//! disruption (a node drain, say) may take down at once. + +use stackable_opa_operator::crd::{OpaRole, v1alpha2}; +use stackable_operator::{ + k8s_openapi::api::policy::v1::PodDisruptionBudget, + v2::builder::pdb::pod_disruption_budget_builder_with_role, +}; + +use crate::controller::{ValidatedCluster, controller_name, operator_name, product_name}; + +/// How many Pods of a role may be unavailable when the administrator configured no +/// `maxUnavailable`. +const DEFAULT_MAX_UNAVAILABLE: u16 = 1; + +/// The role-level [`PodDisruptionBudget`], or `None` when the role has it disabled. +/// +/// One per role rather than per role group, because the budget selects on the role's labels and so +/// covers every role group of that role at once. +pub fn build_role_pod_disruption_budget( + cluster: &ValidatedCluster, + opa_role: &OpaRole, + role_config: &v1alpha2::OpaRoleConfig, +) -> Option { + if !role_config.pod_disruption_budget_enabled() { + return None; + } + + let max_unavailable = role_config + .pod_disruption_budget + .max_unavailable + .unwrap_or(DEFAULT_MAX_UNAVAILABLE); + + Some( + pod_disruption_budget_builder_with_role( + cluster, + &product_name(), + &opa_role.clone().into(), + &operator_name(), + &controller_name(), + ) + .with_max_unavailable(max_unavailable) + .build(), + ) +} + +#[cfg(test)] +mod tests { + use serde_json::json; + + use super::*; + use crate::controller::build::properties::test_support::validated_cluster_from_spec; + + fn build(spec: serde_json::Value) -> Option { + let cluster = validated_cluster_from_spec(spec); + build_role_pod_disruption_budget( + &cluster, + &OpaRole::Server, + cluster.role_config(&OpaRole::Server), + ) + } + + /// A DaemonSet covers every node and `kubectl drain` skips its Pods, so a budget would protect + /// nothing. This is the default, so existing installations gain no new object. + #[test] + fn daemonset_gets_no_pod_disruption_budget() { + assert!( + build(json!({ + "image": { "productVersion": "1.2.3" }, + "servers": { "roleGroups": { "default": {} } }, + })) + .is_none() + ); + } + + /// A Deployment's Pods are evictable, so the budget is created by default. + #[test] + fn deployment_gets_a_pod_disruption_budget_selecting_the_whole_role() { + let pdb = build(json!({ + "image": { "productVersion": "1.2.3" }, + "servers": { + "roleConfig": { "workloadKind": "Deployment" }, + "roleGroups": { "default": {}, "other": {} }, + }, + })) + .expect("a Deployment is protected by default"); + + assert_eq!(pdb.metadata.name.as_deref(), Some("test-opa-server")); + let spec = pdb.spec.expect("the builder always sets a spec"); + assert_eq!( + spec.max_unavailable, + Some( + stackable_operator::k8s_openapi::apimachinery::pkg::util::intstr::IntOrString::Int( + 1 + ) + ) + ); + // Selecting the role rather than a role group is what lets one budget cover both role + // groups configured above. + let match_labels = spec + .selector + .and_then(|selector| selector.match_labels) + .expect("the builder always sets a role selector"); + assert_eq!( + match_labels + .get("app.kubernetes.io/component") + .map(String::as_str), + Some("server") + ); + assert!(!match_labels.contains_key("app.kubernetes.io/role-group")); + } + + /// `enabled` is an explicit override, so a DaemonSet gets a budget when the administrator asks + /// for one, even though it will not do much. + #[test] + fn explicitly_enabling_it_wins_over_the_workload_kind_default() { + assert!( + build(json!({ + "image": { "productVersion": "1.2.3" }, + "servers": { + "roleConfig": { "podDisruptionBudget": { "enabled": true } }, + "roleGroups": { "default": {} }, + }, + })) + .is_some() + ); + } + + /// ...and disabling it opts a Deployment out. + #[test] + fn explicitly_disabling_it_opts_a_deployment_out() { + assert!( + build(json!({ + "image": { "productVersion": "1.2.3" }, + "servers": { + "roleConfig": { + "workloadKind": "Deployment", + "podDisruptionBudget": { "enabled": false }, + }, + "roleGroups": { "default": {} }, + }, + })) + .is_none() + ); + } + + #[test] + fn configured_max_unavailable_overrides_the_default() { + let pdb = build(json!({ + "image": { "productVersion": "1.2.3" }, + "servers": { + "roleConfig": { + "workloadKind": "Deployment", + "podDisruptionBudget": { "maxUnavailable": 2 }, + }, + "roleGroups": { "default": {} }, + }, + })) + .expect("a Deployment is protected by default"); + + assert_eq!( + pdb.spec.unwrap().max_unavailable, + Some( + stackable_operator::k8s_openapi::apimachinery::pkg::util::intstr::IntOrString::Int( + 2 + ) + ) + ); + } +} diff --git a/rust/operator-binary/src/controller/mod.rs b/rust/operator-binary/src/controller/mod.rs index bfb19c0e..37e283ba 100644 --- a/rust/operator-binary/src/controller/mod.rs +++ b/rust/operator-binary/src/controller/mod.rs @@ -18,6 +18,7 @@ use stackable_operator::{ k8s_openapi::api::{ apps::v1::{DaemonSet, Deployment}, core::v1::{ConfigMap, Service, ServiceAccount}, + policy::v1::PodDisruptionBudget, rbac::v1::RoleBinding, }, kube::{Resource as KubeResource, api::ObjectMeta}, @@ -257,6 +258,9 @@ impl KubeResource for ValidatedCluster { /// are no `StatefulSet`s or `Listener`s. `services` holds the role-level `Service` and the /// per-role-group headless and metrics `Service`s; `config_maps` holds the per-role-group /// `ConfigMap`s and the cluster-level discovery `ConfigMap`. +/// +/// `pod_disruption_budgets` holds at most one entry per role, and is empty for roles that have it +/// disabled (the default for a `DaemonSet`). pub struct KubernetesResources { pub daemon_sets: Vec, pub deployments: Vec, @@ -264,6 +268,7 @@ pub struct KubernetesResources { pub config_maps: Vec, pub service_accounts: Vec, pub role_bindings: Vec, + pub pod_disruption_budgets: Vec, } /// Cluster-wide settings resolved once during validation, so the build steps no longer need the diff --git a/rust/operator-binary/src/crd/mod.rs b/rust/operator-binary/src/crd/mod.rs index d2df343b..df27f94c 100644 --- a/rust/operator-binary/src/crd/mod.rs +++ b/rust/operator-binary/src/crd/mod.rs @@ -372,9 +372,6 @@ impl v1alpha2::CurrentlySupportedListenerClasses { } } -// TODO: Remove the `allow` once the PodDisruptionBudget builder calls -// `pod_disruption_budget_enabled`. Part of https://github.com/stackabletech/opa-operator/issues/525. -#[allow(dead_code)] impl v1alpha2::OpaRoleConfig { /// The `internalTrafficPolicy` to write into the role Service. /// diff --git a/rust/operator-binary/src/opa_controller.rs b/rust/operator-binary/src/opa_controller.rs index 22abebca..01c05556 100644 --- a/rust/operator-binary/src/opa_controller.rs +++ b/rust/operator-binary/src/opa_controller.rs @@ -151,6 +151,12 @@ pub async fn reconcile_opa( .await .context(ApplyResourceSnafu)?; } + for pod_disruption_budget in resources.pod_disruption_budgets { + cluster_resources + .add(client, pod_disruption_budget) + .await + .context(ApplyResourceSnafu)?; + } for daemon_set in resources.daemon_sets { ds_cond_builder.add( cluster_resources From a10d47116e5b31272b92f981b7e3b38040f47991 Mon Sep 17 00:00:00 2001 From: Maxi Wittich Date: Tue, 11 Aug 2026 14:36:21 +0200 Subject: [PATCH 10/22] Adding soft affinities to deployment --- CHANGELOG.md | 2 + .../build/resource/workload/deployment.rs | 30 +++++ .../src/controller/validate.rs | 14 ++- rust/operator-binary/src/crd/affinity.rs | 109 ++++++++++++++++++ rust/operator-binary/src/crd/mod.rs | 12 +- 5 files changed, 157 insertions(+), 10 deletions(-) create mode 100644 rust/operator-binary/src/crd/affinity.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index ba679757..d7a346df 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,8 @@ All notable changes to this project will be documented in this file. ### Changed +- OPA Pods now default to a soft anti-affinity that spreads them across nodes. This is a no-op for a + `DaemonSet`, which already runs one Pod per node, but keeps a `Deployment`'s replicas from being deployed together ([#873]). - Internal operator refactoring: introduce a build() step in the reconciler that assembles all relevant Kubernetes resources before anything is applied ([#852]). - Bump `stackable-operator` to 0.114.0 ([#867]). diff --git a/rust/operator-binary/src/controller/build/resource/workload/deployment.rs b/rust/operator-binary/src/controller/build/resource/workload/deployment.rs index 29395e88..fa78a746 100644 --- a/rust/operator-binary/src/controller/build/resource/workload/deployment.rs +++ b/rust/operator-binary/src/controller/build/resource/workload/deployment.rs @@ -183,4 +183,34 @@ mod tests { Some("default") ); } + + /// Replicas are only worth having if they are deployed on different nodes, so the default anti-affinity + /// has to survive the config merge into the Pod template. + #[test] + fn deployment_pods_are_spread_across_nodes_by_default() { + let deployment = build(&validated_cluster_from_spec(json!({ + "image": { "productVersion": "1.2.3" }, + "servers": { + "roleConfig": { "workloadKind": "Deployment" }, + "roleGroups": { "default": { "replicas": 3 } }, + }, + }))); + + let anti_affinity = deployment + .spec + .and_then(|spec| spec.template.spec) + .and_then(|pod_spec| pod_spec.affinity) + .and_then(|affinity| affinity.pod_anti_affinity) + .expect("the default affinity spreads the role's Pods"); + + let preferred = anti_affinity + .preferred_during_scheduling_ignored_during_execution + .expect("the spread is a soft term"); + assert_eq!(preferred.len(), 1); + assert_eq!(preferred[0].weight, 70); + assert_eq!( + preferred[0].pod_affinity_term.topology_key, + "kubernetes.io/hostname" + ); + } } diff --git a/rust/operator-binary/src/controller/validate.rs b/rust/operator-binary/src/controller/validate.rs index 4218515d..4844c981 100644 --- a/rust/operator-binary/src/controller/validate.rs +++ b/rust/operator-binary/src/controller/validate.rs @@ -180,12 +180,14 @@ pub fn validate( // Merge default <- role <- role group and validate the config fragment, plus merge all // four override kinds (config/env/cli/pod) in one shot. Role group wins over role wins // over defaults. - let merged: RoleGroup = - with_validated_config(role_group, role, &OpaConfig::default_config()).context( - ValidateRoleGroupConfigSnafu { - role_group: role_group_name.clone(), - }, - )?; + let merged: RoleGroup = with_validated_config( + role_group, + role, + &OpaConfig::default_config(&name.to_string(), &opa_role), + ) + .context(ValidateRoleGroupConfigSnafu { + role_group: role_group_name.clone(), + })?; // `envOverrides` is kept as a `HashMap`; lift it into the type-safe // `EnvVarSet` consumed by the build step. diff --git a/rust/operator-binary/src/crd/affinity.rs b/rust/operator-binary/src/crd/affinity.rs new file mode 100644 index 00000000..ac934ba2 --- /dev/null +++ b/rust/operator-binary/src/crd/affinity.rs @@ -0,0 +1,109 @@ +//! The default [`StackableAffinityFragment`] of an OPA role. + +use stackable_operator::{ + commons::affinity::{StackableAffinityFragment, affinity_between_role_pods}, + k8s_openapi::api::core::v1::PodAntiAffinity, +}; + +use crate::crd::{APP_NAME, OpaRole}; + +/// Weight of the anti-affinity that spreads the Pods of a role across nodes. +/// +/// The absolute value only matters once a second, competing term exists; see the `PreferSameNode` +/// note on [`get_affinity`]. +const ANTI_AFFINITY_BETWEEN_ROLE_PODS_WEIGHT: i32 = 70; + +/// The default affinity of `role`: prefer to spread its Pods across nodes. +/// +/// Soft (`preferred`), so it can never leave a Pod unschedulable, and inert for a `DaemonSet`, which +/// already places exactly one Pod per node. It matters in `Deployment` mode only. +// +// TODO: Revisit once our minimum supported Kubernetes version is 1.35 and the role Service can use +// `trafficDistribution: PreferSameNode` instead of `internalTrafficPolicy` (see +// `controller::build::resource::service`). +// +// The chance: with node-local routing that degrades gracefully, an affinity attracting OPA Pods +// towards the products that query them would genuinely pay off, because traffic would prefer a +// node-local OPA Pod without the current risk of failing outright when there is none. +// +// The concerns: +// +// * `PreferSameNode` falls back to other nodes only when there is no *ready* local endpoint, never +// because the local one is busy. A request-heavy client (Trino, Kafka, depending on their +// config) would keep hitting its local Pod while the others idle. +// +// * Field experience points the other way: spreading the load across Pods outperformed avoiding the +// network hop by a wide margin. +// +// * The scheduler scores `podAffinity` and `podAntiAffinity` on one scale, so the two weights would +// compete. Keeping this one at 70 above a lower attraction weight encodes "spreading wins", where +// equal weights would cancel out. +pub fn get_affinity(cluster_name: &str, role: &OpaRole) -> StackableAffinityFragment { + StackableAffinityFragment { + pod_affinity: None, + pod_anti_affinity: Some(PodAntiAffinity { + preferred_during_scheduling_ignored_during_execution: Some(vec![ + affinity_between_role_pods( + APP_NAME, + cluster_name, + &role.to_string(), + ANTI_AFFINITY_BETWEEN_ROLE_PODS_WEIGHT, + ), + ]), + required_during_scheduling_ignored_during_execution: None, + }), + node_affinity: None, + node_selector: None, + } +} + +#[cfg(test)] +mod tests { + use stackable_operator::k8s_openapi::{ + api::core::v1::{PodAffinityTerm, WeightedPodAffinityTerm}, + apimachinery::pkg::apis::meta::v1::LabelSelector, + }; + + use super::*; + + /// Locks the shape of the default: a soft, per-node anti-affinity selecting the whole role + /// (so across every role group), which is what makes replicas spread instead of piling up. + #[test] + fn default_affinity_spreads_the_role_across_nodes() { + let affinity = get_affinity("simple-opa", &OpaRole::Server); + + assert_eq!(affinity.pod_affinity, None); + assert_eq!(affinity.node_affinity, None); + assert_eq!(affinity.node_selector, None); + + let anti_affinity = affinity.pod_anti_affinity.expect("is always set"); + // Soft only: a `required` term would leave Pods Pending once the replica count exceeds the + // number of schedulable nodes. + assert_eq!( + anti_affinity.required_during_scheduling_ignored_during_execution, + None + ); + assert_eq!( + anti_affinity.preferred_during_scheduling_ignored_during_execution, + Some(vec![WeightedPodAffinityTerm { + weight: ANTI_AFFINITY_BETWEEN_ROLE_PODS_WEIGHT, + pod_affinity_term: PodAffinityTerm { + label_selector: Some(LabelSelector { + match_expressions: None, + match_labels: Some( + [ + ("app.kubernetes.io/name", "opa"), + ("app.kubernetes.io/instance", "simple-opa"), + ("app.kubernetes.io/component", "server"), + ] + .map(|(key, value)| (key.to_string(), value.to_string())) + .into() + ), + }), + topology_key: "kubernetes.io/hostname".to_string(), + ..PodAffinityTerm::default() + }, + }]) + ); + } +} diff --git a/rust/operator-binary/src/crd/mod.rs b/rust/operator-binary/src/crd/mod.rs index df27f94c..d2bd1d4b 100644 --- a/rust/operator-binary/src/crd/mod.rs +++ b/rust/operator-binary/src/crd/mod.rs @@ -32,6 +32,7 @@ use stackable_operator::{ }; use strum::{Display, EnumIter, EnumString}; +pub mod affinity; pub mod cache; pub mod resource_info_fetcher; pub mod user_info_fetcher; @@ -399,7 +400,9 @@ impl v1alpha2::OpaRoleConfig { } impl OpaConfig { - pub fn default_config() -> OpaConfigFragment { + /// `cluster_name` and `role` are needed for the default affinity, whose selector is specific to + /// this cluster's role rather than a static value. + pub fn default_config(cluster_name: &str, role: &OpaRole) -> OpaConfigFragment { OpaConfigFragment { logging: product_logging::spec::default_logging(), resources: ResourcesFragment { @@ -413,9 +416,10 @@ impl OpaConfig { }, storage: OpaStorageConfigFragment {}, }, - // There is no point in having a default affinity, as exactly one OPA Pods should run on every node. - // We only have the affinity configurable to let users limit the nodes the OPA Pods run on. - affinity: Default::default(), + // Spreads the role's Pods across nodes. A no-op for a DaemonSet, which already runs + // exactly one Pod per node, but it is what keeps a Deployment's replicas from landing + // together. See `affinity::get_affinity`. + affinity: affinity::get_affinity(cluster_name, role), graceful_shutdown_timeout: Some(DEFAULT_SERVER_GRACEFUL_SHUTDOWN_TIMEOUT), } } From 53dab8422dbeb460b2be5a23c64333162fb3d0a7 Mon Sep 17 00:00:00 2001 From: Maxi Wittich Date: Tue, 11 Aug 2026 16:52:48 +0200 Subject: [PATCH 11/22] Adding one test for opa deployment switch and PDBs --- tests/templates/kuttl/smoke/12-assert.yaml.j2 | 6 ++- .../kuttl/workload-kind/00-assert.yaml.j2 | 10 ++++ ...tor-aggregator-discovery-configmap.yaml.j2 | 9 ++++ .../kuttl/workload-kind/00-patch-ns.yaml.j2 | 9 ++++ .../kuttl/workload-kind/10-assert.yaml | 21 ++++++++ .../kuttl/workload-kind/10-errors.yaml | 13 +++++ .../workload-kind/10-install-opa.yaml.j2 | 30 ++++++++++++ .../kuttl/workload-kind/20-assert.yaml | 49 +++++++++++++++++++ .../kuttl/workload-kind/20-errors.yaml | 8 +++ .../20-switch-to-deployment.yaml.j2 | 28 +++++++++++ .../kuttl/workload-kind/30-assert.yaml | 23 +++++++++ .../kuttl/workload-kind/30-errors.yaml | 14 ++++++ .../30-switch-back-to-daemonset.yaml.j2 | 30 ++++++++++++ tests/test-definition.yaml | 6 +++ 14 files changed, 254 insertions(+), 2 deletions(-) create mode 100644 tests/templates/kuttl/workload-kind/00-assert.yaml.j2 create mode 100644 tests/templates/kuttl/workload-kind/00-install-vector-aggregator-discovery-configmap.yaml.j2 create mode 100644 tests/templates/kuttl/workload-kind/00-patch-ns.yaml.j2 create mode 100644 tests/templates/kuttl/workload-kind/10-assert.yaml create mode 100644 tests/templates/kuttl/workload-kind/10-errors.yaml create mode 100644 tests/templates/kuttl/workload-kind/10-install-opa.yaml.j2 create mode 100644 tests/templates/kuttl/workload-kind/20-assert.yaml create mode 100644 tests/templates/kuttl/workload-kind/20-errors.yaml create mode 100644 tests/templates/kuttl/workload-kind/20-switch-to-deployment.yaml.j2 create mode 100644 tests/templates/kuttl/workload-kind/30-assert.yaml create mode 100644 tests/templates/kuttl/workload-kind/30-errors.yaml create mode 100644 tests/templates/kuttl/workload-kind/30-switch-back-to-daemonset.yaml.j2 diff --git a/tests/templates/kuttl/smoke/12-assert.yaml.j2 b/tests/templates/kuttl/smoke/12-assert.yaml.j2 index 12971976..452084f5 100644 --- a/tests/templates/kuttl/smoke/12-assert.yaml.j2 +++ b/tests/templates/kuttl/smoke/12-assert.yaml.j2 @@ -8,8 +8,10 @@ # only here — `.data` is asserted in 13-assert), ServiceAccount, RoleBinding. # # Catches drift in labels, owner references, selectors, ports, probe schemes, -# update strategy, container resources and TLS-dependent fields. The operator -# does not create a PodDisruptionBudget for OPA, so none is asserted here. +# update strategy, container resources and TLS-dependent fields. This cluster +# uses the default `workloadKind: DaemonSet`, for which the operator writes no +# PodDisruptionBudget, so none is asserted here; the `workload-kind` test covers +# the `Deployment` case. # # `app.kubernetes.io/version` is intentionally omitted from label matchers so # that product-version bumps in test-definition.yaml don't force snapshot diff --git a/tests/templates/kuttl/workload-kind/00-assert.yaml.j2 b/tests/templates/kuttl/workload-kind/00-assert.yaml.j2 new file mode 100644 index 00000000..50b1d4c3 --- /dev/null +++ b/tests/templates/kuttl/workload-kind/00-assert.yaml.j2 @@ -0,0 +1,10 @@ +--- +apiVersion: kuttl.dev/v1beta1 +kind: TestAssert +{% if lookup('env', 'VECTOR_AGGREGATOR') %} +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: vector-aggregator-discovery +{% endif %} diff --git a/tests/templates/kuttl/workload-kind/00-install-vector-aggregator-discovery-configmap.yaml.j2 b/tests/templates/kuttl/workload-kind/00-install-vector-aggregator-discovery-configmap.yaml.j2 new file mode 100644 index 00000000..2d6a0df5 --- /dev/null +++ b/tests/templates/kuttl/workload-kind/00-install-vector-aggregator-discovery-configmap.yaml.j2 @@ -0,0 +1,9 @@ +{% if lookup('env', 'VECTOR_AGGREGATOR') %} +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: vector-aggregator-discovery +data: + ADDRESS: {{ lookup('env', 'VECTOR_AGGREGATOR') }} +{% endif %} diff --git a/tests/templates/kuttl/workload-kind/00-patch-ns.yaml.j2 b/tests/templates/kuttl/workload-kind/00-patch-ns.yaml.j2 new file mode 100644 index 00000000..67185acf --- /dev/null +++ b/tests/templates/kuttl/workload-kind/00-patch-ns.yaml.j2 @@ -0,0 +1,9 @@ +{% if test_scenario['values']['openshift'] == 'true' %} +# see https://github.com/stackabletech/issues/issues/566 +--- +apiVersion: kuttl.dev/v1beta1 +kind: TestStep +commands: + - script: kubectl patch namespace $NAMESPACE -p '{"metadata":{"labels":{"pod-security.kubernetes.io/enforce":"privileged"}}}' + timeout: 120 +{% endif %} diff --git a/tests/templates/kuttl/workload-kind/10-assert.yaml b/tests/templates/kuttl/workload-kind/10-assert.yaml new file mode 100644 index 00000000..746ba3a2 --- /dev/null +++ b/tests/templates/kuttl/workload-kind/10-assert.yaml @@ -0,0 +1,21 @@ +--- +# The `DaemonSet` default: unchanged behaviour for existing installations. +apiVersion: kuttl.dev/v1beta1 +kind: TestAssert +timeout: 600 +commands: + - script: kubectl -n $NAMESPACE rollout status daemonset test-opa-server-default --timeout 600s + - script: kubectl -n $NAMESPACE wait --for=condition=available opaclusters.opa.stackable.tech/test-opa --timeout 600s +--- +apiVersion: apps/v1 +kind: DaemonSet +metadata: + name: test-opa-server-default +--- +# Node-local routing, which is only safe because a DaemonSet covers every node. +apiVersion: v1 +kind: Service +metadata: + name: test-opa-server +spec: + internalTrafficPolicy: Local diff --git a/tests/templates/kuttl/workload-kind/10-errors.yaml b/tests/templates/kuttl/workload-kind/10-errors.yaml new file mode 100644 index 00000000..2185d571 --- /dev/null +++ b/tests/templates/kuttl/workload-kind/10-errors.yaml @@ -0,0 +1,13 @@ +--- +# A DaemonSet's Pods are skipped by `kubectl drain`, so a PodDisruptionBudget would protect nothing +# and none is written out. +apiVersion: policy/v1 +kind: PodDisruptionBudget +metadata: + name: test-opa-server +--- +# Nothing has asked for a Deployment yet. +apiVersion: apps/v1 +kind: Deployment +metadata: + name: test-opa-server-default diff --git a/tests/templates/kuttl/workload-kind/10-install-opa.yaml.j2 b/tests/templates/kuttl/workload-kind/10-install-opa.yaml.j2 new file mode 100644 index 00000000..37e89006 --- /dev/null +++ b/tests/templates/kuttl/workload-kind/10-install-opa.yaml.j2 @@ -0,0 +1,30 @@ +--- +apiVersion: opa.stackable.tech/v1alpha2 +kind: OpaCluster +metadata: + name: test-opa +spec: + image: +{% if test_scenario['values']['opa-latest'].find(",") > 0 %} + custom: "{{ test_scenario['values']['opa-latest'].split(',')[1] }}" + productVersion: "{{ test_scenario['values']['opa-latest'].split(',')[0] }}" +{% else %} + productVersion: "{{ test_scenario['values']['opa-latest'] }}" +{% endif %} + pullPolicy: IfNotPresent +{% if lookup('env', 'VECTOR_AGGREGATOR') %} + clusterConfig: + vectorAggregatorConfigMapName: vector-aggregator-discovery +{% endif %} + servers: + # No `roleConfig`, so `workloadKind` falls back to its `DaemonSet` default. This is the + # pre-upgrade shape, and the following steps switch away from and back to it. + config: + logging: + enableVectorAgent: {{ lookup('env', 'VECTOR_AGGREGATOR') | length > 0 }} + roleGroups: + default: + # Set from the start so that switching `workloadKind` below is the only change between the + # steps. A DaemonSet has no `spec.replicas` at all, so this is inert here -- it becomes + # meaningful in `20-switch-to-deployment`. + replicas: 2 diff --git a/tests/templates/kuttl/workload-kind/20-assert.yaml b/tests/templates/kuttl/workload-kind/20-assert.yaml new file mode 100644 index 00000000..49042d86 --- /dev/null +++ b/tests/templates/kuttl/workload-kind/20-assert.yaml @@ -0,0 +1,49 @@ +--- +# Switching `workloadKind` to `Deployment` swaps the workload object, widens the role Service's +# traffic policy and adds a PodDisruptionBudget. +apiVersion: kuttl.dev/v1beta1 +kind: TestAssert +timeout: 180 +commands: + - script: kubectl -n $NAMESPACE rollout status deployment test-opa-server-default --timeout 181s + - script: kubectl -n $NAMESPACE wait --for=condition=available opaclusters.opa.stackable.tech/test-opa --timeout 181s +--- +apiVersion: apps/v1 +kind: Deployment +metadata: + name: test-opa-server-default +spec: + # Unlike a DaemonSet, a Deployment takes the role group's `replicas` verbatim. + replicas: 2 +status: + readyReplicas: 2 +--- +# `Local` would strand products on nodes without an OPA Pod, because a Deployment's Pods do not +# cover every node. +apiVersion: v1 +kind: Service +metadata: + name: test-opa-server +spec: + internalTrafficPolicy: Cluster +--- +# Role-level, so it is named after the role rather than the role group. +# +# `status` is asserted, not just `spec`: `currentHealthy` and `disruptionsAllowed` are only populated +# once the disruption controller matches the budget's selector against real Pods. +apiVersion: policy/v1 +kind: PodDisruptionBudget +metadata: + name: test-opa-server +spec: + maxUnavailable: 1 + selector: + matchLabels: + app.kubernetes.io/component: server + app.kubernetes.io/instance: test-opa + app.kubernetes.io/name: opa +status: + currentHealthy: 2 + desiredHealthy: 1 + expectedPods: 2 + disruptionsAllowed: 1 diff --git a/tests/templates/kuttl/workload-kind/20-errors.yaml b/tests/templates/kuttl/workload-kind/20-errors.yaml new file mode 100644 index 00000000..3e9b24d9 --- /dev/null +++ b/tests/templates/kuttl/workload-kind/20-errors.yaml @@ -0,0 +1,8 @@ +--- +# The DaemonSet must be swept once the Deployment takes over. Both workload objects carry the same +# name and the same role labels, so leaving it behind would mean the role Service selects the Pods +# of both. +apiVersion: apps/v1 +kind: DaemonSet +metadata: + name: test-opa-server-default diff --git a/tests/templates/kuttl/workload-kind/20-switch-to-deployment.yaml.j2 b/tests/templates/kuttl/workload-kind/20-switch-to-deployment.yaml.j2 new file mode 100644 index 00000000..8cacf5e6 --- /dev/null +++ b/tests/templates/kuttl/workload-kind/20-switch-to-deployment.yaml.j2 @@ -0,0 +1,28 @@ +--- +apiVersion: opa.stackable.tech/v1alpha2 +kind: OpaCluster +metadata: + name: test-opa +spec: + image: +{% if test_scenario['values']['opa-latest'].find(",") > 0 %} + custom: "{{ test_scenario['values']['opa-latest'].split(',')[1] }}" + productVersion: "{{ test_scenario['values']['opa-latest'].split(',')[0] }}" +{% else %} + productVersion: "{{ test_scenario['values']['opa-latest'] }}" +{% endif %} + pullPolicy: IfNotPresent +{% if lookup('env', 'VECTOR_AGGREGATOR') %} + clusterConfig: + vectorAggregatorConfigMapName: vector-aggregator-discovery +{% endif %} + servers: + roleConfig: + workloadKind: Deployment + config: + logging: + enableVectorAgent: {{ lookup('env', 'VECTOR_AGGREGATOR') | length > 0 }} + roleGroups: + default: + # Now honoured, unlike in the DaemonSet step above. + replicas: 2 diff --git a/tests/templates/kuttl/workload-kind/30-assert.yaml b/tests/templates/kuttl/workload-kind/30-assert.yaml new file mode 100644 index 00000000..6ea4ce49 --- /dev/null +++ b/tests/templates/kuttl/workload-kind/30-assert.yaml @@ -0,0 +1,23 @@ +--- +# Switching back has to be possible, which means every object the Deployment mode added is removed +# again. This is the regression test for the orphan cleanup covering both `Deployment` and +# `PodDisruptionBudget`; an operator-rs that only sweeps DaemonSets would leave them behind and the +# `30-errors.yaml` next to this file would fail. +apiVersion: kuttl.dev/v1beta1 +kind: TestAssert +timeout: 180 +commands: + - script: kubectl -n $NAMESPACE rollout status daemonset test-opa-server-default --timeout 181s + - script: kubectl -n $NAMESPACE wait --for=condition=available opaclusters.opa.stackable.tech/test-opa --timeout 181s +--- +apiVersion: apps/v1 +kind: DaemonSet +metadata: + name: test-opa-server-default +--- +apiVersion: v1 +kind: Service +metadata: + name: test-opa-server +spec: + internalTrafficPolicy: Local diff --git a/tests/templates/kuttl/workload-kind/30-errors.yaml b/tests/templates/kuttl/workload-kind/30-errors.yaml new file mode 100644 index 00000000..c55450c0 --- /dev/null +++ b/tests/templates/kuttl/workload-kind/30-errors.yaml @@ -0,0 +1,14 @@ +--- +# Left behind, the Deployment's Pods would still be selected by the role Service -- which has just +# narrowed back to `internalTrafficPolicy: Local`. +apiVersion: apps/v1 +kind: Deployment +metadata: + name: test-opa-server-default +--- +# A budget over DaemonSet Pods would block node drains while protecting nothing, since `kubectl +# drain` skips them anyway. +apiVersion: policy/v1 +kind: PodDisruptionBudget +metadata: + name: test-opa-server diff --git a/tests/templates/kuttl/workload-kind/30-switch-back-to-daemonset.yaml.j2 b/tests/templates/kuttl/workload-kind/30-switch-back-to-daemonset.yaml.j2 new file mode 100644 index 00000000..74cda7d2 --- /dev/null +++ b/tests/templates/kuttl/workload-kind/30-switch-back-to-daemonset.yaml.j2 @@ -0,0 +1,30 @@ +--- +apiVersion: opa.stackable.tech/v1alpha2 +kind: OpaCluster +metadata: + name: test-opa +spec: + image: +{% if test_scenario['values']['opa-latest'].find(",") > 0 %} + custom: "{{ test_scenario['values']['opa-latest'].split(',')[1] }}" + productVersion: "{{ test_scenario['values']['opa-latest'].split(',')[0] }}" +{% else %} + productVersion: "{{ test_scenario['values']['opa-latest'] }}" +{% endif %} + pullPolicy: IfNotPresent +{% if lookup('env', 'VECTOR_AGGREGATOR') %} + clusterConfig: + vectorAggregatorConfigMapName: vector-aggregator-discovery +{% endif %} + servers: + # Must be spelled out rather than omitted: kuttl applies a step as a merge patch, so an absent + # field means "leave it alone", not "remove it". Dropping this block would keep the + # `workloadKind: Deployment`. + roleConfig: + workloadKind: DaemonSet + config: + logging: + enableVectorAgent: {{ lookup('env', 'VECTOR_AGGREGATOR') | length > 0 }} + roleGroups: + default: + replicas: 2 diff --git a/tests/test-definition.yaml b/tests/test-definition.yaml index a25b1a32..b0933aba 100644 --- a/tests/test-definition.yaml +++ b/tests/test-definition.yaml @@ -60,6 +60,12 @@ tests: dimensions: - opa-latest - openshift + # Deliberately not a dimension of `smoke`: the Pod template is identical for both workload kinds, + # so re-running the whole smoke matrix would double it for no extra coverage. + - name: workload-kind + dimensions: + - opa-latest + - openshift - name: config-overrides dimensions: - opa-latest From 609401474d93952d100c8c64e607897a804ec4b2 Mon Sep 17 00:00:00 2001 From: Maxi Wittich Date: Wed, 12 Aug 2026 08:57:54 +0200 Subject: [PATCH 12/22] Adds docs for PDBs, workloadKind and affinty --- .../operations/pod-disruptions.adoc | 36 +++++++- .../usage-guide/operations/pod-placement.adoc | 33 ++++++++ .../opa/pages/usage-guide/workload-kind.adoc | 84 +++++++++++++++++++ docs/modules/opa/partials/nav.adoc | 3 +- 4 files changed, 152 insertions(+), 4 deletions(-) create mode 100644 docs/modules/opa/pages/usage-guide/operations/pod-placement.adoc create mode 100644 docs/modules/opa/pages/usage-guide/workload-kind.adoc diff --git a/docs/modules/opa/pages/usage-guide/operations/pod-disruptions.adoc b/docs/modules/opa/pages/usage-guide/operations/pod-disruptions.adoc index 69915f83..4a038254 100644 --- a/docs/modules/opa/pages/usage-guide/operations/pod-disruptions.adoc +++ b/docs/modules/opa/pages/usage-guide/operations/pod-disruptions.adoc @@ -1,5 +1,35 @@ = Allowed Pod disruptions +:description: Whether the operator creates a PodDisruptionBudget for OPA depends on the workload kind, and how to configure or disable that budget. -For OPA clusters, the operator does not deploy any PodDisruptionBudgets (PDBs), as there is one instance per Kubernetes node running (Daemonset). -When a Kubernetes node gets drained to gracefully shut it down, the OPA Pod get's evicted - there is no point in blocking the eviction. -In case the OPA Pod terminated before the products depending on OPA (e.g. Trino coordinator) on the same node, the products can still use the OPA Service, as it routes to OPA Pods running on other Kubernetes nodes. +You can configure the allowed Pod disruptions as described in xref:concepts:operations/pod_disruptions.adoc[]. + +What the operator creates depends on the xref:usage-guide/workload-kind.adoc[workload kind] of the role. + +== DaemonSet + +No PodDisruptionBudget is created. +Draining a node has to be able to remove the OPA Pod on that node, and `kubectl drain` skips DaemonSet Pods, means a budget would block maintenance without adding an upside. + +Because the role Service routes node-locally in this mode, products on a drained node cannot reach OPA on another node. + +== Deployment + +The operator creates one PodDisruptionBudget per role, allowing one Pod to be unavailable at a time by default: + +[source,yaml] +---- +spec: + servers: + roleConfig: + podDisruptionBudget: + enabled: true # <1> + maxUnavailable: 1 # <2> +---- +<1> Defaults to true for a Deployment. Set it to false to create no budget. +<2> Defaults to 1. Raise it to allow more Pods to be unavailable at once. + +Keep `maxUnavailable` at 1 unless you have measured that OPA tolerates more. +Products query OPA on every request, budgets that drain too many Pods at once turn a node rotation into a platform-wide slowdown. + +NOTE: A budget only helps when there is another Pod to fall back to. +With `replicas` set to 1, the single Pod can still be evicted. diff --git a/docs/modules/opa/pages/usage-guide/operations/pod-placement.adoc b/docs/modules/opa/pages/usage-guide/operations/pod-placement.adoc new file mode 100644 index 00000000..75d9adc8 --- /dev/null +++ b/docs/modules/opa/pages/usage-guide/operations/pod-placement.adoc @@ -0,0 +1,33 @@ += Pod placement +:description: The default affinity spreads OPA Pods across Kubernetes nodes, which matters for a Deployment, and how to override it with your own affinities. + +You can configure Pod placement for OPA Pods as described in xref:concepts:operations/pod_placement.adoc[]. + +== Defaults + +The default affinity created by the operator is: + +* Distribute all Pods of the `servers` role across nodes, so that multiple Pods don't end up on the same Kubernetes node (weight 70) + +This constrains nothing for a DaemonSet, which already places exactly one Pod per node. +It matters for a Deployment, where several replicas would otherwise be free to share the same node. +See xref:usage-guide/workload-kind.adoc[]. + +[source,yaml] +---- +affinity: + podAntiAffinity: + preferredDuringSchedulingIgnoredDuringExecution: + - podAffinityTerm: + labelSelector: + matchLabels: + app.kubernetes.io/component: server + app.kubernetes.io/instance: cluster-name + app.kubernetes.io/name: opa + topologyKey: kubernetes.io/hostname + weight: 70 +---- + +NOTE: The default affinity is only preferred and not enforced, because not every setup has multiple Kubernetes nodes. +To enforce it, set your own `requiredDuringSchedulingIgnoredDuringExecution` affinity. +Be aware that a Deployment with more replicas than nodes then leaves Pods unschedulable. diff --git a/docs/modules/opa/pages/usage-guide/workload-kind.adoc b/docs/modules/opa/pages/usage-guide/workload-kind.adoc new file mode 100644 index 00000000..b4407df9 --- /dev/null +++ b/docs/modules/opa/pages/usage-guide/workload-kind.adoc @@ -0,0 +1,84 @@ += Workload kind +:description: Run the OPA servers as a DaemonSet or as a Deployment, and learn how that choice affects the Pod count, Service routing and changing it later. + +By default the `servers` role runs as a DaemonSet, which places one OPA Pod on every Kubernetes node. +Set `workloadKind` to Deployment to run a fixed number of Pods instead. + +[source,yaml] +---- +spec: + servers: + roleConfig: + workloadKind: Deployment # <1> + roleGroups: + default: + replicas: 3 # <2> +---- +<1> Either DaemonSet (default) or Deployment. +<2> Only used by Deployment, DaemonSet derives Pod count from the number of nodes. + +The workload kind also decides whether the operator creates a xref:usage-guide/operations/pod-disruptions.adoc[PodDisruptionBudget], and it changes what the default xref:usage-guide/operations/pod-placement.adoc[Pod placement] achieves. + +== Choosing a workload kind + +Use a DaemonSet when every node runs products that query OPA. +Each product then queries the OPA Pod on its own node, so no policy query crosses the network. +The number of OPA Pods grows and shrinks with the node count. + +Use a Deployment when the number of OPA Pods should be fixed. +You set the count with `replicas` and queries are spread across all Pods. +This fits large clusters, and clusters where only a few nodes run products that query OPA. + +== Service routing + +The operator derives the role Service's `internalTrafficPolicy` from the workload kind. + +* A DaemonSet gets `Local`, so a query only reaches the OPA Pod on the client's own node. + This avoids the network hop, and a DaemonSet covers every node, thus such a Pod always exists. +* A Deployment gets `Cluster`, so a query reaches any OPA Pod of the role. + A Deployment's Pods do not cover every node necessarily, so node-local routing would leave products on the remaining nodes unable to reach OPA at all. + +If the derived value doesn't suit your cluster, you can override it as described in <>. + +[#override-traffic-policy] +== Override traffic policy + +In edge cases (e.g. node autoscaling under load), it is useful to use a DaemonSet with `internalTrafficPolicy: Cluster`. +This can be achieved using xref:concepts:overrides.adoc#object-overrides[object overrides]: + +[source,yaml] +---- +apiVersion: opa.stackable.tech/v1alpha2 +kind: OpaCluster +metadata: + name: simple-opa + namespace: default +spec: + image: + productVersion: 1.16.2 + objectOverrides: + - apiVersion: v1 + kind: Service + metadata: + name: simple-opa-server + namespace: default + spec: + internalTrafficPolicy: Cluster # <1> + servers: + roleGroups: + default: {} +---- +<1> Changes `internalTrafficPolicy` from Local to Cluster. + +== Changing the workload kind + +Changing `workloadKind` replaces the workload object, so policy queries can fail while the new Pods start up. +Products usually treat a failed policy query as a denied request. + +Changing to DaemonSet is the more disruptive direction. +The role Service narrows to Local as soon as you apply the change, while the running Pods still belong to the outgoing Deployment and cover only some nodes. +Products on the remaining nodes fail until the DaemonSet has rolled out everywhere. + +To ease the interruption, pin the traffic policy to Cluster as described in <>, and remove the override once the rollout has finished. +This keeps every product able to reach any OPA Pod throughout the change. +A short window in which no Pod is ready can still occur, because the outgoing workload is removed as the new one starts. diff --git a/docs/modules/opa/partials/nav.adoc b/docs/modules/opa/partials/nav.adoc index 58dfa4df..70b92a10 100644 --- a/docs/modules/opa/partials/nav.adoc +++ b/docs/modules/opa/partials/nav.adoc @@ -3,6 +3,7 @@ ** xref:opa:getting_started/first_steps.adoc[] * xref:opa:usage-guide/index.adoc[] ** xref:opa:usage-guide/listenerclass.adoc[] +** xref:opa:usage-guide/workload-kind.adoc[] ** xref:opa:usage-guide/policies.adoc[] ** xref:opa:usage-guide/user-info-fetcher.adoc[] ** xref:opa:usage-guide/resource-info-fetcher.adoc[] @@ -14,7 +15,7 @@ ** xref:opa:usage-guide/tls.adoc[] ** xref:opa:usage-guide/operations/index.adoc[] *** xref:opa:usage-guide/operations/cluster-operations.adoc[] -// *** xref:hdfs:usage-guide/operations/pod-placement.adoc[] Missing +*** xref:opa:usage-guide/operations/pod-placement.adoc[] *** xref:opa:usage-guide/operations/pod-disruptions.adoc[] *** xref:opa:usage-guide/operations/graceful-shutdown.adoc[] * xref:opa:reference/index.adoc[] From 2c8f6b27b001c8cb6d74a93d08c2d6212bce4a31 Mon Sep 17 00:00:00 2001 From: Maxi Wittich Date: Wed, 12 Aug 2026 10:33:47 +0200 Subject: [PATCH 13/22] Better comment for smoke test --- tests/templates/kuttl/smoke/12-assert.yaml.j2 | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/tests/templates/kuttl/smoke/12-assert.yaml.j2 b/tests/templates/kuttl/smoke/12-assert.yaml.j2 index 452084f5..3e8480fa 100644 --- a/tests/templates/kuttl/smoke/12-assert.yaml.j2 +++ b/tests/templates/kuttl/smoke/12-assert.yaml.j2 @@ -8,10 +8,7 @@ # only here — `.data` is asserted in 13-assert), ServiceAccount, RoleBinding. # # Catches drift in labels, owner references, selectors, ports, probe schemes, -# update strategy, container resources and TLS-dependent fields. This cluster -# uses the default `workloadKind: DaemonSet`, for which the operator writes no -# PodDisruptionBudget, so none is asserted here; the `workload-kind` test covers -# the `Deployment` case. +# update strategy, container resources and TLS-dependent fields. # # `app.kubernetes.io/version` is intentionally omitted from label matchers so # that product-version bumps in test-definition.yaml don't force snapshot From b703b7e35bd10842bf087a9b3fe27c80a26568b6 Mon Sep 17 00:00:00 2001 From: Maxi Wittich Date: Wed, 12 Aug 2026 11:01:10 +0200 Subject: [PATCH 14/22] cargo-rustfmt --- .../src/controller/build/resource/workload/daemonset.rs | 2 +- .../src/controller/build/resource/workload/deployment.rs | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/rust/operator-binary/src/controller/build/resource/workload/daemonset.rs b/rust/operator-binary/src/controller/build/resource/workload/daemonset.rs index 8f6de324..dbe5060b 100644 --- a/rust/operator-binary/src/controller/build/resource/workload/daemonset.rs +++ b/rust/operator-binary/src/controller/build/resource/workload/daemonset.rs @@ -71,11 +71,11 @@ pub fn build_server_rolegroup_daemonset( #[cfg(test)] mod tests { use serde_json::json; + use stackable_opa_operator::crd::OpaRole; use stackable_operator::{ commons::networking::DomainName, k8s_openapi::api::core::v1::Container, }; - use stackable_opa_operator::crd::OpaRole; use super::*; use crate::controller::build::properties::test_support::validated_cluster_from_spec; diff --git a/rust/operator-binary/src/controller/build/resource/workload/deployment.rs b/rust/operator-binary/src/controller/build/resource/workload/deployment.rs index fa78a746..e7ffc11e 100644 --- a/rust/operator-binary/src/controller/build/resource/workload/deployment.rs +++ b/rust/operator-binary/src/controller/build/resource/workload/deployment.rs @@ -69,9 +69,8 @@ pub fn build_server_rolegroup_deployment( #[cfg(test)] mod tests { use serde_json::json; - use stackable_operator::commons::networking::DomainName; - use stackable_opa_operator::crd::OpaRole; + use stackable_operator::commons::networking::DomainName; use super::*; use crate::controller::build::properties::test_support::validated_cluster_from_spec; From f902c4c0148a8bb2ea5e25ffe3539018270a36de Mon Sep 17 00:00:00 2001 From: Maxi Wittich Date: Wed, 12 Aug 2026 11:03:15 +0200 Subject: [PATCH 15/22] cargo-clippy --- rust/operator-binary/src/controller/validate.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust/operator-binary/src/controller/validate.rs b/rust/operator-binary/src/controller/validate.rs index 4844c981..f266647b 100644 --- a/rust/operator-binary/src/controller/validate.rs +++ b/rust/operator-binary/src/controller/validate.rs @@ -183,7 +183,7 @@ pub fn validate( let merged: RoleGroup = with_validated_config( role_group, role, - &OpaConfig::default_config(&name.to_string(), &opa_role), + &OpaConfig::default_config(name.as_ref(), &opa_role), ) .context(ValidateRoleGroupConfigSnafu { role_group: role_group_name.clone(), From d6242d9ff929a5c7f882db6115c44cd6a4551c7a Mon Sep 17 00:00:00 2001 From: Maxi Wittich Date: Wed, 12 Aug 2026 11:27:09 +0200 Subject: [PATCH 16/22] rustfmt round 2 --- .../src/controller/build/resource/workload/daemonset.rs | 1 - .../src/controller/build/resource/workload/deployment.rs | 6 ++---- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/rust/operator-binary/src/controller/build/resource/workload/daemonset.rs b/rust/operator-binary/src/controller/build/resource/workload/daemonset.rs index dbe5060b..a68f0cea 100644 --- a/rust/operator-binary/src/controller/build/resource/workload/daemonset.rs +++ b/rust/operator-binary/src/controller/build/resource/workload/daemonset.rs @@ -76,7 +76,6 @@ mod tests { commons::networking::DomainName, k8s_openapi::api::core::v1::Container, }; - use super::*; use crate::controller::build::properties::test_support::validated_cluster_from_spec; diff --git a/rust/operator-binary/src/controller/build/resource/workload/deployment.rs b/rust/operator-binary/src/controller/build/resource/workload/deployment.rs index e7ffc11e..c30cc9ab 100644 --- a/rust/operator-binary/src/controller/build/resource/workload/deployment.rs +++ b/rust/operator-binary/src/controller/build/resource/workload/deployment.rs @@ -7,9 +7,7 @@ use stackable_operator::k8s_openapi::{ use super::*; -/// Runs a fixed number of replicas, unlike [`daemonset`](super::daemonset), which covers every -/// node. The Pods therefore do not cover every node and the role Service has to route to any of -/// them rather than to a node-local one. +/// Runs a fixed number of replicas, unlike [`daemonset`](super::daemonset). #[allow(clippy::too_many_arguments)] pub fn build_server_rolegroup_deployment( cluster: &ValidatedCluster, @@ -41,7 +39,7 @@ pub fn build_server_rolegroup_deployment( .build(); let deployment_spec = DeploymentSpec { - // Left unset so Kubernetes applies its default of one, rather than the operator inventing + // Left unset so Kubernetes applies its default of one, rather than the operator guessing // a replica count. replicas: role_group.replicas.map(i32::from), selector: LabelSelector { From b1cc0ca9e1d8a5b96d57a7118e4b314cd443f17e Mon Sep 17 00:00:00 2001 From: Maxi Wittich Date: Wed, 12 Aug 2026 12:13:30 +0200 Subject: [PATCH 17/22] More fmt, clippy etc --- deploy/helm/opa-operator/templates/clusterrole-operator.yaml | 2 +- .../src/controller/build/resource/workload/deployment.rs | 4 ++-- rust/operator-binary/src/crd/affinity.rs | 4 ++-- tests/templates/kuttl/smoke/12-assert.yaml.j2 | 2 +- tests/templates/kuttl/workload-kind/10-errors.yaml | 2 +- .../kuttl/workload-kind/30-switch-back-to-daemonset.yaml.j2 | 2 +- 6 files changed, 8 insertions(+), 8 deletions(-) diff --git a/deploy/helm/opa-operator/templates/clusterrole-operator.yaml b/deploy/helm/opa-operator/templates/clusterrole-operator.yaml index f44eea59..6d407dae 100644 --- a/deploy/helm/opa-operator/templates/clusterrole-operator.yaml +++ b/deploy/helm/opa-operator/templates/clusterrole-operator.yaml @@ -64,7 +64,7 @@ rules: resourceNames: - {{ include "operator.name" . }}-clusterrole # DaemonSet or Deployment created per role group, depending on the role's `workloadKind`. - # Applied via SSA, tracked for orphan cleanup, and owned by the controller. + # Applied via SSA, tracked for orphan cleanup, and owned by the controller. - apiGroups: - apps resources: diff --git a/rust/operator-binary/src/controller/build/resource/workload/deployment.rs b/rust/operator-binary/src/controller/build/resource/workload/deployment.rs index c30cc9ab..a2e68b0d 100644 --- a/rust/operator-binary/src/controller/build/resource/workload/deployment.rs +++ b/rust/operator-binary/src/controller/build/resource/workload/deployment.rs @@ -7,7 +7,7 @@ use stackable_operator::k8s_openapi::{ use super::*; -/// Runs a fixed number of replicas, unlike [`daemonset`](super::daemonset). +/// Runs a fixed number of replicas, unlike [`daemonset`](super::daemonset). #[allow(clippy::too_many_arguments)] pub fn build_server_rolegroup_deployment( cluster: &ValidatedCluster, @@ -182,7 +182,7 @@ mod tests { } /// Replicas are only worth having if they are deployed on different nodes, so the default anti-affinity - /// has to survive the config merge into the Pod template. + /// has to survive the config merge into the Pod template. #[test] fn deployment_pods_are_spread_across_nodes_by_default() { let deployment = build(&validated_cluster_from_spec(json!({ diff --git a/rust/operator-binary/src/crd/affinity.rs b/rust/operator-binary/src/crd/affinity.rs index ac934ba2..dd4eaa3b 100644 --- a/rust/operator-binary/src/crd/affinity.rs +++ b/rust/operator-binary/src/crd/affinity.rs @@ -24,7 +24,7 @@ const ANTI_AFFINITY_BETWEEN_ROLE_PODS_WEIGHT: i32 = 70; // // The chance: with node-local routing that degrades gracefully, an affinity attracting OPA Pods // towards the products that query them would genuinely pay off, because traffic would prefer a -// node-local OPA Pod without the current risk of failing outright when there is none. +// node-local OPA Pod without the current risk of failing outright when there is none. // // The concerns: // @@ -36,7 +36,7 @@ const ANTI_AFFINITY_BETWEEN_ROLE_PODS_WEIGHT: i32 = 70; // network hop by a wide margin. // // * The scheduler scores `podAffinity` and `podAntiAffinity` on one scale, so the two weights would -// compete. Keeping this one at 70 above a lower attraction weight encodes "spreading wins", where +// compete. Keeping this one at 70 above a lower attraction weight encodes "spreading wins", where // equal weights would cancel out. pub fn get_affinity(cluster_name: &str, role: &OpaRole) -> StackableAffinityFragment { StackableAffinityFragment { diff --git a/tests/templates/kuttl/smoke/12-assert.yaml.j2 b/tests/templates/kuttl/smoke/12-assert.yaml.j2 index 3e8480fa..e122aecf 100644 --- a/tests/templates/kuttl/smoke/12-assert.yaml.j2 +++ b/tests/templates/kuttl/smoke/12-assert.yaml.j2 @@ -8,7 +8,7 @@ # only here — `.data` is asserted in 13-assert), ServiceAccount, RoleBinding. # # Catches drift in labels, owner references, selectors, ports, probe schemes, -# update strategy, container resources and TLS-dependent fields. +# update strategy, container resources and TLS-dependent fields. # # `app.kubernetes.io/version` is intentionally omitted from label matchers so # that product-version bumps in test-definition.yaml don't force snapshot diff --git a/tests/templates/kuttl/workload-kind/10-errors.yaml b/tests/templates/kuttl/workload-kind/10-errors.yaml index 2185d571..8d69d588 100644 --- a/tests/templates/kuttl/workload-kind/10-errors.yaml +++ b/tests/templates/kuttl/workload-kind/10-errors.yaml @@ -1,6 +1,6 @@ --- # A DaemonSet's Pods are skipped by `kubectl drain`, so a PodDisruptionBudget would protect nothing -# and none is written out. +# and none is written out. apiVersion: policy/v1 kind: PodDisruptionBudget metadata: diff --git a/tests/templates/kuttl/workload-kind/30-switch-back-to-daemonset.yaml.j2 b/tests/templates/kuttl/workload-kind/30-switch-back-to-daemonset.yaml.j2 index 74cda7d2..0a5f0317 100644 --- a/tests/templates/kuttl/workload-kind/30-switch-back-to-daemonset.yaml.j2 +++ b/tests/templates/kuttl/workload-kind/30-switch-back-to-daemonset.yaml.j2 @@ -19,7 +19,7 @@ spec: servers: # Must be spelled out rather than omitted: kuttl applies a step as a merge patch, so an absent # field means "leave it alone", not "remove it". Dropping this block would keep the - # `workloadKind: Deployment`. + # `workloadKind: Deployment`. roleConfig: workloadKind: DaemonSet config: From a138435c37d6ba9d978ecee4da076015b752ae1b Mon Sep 17 00:00:00 2001 From: Maxi Wittich Date: Wed, 12 Aug 2026 13:45:22 +0200 Subject: [PATCH 18/22] Better docs wording --- docs/modules/opa/pages/usage-guide/workload-kind.adoc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/modules/opa/pages/usage-guide/workload-kind.adoc b/docs/modules/opa/pages/usage-guide/workload-kind.adoc index b4407df9..4b971856 100644 --- a/docs/modules/opa/pages/usage-guide/workload-kind.adoc +++ b/docs/modules/opa/pages/usage-guide/workload-kind.adoc @@ -33,12 +33,12 @@ This fits large clusters, and clusters where only a few nodes run products that The operator derives the role Service's `internalTrafficPolicy` from the workload kind. -* A DaemonSet gets `Local`, so a query only reaches the OPA Pod on the client's own node. +* A DaemonSet gets Local, so a query only reaches the OPA Pod on the client's own node. This avoids the network hop, and a DaemonSet covers every node, thus such a Pod always exists. -* A Deployment gets `Cluster`, so a query reaches any OPA Pod of the role. +* A Deployment gets Cluster, so a query reaches any OPA Pod of the role. A Deployment's Pods do not cover every node necessarily, so node-local routing would leave products on the remaining nodes unable to reach OPA at all. -If the derived value doesn't suit your cluster, you can override it as described in <>. +If the derived value doesn't suit your cluster, you can override it as described in <>. [#override-traffic-policy] == Override traffic policy @@ -79,6 +79,6 @@ Changing to DaemonSet is the more disruptive direction. The role Service narrows to Local as soon as you apply the change, while the running Pods still belong to the outgoing Deployment and cover only some nodes. Products on the remaining nodes fail until the DaemonSet has rolled out everywhere. -To ease the interruption, pin the traffic policy to Cluster as described in <>, and remove the override once the rollout has finished. +To ease the interruption, pin the traffic policy to Cluster as described in <>, and remove the override once the rollout has finished. This keeps every product able to reach any OPA Pod throughout the change. A short window in which no Pod is ready can still occur, because the outgoing workload is removed as the new one starts. From 7d2719a4bf46f5d8823018bd1cdcae1f38d6bbeb Mon Sep 17 00:00:00 2001 From: Maxi Wittich Date: Wed, 12 Aug 2026 15:20:25 +0200 Subject: [PATCH 19/22] fix: watch PDBs in reconcile --- rust/operator-binary/src/main.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/rust/operator-binary/src/main.rs b/rust/operator-binary/src/main.rs index e1469214..47a4dd8b 100644 --- a/rust/operator-binary/src/main.rs +++ b/rust/operator-binary/src/main.rs @@ -15,6 +15,7 @@ use stackable_operator::{ k8s_openapi::api::{ apps::v1::{DaemonSet, Deployment}, core::v1::{ConfigMap, Service}, + policy::v1::PodDisruptionBudget, }, kube::{ CustomResourceExt as _, @@ -154,6 +155,14 @@ async fn main() -> anyhow::Result<()> { watch_namespace.get_api::>(&client), watcher::Config::default(), ) + // Watched so that deleting the budget is noticed. Reconciliation is only triggered + // by watched objects (`Action::await_change`, no periodic requeue), so without this + // a removed PodDisruptionBudget would stay removed until something else changed, + // silently dropping the role's disruption protection. + .owns( + watch_namespace.get_api::>(&client), + watcher::Config::default(), + ) .owns( watch_namespace.get_api::>(&client), watcher::Config::default(), From 8765b14c39bd6086e41a8f3b8a29fbd377e37ba4 Mon Sep 17 00:00:00 2001 From: Maxi Wittich Date: Wed, 12 Aug 2026 15:24:04 +0200 Subject: [PATCH 20/22] Better comment on PDB watch --- rust/operator-binary/src/main.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/rust/operator-binary/src/main.rs b/rust/operator-binary/src/main.rs index 47a4dd8b..5fb80113 100644 --- a/rust/operator-binary/src/main.rs +++ b/rust/operator-binary/src/main.rs @@ -155,10 +155,7 @@ async fn main() -> anyhow::Result<()> { watch_namespace.get_api::>(&client), watcher::Config::default(), ) - // Watched so that deleting the budget is noticed. Reconciliation is only triggered - // by watched objects (`Action::await_change`, no periodic requeue), so without this - // a removed PodDisruptionBudget would stay removed until something else changed, - // silently dropping the role's disruption protection. + // Deleting the budget must be noticed. .owns( watch_namespace.get_api::>(&client), watcher::Config::default(), From 214751c5c7b5ceaa3a4977a0b69c447649572c59 Mon Sep 17 00:00:00 2001 From: Maxi Wittich Date: Wed, 12 Aug 2026 15:58:11 +0200 Subject: [PATCH 21/22] Corrected statement about PDBs in case of DS in docs --- .../opa/pages/usage-guide/operations/pod-disruptions.adoc | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/docs/modules/opa/pages/usage-guide/operations/pod-disruptions.adoc b/docs/modules/opa/pages/usage-guide/operations/pod-disruptions.adoc index 4a038254..b5f9f471 100644 --- a/docs/modules/opa/pages/usage-guide/operations/pod-disruptions.adoc +++ b/docs/modules/opa/pages/usage-guide/operations/pod-disruptions.adoc @@ -7,10 +7,14 @@ What the operator creates depends on the xref:usage-guide/workload-kind.adoc[wor == DaemonSet -No PodDisruptionBudget is created. -Draining a node has to be able to remove the OPA Pod on that node, and `kubectl drain` skips DaemonSet Pods, means a budget would block maintenance without adding an upside. +No PodDisruptionBudget is created, and you should not enable one. + +`kubectl drain` skips DaemonSet Pods, a budget is never consulted during a node drain. +Moreover, PodDisruptionBudgets over DaemonSets can never be evaluated: DaemonSet does not implement the scale subresource. +Therefore the budget stays at `disruptionsAllowed: 0` with a `SyncFailed` condition and refuses every direct eviction. Because the role Service routes node-locally in this mode, products on a drained node cannot reach OPA on another node. +Drain nodes together with the products that query OPA on them. == Deployment From a6c9fab4c29fede28f76711259052951909fea83 Mon Sep 17 00:00:00 2001 From: Maxi Wittich Date: Wed, 12 Aug 2026 16:50:19 +0200 Subject: [PATCH 22/22] Precise comments, self-review --- .../src/controller/build/resource/pdb.rs | 5 +---- .../controller/build/resource/workload/mod.rs | 4 +--- rust/operator-binary/src/crd/affinity.rs | 17 ++++------------- rust/operator-binary/src/crd/mod.rs | 15 ++++++--------- 4 files changed, 12 insertions(+), 29 deletions(-) diff --git a/rust/operator-binary/src/controller/build/resource/pdb.rs b/rust/operator-binary/src/controller/build/resource/pdb.rs index fbeb5b72..64a586dd 100644 --- a/rust/operator-binary/src/controller/build/resource/pdb.rs +++ b/rust/operator-binary/src/controller/build/resource/pdb.rs @@ -9,14 +9,11 @@ use stackable_operator::{ use crate::controller::{ValidatedCluster, controller_name, operator_name, product_name}; -/// How many Pods of a role may be unavailable when the administrator configured no -/// `maxUnavailable`. const DEFAULT_MAX_UNAVAILABLE: u16 = 1; /// The role-level [`PodDisruptionBudget`], or `None` when the role has it disabled. /// -/// One per role rather than per role group, because the budget selects on the role's labels and so -/// covers every role group of that role at once. +/// One per role rather than per role group. pub fn build_role_pod_disruption_budget( cluster: &ValidatedCluster, opa_role: &OpaRole, diff --git a/rust/operator-binary/src/controller/build/resource/workload/mod.rs b/rust/operator-binary/src/controller/build/resource/workload/mod.rs index 6bbc4735..d0549a34 100644 --- a/rust/operator-binary/src/controller/build/resource/workload/mod.rs +++ b/rust/operator-binary/src/controller/build/resource/workload/mod.rs @@ -225,9 +225,7 @@ fn http_liveness_probe(path: &str, port: IntOrString, scheme: Option) -> /// Builds the [`PodTemplateSpec`] for a rolegroup, shared by every deployment mode. /// -/// The template carries the `prepare` init container, the OPA and bundle-builder containers, the -/// optional user-info-fetcher and Vector sidecars, and all volumes they mount. Callers wrap it in -/// the workload object of their choice; see [`daemonset::build_server_rolegroup_daemonset`] and +/// Callers wrap it in the workload object of their choice; see [`daemonset::build_server_rolegroup_daemonset`] and /// [`deployment::build_server_rolegroup_deployment`]. #[allow(clippy::too_many_arguments)] pub(crate) fn build_server_rolegroup_pod_template( diff --git a/rust/operator-binary/src/crd/affinity.rs b/rust/operator-binary/src/crd/affinity.rs index dd4eaa3b..50a13acf 100644 --- a/rust/operator-binary/src/crd/affinity.rs +++ b/rust/operator-binary/src/crd/affinity.rs @@ -15,25 +15,16 @@ const ANTI_AFFINITY_BETWEEN_ROLE_PODS_WEIGHT: i32 = 70; /// The default affinity of `role`: prefer to spread its Pods across nodes. /// -/// Soft (`preferred`), so it can never leave a Pod unschedulable, and inert for a `DaemonSet`, which -/// already places exactly one Pod per node. It matters in `Deployment` mode only. -// // TODO: Revisit once our minimum supported Kubernetes version is 1.35 and the role Service can use // `trafficDistribution: PreferSameNode` instead of `internalTrafficPolicy` (see // `controller::build::resource::service`). // -// The chance: with node-local routing that degrades gracefully, an affinity attracting OPA Pods -// towards the products that query them would genuinely pay off, because traffic would prefer a -// node-local OPA Pod without the current risk of failing outright when there is none. -// -// The concerns: +// Concerns: // -// * `PreferSameNode` falls back to other nodes only when there is no *ready* local endpoint, never -// because the local one is busy. A request-heavy client (Trino, Kafka, depending on their -// config) would keep hitting its local Pod while the others idle. +// * `PreferSameNode` falls back to other nodes only when there is no *ready* local endpoint. +// A request-heavy client (Trino, Kafka, depending on their config) would keep hitting its local Pod while the others idle. // -// * Field experience points the other way: spreading the load across Pods outperformed avoiding the -// network hop by a wide margin. +// * Field experience: spreading the load across Pods outperformed avoiding the network hop by margin. // // * The scheduler scores `podAffinity` and `podAntiAffinity` on one scale, so the two weights would // compete. Keeping this one at 70 above a lower attraction weight encodes "spreading wins", where diff --git a/rust/operator-binary/src/crd/mod.rs b/rust/operator-binary/src/crd/mod.rs index d2bd1d4b..d1da36e5 100644 --- a/rust/operator-binary/src/crd/mod.rs +++ b/rust/operator-binary/src/crd/mod.rs @@ -376,11 +376,11 @@ impl v1alpha2::CurrentlySupportedListenerClasses { impl v1alpha2::OpaRoleConfig { /// The `internalTrafficPolicy` to write into the role Service. /// - /// Derived from the [`v1alpha2::WorkloadKind`]: `Local` for a DaemonSet, which covers every - /// node, and `Cluster` for a Deployment, whose Pods do not. + /// Derived from the [`v1alpha2::WorkloadKind`]: `Local` for a DaemonSet + /// and `Cluster` for a Deployment. /// - /// This is the single place the policy is decided, so exposing a user override later means - /// adding the CRD field back and wrapping this `match` in an `unwrap_or`; no call site changes. + /// TODO: This is the single place the policy is decided, so exposing a user override later means + /// adding the CRD field back and wrapping this `match` in an `unwrap_or`. pub fn internal_traffic_policy(&self) -> v1alpha2::InternalTrafficPolicy { match self.workload_kind { v1alpha2::WorkloadKind::DaemonSet => v1alpha2::InternalTrafficPolicy::Local, @@ -390,8 +390,7 @@ impl v1alpha2::OpaRoleConfig { /// Whether a PodDisruptionBudget should be written out for this role. /// - /// Falls back to `true` for a Deployment only: `kubectl drain` requires `--ignore-daemonsets` - /// and then leaves those Pods alone, so a PDB would protect nothing in DaemonSet mode. + /// Falls back to `true` for a Deployment only. pub fn pod_disruption_budget_enabled(&self) -> bool { self.pod_disruption_budget .enabled @@ -416,9 +415,7 @@ impl OpaConfig { }, storage: OpaStorageConfigFragment {}, }, - // Spreads the role's Pods across nodes. A no-op for a DaemonSet, which already runs - // exactly one Pod per node, but it is what keeps a Deployment's replicas from landing - // together. See `affinity::get_affinity`. + // Spreads the role's Pods across nodes. A no-op for a DaemonSet. affinity: affinity::get_affinity(cluster_name, role), graceful_shutdown_timeout: Some(DEFAULT_SERVER_GRACEFUL_SHUTDOWN_TIMEOUT), }