From ea4f5b7d6fbf293c302c918e84756425f7587877 Mon Sep 17 00:00:00 2001 From: Malte Sander Date: Mon, 17 Aug 2026 12:04:02 +0200 Subject: [PATCH 01/18] fix: Mount the log volume into the info-fetcher sidecars The user-info-fetcher and resource-info-fetcher both write their file logs below /stackable/log (via FILE_LOG_DIRECTORY), but neither mounted the shared `log` volume. Their logs therefore landed in the container's own filesystem, where the Vector agent - which only reads the shared volume - could not collect them, and their size was not accounted for in the volume's size limit. Mount the volume in both sidecars and budget for their log files in calculate_log_volume_size_limit, but only when the respective sidecar is actually configured, so clusters without info-fetchers keep their current size limit. The kuttl logging test now runs both sidecars and asserts that their logs reach the Vector aggregator. --- CHANGELOG.md | 3 + .../properties/product_logging/vector.yaml | 2 +- .../build/resource/daemonset/mod.rs | 146 ++++++++++++++++-- .../daemonset/resource_info_fetcher.rs | 12 +- .../resource/daemonset/user_info_fetcher.rs | 14 +- .../kuttl/logging/03-install-opa.yaml.j2 | 30 ++++ .../opa-vector-aggregator-values.yaml.j2 | 12 ++ 7 files changed, 200 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9e5c6889..b872c838 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,9 @@ All notable changes to this project will be documented in this file. - Fix a longstanding problem of including empty `categories`, `shortNames` and `additionalPrinterColumns` in the CRDs, 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]). +- The file logs of the user-info-fetcher and resource-info-fetcher sidecars are now collected by the + Vector agent. Both sidecars log below `/stackable/log`, but did not mount the shared `log` volume, + so their logs were unreachable for Vector and not accounted for in the volume's size limit ([#863]). [#852]: https://github.com/stackabletech/opa-operator/pull/852 [#861]: https://github.com/stackabletech/opa-operator/pull/861 diff --git a/rust/operator-binary/src/controller/build/properties/product_logging/vector.yaml b/rust/operator-binary/src/controller/build/properties/product_logging/vector.yaml index eb01ff24..244b4f6d 100644 --- a/rust/operator-binary/src/controller/build/properties/product_logging/vector.yaml +++ b/rust/operator-binary/src/controller/build/properties/product_logging/vector.yaml @@ -20,7 +20,7 @@ sources: include: - ${LOG_DIR}/*/*.stderr.log - # Logs of the Stackable Rust sidecars (bundle-builder, user-info-fetcher) + # Logs of the Stackable Rust sidecars (bundle-builder, user-info-fetcher, resource-info-fetcher) files_tracing_rs: type: file include: diff --git a/rust/operator-binary/src/controller/build/resource/daemonset/mod.rs b/rust/operator-binary/src/controller/build/resource/daemonset/mod.rs index e6ab8c44..3c867046 100644 --- a/rust/operator-binary/src/controller/build/resource/daemonset/mod.rs +++ b/rust/operator-binary/src/controller/build/resource/daemonset/mod.rs @@ -28,7 +28,9 @@ use stackable_operator::{ ResourceRequirements, }, }, - apimachinery::pkg::{apis::meta::v1::LabelSelector, util::intstr::IntOrString}, + apimachinery::pkg::{ + api::resource::Quantity, apis::meta::v1::LabelSelector, util::intstr::IntOrString, + }, }, memory::{BinaryMultiple, MemoryQuantity}, product_logging::{ @@ -133,6 +135,14 @@ const MAX_PREPARE_LOG_FILE_SIZE: MemoryQuantity = MemoryQuantity { unit: BinaryMultiple::Mebi, }; +// The info-fetcher sidecars log one line per startup and one per failed request, so they are far +// less chatty than the bundle-builder. They are only budgeted for when the corresponding sidecar is +// actually part of the Pod, see `log_volume_size_limit`. +const MAX_INFO_FETCHER_LOG_FILE_SIZE: MemoryQuantity = MemoryQuantity { + value: 5.0, + unit: BinaryMultiple::Mebi, +}; + #[derive(Snafu, Debug)] pub enum Error { #[snafu(display("failed to configure graceful shutdown"))] @@ -385,13 +395,7 @@ pub fn build_server_rolegroup_daemonset( VolumeBuilder::new(LOG_VOLUME_NAME.as_ref()) .empty_dir(EmptyDirVolumeSource { medium: None, - size_limit: Some(product_logging::framework::calculate_log_volume_size_limit( - &[ - MAX_OPA_BUNDLE_BUILDER_LOG_FILE_SIZE, - MAX_OPA_LOG_FILE_SIZE, - MAX_PREPARE_LOG_FILE_SIZE, - ], - )), + size_limit: Some(log_volume_size_limit(cluster)), }) .build(), ) @@ -507,9 +511,33 @@ pub fn build_server_rolegroup_daemonset( }) } +/// The size limit of the shared `log` [`EmptyDirVolumeSource`], which has to accommodate the log +/// files of every container that mounts it. The always-present containers are budgeted for +/// unconditionally; the optional info-fetcher sidecars only when the cluster configures them. +fn log_volume_size_limit(cluster: &ValidatedCluster) -> Quantity { + let mut max_log_file_sizes = vec![ + MAX_OPA_BUNDLE_BUILDER_LOG_FILE_SIZE, + MAX_OPA_LOG_FILE_SIZE, + MAX_PREPARE_LOG_FILE_SIZE, + ]; + + if cluster.cluster_config.user_info.is_some() { + max_log_file_sizes.push(MAX_INFO_FETCHER_LOG_FILE_SIZE); + } + if cluster.cluster_config.resource_info.is_some() { + max_log_file_sizes.push(MAX_INFO_FETCHER_LOG_FILE_SIZE); + } + + product_logging::framework::calculate_log_volume_size_limit(&max_log_file_sizes) +} + /// Env variables that are need to run stackable Rust binaries, such as /// * opa-bundle-builder /// * user-info-fetcher +/// * resource-info-fetcher +/// +/// Note that [`FILE_LOG_DIRECTORY_ENV`] points below [`STACKABLE_LOG_DIR`], so every container this +/// is applied to has to mount the `log` volume for the Vector agent to see its logs. fn add_stackable_rust_cli_env_vars( container_builder: &mut ContainerBuilder, cluster_info: &KubernetesClusterInfo, @@ -1008,7 +1036,7 @@ mod tests { ); } - fn uif_container(ds: &DaemonSet) -> Container { + fn container_by_name(ds: &DaemonSet, name: &str) -> Container { ds.spec .as_ref() .unwrap() @@ -1018,11 +1046,15 @@ mod tests { .unwrap() .containers .iter() - .find(|c| c.name == "user-info-fetcher") - .expect("the user-info-fetcher container should exist") + .find(|c| c.name == name) + .unwrap_or_else(|| panic!("the {name} container should exist")) .clone() } + fn uif_container(ds: &DaemonSet) -> Container { + container_by_name(ds, "user-info-fetcher") + } + fn env_var(container: &Container, name: &str) -> String { container .env @@ -1140,4 +1172,96 @@ mod tests { "/stackable/credentials" ); } + + /// A cluster running both info-fetcher sidecars, so their shared wiring can be asserted in one go. + fn cluster_with_both_info_fetchers() -> ValidatedCluster { + validated_cluster_from_spec(json!({ + "image": { "productVersion": "1.2.3" }, + "clusterConfig": { + "userInfo": { + "backend": { + "experimentalXfscAas": { + "hostname": "aas.default.svc.cluster.local", + "port": 5000, + } + } + }, + "resourceInfo": { + "backend": { + "dataHub": { + "hostname": "datahub-gms.default.svc.cluster.local", + "credentialsSecretName": "datahub-credentials", + } + } + }, + }, + "servers": { "roleGroups": { "default": {} } }, + })) + } + + fn log_volume_size_limit(ds: &DaemonSet) -> Quantity { + ds.spec + .as_ref() + .unwrap() + .template + .spec + .as_ref() + .unwrap() + .volumes + .as_ref() + .unwrap() + .iter() + .find(|volume| volume.name == LOG_VOLUME_NAME.as_ref()) + .expect("the log volume should exist") + .empty_dir + .as_ref() + .expect("the log volume should be an emptyDir") + .size_limit + .clone() + .expect("the log volume should have a size limit") + } + + /// Both sidecars write their file logs below `STACKABLE_LOG_DIR`, so they have to mount the `log` + /// volume - otherwise the logs land in the container's own filesystem where the Vector agent, + /// which only sees the shared volume, cannot collect them. + #[test] + fn info_fetcher_sidecars_mount_the_log_volume() { + let ds = build(&cluster_with_both_info_fetchers()); + + for container_name in ["user-info-fetcher", "resource-info-fetcher"] { + let container = container_by_name(&ds, container_name); + assert_eq!( + mount_path(&container, "log"), + "/stackable/log", + "{container_name} should mount the log volume" + ); + // The directory the sidecar logs into must be inside the mounted volume. + assert_eq!( + env_var(&container, "FILE_LOG_DIRECTORY"), + format!("/stackable/log/{container_name}") + ); + } + } + + /// The sidecars share the `log` volume with the other containers, so their log files have to be + /// budgeted for in its size limit as well. + #[test] + fn log_volume_size_limit_accounts_for_the_info_fetcher_sidecars() { + let without_sidecars = build(&validated_cluster_from_spec(json!({ + "image": { "productVersion": "1.2.3" }, + "servers": { "roleGroups": { "default": {} } }, + }))); + let with_sidecars = build(&cluster_with_both_info_fetchers()); + + // prepare + opa + bundle-builder + assert_eq!( + log_volume_size_limit(&without_sidecars), + Quantity("108Mi".to_owned()) + ); + // ... plus the two info-fetcher sidecars + assert_eq!( + log_volume_size_limit(&with_sidecars), + Quantity("138Mi".to_owned()) + ); + } } diff --git a/rust/operator-binary/src/controller/build/resource/daemonset/resource_info_fetcher.rs b/rust/operator-binary/src/controller/build/resource/daemonset/resource_info_fetcher.rs index 21f64222..b4bc10bf 100644 --- a/rust/operator-binary/src/controller/build/resource/daemonset/resource_info_fetcher.rs +++ b/rust/operator-binary/src/controller/build/resource/daemonset/resource_info_fetcher.rs @@ -16,9 +16,10 @@ use crate::controller::{ build::{ self, resource::daemonset::{ - 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, + CONFIG_DIR, CONFIG_VOLUME_NAME, LOG_VOLUME_NAME, RESOURCE_INFO_FETCHER_CREDENTIALS_DIR, + RESOURCE_INFO_FETCHER_CREDENTIALS_VOLUME_NAME, STACKABLE_LOG_DIR, + add_stackable_rust_cli_env_vars, container_name, sidecar_container_log_level, + sidecar_resource_requirements, }, }, }; @@ -66,6 +67,11 @@ pub fn add_resource_info_fetcher_sidecar( .add_env_var("CREDENTIALS_DIR", RESOURCE_INFO_FETCHER_CREDENTIALS_DIR) .add_volume_mount(CONFIG_VOLUME_NAME.as_ref(), CONFIG_DIR) .context(AddVolumeMountSnafu)? + // The sidecar writes its file logs below this directory (see + // `add_stackable_rust_cli_env_vars`). They have to land on the shared log volume, + // because that is the only place the Vector agent collects them from. + .add_volume_mount(LOG_VOLUME_NAME.as_ref(), STACKABLE_LOG_DIR) + .context(AddVolumeMountSnafu)? .resources(sidecar_resource_requirements()); add_stackable_rust_cli_env_vars( &mut cb_rif, diff --git a/rust/operator-binary/src/controller/build/resource/daemonset/user_info_fetcher.rs b/rust/operator-binary/src/controller/build/resource/daemonset/user_info_fetcher.rs index 31b4f542..81d59ecf 100644 --- a/rust/operator-binary/src/controller/build/resource/daemonset/user_info_fetcher.rs +++ b/rust/operator-binary/src/controller/build/resource/daemonset/user_info_fetcher.rs @@ -22,10 +22,11 @@ use crate::controller::{ build::{ self, resource::daemonset::{ - 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, - container_name, sidecar_container_log_level, sidecar_resource_requirements, + CONFIG_DIR, CONFIG_VOLUME_NAME, LOG_VOLUME_NAME, STACKABLE_LOG_DIR, + 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, container_name, sidecar_container_log_level, + sidecar_resource_requirements, }, }, }; @@ -93,6 +94,11 @@ pub fn add_user_info_fetcher_sidecar( .add_env_var("CREDENTIALS_DIR", USER_INFO_FETCHER_CREDENTIALS_DIR) .add_volume_mount(CONFIG_VOLUME_NAME.as_ref(), CONFIG_DIR) .context(AddVolumeMountSnafu)? + // The sidecar writes its file logs below this directory (see + // `add_stackable_rust_cli_env_vars`). They have to land on the shared log volume, + // because that is the only place the Vector agent collects them from. + .add_volume_mount(LOG_VOLUME_NAME.as_ref(), STACKABLE_LOG_DIR) + .context(AddVolumeMountSnafu)? .resources(sidecar_resource_requirements()); add_stackable_rust_cli_env_vars( &mut cb_user_info_fetcher, diff --git a/tests/templates/kuttl/logging/03-install-opa.yaml.j2 b/tests/templates/kuttl/logging/03-install-opa.yaml.j2 index bf29f3ee..f6b7bef2 100644 --- a/tests/templates/kuttl/logging/03-install-opa.yaml.j2 +++ b/tests/templates/kuttl/logging/03-install-opa.yaml.j2 @@ -17,6 +17,16 @@ data: false } --- +apiVersion: v1 +kind: Secret +metadata: + name: datahub-credentials +stringData: + # The resource-info-fetcher only reads this token at startup; this test never sends it a request, + # so the token does not have to be valid and the configured DataHub does not have to exist. The + # sidecar is only here so that its logs can be asserted on. + token: not-a-real-datahub-token +--- apiVersion: opa.stackable.tech/v1alpha1 kind: OpaCluster metadata: @@ -32,6 +42,16 @@ spec: pullPolicy: IfNotPresent clusterConfig: vectorAggregatorConfigMapName: opa-vector-aggregator-discovery + # Both info-fetcher sidecars are enabled so that the test covers the collection of their file + # logs as well. Neither of them is queried by this test. + userInfo: + backend: + none: {} + resourceInfo: + backend: + dataHub: + hostname: datahub-gms + credentialsSecretName: datahub-credentials servers: roleGroups: automatic-log-config: @@ -52,6 +72,16 @@ spec: level: NONE file: level: INFO + user-info-fetcher: + console: + level: NONE + file: + level: INFO + resource-info-fetcher: + console: + level: NONE + file: + level: INFO vector: console: level: INFO diff --git a/tests/templates/kuttl/logging/opa-vector-aggregator-values.yaml.j2 b/tests/templates/kuttl/logging/opa-vector-aggregator-values.yaml.j2 index 4bb9fdc4..64f3e288 100644 --- a/tests/templates/kuttl/logging/opa-vector-aggregator-values.yaml.j2 +++ b/tests/templates/kuttl/logging/opa-vector-aggregator-values.yaml.j2 @@ -44,6 +44,18 @@ customConfig: condition: >- starts_with(string!(.pod), "test-opa-server-automatic-log-config") && .container == "bundle-builder" + filteredAutomaticLogConfigServerUserInfoFetcher: + type: filter + inputs: [validEvents] + condition: >- + starts_with(string!(.pod), "test-opa-server-automatic-log-config") && + .container == "user-info-fetcher" + filteredAutomaticLogConfigServerResourceInfoFetcher: + type: filter + inputs: [validEvents] + condition: >- + starts_with(string!(.pod), "test-opa-server-automatic-log-config") && + .container == "resource-info-fetcher" filteredAutomaticLogConfigServerVector: type: filter inputs: [validEvents] From 38d4773140836966ad7b01804239f35b8673b7df Mon Sep 17 00:00:00 2001 From: Malte Sander Date: Mon, 17 Aug 2026 12:12:00 +0200 Subject: [PATCH 02/18] docs: Document that a failed metadata lookup does not deny by itself The docs only mentioned that a resource unknown to the backend yields an empty record. A backend that is unavailable - down, unreachable, or an expired Personal Access Token - has the same effect on a Rego rule: it finds no tags to match on, the expression becomes undefined, and an undefined `deny` means not denied. A deny-list rule therefore grants access to every resource, including the ones it is meant to exclude. --- .../usage-guide/resource-info-fetcher.adoc | 38 +++++++++++++++++-- 1 file changed, 35 insertions(+), 3 deletions(-) diff --git a/docs/modules/opa/pages/usage-guide/resource-info-fetcher.adoc b/docs/modules/opa/pages/usage-guide/resource-info-fetcher.adoc index a75f5526..4908a8d2 100644 --- a/docs/modules/opa/pages/usage-guide/resource-info-fetcher.adoc +++ b/docs/modules/opa/pages/usage-guide/resource-info-fetcher.adoc @@ -174,6 +174,38 @@ allow if { } ---- -A resource the backend does not know about is not reported as an error: the resource-info-fetcher returns a record with empty `tags`, `owners` and `dataProducts` and a `null` `domain`. -Prefer rules that require a positive signal, like the one above, which denies access in that case. -A rule that merely excludes a tag would instead grant access to every resource missing from the backend. +=== Behaviour when metadata is unavailable + +[WARNING] +==== +A failed metadata lookup does not deny access by itself. +Only rules that require a positive signal deny access when the lookup fails - see below. +==== + +There are two ways a lookup can come back without the metadata a rule expects. + +Unknown resource:: +The backend does not know the resource. +This is not an error: the resource-info-fetcher answers `200 OK` with a record whose `tags`, `owners` and `dataProducts` are empty and whose `domain` is `null`. +It is the normal answer for a resource that was never ingested into the catalog. + +Backend unavailable:: +The backend cannot be reached or rejects the request, for example because DataHub is down or unreachable, or because the Personal Access Token has expired or been revoked. +The resource-info-fetcher then answers with an HTTP error status and an error envelope instead of a metadata record: ++ +[source,json] +---- +{"error": {"message": "...", "causes": ["..."]}} +---- + +In both cases the rule finds no `tags` (and no `owners`, `domain` or `dataProducts`) to match on, so every expression reading them becomes undefined. +What that means for the decision depends entirely on how the rule is written: + +* A rule that grants access on a *positive* signal - such as the `allow` rule above, which requires the `public` tag - becomes undefined and therefore denies access. This is what you want. +* A rule that grants access through the *absence* of a signal - `deny` if the resource is tagged `pii`, everything else allowed - also becomes undefined, and an undefined `deny` means *not denied*. A DataHub outage or an expired token then grants access to every resource, including the ones tagged `pii`. + +So always write rules that require a positive signal. +The resource-info-fetcher deliberately does not paper over the difference between the two cases, because it cannot know whether an empty record should mean allow or deny for a given policy. +This holds for a resource that is simply not in the catalog just as much as for a backend outage: neither can be turned into a denial by the resource-info-fetcher, only by the shape of the rule. + +Every failed lookup is logged by the resource-info-fetcher sidecar at `WARN` level, so a backend that has become unavailable is visible in the logs (see xref:opa:usage-guide/logging.adoc[]). From dde8df32a25f4f107dfb803b20b74a7d92872fc1 Mon Sep 17 00:00:00 2001 From: Malte Sander Date: Mon, 17 Aug 2026 12:59:48 +0200 Subject: [PATCH 03/18] chore: Remove dead code from the resource-info-fetcher Drop the `urlencoding` workspace dependency, which was added but never used by any crate. It was not referenced in Cargo.lock or Cargo.nix either, so neither needs regenerating. Split `data_hub::Error` into `ResolveError` for the startup path and `Error` for the request path. `ReadToken`, `ConfigureTls`, `ConstructHttpClient` and `BuildDataHubEndpoint` can only occur while resolving the backend, which happens before the server starts listening, so they never reach a caller and had no business claiming an HTTP status code. `status_code` now has three reachable arms instead of seven. --- Cargo.toml | 1 - .../src/backend/data_hub/mod.rs | 19 ++++++++++++------- rust/resource-info-fetcher/src/main.rs | 4 +++- 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 78011986..b64a7e7d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -49,7 +49,6 @@ tar = "0.4" tokio = { version = "1.53", features = ["full"] } tracing = "0.1" url = "2.5" -urlencoding = "2.1" uuid = "1.24" wiremock = "0.6" diff --git a/rust/resource-info-fetcher/src/backend/data_hub/mod.rs b/rust/resource-info-fetcher/src/backend/data_hub/mod.rs index cd55d79b..6e224db1 100644 --- a/rust/resource-info-fetcher/src/backend/data_hub/mod.rs +++ b/rust/resource-info-fetcher/src/backend/data_hub/mod.rs @@ -23,8 +23,12 @@ use crate::{ mod graphql; mod resource_to_urn_mapping; +/// Errors that can occur while resolving the backend, which happens once at startup. +/// +/// Kept apart from [`Error`] because these never reach a caller: a failure here means the process +/// does not come up at all, so - unlike [`Error`] - they have no HTTP status code to map to. #[derive(Snafu, Debug)] -pub enum Error { +pub enum ResolveError { #[snafu(display("failed to read DataHub token from {path:?}"))] ReadToken { source: std::io::Error, @@ -42,7 +46,12 @@ pub enum Error { source: url::ParseError, endpoint: String, }, +} +/// Errors that can occur while answering a request, and which are therefore rendered as an HTTP +/// response to the caller. +#[derive(Snafu, Debug)] +pub enum Error { #[snafu(display("failed to execute GraphQL query for URN {urn:?}"))] ExecuteGraphQlQuery { source: utils::http::Error, @@ -64,10 +73,6 @@ pub enum Error { impl http_error::Error for Error { fn status_code(&self) -> StatusCode { match self { - Self::ReadToken { .. } => StatusCode::SERVICE_UNAVAILABLE, - Self::ConfigureTls { .. } => StatusCode::SERVICE_UNAVAILABLE, - Self::ConstructHttpClient { .. } => StatusCode::SERVICE_UNAVAILABLE, - Self::BuildDataHubEndpoint { .. } => StatusCode::BAD_REQUEST, Self::ExecuteGraphQlQuery { .. } => StatusCode::INTERNAL_SERVER_ERROR, Self::GraphQlErrors { .. } => StatusCode::INTERNAL_SERVER_ERROR, Self::TruncatedDataProducts { .. } => StatusCode::INTERNAL_SERVER_ERROR, @@ -177,7 +182,7 @@ impl ResolvedDataHubBackend { pub async fn resolve( config: v1alpha1::DataHubBackend, credentials_dir: &Path, - ) -> Result { + ) -> Result { let token_path = credentials_dir.join("token"); // Trim trailing whitespace/newlines so the value is safe to use in an HTTP header. @@ -264,7 +269,7 @@ impl ResolvedDataHubBackend { } /// Builds the DataHub GraphQL endpoint from the backend configuration. -fn build_graphql_url(config: &v1alpha1::DataHubBackend) -> Result { +fn build_graphql_url(config: &v1alpha1::DataHubBackend) -> Result { let schema = if config.tls.uses_tls() { "https" } else { diff --git a/rust/resource-info-fetcher/src/main.rs b/rust/resource-info-fetcher/src/main.rs index 6bbf79d8..d5aa5d5a 100644 --- a/rust/resource-info-fetcher/src/main.rs +++ b/rust/resource-info-fetcher/src/main.rs @@ -82,7 +82,9 @@ enum StartupError { RunServer { source: std::io::Error }, #[snafu(display("failed to resolve DataHub backend"))] - ResolveDataHubBackend { source: backend::data_hub::Error }, + ResolveDataHubBackend { + source: backend::data_hub::ResolveError, + }, } /// Resolves a backend configuration by loading credentials and creating the appropriate backend implementation. From 28a18084d8a24abb16b05bbee967288ebf46b0c9 Mon Sep 17 00:00:00 2001 From: Malte Sander Date: Mon, 17 Aug 2026 13:02:23 +0200 Subject: [PATCH 04/18] fix: Treat dashboard and chart ids as opaque strings `Dashboard::id` and `Chart::id` were `u64`, which bakes Superset's numbering into a product-agnostic API. Other products identify dashboards and charts by name, and the id is only ever spliced into the URN, so nothing here needs it to be an integer. Widen both to `String` and cover the URN construction with tests for a numeric and a non-numeric id. The Rego library already stringified the id for the query encoder, so callers are unaffected. --- .../usage-guide/resource-info-fetcher.adoc | 3 ++ rust/resource-info-fetcher/src/api.rs | 12 ++++- .../data_hub/resource_to_urn_mapping.rs | 47 +++++++++++++++++++ 3 files changed, 60 insertions(+), 2 deletions(-) diff --git a/docs/modules/opa/pages/usage-guide/resource-info-fetcher.adoc b/docs/modules/opa/pages/usage-guide/resource-info-fetcher.adoc index 4908a8d2..42c313f7 100644 --- a/docs/modules/opa/pages/usage-guide/resource-info-fetcher.adoc +++ b/docs/modules/opa/pages/usage-guide/resource-info-fetcher.adoc @@ -97,6 +97,9 @@ The first two arguments are the same everywhere: * `system` is the kind of product the resource lives in, for example `trino`, `kafka` or `superset`. DataHub calls this the _data platform_. * `instance` identifies _which_ deployment of that product, for example `my-namespace/my-trino`. DataHub calls this the _platform instance_, and the value must match the `platform_instance` of the ingestion source that produced the metadata. +The `id` taken by `dashboardResourceInfo` and `chartResourceInfo` is whatever the product identifies the resource by, and is passed through as an opaque string. +Superset numbers its dashboards and charts, but products that name them instead work just as well. + The DataHub environment (fabric) is deliberately *not* an argument: it describes how the catalog was populated rather than the resource being authorized, so it is configured once on the OpaCluster (see `env` above) instead of being passed in by every Rego rule. An example of the returned structure: diff --git a/rust/resource-info-fetcher/src/api.rs b/rust/resource-info-fetcher/src/api.rs index b270c2ed..cd2e799b 100644 --- a/rust/resource-info-fetcher/src/api.rs +++ b/rust/resource-info-fetcher/src/api.rs @@ -75,14 +75,22 @@ pub struct Stream { pub struct Dashboard { pub system: String, pub instance: String, - pub id: u64, + + /// The dashboard's identifier within its product, treated as an opaque string. + /// + /// Superset numbers its dashboards, but other products (e.g. Looker or Tableau) identify them by + /// name, so this must not be narrowed to an integer. It is only ever spliced into the URN. + pub id: String, } #[derive(Debug, Clone, PartialEq, Eq, Hash, Deserialize)] pub struct Chart { pub system: String, pub instance: String, - pub id: u64, + + /// The chart's identifier within its product, treated as an opaque string. See + /// [`Dashboard::id`]. + pub id: String, } #[derive(Debug, Clone, PartialEq, Eq, Hash, Deserialize)] diff --git a/rust/resource-info-fetcher/src/backend/data_hub/resource_to_urn_mapping.rs b/rust/resource-info-fetcher/src/backend/data_hub/resource_to_urn_mapping.rs index f42d6a28..e40b10a5 100644 --- a/rust/resource-info-fetcher/src/backend/data_hub/resource_to_urn_mapping.rs +++ b/rust/resource-info-fetcher/src/backend/data_hub/resource_to_urn_mapping.rs @@ -87,3 +87,50 @@ fn container_urn( .expect("serializing a BTreeMap<&str, &str> cannot fail"); format!("urn:li:container:{:x}", md5::compute(key_json.as_bytes())) } + +#[cfg(test)] +mod tests { + use rstest::rstest; + + use super::*; + + /// Dashboard and chart ids are opaque to us. Superset happens to number them, but other + /// platforms (e.g. Looker or Tableau) identify their dashboards by name, so neither the API nor + /// the URN construction may assume an integer. + /// + /// Neither URN carries the fabric, so the configured [`v1alpha1::FabricType`] is irrelevant here. + #[rstest] + #[case::numeric_id("1", "urn:li:chart:(superset,my-superset.1)")] + #[case::non_numeric_id( + "orders-by-region", + "urn:li:chart:(superset,my-superset.orders-by-region)" + )] + fn chart_urn(#[case] id: &str, #[case] expected_urn: &str) { + let request = ResourceInfoRequest::Chart(Chart { + system: "superset".to_owned(), + instance: "my-superset".to_owned(), + id: id.to_owned(), + }); + + assert_eq!( + urn_for_request(&request, &v1alpha1::FabricType::Prod).0, + expected_urn + ); + } + + #[rstest] + #[case::numeric_id("1", "urn:li:dashboard:(superset,my-superset.1)")] + #[case::non_numeric_id("sales", "urn:li:dashboard:(superset,my-superset.sales)")] + fn dashboard_urn(#[case] id: &str, #[case] expected_urn: &str) { + let request = ResourceInfoRequest::Dashboard(Dashboard { + system: "superset".to_owned(), + instance: "my-superset".to_owned(), + id: id.to_owned(), + }); + + assert_eq!( + urn_for_request(&request, &v1alpha1::FabricType::Prod).0, + expected_urn + ); + } +} From 2ebbd09b14ebd50ee8683e03d33e78dac2bc322b Mon Sep 17 00:00:00 2001 From: Malte Sander Date: Mon, 17 Aug 2026 13:09:31 +0200 Subject: [PATCH 05/18] fix: Bound the length of query parameter values Parameter values end up in the response cache key, so an unbounded value let any caller who can name a resource fill the cache with large keys - a 60 KB table name was accepted and cached. Real identifiers are nowhere near that; even a fully qualified DataHub URN stays in the low hundreds of bytes. Introduce a `ParamValue` newtype that rejects values over 1024 bytes while deserializing, so an over-long value is reported through the same 400 envelope as any other malformed parameter and never reaches the backend or the cache. It can only be constructed by deserializing, outside of tests. `ParamValue` is part of the input to the container URN hashes, so two tests pin that it stays invisible in the serialized form: one on the serialization itself, and a golden URN cross-checked against an independent Python computation of DataHub's `datahub_guid`. --- .../usage-guide/resource-info-fetcher.adoc | 3 + rust/resource-info-fetcher/src/api.rs | 152 +++++++++++++++--- .../data_hub/resource_to_urn_mapping.rs | 34 +++- 3 files changed, 160 insertions(+), 29 deletions(-) diff --git a/docs/modules/opa/pages/usage-guide/resource-info-fetcher.adoc b/docs/modules/opa/pages/usage-guide/resource-info-fetcher.adoc index 42c313f7..70800887 100644 --- a/docs/modules/opa/pages/usage-guide/resource-info-fetcher.adoc +++ b/docs/modules/opa/pages/usage-guide/resource-info-fetcher.adoc @@ -100,6 +100,9 @@ The first two arguments are the same everywhere: The `id` taken by `dashboardResourceInfo` and `chartResourceInfo` is whatever the product identifies the resource by, and is passed through as an opaque string. Superset numbers its dashboards and charts, but products that name them instead work just as well. +Every argument is limited to 1024 bytes, which is far more than any real identifier needs. +A longer value is rejected with `400 Bad Request` rather than looked up, because arguments end up in the response cache key. + The DataHub environment (fabric) is deliberately *not* an argument: it describes how the catalog was populated rather than the resource being authorized, so it is configured once on the OpaCluster (see `env` above) instead of being passed in by every Rego rule. An example of the returned structure: diff --git a/rust/resource-info-fetcher/src/api.rs b/rust/resource-info-fetcher/src/api.rs index cd2e799b..c0ef8d21 100644 --- a/rust/resource-info-fetcher/src/api.rs +++ b/rust/resource-info-fetcher/src/api.rs @@ -1,3 +1,5 @@ +use std::fmt; + use hyper::StatusCode; use info_fetcher_commons::http_error; use serde::{Deserialize, Serialize}; @@ -38,64 +40,122 @@ pub enum ResourceInfoRequest { RawIdentifier(RawIdentifier), } +/// The maximum length, in bytes, of a single query parameter value. +/// +/// Every parameter value ends up in the response cache key, so without a bound any caller who can +/// name a resource can fill the cache with arbitrarily large keys. Real identifiers are nowhere near +/// this: even a fully qualified DataHub URN stays in the low hundreds of bytes. +const MAX_PARAM_VALUE_LENGTH: usize = 1024; + +/// A query parameter value, bounded to [`MAX_PARAM_VALUE_LENGTH`] bytes. +/// +/// The bound is enforced while deserializing, so an over-long value is reported through the same +/// `400` envelope as any other malformed parameter (see [`crate::MetadataQuery`]) and never reaches +/// the backend or the cache. +/// +/// Serializes as a plain string, so the container key hashes in +/// [`urn_for_request`](crate::backend::data_hub::resource_to_urn_mapping::urn_for_request) are +/// unaffected by the wrapper. +#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize)] +pub struct ParamValue(String); + +impl<'de> Deserialize<'de> for ParamValue { + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + let value = String::deserialize(deserializer)?; + + if value.len() > MAX_PARAM_VALUE_LENGTH { + return Err(serde::de::Error::custom(format!( + "value is {length} bytes long, but at most {MAX_PARAM_VALUE_LENGTH} are allowed", + length = value.len(), + ))); + } + + Ok(Self(value)) + } +} + +impl fmt::Display for ParamValue { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str(&self.0) + } +} + +impl AsRef for ParamValue { + fn as_ref(&self) -> &str { + &self.0 + } +} + +/// Only available in tests: outside of them a [`ParamValue`] is always deserialized from a request, +/// which is where the length bound has to be enforced. +#[cfg(test)] +impl From<&str> for ParamValue { + fn from(value: &str) -> Self { + Self(value.to_owned()) + } +} + #[derive(Debug, Clone, PartialEq, Eq, Hash, Deserialize)] pub struct Database { - pub system: String, - pub instance: String, - pub database: String, + pub system: ParamValue, + pub instance: ParamValue, + pub database: ParamValue, } #[derive(Debug, Clone, PartialEq, Eq, Hash, Deserialize)] pub struct Schema { - pub system: String, - pub instance: String, - pub database: String, - pub schema: String, + pub system: ParamValue, + pub instance: ParamValue, + pub database: ParamValue, + pub schema: ParamValue, } #[derive(Debug, Clone, PartialEq, Eq, Hash, Deserialize)] pub struct Table { - pub system: String, - pub instance: String, - pub database: String, - pub schema: String, - pub table: String, + pub system: ParamValue, + pub instance: ParamValue, + pub database: ParamValue, + pub schema: ParamValue, + pub table: ParamValue, } #[derive(Debug, Clone, PartialEq, Eq, Hash, Deserialize)] pub struct Stream { - pub system: String, - pub instance: String, + pub system: ParamValue, + pub instance: ParamValue, /// AKA topic - pub queue: String, + pub queue: ParamValue, } #[derive(Debug, Clone, PartialEq, Eq, Hash, Deserialize)] pub struct Dashboard { - pub system: String, - pub instance: String, + pub system: ParamValue, + pub instance: ParamValue, /// The dashboard's identifier within its product, treated as an opaque string. /// /// Superset numbers its dashboards, but other products (e.g. Looker or Tableau) identify them by /// name, so this must not be narrowed to an integer. It is only ever spliced into the URN. - pub id: String, + pub id: ParamValue, } #[derive(Debug, Clone, PartialEq, Eq, Hash, Deserialize)] pub struct Chart { - pub system: String, - pub instance: String, + pub system: ParamValue, + pub instance: ParamValue, /// The chart's identifier within its product, treated as an opaque string. See /// [`Dashboard::id`]. - pub id: String, + pub id: ParamValue, } #[derive(Debug, Clone, PartialEq, Eq, Hash, Deserialize)] pub struct RawIdentifier { - pub identifier: String, + pub identifier: ParamValue, } /// Generates the trivial `From for ResourceInfoRequest` conversions, so each HTTP handler @@ -149,3 +209,51 @@ impl http_error::Error for GetResourceInfoError { } } } + +#[cfg(test)] +mod tests { + use serde_json::json; + + use super::*; + + /// Deserializes a [`Table`] whose `table` parameter is `length` bytes long. + fn table_with_name_of_length(length: usize) -> Result { + serde_json::from_value(json!({ + "system": "trino", + "instance": "my-trino", + "database": "tpch", + "schema": "sf1", + "table": "a".repeat(length), + })) + } + + /// Parameter values end up in the response cache key, so an unbounded one lets any caller who can + /// name a table fill the cache with megabyte-sized keys. They are bounded while deserializing, + /// which is the same path that renders a `400` for any other malformed parameter. + #[test] + fn param_values_within_the_limit_are_accepted() { + table_with_name_of_length(MAX_PARAM_VALUE_LENGTH) + .expect("a value at the limit must be accepted"); + } + + #[test] + fn param_values_over_the_limit_are_rejected() { + let error = table_with_name_of_length(MAX_PARAM_VALUE_LENGTH + 1) + .expect_err("a value over the limit must be rejected"); + + assert!( + error.to_string().contains("1025 bytes"), + "the error should report the offending length, but was: {error}" + ); + } + + /// The wrapper must stay invisible in the serialized form, because the container URNs are MD5 + /// hashes over the serialized parameters and have to keep matching DataHub's own hashes. + #[test] + fn param_values_serialize_as_plain_strings() { + let serialized = serde_json::to_string(&ParamValue::from("tpch")) + .expect("a param value must be serializable"); + + assert_eq!(serialized, r#""tpch""#); + } +} diff --git a/rust/resource-info-fetcher/src/backend/data_hub/resource_to_urn_mapping.rs b/rust/resource-info-fetcher/src/backend/data_hub/resource_to_urn_mapping.rs index e40b10a5..3aab27ae 100644 --- a/rust/resource-info-fetcher/src/backend/data_hub/resource_to_urn_mapping.rs +++ b/rust/resource-info-fetcher/src/backend/data_hub/resource_to_urn_mapping.rs @@ -65,7 +65,7 @@ pub fn urn_for_request(request: &ResourceInfoRequest, env: &v1alpha1::FabricType }) => { format!("urn:li:chart:({system},{instance}.{id})") } - ResourceInfoRequest::RawIdentifier(RawIdentifier { identifier }) => identifier.clone(), + ResourceInfoRequest::RawIdentifier(RawIdentifier { identifier }) => identifier.to_string(), }; Urn(urn) @@ -107,9 +107,9 @@ mod tests { )] fn chart_urn(#[case] id: &str, #[case] expected_urn: &str) { let request = ResourceInfoRequest::Chart(Chart { - system: "superset".to_owned(), - instance: "my-superset".to_owned(), - id: id.to_owned(), + system: "superset".into(), + instance: "my-superset".into(), + id: id.into(), }); assert_eq!( @@ -118,14 +118,34 @@ mod tests { ); } + /// Guards the container hash, which DataHub computes independently on ingestion: it has to keep + /// matching byte for byte, or every database and schema lookup silently stops resolving. The + /// expected value was computed with Python's + /// `json.dumps(key, sort_keys=True, separators=(",", ":"))` and `hashlib.md5`, mirroring what + /// DataHub's `datahub_guid` does. + #[test] + fn schema_container_urn_matches_datahubs_guid() { + let request = ResourceInfoRequest::Schema(Schema { + system: "trino".into(), + instance: "my-namespace/my-trino".into(), + database: "tpch".into(), + schema: "sf1".into(), + }); + + assert_eq!( + urn_for_request(&request, &v1alpha1::FabricType::Prod).0, + "urn:li:container:fb46bf1f985e130eeceeee8a51317cd9" + ); + } + #[rstest] #[case::numeric_id("1", "urn:li:dashboard:(superset,my-superset.1)")] #[case::non_numeric_id("sales", "urn:li:dashboard:(superset,my-superset.sales)")] fn dashboard_urn(#[case] id: &str, #[case] expected_urn: &str) { let request = ResourceInfoRequest::Dashboard(Dashboard { - system: "superset".to_owned(), - instance: "my-superset".to_owned(), - id: id.to_owned(), + system: "superset".into(), + instance: "my-superset".into(), + id: id.into(), }); assert_eq!( From 3836ea13b6f74869252837d148842f981a84fad0 Mon Sep 17 00:00:00 2001 From: Malte Sander Date: Mon, 17 Aug 2026 13:12:34 +0200 Subject: [PATCH 06/18] fix: Answer 400 instead of 500 for identifiers DataHub rejects The URN we query is built entirely from the caller's parameters, so a URN that DataHub refuses to parse or resolve is a bad request, not a server fault. Any user who can name a table can trigger it, for example through Trino's `SELECT * FROM tpch.sf1."a,PROD)"`. Map `GraphQlErrors` to 400 and cover all three request-error arms with a test, so the mapping is pinned. `ExecuteGraphQlQuery` (DataHub unreachable) and `TruncatedDataProducts` (a limit of our own query) stay 500 - neither is something the caller can act on. --- .../src/backend/data_hub/mod.rs | 43 ++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-) diff --git a/rust/resource-info-fetcher/src/backend/data_hub/mod.rs b/rust/resource-info-fetcher/src/backend/data_hub/mod.rs index 6e224db1..c0d26502 100644 --- a/rust/resource-info-fetcher/src/backend/data_hub/mod.rs +++ b/rust/resource-info-fetcher/src/backend/data_hub/mod.rs @@ -73,8 +73,15 @@ pub enum Error { impl http_error::Error for Error { fn status_code(&self) -> StatusCode { match self { + // We could not talk to DataHub at all, which is not something the caller can fix. Self::ExecuteGraphQlQuery { .. } => StatusCode::INTERNAL_SERVER_ERROR, - Self::GraphQlErrors { .. } => StatusCode::INTERNAL_SERVER_ERROR, + + // The URN is built entirely from the caller's parameters, so a URN DataHub refuses to + // parse or resolve is a bad request rather than a server fault. Any user who can name a + // table can reach this, e.g. through Trino's `SELECT * FROM tpch.sf1."a,PROD)"`. + Self::GraphQlErrors { .. } => StatusCode::BAD_REQUEST, + + // A limitation of our own query, see the variant's message. Self::TruncatedDataProducts { .. } => StatusCode::INTERNAL_SERVER_ERROR, } } @@ -319,6 +326,7 @@ impl ResourceInfoBackend for ResolvedDataHubBackend { mod tests { use rstest::rstest; use serde_json::json; + use snafu::IntoError; use super::*; @@ -353,6 +361,39 @@ mod tests { /// [`Url`] omits the port whenever it is the default port of the scheme, hence the expected /// endpoints of the defaulted cases carry no port. + fn urn() -> Urn { + Urn("urn:li:dataset:(urn:li:dataPlatform:trino,a,PROD)".to_owned()) + } + + /// The status code tells the caller whose problem a failure is. The URN we query is built + /// entirely from the caller's parameters, so a URN DataHub refuses to parse or resolve is a bad + /// request - reachable by any user who can name a table, e.g. via Trino's + /// `SELECT * FROM tpch.sf1."a,PROD)"`. A backend we could not reach at all, or a limitation of + /// our own query, is not something the caller can do anything about. + #[rstest] + #[case::graphql_errors( + GraphQlErrorsSnafu { messages: "Failed to parse urn", urn: urn() }.build(), + StatusCode::BAD_REQUEST + )] + #[case::unreachable_backend( + ExecuteGraphQlQuerySnafu { urn: urn() }.into_error(utils::http::Error::HttpErrorResponse { + status: StatusCode::UNAUTHORIZED, + url: "http://datahub-gms/api/graphql".to_owned(), + text: "Unauthorized".to_owned(), + }), + StatusCode::INTERNAL_SERVER_ERROR + )] + #[case::truncated_data_products( + TruncatedDataProductsSnafu { urn: urn(), total: 11u32, received: 10u32 }.build(), + StatusCode::INTERNAL_SERVER_ERROR + )] + fn status_code(#[case] error: Error, #[case] expected_status_code: StatusCode) { + assert_eq!( + info_fetcher_commons::http_error::Error::status_code(&error), + expected_status_code + ); + } + #[rstest] #[case::default_scheme_and_port(json!({}), format!("http://{HOSTNAME}/api/graphql"))] #[case::default_tls_scheme_and_port( From cd65817d987fe699a17abd409abe3db08d920a8d Mon Sep 17 00:00:00 2001 From: Malte Sander Date: Mon, 17 Aug 2026 13:15:37 +0200 Subject: [PATCH 07/18] fix: Warn when a URN resolves to an entity type we cannot read The query only reads tags, owners and domains off the four entity types it has inline fragments for. Any other type - reachable through `rawIdentifier`, which accepts any URN - still resolves, but only the fields common to every `Entity` come back, so the response is indistinguishable from that of a resource with no metadata at all. Add `__typename` to the fragment and log a warning naming the entity type when it is not one we cover. Also state in the docs which types yield metadata, since the page sold `rawIdentifierResourceInfo` as a general escape hatch. --- .../usage-guide/resource-info-fetcher.adoc | 4 ++ .../src/backend/data_hub/graphql.rs | 67 +++++++++++++++++++ .../src/backend/data_hub/mod.rs | 16 ++++- 3 files changed, 86 insertions(+), 1 deletion(-) diff --git a/docs/modules/opa/pages/usage-guide/resource-info-fetcher.adoc b/docs/modules/opa/pages/usage-guide/resource-info-fetcher.adoc index 70800887..bbd7b762 100644 --- a/docs/modules/opa/pages/usage-guide/resource-info-fetcher.adoc +++ b/docs/modules/opa/pages/usage-guide/resource-info-fetcher.adoc @@ -92,6 +92,10 @@ The naming is intentionally product-agnostic, so that one function serves the eq `rawIdentifierResourceInfo` is the escape hatch for resources the functions above do not cover: it passes the identifier to the backend as-is. For DataHub that is a URN, such as `urn:li:chart:(superset,my-namespace/my-superset.1)`. +It lets you address any URN, but metadata is only read from the DataHub entity types the functions above map to: datasets, containers, charts and dashboards. +A URN of any other type - a `dataJob`, a `dataFlow` or one of the ML entities, for example - resolves to a record with empty `tags`, `owners` and `dataProducts`, which is indistinguishable from a resource that has no metadata. +The resource-info-fetcher logs a warning naming the entity type whenever this happens, so check its logs if a `rawIdentifierResourceInfo` lookup unexpectedly comes back empty. + The first two arguments are the same everywhere: * `system` is the kind of product the resource lives in, for example `trino`, `kafka` or `superset`. DataHub calls this the _data platform_. diff --git a/rust/resource-info-fetcher/src/backend/data_hub/graphql.rs b/rust/resource-info-fetcher/src/backend/data_hub/graphql.rs index d4d77ac2..bf5d9116 100644 --- a/rust/resource-info-fetcher/src/backend/data_hub/graphql.rs +++ b/rust/resource-info-fetcher/src/backend/data_hub/graphql.rs @@ -29,6 +29,11 @@ query ResourceInfo($urn: String!, $dataProductsCount: Int!) { } } fragment ResourceInfo on Entity { + # The concrete type DataHub resolved the URN to. Only the types with an inline fragment below carry + # tags, owners and a domain in our response, so this lets us tell an entity we cannot read from one + # that genuinely has no metadata, see `Entity::uncovered_type`. + __typename + # DataHub has no direct "dataProduct" field on assets; membership is a graph edge that points from # the data product to its assets. From the asset's side it is therefore an INCOMING relationship. # `total` is the number of edges DataHub has, which we compare against the number we received to @@ -119,12 +124,24 @@ pub struct ResponseData { #[derive(Debug, Default, Deserialize)] #[serde(rename_all = "camelCase")] pub struct Entity { + /// The concrete DataHub type the URN resolved to, e.g. `Dataset`. [`None`] for the substituted + /// "no metadata" entity, and for a DataHub that does not report it. + #[serde(rename = "__typename")] + typename: Option, + tags: Option, ownership: Option, domain: Option, data_products: Option, } +/// The entity types [`RESOURCE_INFO_QUERY`] has an inline fragment for, and whose tags, owners and +/// domain we therefore read. +/// +/// Anything else still resolves - `rawIdentifier` accepts any URN - but only the fields common to +/// every `Entity` come back, so the response looks exactly like that of a resource without metadata. +const COVERED_ENTITY_TYPES: &[&str] = &["Dataset", "Container", "Chart", "Dashboard"]; + #[derive(Debug, Deserialize)] struct GlobalTags { tags: Vec, @@ -263,6 +280,19 @@ pub struct DataProductsTruncation { } impl Entity { + /// The entity's DataHub type, if [`RESOURCE_INFO_QUERY`] does not cover it. + /// + /// [`None`] means the type is covered, or that DataHub did not report one - in which case there + /// is nothing to compare against and we must not report a problem we cannot substantiate. + /// + /// Callers should surface this: the response for an uncovered type is empty, and a policy has no + /// way to distinguish that from a resource that carries no tags, owners or domain at all. + pub fn uncovered_type(&self) -> Option<&str> { + let typename = self.typename.as_deref()?; + + (!COVERED_ENTITY_TYPES.contains(&typename)).then_some(typename) + } + /// Checks whether the data product list was truncated by [`DATA_PRODUCTS_PAGE_SIZE`]. /// /// Callers must turn this into an error rather than serving the truncated list: a policy that @@ -457,6 +487,43 @@ mod tests { assert!(Entity::default().data_products_truncation().is_none()); } + /// Deserializes an entity of the given DataHub type, as reported by `__typename`. + fn entity_of_type(typename: Option<&str>) -> Entity { + serde_json::from_value(json!({"__typename": typename})) + .expect("test entity must be a valid GraphQL entity payload") + } + + /// An entity type the query has an inline fragment for is read normally. + #[rstest] + #[case::dataset("Dataset")] + #[case::container("Container")] + #[case::chart("Chart")] + #[case::dashboard("Dashboard")] + fn covered_entity_types(#[case] typename: &str) { + assert_eq!(entity_of_type(Some(typename)).uncovered_type(), None); + } + + /// Any other type deserializes into an entity with no tags, owners or domain, which a policy + /// cannot tell apart from a resource that genuinely has none - so it has to be reported. + #[rstest] + #[case::data_job("DataJob")] + #[case::data_flow("DataFlow")] + #[case::notebook("Notebook")] + #[case::ml_model("MLModel")] + fn uncovered_entity_types(#[case] typename: &str) { + assert_eq!( + entity_of_type(Some(typename)).uncovered_type(), + Some(typename) + ); + } + + /// Without a `__typename` there is nothing to check against, so we must not cry wolf. + #[test] + fn entities_without_a_typename_are_not_reported() { + assert_eq!(entity_of_type(None).uncovered_type(), None); + assert_eq!(Entity::default().uncovered_type(), None); + } + #[test] fn truncated_data_products_are_detected() { let truncation = entity(Some(DATA_PRODUCTS_PAGE_SIZE + 1), DATA_PRODUCTS_PAGE_SIZE) diff --git a/rust/resource-info-fetcher/src/backend/data_hub/mod.rs b/rust/resource-info-fetcher/src/backend/data_hub/mod.rs index c0d26502..856026e4 100644 --- a/rust/resource-info-fetcher/src/backend/data_hub/mod.rs +++ b/rust/resource-info-fetcher/src/backend/data_hub/mod.rs @@ -13,7 +13,7 @@ use reqwest::Url; use serde::{Deserialize, Serialize}; use snafu::{ResultExt, Snafu}; use stackable_opa_operator::crd::resource_info_fetcher::v1alpha1; -use tracing::{debug, instrument, trace}; +use tracing::{debug, instrument, trace, warn}; use crate::{ api::{GetResourceInfoError, ResourceInfoBackend, ResourceInfoRequest}, @@ -257,6 +257,20 @@ impl ResolvedDataHubBackend { return Ok(graphql::Entity::default()); }; + // The query only reads tags, owners and domains off the entity types it has inline fragments + // for. Anything else - reachable through `rawIdentifier`, which accepts any URN - resolves to + // a response that looks just like that of a resource with no metadata, so say so rather than + // letting a policy silently decide on an empty record. + if let Some(entity_type) = entity.uncovered_type() { + warn!( + %urn, + entity_type, + "DataHub resolved this URN to an entity type the resource-info-fetcher cannot read \ + metadata from; answering with empty tags, owners and data products. A policy cannot \ + tell this apart from a resource that has no metadata, so do not rely on it" + ); + } + // Fail loudly instead of serving a partial list of data products: a policy that keys off data // product membership would otherwise silently decide based on incomplete metadata. if let Some(truncation) = entity.data_products_truncation() { From 481baa8821e8782d86720b1c47cc52fa0c461c79 Mon Sep 17 00:00:00 2001 From: Malte Sander Date: Mon, 17 Aug 2026 13:19:18 +0200 Subject: [PATCH 08/18] fix: Raise the data product page size to 1000 The page size was 10, and exceeding it fails the request. An asset that legitimately belongs to 11 data products therefore got a hard error instead of an answer, which denies access under an allow-list rule and grants it under a deny-list one. Raise it to 1000. The point is to sit far above anything realistic rather than at a plausible maximum, so that the error becomes unreachable in practice while the property that matters is kept: never silently serve a truncated list, which a policy evaluating data product membership cannot tell apart from a complete one. This stays a single request at any page size and only costs DataHub more when an asset really has that many relationships, so pagination - one round trip per page for a case that should not occur - is not worth it. --- .../usage-guide/resource-info-fetcher.adoc | 7 +++++++ .../src/backend/data_hub/graphql.rs | 17 ++++++++++++----- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/docs/modules/opa/pages/usage-guide/resource-info-fetcher.adoc b/docs/modules/opa/pages/usage-guide/resource-info-fetcher.adoc index bbd7b762..d91a4c77 100644 --- a/docs/modules/opa/pages/usage-guide/resource-info-fetcher.adoc +++ b/docs/modules/opa/pages/usage-guide/resource-info-fetcher.adoc @@ -139,6 +139,13 @@ An example of the returned structure: } ---- +[NOTE] +==== +DataHub models data product membership as graph edges rather than a field on the asset, so `dataProducts` is fetched as a single page of up to 1000 entries. +An asset is expected to belong to one or two, so this should not be reachable in practice. +If it ever is, the lookup fails with an error rather than returning the first 1000: a policy evaluating data product membership has no way to tell a truncated list from a complete one, and would silently decide on partial metadata. +==== + === Debug request To debug the resource-info-fetcher you can `curl` its API for a given resource. diff --git a/rust/resource-info-fetcher/src/backend/data_hub/graphql.rs b/rust/resource-info-fetcher/src/backend/data_hub/graphql.rs index bf5d9116..a569a756 100644 --- a/rust/resource-info-fetcher/src/backend/data_hub/graphql.rs +++ b/rust/resource-info-fetcher/src/backend/data_hub/graphql.rs @@ -13,11 +13,18 @@ use crate::backend::data_hub::{ }; /// The page size of the `DataProductContains` relationship query, passed to DataHub as the -/// `$dataProductsCount` variable. DataHub paginates the `relationships` resolver, so some page size -/// has to be picked; an asset normally belongs to a single data product, which leaves ample -/// headroom. If it is ever exceeded we fail the request instead of answering with a truncated list, -/// see [`Entity::data_products_truncation`]. -const DATA_PRODUCTS_PAGE_SIZE: u32 = 10; +/// `$dataProductsCount` variable. +/// +/// DataHub paginates the `relationships` resolver, so some page size has to be picked. An asset +/// normally belongs to one or two data products, so this is deliberately set far above anything +/// realistic rather than at a plausible maximum: exceeding it fails the request (see +/// [`Entity::data_products_truncation`]), and failing a lookup that should have succeeded is the +/// worse outcome. It stays a single request at any size, and only costs DataHub more when an asset +/// really does have that many relationships. +/// +/// We do not paginate. That would mean one round trip per page for a case that should not occur, +/// whereas overshooting the page size costs nothing until it is actually needed. +const DATA_PRODUCTS_PAGE_SIZE: u32 = 1000; /// A single query covering every entity kind we build URNs for. We use the generic `entity(urn:)` /// resolver plus per-type inline fragments, because a request can target a dataset (Trino table or From 71b635ae2e2d6bbba4aacfaea2a38f19b68f1145 Mon Sep 17 00:00:00 2001 From: Malte Sander Date: Mon, 17 Aug 2026 13:36:02 +0200 Subject: [PATCH 09/18] feat: Add a cached bearer token to info-fetcher-commons The Keycloak and Entra backends mint an access token for every single user lookup and discard the `expires_in` the issuer reports, so each lookup costs an extra round trip. Add the machinery to cache it, ahead of adopting it in the backends. `CachedToken::get` serves a token until 30s before its stated expiry, and mints under a write lock with a re-check, so a burst of lookups arriving just after a token expired results in one token request rather than one per lookup. A token with no stated lifetime, or one already inside the margin, is used but not cached - degrading to the current mint-per-request behaviour instead of guessing a lifetime. `CachedToken::invalidate` covers a token that stops working before its stated expiry, e.g. by being revoked. To let backends detect that case, `utils::http::Error` gains a `status` accessor and `is_unauthorized` walks the error's source chain, since a 401 is wrapped in backend-specific error types by the time a caller can act on it. --- Cargo.lock | 1 + Cargo.nix | 6 + rust/info-fetcher-commons/Cargo.toml | 3 + rust/info-fetcher-commons/src/utils/http.rs | 81 +++++++ rust/info-fetcher-commons/src/utils/mod.rs | 1 + rust/info-fetcher-commons/src/utils/token.rs | 238 +++++++++++++++++++ 6 files changed, 330 insertions(+) create mode 100644 rust/info-fetcher-commons/src/utils/token.rs diff --git a/Cargo.lock b/Cargo.lock index 07d200d7..9eb2af31 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1699,6 +1699,7 @@ name = "info-fetcher-commons" version = "0.0.0-dev" dependencies = [ "axum", + "futures", "hyper", "native-tls", "reqwest", diff --git a/Cargo.nix b/Cargo.nix index 10460b02..076befcd 100644 --- a/Cargo.nix +++ b/Cargo.nix @@ -5497,6 +5497,12 @@ rec { packageId = "tracing"; } ]; + devDependencies = [ + { + name = "futures"; + packageId = "futures"; + } + ]; }; "ipnet" = rec { diff --git a/rust/info-fetcher-commons/Cargo.toml b/rust/info-fetcher-commons/Cargo.toml index aa246ee2..90407b7d 100644 --- a/rust/info-fetcher-commons/Cargo.toml +++ b/rust/info-fetcher-commons/Cargo.toml @@ -21,3 +21,6 @@ serde_json.workspace = true snafu.workspace = true tracing.workspace = true tokio.workspace = true + +[dev-dependencies] +futures.workspace = true diff --git a/rust/info-fetcher-commons/src/utils/http.rs b/rust/info-fetcher-commons/src/utils/http.rs index 31ea5f2b..4557da1e 100644 --- a/rust/info-fetcher-commons/src/utils/http.rs +++ b/rust/info-fetcher-commons/src/utils/http.rs @@ -59,6 +59,28 @@ pub async fn send_json_request(req: RequestBuilder) -> Resu serde_json::from_str(&json).context(ParseJsonSnafu) } +/// Whether `error`, or any error it wraps, is a `401 Unauthorized` answer from a backend. +/// +/// Walks the source chain because backends wrap [`Error`] in their own error types, so the 401 is +/// never the outermost error by the time a caller gets to decide whether to re-authenticate. +pub fn is_unauthorized(error: &(dyn std::error::Error + 'static)) -> bool { + std::iter::successors(Some(error), |error| error.source()) + .filter_map(|error| error.downcast_ref::()) + .any(|error| error.status() == Some(StatusCode::UNAUTHORIZED)) +} + +impl Error { + /// The status code the backend answered with, or [`None`] if the failure happened before there was + /// a response to read a status off. + pub fn status(&self) -> Option { + match self { + Self::HttpErrorResponse { status, .. } => Some(*status), + Self::HttpErrorResponseUndecodableText { status, .. } => Some(*status), + Self::HttpRequest { .. } | Self::ParseJson { .. } => None, + } + } +} + /// Wraps a Response into a Result. If there is an HTTP Client or Server error, /// extract the HTTP body (if possible) to be used as context in the returned Err. /// This is done this because the `Response::error_for_status()` method Err variant @@ -84,3 +106,62 @@ async fn error_for_status(response: Response) -> Result { } Ok(response) } + +#[cfg(test)] +mod tests { + use snafu::IntoError; + + use super::*; + + /// Backends wrap transport errors in their own error types, sometimes several layers deep, so the + /// check has to walk the source chain instead of inspecting the outermost error. + #[derive(Snafu, Debug)] + #[snafu(display("failed to fetch the user"))] + struct FetchUser { + source: Error, + } + + #[derive(Snafu, Debug)] + #[snafu(display("failed to get user info"))] + struct GetUserInfo { + source: FetchUser, + } + + /// A backend response with `status`, wrapped the way a backend would wrap it. + fn wrapped_response(status: StatusCode) -> GetUserInfo { + let response = Error::HttpErrorResponse { + status, + url: "https://keycloak.example.com/admin/realms/my-realm/users/".to_owned(), + text: "denied".to_owned(), + }; + + GetUserInfoSnafu.into_error(FetchUserSnafu.into_error(response)) + } + + #[test] + fn a_wrapped_unauthorized_response_is_detected() { + assert!(is_unauthorized(&wrapped_response(StatusCode::UNAUTHORIZED))); + } + + /// Only a 401 means "your token is no good". A 403 says the token was understood and the actor is + /// not allowed, which re-minting cannot fix. + #[test] + fn other_error_responses_are_not_unauthorized() { + assert!(!is_unauthorized(&wrapped_response(StatusCode::FORBIDDEN))); + assert!(!is_unauthorized(&wrapped_response( + StatusCode::INTERNAL_SERVER_ERROR + ))); + } + + /// A request that never got an answer has no status to look at. + #[test] + fn errors_without_a_response_are_not_unauthorized() { + let error = Error::ParseJson { + source: serde_json::from_str::("not json") + .expect_err("the input is not valid JSON"), + }; + + assert_eq!(error.status(), None); + assert!(!is_unauthorized(&error)); + } +} diff --git a/rust/info-fetcher-commons/src/utils/mod.rs b/rust/info-fetcher-commons/src/utils/mod.rs index cb1f965d..4ad25bd9 100644 --- a/rust/info-fetcher-commons/src/utils/mod.rs +++ b/rust/info-fetcher-commons/src/utils/mod.rs @@ -1,2 +1,3 @@ pub mod http; pub mod tls; +pub mod token; diff --git a/rust/info-fetcher-commons/src/utils/token.rs b/rust/info-fetcher-commons/src/utils/token.rs new file mode 100644 index 00000000..b060e457 --- /dev/null +++ b/rust/info-fetcher-commons/src/utils/token.rs @@ -0,0 +1,238 @@ +//! Caching of the bearer tokens the info-fetcher backends authenticate with. + +use std::{ + future::Future, + time::{Duration, Instant}, +}; + +use tokio::sync::RwLock; + +/// How long before its stated expiry a token stops being handed out. +/// +/// A token is minted, then travels to the backend and is validated there, so handing out one that is +/// about to expire risks it being rejected mid-request. Refreshing slightly early avoids that without +/// needing to know anything about the backend's clock. +const EXPIRY_MARGIN: Duration = Duration::from_secs(30); + +/// A freshly minted bearer token. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct MintedToken { + pub token: String, + + /// How long the token remains valid, as reported by whoever issued it (for OAuth: `expires_in`). + /// + /// [`None`] when the issuer does not say. The token is then used but not cached, because we have + /// no basis for deciding when it goes stale. + pub lifetime: Option, +} + +/// A bearer token that is minted on demand and kept until shortly before it expires. +/// +/// Backends previously minted a token for every single request, which doubled the round trips per +/// lookup and threw the issuer's `expires_in` away. This caches the token for the lifetime the issuer +/// reported, and lets the caller drop it early via [`CachedToken::invalidate`] when the backend +/// rejects it - a token can stop working before its stated expiry, e.g. by being revoked. +#[derive(Debug, Default)] +pub struct CachedToken { + cached: RwLock>, +} + +#[derive(Debug)] +struct Entry { + token: String, + + /// The stated expiry, already brought forward by [`EXPIRY_MARGIN`]. + usable_until: Instant, +} + +impl CachedToken { + pub fn new() -> Self { + Self::default() + } + + /// Returns a usable token, minting one with `mint` if the cache has none or the cached one is + /// within [`EXPIRY_MARGIN`] of expiring. + /// + /// Concurrent callers that all miss the cache do not each mint: the first one to get the write + /// lock mints while the others wait, and they then find its result in the cache. Without that, a + /// burst of requests arriving just after a token expired would produce a burst of token requests. + pub async fn get(&self, mint: F) -> Result + where + F: FnOnce() -> Fut, + Fut: Future>, + { + if let Some(token) = Self::usable_token(&*self.cached.read().await) { + return Ok(token); + } + + // The read lock is released above, so another caller may have minted in between - hence the + // second look before minting ourselves. + let mut cached = self.cached.write().await; + if let Some(token) = Self::usable_token(&cached) { + return Ok(token); + } + + let MintedToken { token, lifetime } = mint().await?; + + // Only cache a token we know is still usable for a worthwhile amount of time. A failed mint + // returns above, so nothing is cached in that case either. + *cached = lifetime + .and_then(|lifetime| lifetime.checked_sub(EXPIRY_MARGIN)) + .map(|usable_for| Entry { + token: token.clone(), + usable_until: Instant::now() + usable_for, + }); + + Ok(token) + } + + /// Drops the cached token, so the next [`CachedToken::get`] mints a fresh one. + /// + /// Call this when the backend rejects the token, which is the only way to find out that it stopped + /// being valid ahead of its stated expiry. + pub async fn invalidate(&self) { + *self.cached.write().await = None; + } + + /// The cached token, if there is one and it is not within [`EXPIRY_MARGIN`] of expiring. + fn usable_token(cached: &Option) -> Option { + let entry = cached.as_ref()?; + + (entry.usable_until > Instant::now()).then(|| entry.token.clone()) + } +} + +#[cfg(test)] +mod tests { + use std::{ + sync::atomic::{AtomicUsize, Ordering}, + time::Duration, + }; + + use super::*; + + /// Counts how often a token was minted, so the tests can assert on cache hits rather than on + /// timing. + struct Minter { + calls: AtomicUsize, + lifetime: Option, + } + + impl Minter { + fn new(lifetime: Option) -> Self { + Self { + calls: AtomicUsize::new(0), + lifetime, + } + } + + async fn mint(&self) -> Result { + let call = self.calls.fetch_add(1, Ordering::SeqCst); + + Ok(MintedToken { + token: format!("token-{call}"), + lifetime: self.lifetime, + }) + } + + fn calls(&self) -> usize { + self.calls.load(Ordering::SeqCst) + } + } + + /// A lifetime comfortably longer than [`EXPIRY_MARGIN`], so the token stays usable for the whole + /// test without any waiting. + fn long_lifetime() -> Option { + Some(EXPIRY_MARGIN + Duration::from_secs(600)) + } + + #[tokio::test] + async fn token_is_minted_once_and_then_served_from_the_cache() { + let minter = Minter::new(long_lifetime()); + let cached = CachedToken::new(); + + let first = cached.get(|| minter.mint()).await.expect("minting works"); + let second = cached.get(|| minter.mint()).await.expect("minting works"); + + assert_eq!(first, "token-0"); + assert_eq!(second, "token-0"); + assert_eq!(minter.calls(), 1); + } + + #[tokio::test] + async fn invalidating_forces_the_next_get_to_mint() { + let minter = Minter::new(long_lifetime()); + let cached = CachedToken::new(); + + let first = cached.get(|| minter.mint()).await.expect("minting works"); + cached.invalidate().await; + let second = cached.get(|| minter.mint()).await.expect("minting works"); + + assert_eq!(first, "token-0"); + assert_eq!(second, "token-1"); + assert_eq!(minter.calls(), 2); + } + + /// Without a stated lifetime we have no idea how long the token is good for, so we must not keep + /// it. That degrades to minting per request, which is what the backends did before caching. + #[tokio::test] + async fn a_token_without_a_lifetime_is_not_cached() { + let minter = Minter::new(None); + let cached = CachedToken::new(); + + cached.get(|| minter.mint()).await.expect("minting works"); + cached.get(|| minter.mint()).await.expect("minting works"); + + assert_eq!(minter.calls(), 2); + } + + /// A token that expires within the safety margin has no usable lifetime left, so caching it would + /// only hand out a token that is about to be rejected. + #[tokio::test] + async fn a_token_expiring_within_the_margin_is_not_cached() { + let minter = Minter::new(Some(EXPIRY_MARGIN)); + let cached = CachedToken::new(); + + cached.get(|| minter.mint()).await.expect("minting works"); + cached.get(|| minter.mint()).await.expect("minting works"); + + assert_eq!(minter.calls(), 2); + } + + /// Concurrent lookups that all miss the cache must not each mint a token: the backend would see a + /// burst of token requests every time the cached one expires. + #[tokio::test] + async fn concurrent_gets_mint_only_once() { + let minter = Minter::new(long_lifetime()); + let cached = CachedToken::new(); + + let tokens = futures::future::join_all((0..20).map(|_| { + cached.get(|| async { + // Yield inside the critical section, so the tasks actually overlap. + tokio::time::sleep(Duration::from_millis(20)).await; + minter.mint().await + }) + })) + .await; + + for token in tokens { + assert_eq!(token.expect("minting works"), "token-0"); + } + assert_eq!(minter.calls(), 1); + } + + /// A failed mint must not be cached, and must not poison a later attempt. + #[tokio::test] + async fn a_failed_mint_is_not_cached() { + let minter = Minter::new(long_lifetime()); + let cached = CachedToken::new(); + + let failed = cached + .get(|| async { Err::("no token for you".to_owned()) }) + .await; + let succeeded = cached.get(|| minter.mint()).await; + + assert_eq!(failed, Err("no token for you".to_owned())); + assert_eq!(succeeded, Ok("token-0".to_owned())); + } +} From 22e7f06eb9d11a83d5e883fa3c55b2a2873158b2 Mon Sep 17 00:00:00 2001 From: Malte Sander Date: Mon, 17 Aug 2026 13:42:08 +0200 Subject: [PATCH 10/18] feat: Cache the OAuth2 access token in the Keycloak and Entra backends Both backends minted an access token for every single user lookup and discarded the `expires_in` the provider reports, so each lookup paid for an extra round trip that returned a token good for the next hour. Deserialize `expires_in` and hold the token in a `CachedToken`, so it is minted once and reused until shortly before it expires. A provider that reports no lifetime keeps the previous mint-per-lookup behaviour. A token can also stop being accepted before its stated expiry, by being revoked or through clock skew, and the rejection is the only way to find out. Split the lookup so it takes an already-obtained token, and on a 401 anywhere in the error's source chain invalidate the cached token and retry the lookup exactly once. --- CHANGELOG.md | 4 + rust/user-info-fetcher/src/backend/entra.rs | 162 ++++++++++-- .../user-info-fetcher/src/backend/keycloak.rs | 235 ++++++++++++++++-- 3 files changed, 362 insertions(+), 39 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b872c838..936e2679 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,10 @@ All notable changes to this project will be documented in this file. - The RBAC ServiceAccount and RoleBinding are now built with the operator-rs `v2::rbac` functions and carry the full set of recommended labels ([#861]). - All product containers now run with `securityContext.runAsNonRoot` set to `true` to improve security ([#871]). +- The user-info-fetcher Keycloak and Entra backends now cache their OAuth2 access token for the + lifetime the identity provider reports, instead of minting a new one for every user lookup. This + removes one round trip per lookup. If the provider rejects the cached token before it expires, it is + re-minted and the lookup is retried once ([#863]). ### Fixed diff --git a/rust/user-info-fetcher/src/backend/entra.rs b/rust/user-info-fetcher/src/backend/entra.rs index 3cc9dd76..fe8407dd 100644 --- a/rust/user-info-fetcher/src/backend/entra.rs +++ b/rust/user-info-fetcher/src/backend/entra.rs @@ -1,7 +1,11 @@ -use std::{collections::HashMap, path::Path}; +use std::{collections::HashMap, path::Path, time::Duration}; use hyper::StatusCode; -use info_fetcher_commons::utils::{self, http::send_json_request}; +use info_fetcher_commons::utils::{ + self, + http::send_json_request, + token::{CachedToken, MintedToken}, +}; use serde::Deserialize; use snafu::{ResultExt, Snafu}; use stackable_opa_operator::crd::user_info_fetcher::v1alpha2; @@ -80,6 +84,10 @@ impl http_error::Error for Error { #[derive(Deserialize)] struct OAuthResponse { access_token: String, + + /// How many seconds the token stays valid, which lets us cache it rather than mint one per + /// lookup. Treated as optional so a response without it degrades to minting per lookup. + expires_in: Option, } #[derive(Clone, Deserialize)] @@ -123,6 +131,9 @@ pub struct ResolvedEntraBackend { client_id: String, client_secret: String, http_client: reqwest::Client, + + /// The OAuth2 access token, minted on demand and reused until it is about to expire. + access_token: CachedToken, } impl ResolvedEntraBackend { @@ -165,6 +176,7 @@ impl ResolvedEntraBackend { client_id, client_secret, http_client, + access_token: CachedToken::new(), }) } @@ -186,23 +198,67 @@ impl ResolvedEntraBackend { TlsClientDetails { tls: tls.clone() }.uses_tls(), )?; - let token_url = entra_backend.oauth2_token(); - let authn = send_json_request::(self.http_client.post(token_url).form(&[ - ("client_id", self.client_id.as_str()), - ("client_secret", self.client_secret.as_str()), - ("scope", "https://graph.microsoft.com/.default"), - ("grant_type", "client_credentials"), - ])) - .await - .context(AccessTokenSnafu)?; + let access_token = self.access_token(&entra_backend).await?; + match self + .get_user_info_with(req, &entra_backend, &access_token) + .await + { + Err(error) if utils::http::is_unauthorized(&error) => { + // The token was accepted when it was minted, so it has stopped being valid ahead of + // its stated expiry - it was revoked, or the issuer's and our clock disagree. Drop it + // and give the lookup exactly one more go with a fresh one. + tracing::warn!( + error = &error as &dyn std::error::Error, + "Entra rejected the cached access token; re-authenticating and retrying once" + ); + self.access_token.invalidate().await; + let access_token = self.access_token(&entra_backend).await?; + self.get_user_info_with(req, &entra_backend, &access_token) + .await + } + result => result, + } + } + + /// The cached access token, minting one if there is none or it is about to expire. + async fn access_token(&self, entra_backend: &EntraBackend) -> Result { + self.access_token + .get(|| async { + let response = send_json_request::( + self.http_client.post(entra_backend.oauth2_token()).form(&[ + ("client_id", self.client_id.as_str()), + ("client_secret", self.client_secret.as_str()), + ("scope", "https://graph.microsoft.com/.default"), + ("grant_type", "client_credentials"), + ]), + ) + .await + .context(AccessTokenSnafu)?; + + Ok(MintedToken { + token: response.access_token, + lifetime: response.expires_in.map(Duration::from_secs), + }) + }) + .await + } + + /// Looks the user up with an already-obtained `access_token`, so the caller can retry with a fresh + /// one if this token turns out to be rejected. + async fn get_user_info_with( + &self, + req: &UserInfoRequest, + entra_backend: &EntraBackend, + access_token: &str, + ) -> Result { let user_info = match req { UserInfoRequest::UserInfoRequestById(req) => { let user_id = &req.id; send_json_request::( self.http_client .get(entra_backend.user_info(user_id)) - .bearer_auth(&authn.access_token), + .bearer_auth(access_token), ) .await .with_context(|_| UserNotFoundByIdSnafu { @@ -214,7 +270,7 @@ impl ResolvedEntraBackend { send_json_request::( self.http_client .get(entra_backend.user_info(username)) - .bearer_auth(&authn.access_token), + .bearer_auth(access_token), ) .await .with_context(|_| SearchForUserSnafu { @@ -242,7 +298,7 @@ impl ResolvedEntraBackend { pages_remaining -= 1; let response = send_json_request::( - self.http_client.get(url).bearer_auth(&authn.access_token), + self.http_client.get(url).bearer_auth(access_token), ) .await .with_context(|_| RequestUserGroupsSnafu { @@ -373,10 +429,31 @@ mod tests { }, client_id: "client-id".to_owned(), client_secret: "client-secret".to_owned(), + access_token: CachedToken::new(), http_client: reqwest::Client::new(), } } + /// Mounts the OAuth2 token endpoint, answering with a token that is valid for `expires_in` + /// seconds. Without an `expires_in` the token is deliberately not cached, see + /// [`utils::token::CachedToken`]. + /// + /// `expected_calls` asserts how often the endpoint is hit; wiremock verifies it when the server + /// is dropped. + async fn mock_token(mock_server: &MockServer, expires_in: Option, expected_calls: u64) { + let mut body = serde_json::json!({"access_token": "access-token"}); + if let Some(expires_in) = expires_in { + body["expires_in"] = expires_in.into(); + } + + Mock::given(method("POST")) + .and(path(format!("/{TENANT_ID}/oauth2/v2.0/token"))) + .respond_with(ResponseTemplate::new(200).set_body_json(body)) + .expect(expected_calls) + .mount(mock_server) + .await; + } + /// Mocks the OAuth2 token and user metadata endpoints, which every `get_user_info` call hits /// before it gets to the group memberships we actually care about. async fn mock_token_and_user(mock_server: &MockServer) { @@ -482,6 +559,63 @@ mod tests { assert_eq!(user_info.groups.len(), MAX_GROUP_PAGES); } + /// Mounts the user metadata and (empty) group endpoints, so a lookup gets all the way through. + async fn mock_user_and_groups(mock_server: &MockServer, user_status: u16) { + Mock::given(method("GET")) + .and(path(format!("/v1.0/users/{USER_ID}"))) + .respond_with( + ResponseTemplate::new(user_status).set_body_json(serde_json::json!({ + "id": USER_ID, + "userPrincipalName": "alice@example.com", + })), + ) + .mount(mock_server) + .await; + + Mock::given(method("GET")) + .and(path(format!("/v1.0/users/{USER_ID}/memberOf"))) + .respond_with( + ResponseTemplate::new(200).set_body_json(serde_json::json!({"value": []})), + ) + .mount(mock_server) + .await; + } + + /// The access token was minted for every single lookup, doubling the round trips. Entra reports + /// how long it is valid for, so it only has to be minted once. + #[tokio::test] + async fn test_entra_reuses_a_cached_access_token_across_lookups() { + let mock_server = MockServer::start().await; + mock_token(&mock_server, Some(3600), 1).await; + mock_user_and_groups(&mock_server, 200).await; + + let backend = backend_for(&mock_server); + get_user_info_by_id(&backend).await; + get_user_info_by_id(&backend).await; + + // The token endpoint's `.expect(1)` is verified when `mock_server` is dropped. + } + + /// A token can stop being accepted before it expires, e.g. by being revoked. The rejection is the + /// only way to find that out, so it has to trigger exactly one re-authentication - not none + /// (the lookup would keep failing until the token expired) and not a retry loop. + #[tokio::test] + async fn test_entra_reauthenticates_once_when_the_token_is_rejected() { + let mock_server = MockServer::start().await; + mock_token(&mock_server, Some(3600), 2).await; + mock_user_and_groups(&mock_server, 401).await; + + let error = backend_for(&mock_server) + .get_user_info(&UserInfoRequest::UserInfoRequestById(UserInfoRequestById { + id: USER_ID.to_owned(), + })) + .await + .expect_err("a permanently rejected token must surface as an error"); + + assert!(utils::http::is_unauthorized(&error), "{error}"); + // The token endpoint's `.expect(2)` is verified when `mock_server` is dropped. + } + #[test] fn test_entra_defaults_id() { let tenant_id = "1234-5678-1234-5678"; diff --git a/rust/user-info-fetcher/src/backend/keycloak.rs b/rust/user-info-fetcher/src/backend/keycloak.rs index e26cf9b1..2b61ca55 100644 --- a/rust/user-info-fetcher/src/backend/keycloak.rs +++ b/rust/user-info-fetcher/src/backend/keycloak.rs @@ -1,11 +1,16 @@ -use std::{collections::HashMap, path::Path}; +use std::{collections::HashMap, path::Path, time::Duration}; use hyper::StatusCode; -use info_fetcher_commons::utils::{self, http::send_json_request}; +use info_fetcher_commons::utils::{ + self, + http::send_json_request, + token::{CachedToken, MintedToken}, +}; use serde::Deserialize; use snafu::{OptionExt, ResultExt, Snafu}; use stackable_opa_operator::crd::user_info_fetcher::v1alpha2; use stackable_operator::crd::authentication::oidc; +use url::Url; use crate::{UserInfo, UserInfoRequest, http_error}; @@ -85,6 +90,10 @@ impl http_error::Error for Error { #[derive(Deserialize)] struct OAuthResponse { access_token: String, + + /// How many seconds the token stays valid, which lets us cache it rather than mint one per + /// lookup. Treated as optional so a response without it degrades to minting per lookup. + expires_in: Option, } /// The minimal structure of [UserRepresentation] that is returned by [`/users`][users] and [`/users/{id}`][user-by-id]. @@ -118,6 +127,9 @@ pub struct ResolvedKeycloakBackend { client_id: String, client_secret: String, http_client: reqwest::Client, + + /// The OAuth2 access token, minted on demand and reused until it is about to expire. + access_token: CachedToken, } impl ResolvedKeycloakBackend { @@ -155,18 +167,44 @@ impl ResolvedKeycloakBackend { client_id, client_secret, http_client, + access_token: CachedToken::new(), }) } pub(crate) async fn get_user_info(&self, req: &UserInfoRequest) -> Result { + let keycloak_url = self.keycloak_url()?; + + let access_token = self.access_token(&keycloak_url).await?; + match self + .get_user_info_with(req, &keycloak_url, &access_token) + .await + { + Err(error) if utils::http::is_unauthorized(&error) => { + // The token was accepted when it was minted, so it has stopped being valid ahead of + // its stated expiry - it was revoked, or the issuer's and our clock disagree. Drop it + // and give the lookup exactly one more go with a fresh one. + tracing::warn!( + error = &error as &dyn std::error::Error, + "Keycloak rejected the cached access token; re-authenticating and retrying once" + ); + self.access_token.invalidate().await; + + let access_token = self.access_token(&keycloak_url).await?; + self.get_user_info_with(req, &keycloak_url, &access_token) + .await + } + result => result, + } + } + + /// The base URL of the configured Keycloak. + fn keycloak_url(&self) -> Result { let v1alpha2::KeycloakBackend { - client_credentials_secret: _, - admin_realm, - user_realm, hostname, port, root_path, tls, + .. } = &self.config; // We re-use existent functionality from operator-rs, besides it being a bit of miss-use. @@ -180,24 +218,50 @@ impl ResolvedKeycloakBackend { Vec::new(), None, ); - let keycloak_url = wrapping_auth_provider + + wrapping_auth_provider .endpoint_url() - .context(ParseOidcEndpointUrlSnafu)?; + .context(ParseOidcEndpointUrlSnafu) + } - let authn = send_json_request::( - self.http_client - .post( - keycloak_url - .join(&format!( - "realms/{admin_realm}/protocol/openid-connect/token" - )) - .context(ConstructOidcEndpointPathSnafu)?, + /// The cached access token, minting one if there is none or it is about to expire. + async fn access_token(&self, keycloak_url: &Url) -> Result { + let admin_realm = &self.config.admin_realm; + + self.access_token + .get(|| async { + let response = send_json_request::( + self.http_client + .post( + keycloak_url + .join(&format!( + "realms/{admin_realm}/protocol/openid-connect/token" + )) + .context(ConstructOidcEndpointPathSnafu)?, + ) + .basic_auth(&self.client_id, Some(&self.client_secret)) + .form(&[("grant_type", "client_credentials")]), ) - .basic_auth(&self.client_id, Some(&self.client_secret)) - .form(&[("grant_type", "client_credentials")]), - ) - .await - .context(AccessTokenSnafu)?; + .await + .context(AccessTokenSnafu)?; + + Ok(MintedToken { + token: response.access_token, + lifetime: response.expires_in.map(Duration::from_secs), + }) + }) + .await + } + + /// Looks the user up with an already-obtained `access_token`, so the caller can retry with a fresh + /// one if this token turns out to be rejected. + async fn get_user_info_with( + &self, + req: &UserInfoRequest, + keycloak_url: &Url, + access_token: &str, + ) -> Result { + let user_realm = &self.config.user_realm; let users_base_url = keycloak_url .join(&format!("admin/realms/{user_realm}/users/")) @@ -213,7 +277,7 @@ impl ResolvedKeycloakBackend { .join(&req.id) .context(ConstructOidcEndpointPathSnafu)?, ) - .bearer_auth(&authn.access_token), + .bearer_auth(access_token), ) .await .context(UserNotFoundByIdSnafu { user_id })? @@ -225,9 +289,7 @@ impl ResolvedKeycloakBackend { .context(ConstructOidcEndpointPathSnafu)?; let users = send_json_request::>( - self.http_client - .get(users_url) - .bearer_auth(&authn.access_token), + self.http_client.get(users_url).bearer_auth(access_token), ) .await .context(SearchForUserSnafu)?; @@ -250,7 +312,7 @@ impl ResolvedKeycloakBackend { .join(&format!("{}/groups", user_info.id)) .context(ConstructOidcEndpointPathSnafu)?, ) - .bearer_auth(&authn.access_token), + .bearer_auth(access_token), ) .await .context(RequestUserGroupsSnafu { @@ -266,3 +328,126 @@ impl ResolvedKeycloakBackend { }) } } + +#[cfg(test)] +mod tests { + use std::str::FromStr; + + use stackable_operator::{ + commons::{networking::HostName, tls_verification::TlsClientDetails}, + v2::types::kubernetes::SecretName, + }; + use wiremock::{ + Mock, MockServer, ResponseTemplate, + matchers::{method, path}, + }; + + use super::*; + use crate::{UserInfoRequestById, backend::keycloak::ResolvedKeycloakBackend}; + + const ADMIN_REALM: &str = "master"; + const USER_REALM: &str = "my-realm"; + const USER_ID: &str = "8765-4321-8765-4321"; + + /// Builds a backend pointing at `mock_server`, bypassing [`ResolvedKeycloakBackend::resolve`] so + /// that no credentials have to be read from disk. + fn backend_for(mock_server: &MockServer) -> ResolvedKeycloakBackend { + ResolvedKeycloakBackend { + config: v1alpha2::KeycloakBackend { + hostname: HostName::from_str(&mock_server.address().ip().to_string()).unwrap(), + port: Some(mock_server.address().port()), + root_path: "/".to_owned(), + tls: TlsClientDetails { tls: None }, + client_credentials_secret: SecretName::from_str("keycloak-credentials").unwrap(), + admin_realm: ADMIN_REALM.to_owned(), + user_realm: USER_REALM.to_owned(), + }, + client_id: "client-id".to_owned(), + client_secret: "client-secret".to_owned(), + http_client: reqwest::Client::new(), + access_token: CachedToken::new(), + } + } + + /// Mounts the token endpoint, answering with a token valid for `expires_in` seconds. + /// `expected_calls` is verified by wiremock when the server is dropped. + async fn mock_token(mock_server: &MockServer, expires_in: Option, expected_calls: u64) { + let mut body = serde_json::json!({"access_token": "access-token"}); + if let Some(expires_in) = expires_in { + body["expires_in"] = expires_in.into(); + } + + Mock::given(method("POST")) + .and(path(format!( + "/realms/{ADMIN_REALM}/protocol/openid-connect/token" + ))) + .respond_with(ResponseTemplate::new(200).set_body_json(body)) + .expect(expected_calls) + .mount(mock_server) + .await; + } + + /// Mounts the user metadata and (empty) group endpoints, so a lookup gets all the way through. + async fn mock_user_and_groups(mock_server: &MockServer, user_status: u16) { + Mock::given(method("GET")) + .and(path(format!("/admin/realms/{USER_REALM}/users/{USER_ID}"))) + .respond_with( + ResponseTemplate::new(user_status).set_body_json(serde_json::json!({ + "id": USER_ID, + "username": "alice", + })), + ) + .mount(mock_server) + .await; + + Mock::given(method("GET")) + .and(path(format!( + "/admin/realms/{USER_REALM}/users/{USER_ID}/groups" + ))) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!([]))) + .mount(mock_server) + .await; + } + + async fn get_user_info_by_id( + backend: &ResolvedKeycloakBackend, + ) -> Result { + backend + .get_user_info(&UserInfoRequest::UserInfoRequestById(UserInfoRequestById { + id: USER_ID.to_owned(), + })) + .await + } + + /// The access token was minted for every single lookup, doubling the round trips. Keycloak reports + /// how long it is valid for, so it only has to be minted once. + #[tokio::test] + async fn keycloak_reuses_a_cached_access_token_across_lookups() { + let mock_server = MockServer::start().await; + mock_token(&mock_server, Some(3600), 1).await; + mock_user_and_groups(&mock_server, 200).await; + + let backend = backend_for(&mock_server); + get_user_info_by_id(&backend).await.expect("lookup works"); + get_user_info_by_id(&backend).await.expect("lookup works"); + + // The token endpoint's `.expect(1)` is verified when `mock_server` is dropped. + } + + /// A token can stop being accepted before it expires, e.g. by being revoked. The rejection is the + /// only way to find that out, so it has to trigger exactly one re-authentication - not none + /// (the lookup would keep failing until the token expired) and not a retry loop. + #[tokio::test] + async fn keycloak_reauthenticates_once_when_the_token_is_rejected() { + let mock_server = MockServer::start().await; + mock_token(&mock_server, Some(3600), 2).await; + mock_user_and_groups(&mock_server, 401).await; + + let error = get_user_info_by_id(&backend_for(&mock_server)) + .await + .expect_err("a permanently rejected token must surface as an error"); + + assert!(utils::http::is_unauthorized(&error), "{error}"); + // The token endpoint's `.expect(2)` is verified when `mock_server` is dropped. + } +} From 97ec04998d24f24620cd82648935c2d91d08b31c Mon Sep 17 00:00:00 2001 From: Malte Sander Date: Mon, 17 Aug 2026 13:43:43 +0200 Subject: [PATCH 11/18] docs: Document that metadata is not inherited from parent containers Tagging a Trino schema as PII does not make the tables inside it report that tag, which is easy to mistake for a bug. State it explicitly, and say why: whether a tag applies to a container's children is a property of the policy rather than of the resource - PII plausibly cascades, "deprecated" or an owning team do not - and merging a parent's tags into the child's would leave a rule unable to ask whether the resource itself is tagged. Show the alternative instead: there is one function per level, so a rule can consult the schema as well as the table. The example requires a positive signal at each level, matching the guidance in the section that follows it. --- .../usage-guide/resource-info-fetcher.adoc | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/docs/modules/opa/pages/usage-guide/resource-info-fetcher.adoc b/docs/modules/opa/pages/usage-guide/resource-info-fetcher.adoc index d91a4c77..9d6981a9 100644 --- a/docs/modules/opa/pages/usage-guide/resource-info-fetcher.adoc +++ b/docs/modules/opa/pages/usage-guide/resource-info-fetcher.adoc @@ -191,6 +191,43 @@ allow if { } ---- +=== Metadata is not inherited from parent containers + +Every function returns the metadata of the resource it addresses, and only of that resource. +Nothing is inherited along the containment hierarchy: tagging the schema `tpch.sf1` as `pii` does not make `tableResourceInfo` report `pii` for the tables inside it, and the same holds for domains and data products. + +This is deliberate. +Whether a tag applies to the children of a container is a property of your policy, not of the resource: `pii` or `confidential` plausibly cascade, while `deprecated`, `verified` or an owning team just as plausibly do not. +The resource-info-fetcher cannot tell which is which, and merging a parent's tags into the child's would destroy the distinction — a rule could no longer ask whether _this_ table is tagged, only whether anything above it is. + +Express the inheritance you want in the rule instead. +There is one function per level, so a rule can consult as many of them as it needs: + +[source,rego] +---- +package test + +import data.stackable.opa.resourceinfo.v1 as resourceinfo + +default allow := false + +# The table itself is marked public, ... +allow if { + table := resourceinfo.tableResourceInfo("trino", "my-namespace/my-trino", input.catalog, input.schema, input.table) + some tag in table.tags + tag.urn == "urn:li:tag:public" +} + +# ... or the schema containing it is, which this rule chooses to extend to its tables. +allow if { + schema := resourceinfo.schemaResourceInfo("trino", "my-namespace/my-trino", input.catalog, input.schema) + some tag in schema.tags + tag.urn == "urn:li:tag:public" +} +---- + +Each lookup is cached and served over the loopback interface, so consulting an extra level costs little. + === Behaviour when metadata is unavailable [WARNING] From 3c9d7a92eafa371c3695ffbfba9e2ce895f88af3 Mon Sep 17 00:00:00 2001 From: Malte Sander Date: Mon, 17 Aug 2026 13:47:45 +0200 Subject: [PATCH 12/18] fix: Reject resource names DataHub cannot express as a URN A name containing `,`, `(` or `)` cannot appear in a DataHub URN - DataHub's own parser uses those to delimit the parts of one - so any URN built from it is guaranteed not to resolve. The request was still sent, so every such lookup cost a round trip to DataHub and a WARN line, and any user who can name a table could trigger it at will: Trino's `SELECT * FROM tpch.sf1."a,PROD)"` is enough. Check the names while building the URN and answer 400 without querying DataHub. `rawIdentifier` is exempt, since for DataHub it is a URN and necessarily contains those characters. Dots are deliberately still allowed: they separate the segments of a dataset name, but DataHub's ingestion spells a dotted name the same way we do, so such a lookup can legitimately resolve. --- .../usage-guide/resource-info-fetcher.adoc | 4 + .../src/backend/data_hub/mod.rs | 12 +- .../data_hub/resource_to_urn_mapping.rs | 145 +++++++++++++++--- 3 files changed, 137 insertions(+), 24 deletions(-) diff --git a/docs/modules/opa/pages/usage-guide/resource-info-fetcher.adoc b/docs/modules/opa/pages/usage-guide/resource-info-fetcher.adoc index 9d6981a9..e427a205 100644 --- a/docs/modules/opa/pages/usage-guide/resource-info-fetcher.adoc +++ b/docs/modules/opa/pages/usage-guide/resource-info-fetcher.adoc @@ -107,6 +107,10 @@ Superset numbers its dashboards and charts, but products that name them instead Every argument is limited to 1024 bytes, which is far more than any real identifier needs. A longer value is rejected with `400 Bad Request` rather than looked up, because arguments end up in the response cache key. +Arguments are likewise rejected if they contain `,`, `(` or `)`. +DataHub uses those characters to delimit the parts of a URN, so a resource whose name contains one cannot be addressed at all — sending it to DataHub would only produce an error there. +`rawIdentifierResourceInfo` is exempt, since a URN necessarily contains them. + The DataHub environment (fabric) is deliberately *not* an argument: it describes how the catalog was populated rather than the resource being authorized, so it is configured once on the OpaCluster (see `env` above) instead of being passed in by every Rego rule. An example of the returned structure: diff --git a/rust/resource-info-fetcher/src/backend/data_hub/mod.rs b/rust/resource-info-fetcher/src/backend/data_hub/mod.rs index 856026e4..f048d86c 100644 --- a/rust/resource-info-fetcher/src/backend/data_hub/mod.rs +++ b/rust/resource-info-fetcher/src/backend/data_hub/mod.rs @@ -61,6 +61,13 @@ pub enum Error { #[snafu(display("DataHub returned GraphQL errors for URN {urn:?}: {messages}"))] GraphQlErrors { messages: String, urn: Urn }, + #[snafu(display( + "the resource name {name:?} contains {delimiter:?}, which DataHub uses to delimit the parts \ + of a URN. No URN containing it can resolve, so the request is rejected without querying \ + DataHub." + ))] + InvalidResourceName { name: String, delimiter: char }, + #[snafu(display( "DataHub reported {total} data products for URN {urn:?}, but the GraphQL query only fetches \ up to {received} of them. Refusing to answer with a truncated list of data products, as \ @@ -81,6 +88,9 @@ impl http_error::Error for Error { // table can reach this, e.g. through Trino's `SELECT * FROM tpch.sf1."a,PROD)"`. Self::GraphQlErrors { .. } => StatusCode::BAD_REQUEST, + // The caller named a resource that cannot be expressed as a URN at all. + Self::InvalidResourceName { .. } => StatusCode::BAD_REQUEST, + // A limitation of our own query, see the variant's message. Self::TruncatedDataProducts { .. } => StatusCode::INTERNAL_SERVER_ERROR, } @@ -329,7 +339,7 @@ impl ResourceInfoBackend for ResolvedDataHubBackend { &self, request: &ResourceInfoRequest, ) -> Result { - let urn = urn_for_request(request, &self.env); + let urn = urn_for_request(request, &self.env)?; let entity = self.query_entity(&urn).await?; Ok(entity.into_response(urn)) diff --git a/rust/resource-info-fetcher/src/backend/data_hub/resource_to_urn_mapping.rs b/rust/resource-info-fetcher/src/backend/data_hub/resource_to_urn_mapping.rs index 3aab27ae..9a4d9344 100644 --- a/rust/resource-info-fetcher/src/backend/data_hub/resource_to_urn_mapping.rs +++ b/rust/resource-info-fetcher/src/backend/data_hub/resource_to_urn_mapping.rs @@ -4,8 +4,11 @@ use serde::Serialize; use stackable_opa_operator::crd::resource_info_fetcher::v1alpha1; use crate::{ - api::{Chart, Dashboard, Database, RawIdentifier, ResourceInfoRequest, Schema, Stream, Table}, - backend::data_hub::Urn, + api::{ + Chart, Dashboard, Database, ParamValue, RawIdentifier, ResourceInfoRequest, Schema, Stream, + Table, + }, + backend::data_hub::{Error, InvalidResourceNameSnafu, Urn}, }; /// Maps a request to the URN of the DataHub entity that holds the resource's metadata. @@ -13,28 +16,37 @@ use crate::{ /// `env` is DataHub's fabric (e.g. `PROD`) and comes from the backend configuration rather than from /// the request - see [`v1alpha1::DataHubBackend::env`] for why. It is part of every dataset URN, so /// it has to match the `env` the metadata was ingested with, otherwise the URN does not resolve. -pub fn urn_for_request(request: &ResourceInfoRequest, env: &v1alpha1::FabricType) -> Urn { +pub fn urn_for_request( + request: &ResourceInfoRequest, + env: &v1alpha1::FabricType, +) -> Result { let urn = match request { ResourceInfoRequest::Database(Database { system, instance, database, - }) => container_urn(&BTreeMap::from([ - ("platform", system), - ("instance", instance), - ("database", database), - ])), + }) => { + reject_urn_delimiters(&[system, instance, database])?; + container_urn(&BTreeMap::from([ + ("platform", system), + ("instance", instance), + ("database", database), + ])) + } ResourceInfoRequest::Schema(Schema { system, instance, database, schema, - }) => container_urn(&BTreeMap::from([ - ("platform", system), - ("instance", instance), - ("database", database), - ("schema", schema), - ])), + }) => { + reject_urn_delimiters(&[system, instance, database, schema])?; + container_urn(&BTreeMap::from([ + ("platform", system), + ("instance", instance), + ("database", database), + ("schema", schema), + ])) + } ResourceInfoRequest::Table(Table { system, instance, @@ -42,6 +54,7 @@ pub fn urn_for_request(request: &ResourceInfoRequest, env: &v1alpha1::FabricType schema, table, }) => { + reject_urn_delimiters(&[system, instance, database, schema, table])?; format!( "urn:li:dataset:(urn:li:dataPlatform:{system},{instance}.{database}.{schema}.{table},{env})" ) @@ -50,12 +63,16 @@ pub fn urn_for_request(request: &ResourceInfoRequest, env: &v1alpha1::FabricType system, instance, queue, - }) => format!("urn:li:dataset:(urn:li:dataPlatform:{system},{instance}.{queue},{env})"), + }) => { + reject_urn_delimiters(&[system, instance, queue])?; + format!("urn:li:dataset:(urn:li:dataPlatform:{system},{instance}.{queue},{env})") + } ResourceInfoRequest::Dashboard(Dashboard { system, instance, id, }) => { + reject_urn_delimiters(&[system, instance, id])?; format!("urn:li:dashboard:({system},{instance}.{id})") } ResourceInfoRequest::Chart(Chart { @@ -63,12 +80,42 @@ pub fn urn_for_request(request: &ResourceInfoRequest, env: &v1alpha1::FabricType instance, id, }) => { + reject_urn_delimiters(&[system, instance, id])?; format!("urn:li:chart:({system},{instance}.{id})") } + // Deliberately unchecked: a raw identifier *is* a URN, so it necessarily contains the + // delimiters that are rejected above. ResourceInfoRequest::RawIdentifier(RawIdentifier { identifier }) => identifier.to_string(), }; - Urn(urn) + Ok(Urn(urn)) +} + +/// The characters DataHub's URN grammar uses to delimit the parts of a URN. +/// +/// A resource name containing one of these cannot be expressed as a URN at all - DataHub's own parser +/// would split the name apart - so no URN we could build for it would ever resolve. +const URN_DELIMITERS: [char; 3] = [',', '(', ')']; + +/// Fails if any of `names` contains a [`URN_DELIMITERS`] character. +/// +/// This is checked before querying DataHub rather than after: the query is guaranteed to fail, and any +/// caller who can name a resource could otherwise turn every such name into a round trip to DataHub +/// plus a log line - for a Trino table, `SELECT * FROM tpch.sf1."a,PROD)"` is enough. +fn reject_urn_delimiters(names: &[&ParamValue]) -> Result<(), Error> { + for name in names { + if let Some(delimiter) = name.as_ref().find(URN_DELIMITERS) { + let name = name.to_string(); + let delimiter = name[delimiter..] + .chars() + .next() + .expect("the match starts at a character boundary"); + + return InvalidResourceNameSnafu { name, delimiter }.fail(); + } + } + + Ok(()) } /// Reproduces DataHub's `datahub_guid`: the container key is serialized to compact, key-sorted JSON @@ -90,6 +137,7 @@ fn container_urn( #[cfg(test)] mod tests { + use hyper::StatusCode; use rstest::rstest; use super::*; @@ -112,12 +160,66 @@ mod tests { id: id.into(), }); + assert_eq!(urn_of(request).expect("the name is valid").0, expected_urn); + } + + fn urn_of(request: ResourceInfoRequest) -> Result { + urn_for_request(&request, &v1alpha1::FabricType::Prod) + } + + fn table_named(table: &str) -> ResourceInfoRequest { + ResourceInfoRequest::Table(Table { + system: "trino".into(), + instance: "my-trino".into(), + database: "tpch".into(), + schema: "sf1".into(), + table: table.into(), + }) + } + + /// A name containing a URN delimiter cannot be expressed as a DataHub URN at all, so querying + /// DataHub with it is guaranteed to fail. Rejecting it here keeps a caller who can name a table - + /// e.g. via Trino's `SELECT * FROM tpch.sf1."a,PROD)"` - from turning every such query into a + /// round trip to DataHub. + #[rstest] + #[case::comma("a,PROD)")] + #[case::opening_paren("a(b")] + #[case::closing_paren("a)b")] + fn names_containing_urn_delimiters_are_rejected(#[case] table: &str) { + let error = urn_of(table_named(table)) + .expect_err("a name containing a URN delimiter must be rejected"); + assert_eq!( - urn_for_request(&request, &v1alpha1::FabricType::Prod).0, - expected_urn + info_fetcher_commons::http_error::Error::status_code(&error), + StatusCode::BAD_REQUEST ); } + /// Dots are not delimiters: they separate the segments of the dataset name, and a name that + /// genuinely contains one resolves as long as it was ingested the same way. + #[rstest] + #[case::plain("customer")] + #[case::dotted("a.b")] + #[case::dashed_and_underscored("my_table-2")] + #[case::colon("a:b")] + fn ordinary_names_are_accepted(#[case] table: &str) { + urn_of(table_named(table)).expect("an ordinary name must be accepted"); + } + + /// `rawIdentifier` is passed through verbatim, and for DataHub it *is* a URN - so it necessarily + /// contains the very delimiters the other endpoints reject. + #[test] + fn raw_identifiers_may_contain_urn_delimiters() { + let identifier = "urn:li:chart:(superset,my-superset.1)"; + + let urn = urn_of(ResourceInfoRequest::RawIdentifier(RawIdentifier { + identifier: identifier.into(), + })) + .expect("a raw identifier must be passed through as-is"); + + assert_eq!(urn.0, identifier); + } + /// Guards the container hash, which DataHub computes independently on ingestion: it has to keep /// matching byte for byte, or every database and schema lookup silently stops resolving. The /// expected value was computed with Python's @@ -133,7 +235,7 @@ mod tests { }); assert_eq!( - urn_for_request(&request, &v1alpha1::FabricType::Prod).0, + urn_of(request).expect("the name is valid").0, "urn:li:container:fb46bf1f985e130eeceeee8a51317cd9" ); } @@ -148,9 +250,6 @@ mod tests { id: id.into(), }); - assert_eq!( - urn_for_request(&request, &v1alpha1::FabricType::Prod).0, - expected_urn - ); + assert_eq!(urn_of(request).expect("the name is valid").0, expected_urn); } } From 66160b48a4f19cbd7ded28cb7c9ca3931ee4ad13 Mon Sep 17 00:00:00 2001 From: Malte Sander Date: Mon, 17 Aug 2026 13:50:53 +0200 Subject: [PATCH 13/18] test: Cover the URN mapping and the GraphQL response mapping `urn_for_request` and `Entity::into_response` were exercised only by the kuttl test, which needs DataHub, Trino, Kafka and Superset to run. Add golden URNs for every request variant, including the database container hash - computed independently with Python's json.dumps + hashlib.md5 the way DataHub's `datahub_guid` does, not read back out of this implementation - and one case pinning that the configured fabric reaches the dataset URN, since a fabric mismatch is what makes an otherwise correct lookup silently return nothing. For `into_response`, cover a fully populated entity and each of the paths taken when DataHub did not populate a `properties` aspect: tags, domains and data products fall back to their URN, users to the name in their URN, groups to the whole URN, and an owner with no ownership type entity to its legacy type or "unknown". Also pin that an absent `active` does not report a user as deactivated, and that owners sharing an ownership type are grouped together. --- .../src/backend/data_hub/graphql.rs | 217 ++++++++++++++++++ .../data_hub/resource_to_urn_mapping.rs | 81 ++++++- 2 files changed, 289 insertions(+), 9 deletions(-) diff --git a/rust/resource-info-fetcher/src/backend/data_hub/graphql.rs b/rust/resource-info-fetcher/src/backend/data_hub/graphql.rs index a569a756..4cf34755 100644 --- a/rust/resource-info-fetcher/src/backend/data_hub/graphql.rs +++ b/rust/resource-info-fetcher/src/backend/data_hub/graphql.rs @@ -540,4 +540,221 @@ mod tests { assert_eq!(truncation.total, DATA_PRODUCTS_PAGE_SIZE + 1); assert_eq!(truncation.received, DATA_PRODUCTS_PAGE_SIZE); } + + fn urn() -> Urn { + Urn("urn:li:dataset:(urn:li:dataPlatform:trino,my-trino.tpch.sf1.customer,PROD)".to_owned()) + } + + /// Deserializes an entity from the payload DataHub would return. + fn entity_from(payload: serde_json::Value) -> Entity { + serde_json::from_value(payload).expect("test entity must be a valid GraphQL entity payload") + } + + /// The mapping when DataHub populated every `properties` aspect, i.e. the case the fallbacks below + /// are fallbacks for. + #[test] + fn a_fully_populated_entity_maps_every_field() { + let response = entity_from(json!({ + "__typename": "Dataset", + "tags": {"tags": [{"tag": {"urn": "urn:li:tag:PII", "properties": {"name": "PII"}}}]}, + "domain": {"domain": {"urn": "urn:li:domain:finance", "properties": { + "name": "Finance", "description": "Financial data", + }}}, + "dataProducts": {"total": 1, "relationships": [{"entity": { + "urn": "urn:li:dataProduct:orders", + "properties": {"name": "Orders", "description": "Order data"}, + }}]}, + "ownership": {"owners": [ + { + "owner": { + "__typename": "CorpUser", + "urn": "urn:li:corpuser:alice", + "properties": { + "fullName": "Alice Example", "displayName": "Alice", + "email": "alice@example.com", "active": true, + }, + }, + "ownershipType": { + "urn": "urn:li:ownershipType:__system__technical_owner", + "info": {"name": "Technical Owner"}, + }, + }, + { + "owner": { + "__typename": "CorpGroup", + "urn": "urn:li:corpGroup:analytics", + "properties": {"displayName": "Analytics", "description": "The team"}, + }, + "ownershipType": { + "urn": "urn:li:ownershipType:__system__technical_owner", + "info": {"name": "Technical Owner"}, + }, + }, + ]}, + })) + .into_response(urn()); + + assert_eq!( + response.tags, + vec![Tag { + urn: Urn("urn:li:tag:PII".to_owned()), + name: "PII".to_owned(), + }] + ); + assert_eq!( + response.domain, + Some(Domain { + urn: Urn("urn:li:domain:finance".to_owned()), + name: "Finance".to_owned(), + description: Some("Financial data".to_owned()), + }) + ); + assert_eq!( + response.data_products, + vec![DataProduct { + urn: Urn("urn:li:dataProduct:orders".to_owned()), + name: "Orders".to_owned(), + description: Some("Order data".to_owned()), + }] + ); + + // Both owners share an ownership type, so they land in the same bucket rather than two. + let technical_owner = Urn("urn:li:ownershipType:__system__technical_owner".to_owned()); + assert_eq!(response.owners.len(), 1); + let owners = &response.owners[&technical_owner]; + assert_eq!( + owners.ownership_type_name.as_deref(), + Some("Technical Owner") + ); + assert_eq!( + owners.users, + vec![User { + urn: Urn("urn:li:corpuser:alice".to_owned()), + full_name: Some("Alice Example".to_owned()), + display_name: "Alice".to_owned(), + email: Some("alice@example.com".to_owned()), + active: true, + }] + ); + assert_eq!( + owners.groups, + vec![Group { + urn: Urn("urn:li:corpGroup:analytics".to_owned()), + display_name: "Analytics".to_owned(), + description: Some("The team".to_owned()), + }] + ); + } + + /// An entity whose referenced tag, domain and data product have no `properties` aspect. There is no + /// name to show, so each falls back to its URN rather than to an empty string, which would render + /// as a nameless entry in a policy decision. + #[test] + fn entities_without_properties_fall_back_to_urns() { + let response = entity_from(json!({ + "tags": {"tags": [{"tag": {"urn": "urn:li:tag:PII"}}]}, + "domain": {"domain": {"urn": "urn:li:domain:finance"}}, + "dataProducts": { + "total": 1, + "relationships": [{"entity": {"urn": "urn:li:dataProduct:orders"}}], + }, + })) + .into_response(urn()); + + assert_eq!(response.tags[0].name, "urn:li:tag:PII"); + + let domain = response.domain.expect("the domain is present"); + assert_eq!(domain.name, "urn:li:domain:finance"); + assert_eq!(domain.description, None); + + assert_eq!(response.data_products[0].name, "urn:li:dataProduct:orders"); + assert_eq!(response.data_products[0].description, None); + } + + /// Owners whose `properties` aspect is missing entirely, and owners where it exists but carries no + /// display name. A user's display name is derived from the URN; a group's falls back to the whole + /// URN, as there is no group-name convention to strip. + #[rstest] + #[case::no_properties_aspect(json!({"__typename": "CorpUser", "urn": "urn:li:corpuser:alice"}))] + #[case::no_display_name( + json!({"__typename": "CorpUser", "urn": "urn:li:corpuser:alice", "properties": {}}) + )] + fn users_without_a_display_name_are_named_after_their_urn(#[case] owner: serde_json::Value) { + let response = + entity_from(json!({"ownership": {"owners": [{"owner": owner}]}})).into_response(urn()); + + let owners = response + .owners + .values() + .next() + .expect("the owner is present"); + assert_eq!(owners.users[0].display_name, "alice"); + assert_eq!(owners.users[0].full_name, None); + assert_eq!(owners.users[0].email, None); + // Absent `active` means we must not report the user as deactivated. + assert!(owners.users[0].active); + } + + #[rstest] + #[case::no_properties_aspect( + json!({"__typename": "CorpGroup", "urn": "urn:li:corpGroup:analytics"}) + )] + #[case::no_display_name( + json!({"__typename": "CorpGroup", "urn": "urn:li:corpGroup:analytics", "properties": {}}) + )] + fn groups_without_a_display_name_are_named_after_their_urn(#[case] owner: serde_json::Value) { + let response = + entity_from(json!({"ownership": {"owners": [{"owner": owner}]}})).into_response(urn()); + + let owners = response + .owners + .values() + .next() + .expect("the owner is present"); + assert_eq!(owners.groups[0].display_name, "urn:li:corpGroup:analytics"); + assert_eq!(owners.groups[0].description, None); + } + + /// Owners predating ownership type entities only carry the legacy `type` enum, and some carry + /// neither. The key has to stay stable either way, because it is what a policy looks owners up by. + #[rstest] + #[case::legacy_type_only(json!({"type": "TECHNICAL_OWNER"}), "TECHNICAL_OWNER")] + #[case::no_type_at_all(json!({}), "unknown")] + fn owners_without_an_ownership_type_entity_fall_back_to_the_legacy_type( + #[case] extra_owner_fields: serde_json::Value, + #[case] expected_key: &str, + ) { + let mut owner = json!({ + "owner": {"__typename": "CorpUser", "urn": "urn:li:corpuser:alice"}, + }); + owner + .as_object_mut() + .expect("the owner is a JSON object") + .extend( + extra_owner_fields + .as_object() + .expect("the extra fields are a JSON object") + .clone(), + ); + + let response = entity_from(json!({"ownership": {"owners": [owner]}})).into_response(urn()); + + let (key, owners) = response.owners.iter().next().expect("the owner is present"); + assert_eq!(key.0, expected_key); + // There is no ownership type entity, so there is no human-readable name for it either. + assert_eq!(owners.ownership_type_name, None); + } + + /// The "no metadata" entity we substitute for a URN DataHub does not know must map to a response + /// that is empty rather than one that fails to build. + #[test] + fn the_default_entity_maps_to_an_empty_response() { + let response = Entity::default().into_response(urn()); + + assert_eq!(response.urn, urn()); + assert!(response.tags.is_empty()); + assert_eq!(response.domain, None); + assert!(response.data_products.is_empty()); + assert!(response.owners.is_empty()); + } } diff --git a/rust/resource-info-fetcher/src/backend/data_hub/resource_to_urn_mapping.rs b/rust/resource-info-fetcher/src/backend/data_hub/resource_to_urn_mapping.rs index 9a4d9344..ff3d02bd 100644 --- a/rust/resource-info-fetcher/src/backend/data_hub/resource_to_urn_mapping.rs +++ b/rust/resource-info-fetcher/src/backend/data_hub/resource_to_urn_mapping.rs @@ -220,23 +220,86 @@ mod tests { assert_eq!(urn.0, identifier); } - /// Guards the container hash, which DataHub computes independently on ingestion: it has to keep - /// matching byte for byte, or every database and schema lookup silently stops resolving. The - /// expected value was computed with Python's + /// Guards the container hashes, which DataHub computes independently on ingestion: they have to + /// keep matching byte for byte, or every database and schema lookup silently stops resolving. + /// + /// The expected values were computed with Python's /// `json.dumps(key, sort_keys=True, separators=(",", ":"))` and `hashlib.md5`, mirroring what - /// DataHub's `datahub_guid` does. - #[test] - fn schema_container_urn_matches_datahubs_guid() { - let request = ResourceInfoRequest::Schema(Schema { + /// DataHub's `datahub_guid` does - not read back out of this implementation. + #[rstest] + #[case::database( + ResourceInfoRequest::Database(Database { + system: "trino".into(), + instance: "my-namespace/my-trino".into(), + database: "tpch".into(), + }), + "urn:li:container:9c4275840ebdb59d3b21b4b4102e8999" + )] + #[case::schema( + ResourceInfoRequest::Schema(Schema { + system: "trino".into(), + instance: "my-namespace/my-trino".into(), + database: "tpch".into(), + schema: "sf1".into(), + }), + "urn:li:container:fb46bf1f985e130eeceeee8a51317cd9" + )] + fn container_urns_match_datahubs_guid( + #[case] request: ResourceInfoRequest, + #[case] expected_urn: &str, + ) { + assert_eq!(urn_of(request).expect("the name is valid").0, expected_urn); + } + + /// The dataset URNs are built by string interpolation rather than hashed, so these pin the exact + /// layout - including that the fabric is appended and that a table's four name segments are + /// dot-joined in order. + #[rstest] + #[case::table( + ResourceInfoRequest::Table(Table { system: "trino".into(), instance: "my-namespace/my-trino".into(), database: "tpch".into(), schema: "sf1".into(), + table: "customer".into(), + }), + "urn:li:dataset:(urn:li:dataPlatform:trino,my-namespace/my-trino.tpch.sf1.customer,PROD)" + )] + #[case::stream( + ResourceInfoRequest::Stream(Stream { + system: "kafka".into(), + instance: "my-namespace/my-kafka".into(), + queue: "orders".into(), + }), + "urn:li:dataset:(urn:li:dataPlatform:kafka,my-namespace/my-kafka.orders,PROD)" + )] + #[case::dashboard( + ResourceInfoRequest::Dashboard(Dashboard { + system: "superset".into(), + instance: "my-namespace/my-superset".into(), + id: "1".into(), + }), + "urn:li:dashboard:(superset,my-namespace/my-superset.1)" + )] + fn interpolated_urns(#[case] request: ResourceInfoRequest, #[case] expected_urn: &str) { + assert_eq!(urn_of(request).expect("the name is valid").0, expected_urn); + } + + /// The fabric is part of every dataset URN, so a mismatch with the ingestion's `env` is what makes + /// an otherwise correct lookup return nothing. + #[test] + fn the_configured_fabric_ends_up_in_dataset_urns() { + let request = ResourceInfoRequest::Stream(Stream { + system: "kafka".into(), + instance: "my-kafka".into(), + queue: "orders".into(), }); + let urn = urn_for_request(&request, &v1alpha1::FabricType::Dev).expect("the name is valid"); + assert_eq!( - urn_of(request).expect("the name is valid").0, - "urn:li:container:fb46bf1f985e130eeceeee8a51317cd9" + urn.0, + "urn:li:dataset:(urn:li:dataPlatform:kafka,my-kafka.orders,DEV)" ); } From 94a40345cbf1b134e2baeb1951313fcee60b0ef4 Mon Sep 17 00:00:00 2001 From: Malte Sander Date: Mon, 17 Aug 2026 14:08:07 +0200 Subject: [PATCH 14/18] fix: Rotate the file logs of the Stackable Rust containers The bundle-builder, user-info-fetcher and resource-info-fetcher wrote a single file log that was never rolled over and never pruned. stackable-telemetry defaults RotationPeriod to Never, and the operator only set the log level and directory, so nothing bounded those files. Vector reads them but does not rotate them, and its remove_after_secs is neither configured nor applicable to a file that is still being appended to. Set FILE_LOG_ROTATION_PERIOD=hourly and FILE_LOG_MAX_FILES=5 for all three, matching the "x 5" the bundle-builder budget already assumed. This bounds the number of files, not their size, so the budgeted MAX_*_LOG_FILE_SIZE stays an estimate. Products get a size-based appender from operator-rs; stackable-telemetry has no equivalent, see #606. --- .../usage-guide/resource-info-fetcher.adoc | 60 ++++++++----------- rust/info-fetcher-commons/src/utils/token.rs | 4 +- .../build/resource/daemonset/mod.rs | 38 +++++++++++- .../src/backend/data_hub/graphql.rs | 10 ++-- .../src/backend/data_hub/mod.rs | 6 +- .../data_hub/resource_to_urn_mapping.rs | 16 ++--- rust/user-info-fetcher/src/backend/entra.rs | 8 +-- .../user-info-fetcher/src/backend/keycloak.rs | 8 +-- 8 files changed, 86 insertions(+), 64 deletions(-) diff --git a/docs/modules/opa/pages/usage-guide/resource-info-fetcher.adoc b/docs/modules/opa/pages/usage-guide/resource-info-fetcher.adoc index e427a205..e7591970 100644 --- a/docs/modules/opa/pages/usage-guide/resource-info-fetcher.adoc +++ b/docs/modules/opa/pages/usage-guide/resource-info-fetcher.adoc @@ -92,9 +92,9 @@ The naming is intentionally product-agnostic, so that one function serves the eq `rawIdentifierResourceInfo` is the escape hatch for resources the functions above do not cover: it passes the identifier to the backend as-is. For DataHub that is a URN, such as `urn:li:chart:(superset,my-namespace/my-superset.1)`. -It lets you address any URN, but metadata is only read from the DataHub entity types the functions above map to: datasets, containers, charts and dashboards. -A URN of any other type - a `dataJob`, a `dataFlow` or one of the ML entities, for example - resolves to a record with empty `tags`, `owners` and `dataProducts`, which is indistinguishable from a resource that has no metadata. -The resource-info-fetcher logs a warning naming the entity type whenever this happens, so check its logs if a `rawIdentifierResourceInfo` lookup unexpectedly comes back empty. +Note that metadata is only read from the entity types the functions above map to: datasets, containers, charts and dashboards. +Any other type (a `dataJob` or one of the ML entities, say) comes back empty, which looks just like a resource that has no metadata. +The resource-info-fetcher logs a warning naming the entity type when this happens. The first two arguments are the same everywhere: @@ -104,12 +104,9 @@ The first two arguments are the same everywhere: The `id` taken by `dashboardResourceInfo` and `chartResourceInfo` is whatever the product identifies the resource by, and is passed through as an opaque string. Superset numbers its dashboards and charts, but products that name them instead work just as well. -Every argument is limited to 1024 bytes, which is far more than any real identifier needs. -A longer value is rejected with `400 Bad Request` rather than looked up, because arguments end up in the response cache key. - -Arguments are likewise rejected if they contain `,`, `(` or `)`. -DataHub uses those characters to delimit the parts of a URN, so a resource whose name contains one cannot be addressed at all — sending it to DataHub would only produce an error there. -`rawIdentifierResourceInfo` is exempt, since a URN necessarily contains them. +Arguments are limited to 1024 bytes, and may not contain `,`, `(` or `)`. +DataHub delimits the parts of a URN with those characters, so a resource whose name contains one cannot be addressed at all. +Either way the lookup is rejected with `400 Bad Request` instead of being sent to DataHub. `rawIdentifierResourceInfo` is exempt, as a URN necessarily contains them. The DataHub environment (fabric) is deliberately *not* an argument: it describes how the catalog was populated rather than the resource being authorized, so it is configured once on the OpaCluster (see `env` above) instead of being passed in by every Rego rule. @@ -197,15 +194,14 @@ allow if { === Metadata is not inherited from parent containers -Every function returns the metadata of the resource it addresses, and only of that resource. -Nothing is inherited along the containment hierarchy: tagging the schema `tpch.sf1` as `pii` does not make `tableResourceInfo` report `pii` for the tables inside it, and the same holds for domains and data products. +Tags, domains and data products are read from the addressed resource only. +Tagging the schema `tpch.sf1` as `pii` does not make `tableResourceInfo` report `pii` for the tables inside it. -This is deliberate. -Whether a tag applies to the children of a container is a property of your policy, not of the resource: `pii` or `confidential` plausibly cascade, while `deprecated`, `verified` or an owning team just as plausibly do not. -The resource-info-fetcher cannot tell which is which, and merging a parent's tags into the child's would destroy the distinction — a rule could no longer ask whether _this_ table is tagged, only whether anything above it is. +This is deliberate: whether a tag applies to a container's children is a property of your policy, not of the resource. +`pii` plausibly cascades, `deprecated` or an owning team plausibly do not, and we cannot tell which is which. +Merging them would also leave a rule unable to ask whether _this_ table is tagged. -Express the inheritance you want in the rule instead. -There is one function per level, so a rule can consult as many of them as it needs: +Express the inheritance you want in the rule instead, using one function per level: [source,rego] ---- @@ -237,33 +233,25 @@ Each lookup is cached and served over the loopback interface, so consulting an e [WARNING] ==== A failed metadata lookup does not deny access by itself. -Only rules that require a positive signal deny access when the lookup fails - see below. +Only the shape of your rule decides that. ==== -There are two ways a lookup can come back without the metadata a rule expects. +A lookup comes back without the metadata a rule expects in two cases: Unknown resource:: -The backend does not know the resource. -This is not an error: the resource-info-fetcher answers `200 OK` with a record whose `tags`, `owners` and `dataProducts` are empty and whose `domain` is `null`. -It is the normal answer for a resource that was never ingested into the catalog. +The resource was never ingested into the catalog. +This is not an error, so the answer is `200 OK` with empty `tags`, `owners` and `dataProducts`, and a `null` `domain`. Backend unavailable:: -The backend cannot be reached or rejects the request, for example because DataHub is down or unreachable, or because the Personal Access Token has expired or been revoked. -The resource-info-fetcher then answers with an HTTP error status and an error envelope instead of a metadata record: -+ -[source,json] ----- -{"error": {"message": "...", "causes": ["..."]}} ----- +DataHub is down or unreachable, or the Personal Access Token expired or was revoked. +The answer is then an HTTP error status with `{"error": {"message": "...", "causes": ["..."]}}` instead of a metadata record. -In both cases the rule finds no `tags` (and no `owners`, `domain` or `dataProducts`) to match on, so every expression reading them becomes undefined. -What that means for the decision depends entirely on how the rule is written: +Either way the rule finds no `tags` to match on, so any expression reading them becomes undefined: -* A rule that grants access on a *positive* signal - such as the `allow` rule above, which requires the `public` tag - becomes undefined and therefore denies access. This is what you want. -* A rule that grants access through the *absence* of a signal - `deny` if the resource is tagged `pii`, everything else allowed - also becomes undefined, and an undefined `deny` means *not denied*. A DataHub outage or an expired token then grants access to every resource, including the ones tagged `pii`. +* A rule keyed on a *positive* signal, like the `allow` example above, becomes undefined and therefore denies. This is what you want. +* A rule keyed on the *absence* of a signal (`deny` if tagged `pii`, everything else allowed) also becomes undefined, and an undefined `deny` means *not denied*. A DataHub outage then grants access to every resource, `pii` included. -So always write rules that require a positive signal. -The resource-info-fetcher deliberately does not paper over the difference between the two cases, because it cannot know whether an empty record should mean allow or deny for a given policy. -This holds for a resource that is simply not in the catalog just as much as for a backend outage: neither can be turned into a denial by the resource-info-fetcher, only by the shape of the rule. +So always require a positive signal. +The resource-info-fetcher cannot know whether an empty record should mean allow or deny for your policy, so it does not paper over the difference. -Every failed lookup is logged by the resource-info-fetcher sidecar at `WARN` level, so a backend that has become unavailable is visible in the logs (see xref:opa:usage-guide/logging.adoc[]). +Every failed lookup is logged at `WARN` level by the sidecar, so an unavailable backend shows up in the logs (see xref:opa:usage-guide/logging.adoc[]). diff --git a/rust/info-fetcher-commons/src/utils/token.rs b/rust/info-fetcher-commons/src/utils/token.rs index b060e457..240eca2a 100644 --- a/rust/info-fetcher-commons/src/utils/token.rs +++ b/rust/info-fetcher-commons/src/utils/token.rs @@ -31,7 +31,7 @@ pub struct MintedToken { /// Backends previously minted a token for every single request, which doubled the round trips per /// lookup and threw the issuer's `expires_in` away. This caches the token for the lifetime the issuer /// reported, and lets the caller drop it early via [`CachedToken::invalidate`] when the backend -/// rejects it - a token can stop working before its stated expiry, e.g. by being revoked. +/// rejects it. A token can stop working before its stated expiry, for example by being revoked. #[derive(Debug, Default)] pub struct CachedToken { cached: RwLock>, @@ -65,7 +65,7 @@ impl CachedToken { return Ok(token); } - // The read lock is released above, so another caller may have minted in between - hence the + // The read lock is released above, so another caller may have minted in between. Hence the // second look before minting ourselves. let mut cached = self.cached.write().await; if let Some(token) = Self::usable_token(&cached) { diff --git a/rust/operator-binary/src/controller/build/resource/daemonset/mod.rs b/rust/operator-binary/src/controller/build/resource/daemonset/mod.rs index 3c867046..4caaecd8 100644 --- a/rust/operator-binary/src/controller/build/resource/daemonset/mod.rs +++ b/rust/operator-binary/src/controller/build/resource/daemonset/mod.rs @@ -98,6 +98,8 @@ const LIVENESS_PROBE_INITIAL_DELAY_SECONDS: i32 = 30; const CONSOLE_LOG_LEVEL_ENV: &str = "CONSOLE_LOG_LEVEL"; const FILE_LOG_LEVEL_ENV: &str = "FILE_LOG_LEVEL"; const FILE_LOG_DIRECTORY_ENV: &str = "FILE_LOG_DIRECTORY"; +const FILE_LOG_ROTATION_PERIOD_ENV: &str = "FILE_LOG_ROTATION_PERIOD"; +const FILE_LOG_MAX_FILES_ENV: &str = "FILE_LOG_MAX_FILES"; const KUBERNETES_NODE_NAME_ENV: &str = "KUBERNETES_NODE_NAME"; const KUBERNETES_CLUSTER_DOMAIN_ENV: &str = "KUBERNETES_CLUSTER_DOMAIN"; @@ -143,6 +145,18 @@ const MAX_INFO_FETCHER_LOG_FILE_SIZE: MemoryQuantity = MemoryQuantity { unit: BinaryMultiple::Mebi, }; +// Rotation of the file logs written by the Stackable Rust containers. Nothing else bounds those +// files: the Vector agent only reads them, and they are written into the shared `log` volume, whose +// `sizeLimit` evicts the whole Pod (OPA included) once it is exceeded. +// +// Note that this bounds the number of files rather than their size, so the budgeted +// MAX_*_LOG_FILE_SIZE above stays an estimate. The products get a size-based rotating appender from +// operator-rs, but stackable-telemetry offers no size-based policy, see +// https://github.com/stackabletech/opa-operator/issues/606. MAX_FILES matches the "x 5" the +// bundle-builder budget already assumed. +const FILE_LOG_ROTATION_PERIOD: &str = "hourly"; +const FILE_LOG_MAX_FILES: u32 = 5; + #[derive(Snafu, Debug)] pub enum Error { #[snafu(display("failed to configure graceful shutdown"))] @@ -552,6 +566,8 @@ fn add_stackable_rust_cli_env_vars( FILE_LOG_DIRECTORY_ENV, format!("{STACKABLE_LOG_DIR}/{container}",), ) + .add_env_var(FILE_LOG_ROTATION_PERIOD_ENV, FILE_LOG_ROTATION_PERIOD) + .add_env_var(FILE_LOG_MAX_FILES_ENV, FILE_LOG_MAX_FILES.to_string()) .add_env_var_from_source( KUBERNETES_NODE_NAME_ENV, EnvVarSource { @@ -1222,8 +1238,8 @@ mod tests { } /// Both sidecars write their file logs below `STACKABLE_LOG_DIR`, so they have to mount the `log` - /// volume - otherwise the logs land in the container's own filesystem where the Vector agent, - /// which only sees the shared volume, cannot collect them. + /// volume. Otherwise the logs land in the container's own filesystem, where the Vector agent + /// (which only reads the shared volume) cannot see them. #[test] fn info_fetcher_sidecars_mount_the_log_volume() { let ds = build(&cluster_with_both_info_fetchers()); @@ -1243,6 +1259,24 @@ mod tests { } } + /// Nothing else bounds these files. Vector only reads them, and the products' log frameworks + /// (which operator-rs configures with a size-based rotating appender) are not involved here, so + /// the writer has to be told to roll them over. + #[test] + fn the_rust_containers_rotate_their_file_logs() { + let ds = build(&cluster_with_both_info_fetchers()); + + for container in [ + "bundle-builder", + "user-info-fetcher", + "resource-info-fetcher", + ] { + let container = container_by_name(&ds, container); + assert_eq!(env_var(&container, "FILE_LOG_ROTATION_PERIOD"), "hourly"); + assert_eq!(env_var(&container, "FILE_LOG_MAX_FILES"), "5"); + } + } + /// The sidecars share the `log` volume with the other containers, so their log files have to be /// budgeted for in its size limit as well. #[test] diff --git a/rust/resource-info-fetcher/src/backend/data_hub/graphql.rs b/rust/resource-info-fetcher/src/backend/data_hub/graphql.rs index 4cf34755..407ab06a 100644 --- a/rust/resource-info-fetcher/src/backend/data_hub/graphql.rs +++ b/rust/resource-info-fetcher/src/backend/data_hub/graphql.rs @@ -145,8 +145,8 @@ pub struct Entity { /// The entity types [`RESOURCE_INFO_QUERY`] has an inline fragment for, and whose tags, owners and /// domain we therefore read. /// -/// Anything else still resolves - `rawIdentifier` accepts any URN - but only the fields common to -/// every `Entity` come back, so the response looks exactly like that of a resource without metadata. +/// Anything else still resolves, because `rawIdentifier` accepts any URN, but only the fields common +/// to every `Entity` come back. The response then looks just like that of a resource with no metadata. const COVERED_ENTITY_TYPES: &[&str] = &["Dataset", "Container", "Chart", "Dashboard"]; #[derive(Debug, Deserialize)] @@ -289,8 +289,8 @@ pub struct DataProductsTruncation { impl Entity { /// The entity's DataHub type, if [`RESOURCE_INFO_QUERY`] does not cover it. /// - /// [`None`] means the type is covered, or that DataHub did not report one - in which case there - /// is nothing to compare against and we must not report a problem we cannot substantiate. + /// [`None`] means the type is covered, or that DataHub did not report one. In the latter case + /// there is nothing to compare against, so we must not report a problem we cannot substantiate. /// /// Callers should surface this: the response for an uncovered type is empty, and a policy has no /// way to distinguish that from a resource that carries no tags, owners or domain at all. @@ -511,7 +511,7 @@ mod tests { } /// Any other type deserializes into an entity with no tags, owners or domain, which a policy - /// cannot tell apart from a resource that genuinely has none - so it has to be reported. + /// cannot tell apart from a resource that genuinely has none, so it has to be reported. #[rstest] #[case::data_job("DataJob")] #[case::data_flow("DataFlow")] diff --git a/rust/resource-info-fetcher/src/backend/data_hub/mod.rs b/rust/resource-info-fetcher/src/backend/data_hub/mod.rs index f048d86c..ba99b9d5 100644 --- a/rust/resource-info-fetcher/src/backend/data_hub/mod.rs +++ b/rust/resource-info-fetcher/src/backend/data_hub/mod.rs @@ -26,7 +26,7 @@ mod resource_to_urn_mapping; /// Errors that can occur while resolving the backend, which happens once at startup. /// /// Kept apart from [`Error`] because these never reach a caller: a failure here means the process -/// does not come up at all, so - unlike [`Error`] - they have no HTTP status code to map to. +/// does not come up at all, so (unlike [`Error`]) they have no HTTP status code to map to. #[derive(Snafu, Debug)] pub enum ResolveError { #[snafu(display("failed to read DataHub token from {path:?}"))] @@ -268,7 +268,7 @@ impl ResolvedDataHubBackend { }; // The query only reads tags, owners and domains off the entity types it has inline fragments - // for. Anything else - reachable through `rawIdentifier`, which accepts any URN - resolves to + // for. Anything else (reachable through `rawIdentifier`, which accepts any URN) resolves to // a response that looks just like that of a resource with no metadata, so say so rather than // letting a policy silently decide on an empty record. if let Some(entity_type) = entity.uncovered_type() { @@ -391,7 +391,7 @@ mod tests { /// The status code tells the caller whose problem a failure is. The URN we query is built /// entirely from the caller's parameters, so a URN DataHub refuses to parse or resolve is a bad - /// request - reachable by any user who can name a table, e.g. via Trino's + /// request. Any user who can name a table can reach it, for example through Trino's /// `SELECT * FROM tpch.sf1."a,PROD)"`. A backend we could not reach at all, or a limitation of /// our own query, is not something the caller can do anything about. #[rstest] diff --git a/rust/resource-info-fetcher/src/backend/data_hub/resource_to_urn_mapping.rs b/rust/resource-info-fetcher/src/backend/data_hub/resource_to_urn_mapping.rs index ff3d02bd..e666164f 100644 --- a/rust/resource-info-fetcher/src/backend/data_hub/resource_to_urn_mapping.rs +++ b/rust/resource-info-fetcher/src/backend/data_hub/resource_to_urn_mapping.rs @@ -93,15 +93,15 @@ pub fn urn_for_request( /// The characters DataHub's URN grammar uses to delimit the parts of a URN. /// -/// A resource name containing one of these cannot be expressed as a URN at all - DataHub's own parser -/// would split the name apart - so no URN we could build for it would ever resolve. +/// A resource name containing one of these cannot be expressed as a URN at all, because DataHub's own +/// parser would split the name apart. No URN we build for it would ever resolve. const URN_DELIMITERS: [char; 3] = [',', '(', ')']; /// Fails if any of `names` contains a [`URN_DELIMITERS`] character. /// /// This is checked before querying DataHub rather than after: the query is guaranteed to fail, and any /// caller who can name a resource could otherwise turn every such name into a round trip to DataHub -/// plus a log line - for a Trino table, `SELECT * FROM tpch.sf1."a,PROD)"` is enough. +/// plus a log line. For a Trino table, `SELECT * FROM tpch.sf1."a,PROD)"` is enough. fn reject_urn_delimiters(names: &[&ParamValue]) -> Result<(), Error> { for name in names { if let Some(delimiter) = name.as_ref().find(URN_DELIMITERS) { @@ -178,8 +178,8 @@ mod tests { } /// A name containing a URN delimiter cannot be expressed as a DataHub URN at all, so querying - /// DataHub with it is guaranteed to fail. Rejecting it here keeps a caller who can name a table - - /// e.g. via Trino's `SELECT * FROM tpch.sf1."a,PROD)"` - from turning every such query into a + /// DataHub with it is guaranteed to fail. Rejecting it here stops a caller who can name a table + /// (via Trino's `SELECT * FROM tpch.sf1."a,PROD)"`, say) from turning every such query into a /// round trip to DataHub. #[rstest] #[case::comma("a,PROD)")] @@ -206,7 +206,7 @@ mod tests { urn_of(table_named(table)).expect("an ordinary name must be accepted"); } - /// `rawIdentifier` is passed through verbatim, and for DataHub it *is* a URN - so it necessarily + /// `rawIdentifier` is passed through verbatim, and for DataHub it *is* a URN, so it necessarily /// contains the very delimiters the other endpoints reject. #[test] fn raw_identifiers_may_contain_urn_delimiters() { @@ -225,7 +225,7 @@ mod tests { /// /// The expected values were computed with Python's /// `json.dumps(key, sort_keys=True, separators=(",", ":"))` and `hashlib.md5`, mirroring what - /// DataHub's `datahub_guid` does - not read back out of this implementation. + /// DataHub's `datahub_guid` does. They were not read back out of this implementation. #[rstest] #[case::database( ResourceInfoRequest::Database(Database { @@ -252,7 +252,7 @@ mod tests { } /// The dataset URNs are built by string interpolation rather than hashed, so these pin the exact - /// layout - including that the fabric is appended and that a table's four name segments are + /// layout, including that the fabric is appended and that a table's four name segments are /// dot-joined in order. #[rstest] #[case::table( diff --git a/rust/user-info-fetcher/src/backend/entra.rs b/rust/user-info-fetcher/src/backend/entra.rs index fe8407dd..daa9e025 100644 --- a/rust/user-info-fetcher/src/backend/entra.rs +++ b/rust/user-info-fetcher/src/backend/entra.rs @@ -205,8 +205,8 @@ impl ResolvedEntraBackend { { Err(error) if utils::http::is_unauthorized(&error) => { // The token was accepted when it was minted, so it has stopped being valid ahead of - // its stated expiry - it was revoked, or the issuer's and our clock disagree. Drop it - // and give the lookup exactly one more go with a fresh one. + // its stated expiry. It was revoked, or the issuer's and our clock disagree. Drop + // it and give the lookup exactly one more go with a fresh one. tracing::warn!( error = &error as &dyn std::error::Error, "Entra rejected the cached access token; re-authenticating and retrying once" @@ -597,8 +597,8 @@ mod tests { } /// A token can stop being accepted before it expires, e.g. by being revoked. The rejection is the - /// only way to find that out, so it has to trigger exactly one re-authentication - not none - /// (the lookup would keep failing until the token expired) and not a retry loop. + /// only way to find that out, so it has to trigger exactly one re-authentication. Not none + /// (the lookup would keep failing until the token expired), and not a retry loop. #[tokio::test] async fn test_entra_reauthenticates_once_when_the_token_is_rejected() { let mock_server = MockServer::start().await; diff --git a/rust/user-info-fetcher/src/backend/keycloak.rs b/rust/user-info-fetcher/src/backend/keycloak.rs index 2b61ca55..7367f564 100644 --- a/rust/user-info-fetcher/src/backend/keycloak.rs +++ b/rust/user-info-fetcher/src/backend/keycloak.rs @@ -181,8 +181,8 @@ impl ResolvedKeycloakBackend { { Err(error) if utils::http::is_unauthorized(&error) => { // The token was accepted when it was minted, so it has stopped being valid ahead of - // its stated expiry - it was revoked, or the issuer's and our clock disagree. Drop it - // and give the lookup exactly one more go with a fresh one. + // its stated expiry. It was revoked, or the issuer's and our clock disagree. Drop + // it and give the lookup exactly one more go with a fresh one. tracing::warn!( error = &error as &dyn std::error::Error, "Keycloak rejected the cached access token; re-authenticating and retrying once" @@ -435,8 +435,8 @@ mod tests { } /// A token can stop being accepted before it expires, e.g. by being revoked. The rejection is the - /// only way to find that out, so it has to trigger exactly one re-authentication - not none - /// (the lookup would keep failing until the token expired) and not a retry loop. + /// only way to find that out, so it has to trigger exactly one re-authentication. Not none + /// (the lookup would keep failing until the token expired), and not a retry loop. #[tokio::test] async fn keycloak_reauthenticates_once_when_the_token_is_rejected() { let mock_server = MockServer::start().await; From 931b0ae28ad6feb6db588876101b061b95463977 Mon Sep 17 00:00:00 2001 From: Malte Sander Date: Mon, 17 Aug 2026 14:26:41 +0200 Subject: [PATCH 15/18] fix: set log rotation period to minutely --- .../build/resource/daemonset/mod.rs | 22 +++++++++++++------ 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/rust/operator-binary/src/controller/build/resource/daemonset/mod.rs b/rust/operator-binary/src/controller/build/resource/daemonset/mod.rs index 4caaecd8..40226952 100644 --- a/rust/operator-binary/src/controller/build/resource/daemonset/mod.rs +++ b/rust/operator-binary/src/controller/build/resource/daemonset/mod.rs @@ -149,12 +149,17 @@ const MAX_INFO_FETCHER_LOG_FILE_SIZE: MemoryQuantity = MemoryQuantity { // files: the Vector agent only reads them, and they are written into the shared `log` volume, whose // `sizeLimit` evicts the whole Pod (OPA included) once it is exceeded. // -// Note that this bounds the number of files rather than their size, so the budgeted -// MAX_*_LOG_FILE_SIZE above stays an estimate. The products get a size-based rotating appender from -// operator-rs, but stackable-telemetry offers no size-based policy, see -// https://github.com/stackabletech/opa-operator/issues/606. MAX_FILES matches the "x 5" the -// bundle-builder budget already assumed. -const FILE_LOG_ROTATION_PERIOD: &str = "hourly"; +// This bounds how many files are kept, not how large they get, so the budgeted MAX_*_LOG_FILE_SIZE +// above stays an estimate. Products get a size-based rotating appender from operator-rs, but +// stackable-telemetry has no size-based policy, see +// https://github.com/stackabletech/opa-operator/issues/606. +// +// Hence the short period: the worst case is a sustained backend outage, where the info-fetchers log +// one line per failed request, and the retained volume is the log rate times the period times the +// file count. Keeping minutes rather than hours is what makes that survivable. Little is lost by it, +// because the Vector agent ships the lines as they are written, and the console logs (captured by the +// container runtime) are unaffected either way. +const FILE_LOG_ROTATION_PERIOD: &str = "minutely"; const FILE_LOG_MAX_FILES: u32 = 5; #[derive(Snafu, Debug)] @@ -1262,6 +1267,9 @@ mod tests { /// Nothing else bounds these files. Vector only reads them, and the products' log frameworks /// (which operator-rs configures with a size-based rotating appender) are not involved here, so /// the writer has to be told to roll them over. + /// + /// The period is asserted because it is what caps the damage during a sustained backend outage, + /// when the info-fetchers log one line per failed request. #[test] fn the_rust_containers_rotate_their_file_logs() { let ds = build(&cluster_with_both_info_fetchers()); @@ -1272,7 +1280,7 @@ mod tests { "resource-info-fetcher", ] { let container = container_by_name(&ds, container); - assert_eq!(env_var(&container, "FILE_LOG_ROTATION_PERIOD"), "hourly"); + assert_eq!(env_var(&container, "FILE_LOG_ROTATION_PERIOD"), "minutely"); assert_eq!(env_var(&container, "FILE_LOG_MAX_FILES"), "5"); } } From 9e223cc5b7aa8ceadd5c0e150f3ef0db9ecae743 Mon Sep 17 00:00:00 2001 From: Malte Sander Date: Mon, 17 Aug 2026 14:34:38 +0200 Subject: [PATCH 16/18] fix: Cache failed lookups briefly A lookup that kept failing queried DataHub again on every single request, plus one WARN line each. Anyone who can name a resource could reach it, and it was worst exactly when DataHub was already unwell. Hold both outcomes in the cache: the value becomes Found or Failed, so failures share the key space and capacity limit with successes and concurrent lookups of a failing key coalesce onto one backend call. A moka Expiry gives Failed entries a 5s lifetime and leaves Found ones to the configured entryTimeToLive. moka evicts at the earliest of the per-entry expiry and the TTL, so a TTL below 5s wins on its own. The short lifetime is deliberate: a stale success is only out of date, while a stale failure keeps failing after the backend has recovered. --- Cargo.lock | 1 + Cargo.nix | 4 + .../usage-guide/resource-info-fetcher.adoc | 1 + rust/resource-info-fetcher/Cargo.toml | 1 + .../src/backend/data_hub/mod.rs | 14 ++ rust/resource-info-fetcher/src/cache.rs | 191 ++++++++++++++++++ rust/resource-info-fetcher/src/main.rs | 105 ++++++++-- 7 files changed, 299 insertions(+), 18 deletions(-) create mode 100644 rust/resource-info-fetcher/src/cache.rs diff --git a/Cargo.lock b/Cargo.lock index 9eb2af31..b0851e10 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3741,6 +3741,7 @@ dependencies = [ "tokio", "tracing", "url", + "wiremock", ] [[package]] diff --git a/Cargo.nix b/Cargo.nix index 076befcd..4b3ad0cb 100644 --- a/Cargo.nix +++ b/Cargo.nix @@ -12391,6 +12391,10 @@ rec { name = "rstest"; packageId = "rstest"; } + { + name = "wiremock"; + packageId = "wiremock"; + } ]; }; diff --git a/docs/modules/opa/pages/usage-guide/resource-info-fetcher.adoc b/docs/modules/opa/pages/usage-guide/resource-info-fetcher.adoc index e7591970..bec6e76c 100644 --- a/docs/modules/opa/pages/usage-guide/resource-info-fetcher.adoc +++ b/docs/modules/opa/pages/usage-guide/resource-info-fetcher.adoc @@ -255,3 +255,4 @@ So always require a positive signal. The resource-info-fetcher cannot know whether an empty record should mean allow or deny for your policy, so it does not paper over the difference. Every failed lookup is logged at `WARN` level by the sidecar, so an unavailable backend shows up in the logs (see xref:opa:usage-guide/logging.adoc[]). +A failure is also cached for a few seconds, well below `entryTimeToLive`, so a lookup that keeps failing does not query the backend on every single request. diff --git a/rust/resource-info-fetcher/Cargo.toml b/rust/resource-info-fetcher/Cargo.toml index d950c78e..2b1cd116 100644 --- a/rust/resource-info-fetcher/Cargo.toml +++ b/rust/resource-info-fetcher/Cargo.toml @@ -29,6 +29,7 @@ url.workspace = true [dev-dependencies] rstest.workspace = true +wiremock.workspace = true [build-dependencies] built.workspace = true diff --git a/rust/resource-info-fetcher/src/backend/data_hub/mod.rs b/rust/resource-info-fetcher/src/backend/data_hub/mod.rs index ba99b9d5..e6576627 100644 --- a/rust/resource-info-fetcher/src/backend/data_hub/mod.rs +++ b/rust/resource-info-fetcher/src/backend/data_hub/mod.rs @@ -299,6 +299,20 @@ impl ResolvedDataHubBackend { } } +#[cfg(test)] +impl ResolvedDataHubBackend { + /// A backend querying `graphql_url`, bypassing [`ResolvedDataHubBackend::resolve`] so that no + /// credentials have to be read from disk. + pub fn for_tests(graphql_url: Url) -> Self { + Self { + token: "not-a-real-token".to_owned(), + http_client: reqwest::Client::new(), + graphql_url, + env: v1alpha1::FabricType::Prod, + } + } +} + /// Builds the DataHub GraphQL endpoint from the backend configuration. fn build_graphql_url(config: &v1alpha1::DataHubBackend) -> Result { let schema = if config.tls.uses_tls() { diff --git a/rust/resource-info-fetcher/src/cache.rs b/rust/resource-info-fetcher/src/cache.rs new file mode 100644 index 00000000..4832882c --- /dev/null +++ b/rust/resource-info-fetcher/src/cache.rs @@ -0,0 +1,191 @@ +//! The response cache, including the short-lived caching of failed lookups. + +use std::{sync::Arc, time::Duration}; + +use moka::{Expiry, future::Cache}; +use stackable_opa_operator::crd::cache; + +use crate::api::{GetResourceInfoError, ResourceInfoRequest}; + +/// How long a failed lookup is remembered. +/// +/// Without this, a lookup that keeps failing queries the backend again on every single request. That +/// is reachable by anyone who can name a resource, and it is at its worst exactly when the backend is +/// already in trouble. +/// +/// Deliberately far shorter than the configured time-to-live for successful lookups: a stale success +/// is merely out of date, while a stale failure keeps denying (or, for a rule keyed on the absence of +/// a tag, keeps granting) after the backend has recovered. If the configured time-to-live is shorter +/// than this, it wins, because moka evicts at the earliest of the two. +const FAILURE_TIME_TO_LIVE: Duration = Duration::from_secs(5); + +/// What a lookup produced, as held in the cache. +/// +/// Failures are cached as well as successes, so the variants share one key space and one capacity +/// limit, and so that moka coalesces concurrent lookups of a failing key just as it does a +/// successful one. +#[derive(Clone)] +pub enum CachedResponse { + /// The metadata to answer with, already serialized to the JSON we return. + Found(serde_json::Value), + + /// The lookup failed. Held in an [`Arc`] because every caller that hits this entry is handed the + /// same error. + Failed(Arc), +} + +pub type ResourceInfoCache = Cache; + +/// Builds the response cache from the cluster's cache configuration. +pub fn build(config: &cache::Cache) -> ResourceInfoCache { + build_with_failure_time_to_live(config, FAILURE_TIME_TO_LIVE) +} + +fn build_with_failure_time_to_live( + config: &cache::Cache, + failure_time_to_live: Duration, +) -> ResourceInfoCache { + config + .apply_settings_to_cache_builder(Cache::builder().name("resource-info")) + .expire_after(FailureExpiry { + failure_time_to_live, + }) + .build() +} + +/// Expires [`CachedResponse::Failed`] entries early, leaving successful ones to the configured +/// time-to-live. +struct FailureExpiry { + failure_time_to_live: Duration, +} + +impl FailureExpiry { + /// [`None`] leaves the entry to the cache's `time_to_live`, which is what successful lookups get. + fn time_to_live_for(&self, response: &CachedResponse) -> Option { + match response { + CachedResponse::Found(_) => None, + CachedResponse::Failed(_) => Some(self.failure_time_to_live), + } + } +} + +impl Expiry for FailureExpiry { + fn expire_after_create( + &self, + _key: &ResourceInfoRequest, + response: &CachedResponse, + _created_at: std::time::Instant, + ) -> Option { + self.time_to_live_for(response) + } + + fn expire_after_update( + &self, + _key: &ResourceInfoRequest, + response: &CachedResponse, + _updated_at: std::time::Instant, + _duration_until_expiry: Option, + ) -> Option { + self.time_to_live_for(response) + } +} + +#[cfg(test)] +mod tests { + use std::sync::atomic::{AtomicUsize, Ordering}; + + use serde_json::json; + + use super::*; + use crate::api::RawIdentifier; + + /// A cache whose failed entries live for `failure_time_to_live`, and whose successful ones live + /// for a time-to-live long enough not to interfere with any test. + fn cache(failure_time_to_live: Duration) -> ResourceInfoCache { + let config = serde_json::from_value(json!({"entryTimeToLive": "10m"})) + .expect("the cache config must be valid"); + + build_with_failure_time_to_live(&config, failure_time_to_live) + } + + fn request(identifier: &str) -> ResourceInfoRequest { + ResourceInfoRequest::RawIdentifier(RawIdentifier { + identifier: identifier.into(), + }) + } + + fn failure() -> CachedResponse { + let error = GetResourceInfoError::SerializeResponseAsJson { + source: serde_json::from_str::("not json") + .expect_err("the input is not valid JSON"), + }; + + CachedResponse::Failed(Arc::new(error)) + } + + /// Counts how often the cache had to load a value, so the tests assert on cache hits rather than + /// on timing. + #[derive(Default)] + struct Loads(AtomicUsize); + + impl Loads { + async fn load(&self, response: CachedResponse) -> CachedResponse { + self.0.fetch_add(1, Ordering::SeqCst); + response + } + + fn count(&self) -> usize { + self.0.load(Ordering::SeqCst) + } + } + + /// The point of the whole module: a failing lookup must not reach the backend again on the next + /// request. + #[tokio::test] + async fn a_failed_lookup_is_served_from_the_cache() { + let cache = cache(Duration::from_secs(60)); + let loads = Loads::default(); + let request = request("urn:li:dataset:(urn:li:dataPlatform:trino,broken,PROD)"); + + for _ in 0..3 { + cache.get_with_by_ref(&request, loads.load(failure())).await; + } + + assert_eq!(loads.count(), 1); + } + + /// A failure must not be held for the full time-to-live of a successful lookup: the backend may + /// have recovered in the meantime, and until the entry goes away every request keeps failing. + #[tokio::test] + async fn a_failed_lookup_is_forgotten_again_quickly() { + let cache = cache(Duration::from_millis(50)); + let loads = Loads::default(); + let request = request("urn:li:dataset:(urn:li:dataPlatform:trino,broken,PROD)"); + + cache.get_with_by_ref(&request, loads.load(failure())).await; + tokio::time::sleep(Duration::from_millis(100)).await; + cache.run_pending_tasks().await; + cache.get_with_by_ref(&request, loads.load(failure())).await; + + assert_eq!(loads.count(), 2); + } + + /// The short expiry must apply to failures only. A successful lookup keeps the configured + /// time-to-live, which the test's cache sets far beyond the failure one. + #[tokio::test] + async fn a_successful_lookup_keeps_the_configured_time_to_live() { + let cache = cache(Duration::from_millis(50)); + let loads = Loads::default(); + let request = request("urn:li:container:fb46bf1f985e130eeceeee8a51317cd9"); + let found = CachedResponse::Found(json!({"tags": []})); + + cache + .get_with_by_ref(&request, loads.load(found.clone())) + .await; + tokio::time::sleep(Duration::from_millis(100)).await; + cache.run_pending_tasks().await; + cache.get_with_by_ref(&request, loads.load(found)).await; + + assert_eq!(loads.count(), 1); + } +} diff --git a/rust/resource-info-fetcher/src/main.rs b/rust/resource-info-fetcher/src/main.rs index d5aa5d5a..3954022b 100644 --- a/rust/resource-info-fetcher/src/main.rs +++ b/rust/resource-info-fetcher/src/main.rs @@ -16,17 +16,20 @@ use info_fetcher_commons::{ config::{ConfigError, read_config_file}, http_error, }; -use moka::future::Cache; use serde::de::DeserializeOwned; use snafu::{ResultExt, Snafu}; use stackable_opa_operator::crd::resource_info_fetcher::v1alpha1::{self}; use stackable_operator::{cli::CommonOptions, telemetry::Tracing}; use tokio::net::TcpListener; -use crate::api::{GetResourceInfoError, ResourceInfoBackend, ResourceInfoRequest}; +use crate::{ + api::{GetResourceInfoError, ResourceInfoBackend, ResourceInfoRequest}, + cache::{CachedResponse, ResourceInfoCache}, +}; mod api; mod backend; +mod cache; pub mod built_info { include!(concat!(env!("OUT_DIR"), "/built.rs")); @@ -51,7 +54,7 @@ struct AppState { backend: Arc, // 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, + resource_info_cache: ResourceInfoCache, } /// Backend with resolved credentials. @@ -141,10 +144,7 @@ async fn main() -> Result<(), StartupError> { let config: v1alpha1::Config = read_config_file(&args.config) .with_context(|_| ParseConfigFileSnafu { path: args.config })?; let backend = Arc::new(resolve_backend(config.backend, &args.credentials_dir).await?); - let resource_info_cache = config - .cache - .apply_settings_to_cache_builder(Cache::builder().name("resource-info")) - .build(); + let resource_info_cache = cache::build(&config.cache); // One GET endpoint per resource type. They all share the same generic `metadata` handler; only // the query-parameter struct (and thus the resulting `ResourceInfoRequest` variant) differs. let app = Router::new() @@ -234,18 +234,87 @@ async fn get_resource_info( backend, resource_info_cache, } = state; - let resource_info = resource_info_cache - .try_get_with_by_ref(&request, async { - match backend.as_ref() { - ResolvedBackend::DataHub(data_hub) => { - let response = data_hub.get_resource_info(&request).await?; - serde_json::to_value(&response).map_err(|err| { - GetResourceInfoError::SerializeResponseAsJson { source: err } - }) - } + // A failed lookup is cached as well, see [`CachedResponse`], so nothing fails here: the error is + // part of the cached value rather than something the cache passes through. + let cached = resource_info_cache + .get_with_by_ref(&request, async { + match fetch_resource_info(&backend, &request).await { + Ok(resource_info) => CachedResponse::Found(resource_info), + Err(error) => CachedResponse::Failed(Arc::new(error)), } }) - .await?; + .await; + + match cached { + CachedResponse::Found(resource_info) => Ok(Json(resource_info)), + CachedResponse::Failed(error) => Err(error.into()), + } +} + +/// Queries the backend for a single resource and serializes the answer into the JSON we return. +async fn fetch_resource_info( + backend: &ResolvedBackend, + request: &ResourceInfoRequest, +) -> Result { + match backend { + ResolvedBackend::DataHub(data_hub) => { + let response = data_hub.get_resource_info(request).await?; + + serde_json::to_value(&response) + .map_err(|source| GetResourceInfoError::SerializeResponseAsJson { source }) + } + } +} + +#[cfg(test)] +mod tests { + use wiremock::{ + Mock, MockServer, ResponseTemplate, + matchers::{method, path}, + }; - Ok(Json(resource_info)) + use super::*; + use crate::api::RawIdentifier; + + /// State whose backend queries `mock_server`, with the cache a cluster gets by default. + fn state_for(mock_server: &MockServer) -> AppState { + let graphql_url = format!("{}/api/graphql", mock_server.uri()) + .parse() + .expect("the mock server's address must be a valid URL"); + + AppState { + backend: Arc::new(ResolvedBackend::DataHub( + backend::data_hub::ResolvedDataHubBackend::for_tests(graphql_url), + )), + resource_info_cache: cache::build(&Default::default()), + } + } + + fn request() -> ResourceInfoRequest { + ResourceInfoRequest::RawIdentifier(RawIdentifier { + identifier: "urn:li:dataset:(urn:li:dataPlatform:trino,broken,PROD)".into(), + }) + } + + /// A lookup that keeps failing must not query the backend again on every request. Anyone who can + /// name a resource could otherwise amplify their requests against DataHub, and worst of all + /// exactly while DataHub is already unwell. + #[tokio::test] + async fn a_failing_lookup_queries_the_backend_only_once() { + let mock_server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/api/graphql")) + .respond_with(ResponseTemplate::new(500).set_body_string("GMS is having a bad day")) + .expect(1) + .mount(&mock_server) + .await; + + let state = state_for(&mock_server); + for _ in 0..5 { + let result = get_resource_info(state.clone(), request()).await; + assert!(result.is_err(), "the lookup must keep failing"); + } + + // The mock's `.expect(1)` is verified when `mock_server` is dropped. + } } From e19ac0a631cd7d058b49023cbc208692a8982539 Mon Sep 17 00:00:00 2001 From: Malte Sander Date: Mon, 17 Aug 2026 14:38:07 +0200 Subject: [PATCH 17/18] fix: Log a failed lookup where the backend is queried The warning was emitted from `status_code`, which runs while rendering the response. Failures are now cached, so that produced a line for every request that hit a cached failure, which is exactly the burst the cache exists to prevent. Move it into `fetch_resource_info`, so one line is logged per attempt that actually reached the backend. The remaining half of the existing todo, making the level depend on the kind of error, is left in place. --- .../usage-guide/resource-info-fetcher.adoc | 4 +-- rust/resource-info-fetcher/src/api.rs | 8 ++---- rust/resource-info-fetcher/src/main.rs | 28 +++++++++++++------ 3 files changed, 24 insertions(+), 16 deletions(-) diff --git a/docs/modules/opa/pages/usage-guide/resource-info-fetcher.adoc b/docs/modules/opa/pages/usage-guide/resource-info-fetcher.adoc index bec6e76c..fe557483 100644 --- a/docs/modules/opa/pages/usage-guide/resource-info-fetcher.adoc +++ b/docs/modules/opa/pages/usage-guide/resource-info-fetcher.adoc @@ -254,5 +254,5 @@ Either way the rule finds no `tags` to match on, so any expression reading them So always require a positive signal. The resource-info-fetcher cannot know whether an empty record should mean allow or deny for your policy, so it does not paper over the difference. -Every failed lookup is logged at `WARN` level by the sidecar, so an unavailable backend shows up in the logs (see xref:opa:usage-guide/logging.adoc[]). -A failure is also cached for a few seconds, well below `entryTimeToLive`, so a lookup that keeps failing does not query the backend on every single request. +A failure is cached for a few seconds, well below `entryTimeToLive`, so a lookup that keeps failing neither queries the backend nor logs on every request. +Each attempt that does reach the backend is logged at `WARN` level, so an unavailable backend shows up in the logs (see xref:opa:usage-guide/logging.adoc[]). diff --git a/rust/resource-info-fetcher/src/api.rs b/rust/resource-info-fetcher/src/api.rs index c0ef8d21..c021e6c6 100644 --- a/rust/resource-info-fetcher/src/api.rs +++ b/rust/resource-info-fetcher/src/api.rs @@ -196,13 +196,9 @@ pub enum GetResourceInfoError { } impl http_error::Error for GetResourceInfoError { + // todo: we should make the log level (warn vs error) of the log line in `fetch_resource_info` + // more dynamic, based on the backend's impl `http_error::Error for Error`. fn status_code(&self) -> StatusCode { - // todo: the warn here loses context about the scope in which the error occurred, eg: stackable_opa_resource_info_fetcher::backend::DATA_HUB - // Also, we should make the log level (warn vs error) more dynamic in the backend's impl `http_error::Error for Error` - tracing::warn!( - error = self as &dyn std::error::Error, - "Error while processing request" - ); match self { Self::SerializeResponseAsJson { .. } => StatusCode::INTERNAL_SERVER_ERROR, Self::DataHub { source } => source.status_code(), diff --git a/rust/resource-info-fetcher/src/main.rs b/rust/resource-info-fetcher/src/main.rs index 3954022b..ecfd1701 100644 --- a/rust/resource-info-fetcher/src/main.rs +++ b/rust/resource-info-fetcher/src/main.rs @@ -256,14 +256,26 @@ async fn fetch_resource_info( backend: &ResolvedBackend, request: &ResourceInfoRequest, ) -> Result { - match backend { - ResolvedBackend::DataHub(data_hub) => { - let response = data_hub.get_resource_info(request).await?; - - serde_json::to_value(&response) - .map_err(|source| GetResourceInfoError::SerializeResponseAsJson { source }) - } - } + let resource_info = + match backend { + ResolvedBackend::DataHub(data_hub) => data_hub + .get_resource_info(request) + .await + .and_then(|response| { + serde_json::to_value(&response) + .map_err(|source| GetResourceInfoError::SerializeResponseAsJson { source }) + }), + }; + + // Logged here, where the backend was actually queried, rather than while rendering the response. + // A failure is cached (see [`CachedResponse`]), so logging it per response would produce a line + // for every request that hits the cached failure, which is precisely the burst we cache to avoid. + resource_info.inspect_err(|error| { + tracing::warn!( + error = error as &dyn std::error::Error, + "Failed to look up resource information" + ); + }) } #[cfg(test)] From 1f3290a09e8a6d018708255645719bfba89053d2 Mon Sep 17 00:00:00 2001 From: Malte Sander Date: Mon, 17 Aug 2026 14:40:57 +0200 Subject: [PATCH 18/18] docs: Fix the broken rustdoc links `cargo doc` warned about two links to `urn_for_request`, whose module was private, and about `CachedToken::get` referencing the private `EXPIRY_MARGIN`. Make the module `pub(crate)` and the constant `pub`; the margin is part of the contract, as a caller should know the token is refreshed before it expires. fix: Log a rejected request at DEBUG rather than WARN Every failed lookup was logged at WARN, including the ones caused by the request itself: an identifier no DataHub URN can express, for instance. Those say nothing about the health of this process or of the backend, and any user who can name a resource can produce them at will. Pick the level from the error's status code class, so client errors go to DEBUG with their own message and everything else stays at WARN. This is the second half of the todo removed in the previous commit; the first half was moving the log line to where the backend is actually queried. --- .../pages/usage-guide/resource-info-fetcher.adoc | 3 ++- rust/info-fetcher-commons/src/utils/token.rs | 2 +- rust/resource-info-fetcher/src/api.rs | 5 +++-- .../src/backend/data_hub/mod.rs | 2 +- rust/resource-info-fetcher/src/main.rs | 15 +++++++++++---- 5 files changed, 18 insertions(+), 9 deletions(-) diff --git a/docs/modules/opa/pages/usage-guide/resource-info-fetcher.adoc b/docs/modules/opa/pages/usage-guide/resource-info-fetcher.adoc index fe557483..f12999f4 100644 --- a/docs/modules/opa/pages/usage-guide/resource-info-fetcher.adoc +++ b/docs/modules/opa/pages/usage-guide/resource-info-fetcher.adoc @@ -255,4 +255,5 @@ So always require a positive signal. The resource-info-fetcher cannot know whether an empty record should mean allow or deny for your policy, so it does not paper over the difference. A failure is cached for a few seconds, well below `entryTimeToLive`, so a lookup that keeps failing neither queries the backend nor logs on every request. -Each attempt that does reach the backend is logged at `WARN` level, so an unavailable backend shows up in the logs (see xref:opa:usage-guide/logging.adoc[]). +An attempt that reaches the backend and fails is logged at `WARN`, so an unavailable backend shows up in the logs (see xref:opa:usage-guide/logging.adoc[]). +A lookup rejected because of the request itself, such as an identifier no URN can express, is logged at `DEBUG` instead: it says nothing about the health of the backend, and any user who can name a resource can produce those at will. diff --git a/rust/info-fetcher-commons/src/utils/token.rs b/rust/info-fetcher-commons/src/utils/token.rs index 240eca2a..3735f326 100644 --- a/rust/info-fetcher-commons/src/utils/token.rs +++ b/rust/info-fetcher-commons/src/utils/token.rs @@ -12,7 +12,7 @@ use tokio::sync::RwLock; /// A token is minted, then travels to the backend and is validated there, so handing out one that is /// about to expire risks it being rejected mid-request. Refreshing slightly early avoids that without /// needing to know anything about the backend's clock. -const EXPIRY_MARGIN: Duration = Duration::from_secs(30); +pub const EXPIRY_MARGIN: Duration = Duration::from_secs(30); /// A freshly minted bearer token. #[derive(Debug, Clone, PartialEq, Eq)] diff --git a/rust/resource-info-fetcher/src/api.rs b/rust/resource-info-fetcher/src/api.rs index c021e6c6..e4d873d4 100644 --- a/rust/resource-info-fetcher/src/api.rs +++ b/rust/resource-info-fetcher/src/api.rs @@ -196,8 +196,9 @@ pub enum GetResourceInfoError { } impl http_error::Error for GetResourceInfoError { - // todo: we should make the log level (warn vs error) of the log line in `fetch_resource_info` - // more dynamic, based on the backend's impl `http_error::Error for Error`. + /// The status code also decides at which level a failure is logged, see `fetch_resource_info`. + /// Should the client/server split ever prove too coarse for that, the level can be made a + /// property of the individual error instead. fn status_code(&self) -> StatusCode { match self { Self::SerializeResponseAsJson { .. } => StatusCode::INTERNAL_SERVER_ERROR, diff --git a/rust/resource-info-fetcher/src/backend/data_hub/mod.rs b/rust/resource-info-fetcher/src/backend/data_hub/mod.rs index e6576627..372d5795 100644 --- a/rust/resource-info-fetcher/src/backend/data_hub/mod.rs +++ b/rust/resource-info-fetcher/src/backend/data_hub/mod.rs @@ -21,7 +21,7 @@ use crate::{ }; mod graphql; -mod resource_to_urn_mapping; +pub(crate) mod resource_to_urn_mapping; /// Errors that can occur while resolving the backend, which happens once at startup. /// diff --git a/rust/resource-info-fetcher/src/main.rs b/rust/resource-info-fetcher/src/main.rs index ecfd1701..46a1e460 100644 --- a/rust/resource-info-fetcher/src/main.rs +++ b/rust/resource-info-fetcher/src/main.rs @@ -271,10 +271,17 @@ async fn fetch_resource_info( // A failure is cached (see [`CachedResponse`]), so logging it per response would produce a line // for every request that hits the cached failure, which is precisely the burst we cache to avoid. resource_info.inspect_err(|error| { - tracing::warn!( - error = error as &dyn std::error::Error, - "Failed to look up resource information" - ); + let source = error as &dyn std::error::Error; + + if http_error::Error::status_code(error).is_client_error() { + // The caller asked for something that cannot be looked up, such as a name no DataHub URN + // can express. That is their problem and says nothing about the health of this process or + // of the backend, so it does not belong in the log by default. Any user who can name a + // resource can produce these at will. + tracing::debug!(error = source, "Rejected a resource information request"); + } else { + tracing::warn!(error = source, "Failed to look up resource information"); + } }) }