review: fixes for spike/rif branch

CHANGELOG.md

@@ -20,12 +20,19 @@ 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
 
 - 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

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"
 

docs/modules/opa/pages/usage-guide/resource-info-fetcher.adoc

@@ -92,11 +92,22 @@ 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)`.
 
+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:
 
 * `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.
+
+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.
 
 An example of the returned structure:
@@ -129,6 +140,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.
@@ -174,6 +192,68 @@ 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.
+=== Metadata is not inherited from parent containers
+
+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 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, using one function per level:
+
+[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]
+====
+A failed metadata lookup does not deny access by itself.
+Only the shape of your rule decides that.
+====
+
+A lookup comes back without the metadata a rule expects in two cases:
+
+Unknown resource::
+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::
+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.
+
+Either way the rule finds no `tags` to match on, so any expression reading them becomes undefined:
+
+* 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 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.
+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.

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

rust/info-fetcher-commons/src/utils/http.rs

@@ -59,6 +59,28 @@ pub async fn send_json_request<T: DeserializeOwned>(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::<Error>())
+        .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<StatusCode> {
+        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<Response, Error> {
     }
     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::<serde_json::Value>("not json")
+                .expect_err("the input is not valid JSON"),
+        };
+
+        assert_eq!(error.status(), None);
+        assert!(!is_unauthorized(&error));
+    }
+}

rust/info-fetcher-commons/src/utils/mod.rs

@@ -1,2 +1,3 @@
 pub mod http;
 pub mod tls;
+pub mod token;