diff --git a/CHANGELOG.md b/CHANGELOG.md index 75b88a5c..5900c503 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,8 @@ All notable changes to this project will be documented in this file. - BREAKING: The `nodes` role is now required by the CRD; a NifiCluster without it was previously accepted by the API server but failed reconciliation ([#966]). - All product containers now run with `securityContext.runAsNonRoot` set to `true` to improve security ([#975]). +- NiFi 2.x startup and readiness probes now use the local management server's `/health` and + `/health/cluster` endpoints instead of a bare TCP check ([#976]). ### Fixed @@ -25,6 +27,7 @@ All notable changes to this project will be documented in this file. [#966]: https://github.com/stackabletech/nifi-operator/pull/966 [#970]: https://github.com/stackabletech/nifi-operator/pull/970 [#975]: https://github.com/stackabletech/nifi-operator/pull/975 +[#976]: https://github.com/stackabletech/nifi-operator/pull/976 ## [26.7.0] - 2026-07-21 diff --git a/rust/operator-binary/src/controller/build.rs b/rust/operator-binary/src/controller/build.rs index 6376f9df..d0a2d7e6 100644 --- a/rust/operator-binary/src/controller/build.rs +++ b/rust/operator-binary/src/controller/build.rs @@ -46,6 +46,19 @@ pub const PROTOCOL_PORT: Port = Port(9088); pub const BALANCE_PORT_NAME: &str = "balance"; pub const BALANCE_PORT: Port = Port(6243); +/// Loopback address NiFi's management server binds to by default +/// (`org.apache.nifi.management.server.address`, see +/// `ManagementServerProvider.MANAGEMENT_SERVER_DEFAULT_ADDRESS` upstream). +/// The operator pins the JVM system property to this same value (see +/// `build::jvm`), so this constant is the single source of truth for it. +/// Note: a user-supplied `jvmArgumentOverrides` that removes or repoints the +/// corresponding `-D` property would desync this from the actual +/// management-server address, silently breaking both probes. +pub const MANAGEMENT_SERVER_ADDRESS: &str = "127.0.0.1"; +/// Port NiFi's management server binds to by default, see +/// [`MANAGEMENT_SERVER_ADDRESS`]. +pub const MANAGEMENT_SERVER_PORT: u16 = 52020; + // Filesystem paths shared by multiple builders. Single-consumer paths live in their builder. pub const NIFI_CONFIG_DIRECTORY: &str = "/stackable/nifi/conf"; pub const NIFI_PYTHON_WORKING_DIRECTORY: &str = "/nifi-python-working-directory"; diff --git a/rust/operator-binary/src/controller/build/jvm.rs b/rust/operator-binary/src/controller/build/jvm.rs index b6275a07..f28ec785 100644 --- a/rust/operator-binary/src/controller/build/jvm.rs +++ b/rust/operator-binary/src/controller/build/jvm.rs @@ -9,7 +9,10 @@ use stackable_operator::{ use crate::{ controller::{ ValidatedNifiConfig, - build::{NIFI_CONFIG_DIRECTORY, properties::ConfigFileName}, + build::{ + MANAGEMENT_SERVER_ADDRESS, MANAGEMENT_SERVER_PORT, NIFI_CONFIG_DIRECTORY, + properties::ConfigFileName, + }, }, security::{ authentication::{STACKABLE_SERVER_TLS_DIR, STACKABLE_TLS_STORE_PASSWORD}, @@ -86,6 +89,12 @@ pub fn build_merged_jvm_config( "-Djava.security.properties={NIFI_CONFIG_DIRECTORY}/{}", ConfigFileName::SecurityProperties ), + // Pin the NiFi 2.x management server (used by the startup/readiness + // probes in resource::probes) to a known address instead of relying + // on its undocumented upstream default. + format!( + "-Dorg.apache.nifi.management.server.address={MANAGEMENT_SERVER_ADDRESS}:{MANAGEMENT_SERVER_PORT}" + ), ]; // Add JVM truststore properties when OPA TLS is enabled @@ -170,6 +179,28 @@ mod tests { ); } + /// The management-server bind address is pinned explicitly rather than + /// relying on NiFi's undocumented upstream default, so the probes in + /// `resource::probes` always target the right port even if that default + /// ever changes upstream. + #[test] + fn management_server_address_is_pinned_explicitly() { + let cluster = minimal_validated_cluster(); + let args = build_merged_jvm_config( + &default_rg(&cluster).config, + &JvmArgumentOverrides::default(), + None, + ) + .expect("jvm config should build"); + + assert!( + args.contains( + &"-Dorg.apache.nifi.management.server.address=127.0.0.1:52020".to_string() + ), + "expected an explicit management-server address JVM property, got: {args:?}" + ); + } + /// Without OPA TLS, no truststore properties are emitted. #[test] fn without_opa_tls_no_truststore_properties() { diff --git a/rust/operator-binary/src/controller/build/properties/bootstrap_conf.rs b/rust/operator-binary/src/controller/build/properties/bootstrap_conf.rs index 77602fdb..0898e5ea 100644 --- a/rust/operator-binary/src/controller/build/properties/bootstrap_conf.rs +++ b/rust/operator-binary/src/controller/build/properties/bootstrap_conf.rs @@ -122,6 +122,7 @@ mod tests { java.arg.10=-Djavax.security.auth.useSubjectCredsOnly=true java.arg.11=-Dzookeeper.admin.enableServer=false java.arg.12=-Djava.security.properties=/stackable/nifi/conf/security.properties + java.arg.13=-Dorg.apache.nifi.management.server.address=127.0.0.1:52020 java.arg.2=-Xms3276m java.arg.3=-XX:+UseG1GC java.arg.4=-Djava.awt.headless=true @@ -187,10 +188,11 @@ mod tests { java=java java.arg.1=-Xms34406m java.arg.10=-Djava.security.properties=/stackable/nifi/conf/security.properties - java.arg.11=-Dhttps.proxyHost=proxy.my.corp - java.arg.12=-Djava.net.preferIPv4Stack=true - java.arg.13=-Xmx40000m - java.arg.14=-Dhttps.proxyPort=1234 + java.arg.11=-Dorg.apache.nifi.management.server.address=127.0.0.1:52020 + java.arg.12=-Dhttps.proxyHost=proxy.my.corp + java.arg.13=-Djava.net.preferIPv4Stack=true + java.arg.14=-Xmx40000m + java.arg.15=-Dhttps.proxyPort=1234 java.arg.2=-Djava.awt.headless=true java.arg.3=-Dorg.apache.jasper.compiler.disablejsr199=true java.arg.4=-Djava.net.preferIPv4Stack=true diff --git a/rust/operator-binary/src/controller/build/resource/mod.rs b/rust/operator-binary/src/controller/build/resource/mod.rs index 9598a324..a475d2e9 100644 --- a/rust/operator-binary/src/controller/build/resource/mod.rs +++ b/rust/operator-binary/src/controller/build/resource/mod.rs @@ -5,6 +5,7 @@ pub mod config_map; pub mod listener; pub mod pdb; +pub mod probes; pub mod rbac; pub mod service; pub mod statefulset; diff --git a/rust/operator-binary/src/controller/build/resource/probes.rs b/rust/operator-binary/src/controller/build/resource/probes.rs new file mode 100644 index 00000000..7045e6c3 --- /dev/null +++ b/rust/operator-binary/src/controller/build/resource/probes.rs @@ -0,0 +1,130 @@ +//! Builds the exec-based startup and readiness probes that check NiFi 2.x's +//! local, unauthenticated management-server endpoints (`/health` and +//! `/health/cluster`). +//! +//! The management server binds `127.0.0.1` only, so these must be `exec` +//! probes using `curl` from inside the container - a `httpGet` probe cannot +//! reach a loopback-only address. + +use stackable_operator::k8s_openapi::{ + api::core::v1::{ExecAction, Probe, TCPSocketAction}, + apimachinery::pkg::util::intstr::IntOrString, +}; + +use crate::controller::build::{ + HTTPS_PORT_NAME, MANAGEMENT_SERVER_ADDRESS, MANAGEMENT_SERVER_PORT, +}; + +fn management_health_exec(path: &str) -> ExecAction { + ExecAction { + command: Some(vec![ + "/bin/bash".to_string(), + "-euo".to_string(), + "pipefail".to_string(), + "-c".to_string(), + format!( + "curl --fail --silent --show-error --output /dev/null http://{MANAGEMENT_SERVER_ADDRESS}:{MANAGEMENT_SERVER_PORT}{path}" + ), + ]), + } +} +pub fn management_startup_probe() -> Probe { + Probe { + initial_delay_seconds: Some(10), + period_seconds: Some(10), + timeout_seconds: Some(5), + failure_threshold: Some(20 * 6), + exec: Some(management_health_exec("/health")), + ..Probe::default() + } +} +pub fn management_readiness_probe() -> Probe { + Probe { + period_seconds: Some(10), + timeout_seconds: Some(5), + failure_threshold: Some(3), + exec: Some(management_health_exec("/health/cluster")), + ..Probe::default() + } +} + +pub fn tcp_liveliness_probe() -> Probe { + Probe { + initial_delay_seconds: Some(10), + period_seconds: Some(10), + tcp_socket: Some(TCPSocketAction { + port: IntOrString::String(HTTPS_PORT_NAME.to_string()), + ..TCPSocketAction::default() + }), + ..Probe::default() + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn startup_probe_execs_curl_against_health_endpoint() { + let probe = management_startup_probe(); + + let command = probe + .exec + .expect("startup probe must be an exec probe") + .command + .expect("exec action must have a command"); + let script = command.last().expect("bash -c script argument"); + + assert!( + script.contains("http://127.0.0.1:52020/health") && !script.contains("/health/cluster"), + "expected curl against /health, got: {script}" + ); + assert_eq!(probe.failure_threshold, Some(120)); + assert_eq!(probe.timeout_seconds, Some(5)); + assert!( + probe.tcp_socket.is_none(), + "must not fall back to tcp_socket" + ); + } + + #[test] + fn readiness_probe_execs_curl_against_cluster_health_endpoint() { + let probe = management_readiness_probe(); + + let command = probe + .exec + .expect("readiness probe must be an exec probe") + .command + .expect("exec action must have a command"); + let script = command.last().expect("bash -c script argument"); + + assert!( + script.contains("http://127.0.0.1:52020/health/cluster"), + "expected curl against /health/cluster, got: {script}" + ); + assert_eq!(probe.failure_threshold, Some(3)); + assert_eq!(probe.timeout_seconds, Some(5)); + assert_eq!( + probe.initial_delay_seconds, None, + "readiness probe delay is redundant: k8s already suppresses readiness checks \ + until the startup probe succeeds" + ); + } + + #[test] + fn probes_use_bash_pipefail_wrapper_not_bare_curl_argv() { + for probe in [management_startup_probe(), management_readiness_probe()] { + let command = probe.exec.unwrap().command.unwrap(); + assert_eq!( + command[..4], + [ + "/bin/bash".to_string(), + "-euo".to_string(), + "pipefail".to_string(), + "-c".to_string(), + ], + "exec command must follow the repo's bash -euo pipefail -c convention" + ); + } + } +} diff --git a/rust/operator-binary/src/controller/build/resource/statefulset.rs b/rust/operator-binary/src/controller/build/resource/statefulset.rs index 39346f9e..af0c3cc4 100644 --- a/rust/operator-binary/src/controller/build/resource/statefulset.rs +++ b/rust/operator-binary/src/controller/build/resource/statefulset.rs @@ -21,11 +21,11 @@ use stackable_operator::{ apps::v1::{StatefulSet, StatefulSetSpec, StatefulSetUpdateStrategy}, core::v1::{ ConfigMapKeySelector, ConfigMapVolumeSource, EmptyDirVolumeSource, EnvVar, - EnvVarSource, ObjectFieldSelector, PersistentVolumeClaim, Probe, - SecretVolumeSource, TCPSocketAction, Volume, + EnvVarSource, ObjectFieldSelector, PersistentVolumeClaim, SecretVolumeSource, + Volume, }, }, - apimachinery::pkg::{apis::meta::v1::LabelSelector, util::intstr::IntOrString}, + apimachinery::pkg::apis::meta::v1::LabelSelector, }, memory::{BinaryMultiple, MemoryQuantity}, product_logging::{ @@ -54,9 +54,14 @@ use crate::{ graceful_shutdown::add_graceful_shutdown_config, object_meta, properties::ConfigFileName, - resource::listener::{ - LISTENER_VOLUME_DIR, LISTENER_VOLUME_NAME, build_group_listener_pvc, - group_listener_name, + resource::{ + listener::{ + LISTENER_VOLUME_DIR, LISTENER_VOLUME_NAME, build_group_listener_pvc, + group_listener_name, + }, + probes::{ + management_readiness_probe, management_startup_probe, tcp_liveliness_probe, + }, }, }, }, @@ -435,25 +440,9 @@ pub(crate) fn build_node_rolegroup_statefulset( .add_container_port(HTTPS_PORT_NAME, HTTPS_PORT.into()) .add_container_port(PROTOCOL_PORT_NAME, PROTOCOL_PORT.into()) .add_container_port(BALANCE_PORT_NAME, BALANCE_PORT.into()) - .liveness_probe(Probe { - initial_delay_seconds: Some(10), - period_seconds: Some(10), - tcp_socket: Some(TCPSocketAction { - port: IntOrString::String(HTTPS_PORT_NAME.to_string()), - ..TCPSocketAction::default() - }), - ..Probe::default() - }) - .startup_probe(Probe { - initial_delay_seconds: Some(10), - period_seconds: Some(10), - failure_threshold: Some(20 * 6), - tcp_socket: Some(TCPSocketAction { - port: IntOrString::String(HTTPS_PORT_NAME.to_string()), - ..TCPSocketAction::default() - }), - ..Probe::default() - }) + .liveness_probe(tcp_liveliness_probe()) + .startup_probe(management_startup_probe()) + .readiness_probe(management_readiness_probe()) .resources(merged_config.resources.clone().into()); let mut pod_builder = PodBuilder::new(); diff --git a/tests/templates/kuttl/cluster_operation/20-assert.yaml b/tests/templates/kuttl/cluster_operation/20-assert.yaml index 155b0d03..ed248438 100644 --- a/tests/templates/kuttl/cluster_operation/20-assert.yaml +++ b/tests/templates/kuttl/cluster_operation/20-assert.yaml @@ -12,3 +12,31 @@ metadata: status: readyReplicas: 2 replicas: 2 +--- +apiVersion: apps/v1 +kind: StatefulSet +metadata: + name: test-nifi-node-default +spec: + template: + spec: + containers: + - name: nifi + startupProbe: + failureThreshold: 120 + exec: + command: + - /bin/bash + - -euo + - pipefail + - -c + - curl --fail --silent --show-error --output /dev/null http://127.0.0.1:52020/health + readinessProbe: + failureThreshold: 3 + exec: + command: + - /bin/bash + - -euo + - pipefail + - -c + - curl --fail --silent --show-error --output /dev/null http://127.0.0.1:52020/health/cluster diff --git a/tests/templates/kuttl/smoke/35-assert.yaml.j2 b/tests/templates/kuttl/smoke/35-assert.yaml.j2 index 57409959..3a8346d9 100644 --- a/tests/templates/kuttl/smoke/35-assert.yaml.j2 +++ b/tests/templates/kuttl/smoke/35-assert.yaml.j2 @@ -36,6 +36,7 @@ commands: java.arg.10=-Djavax.security.auth.useSubjectCredsOnly=true java.arg.11=-Dzookeeper.admin.enableServer=false java.arg.12=-Djava.security.properties=/stackable/nifi/conf/security.properties + java.arg.13=-Dorg.apache.nifi.management.server.address=127.0.0.1:52020 java.arg.2=-Xms3276m java.arg.3=-XX:+UseG1GC java.arg.4=-Djava.awt.headless=true