Conversation
sbernauer
marked this pull request as ready for review
July 30, 2026 13:49
send_json_request traced every response body verbatim. The Keycloak and Entra OAuth token endpoints go through the same function, so raising the sidecar log level to TRACE wrote bearer tokens - and the PII of every user lookup - into both the console log and the Vector-collected file log. Log the URL and body size instead; the backends already trace their own parsed, secret-free payloads where that is useful.
…ilures DataHub answers 200 OK and reports resolver failures in `errors`, nulling out the field that failed. The response was deserialized in full before `errors` was inspected, so such a partial failure surfaced as "failed to parse json response" and the error messages explaining it were discarded. Keep `data` unparsed until `errors` has been checked, and read a null list field as an empty one - a nulled-out list is what a failed resolver actually returns, and `#[serde(default)]` only covers an absent field.
maxEntries defaulted to no limit, so the moka cache in the user-info- and resource-info-fetcher sidecars could grow without bound inside their 128Mi memory limit. Cache keys are built from caller-supplied parameters, which are bounded in size but not in number, so any caller who can name a resource can mint a distinct entry per request - e.g. by looping over random table names in Trino - and hold it for entryTimeToLive. Default the limit to 10000 entries and drop the unbounded setting; going over the limit now evicts least recently used entries instead of growing.
…ends The Keycloak and Entra backends each carried their own copy of the "401 -> invalidate, re-mint, retry once" block, the access_token() wrapper around CachedToken::get, the OAuthResponse struct and the mock_token test helper. Logic that is duplicated near-verbatim in two places drifts, and this is logic where drift is a security-relevant authentication bug. Move the retry into CachedToken::get_with_retry and OAuthResponse next to MintedToken in info-fetcher-commons, and the token endpoint mock into a shared test module. No behavior change beyond the warning naming the issuer in a field rather than in the message.
This reverts commit 86b119c.
…telemetry tracing-appender prunes old log files before it writes the first one, and that pass prints "Error reading the log directory/files: No such file or directory" to stderr when the directory does not exist yet. It then creates the directory itself, so logging works and the message is harmless - but it is unstructured stderr output that reads like a startup failure and reaches the log collector as an unparsable line. The other containers avoid this because their bash entrypoint runs mkdir -p first; the info-fetcher sidecars exec their binary directly. Add a shared helper and call it before Tracing::init() in both fetchers.
The user-info-fetcher and resource-info-fetcher sidecars only ever read their config, credential and kerberos volumes - the credential ones hold a DataHub PAT and OAuth2 client credentials respectively - but mounted all of them writable. Mount them read-only. The kerberos mount is safe to include because KRB5CCNAME is MEMORY:, so the credential cache never lands in that directory. The log volume stays writable, as the sidecars write their file logs to it.
The userinfo and resourceinfo libraries returned http.send(...).body without checking the status code, so the error envelope a fetcher answers a failed lookup with reached the policy as if it were user or resource information. A rule defaulting a missing field (for example object.get(user, "groups", [])) then read a failed lookup as "this user is in no groups" and allowed what it should have denied. A resource name containing a URN delimiter is enough to trigger this, and any user who can name a table can produce one. Only return a value for 200, so such a lookup is undefined instead, and set the http.send timeout explicitly. It has to stay well below the fetcher's own 60s backend budget, because an authorization decision has to be answered promptly.
Member
|
maltesander
added
release-note
Maleware
requested changes
Aug 28, 2026
| total | ||
| relationships { ...DataProduct } | ||
| } | ||
| ... on Dataset { tags { ...Tags } ownership { ...Owners } domain { ...Domain } } |
Member
There was a problem hiding this comment.
I became aware during testing, that we currently cannot differentiate between metadata empty and resource does not exists.
This is because we only check for
let Some(entity) = data.and_then(|data| data.entity) else {
debug!(%urn, "DataHub returned no entity; responding with empty resource info");
return Ok(graphql::Entity::default());
};
which returns a empty set for both cases, not found and empty. This is in turn because Entity::into_response() takes tags and if empty returns none, which will be true for both cases.
I looked up datahub docs and they offer a bool exists which would, added to the graphQL query, make those cases distinguishable.
Thus I'd suggest something along the lines:
... on Dataset { exists tags { ...Tags } ownership { ...Owners } domain { ...Domain } }
and rather probe for it in datahub::mod.rs e.g. like:
if entity.exists == Some(false) {
debug!(%urn, "DataHub does not hold this entity; reporting it as not found");
return NotFoundSnafu { urn: urn.clone() }.fail();
}
I'd argue a resource not found should error out to make the user aware of it. If we don't, no metadata present becomes equal to resource not found and might end in a debugging rabbit hole.
Member
There was a problem hiding this comment.
We talked about that and it is documented in usage-guide/resource-info-fetcher.adoc:
Behaviour when metadata is unavailable: the resource fetcher cannot know whether an empty record should mean allow or deny for a given policy...
So i would keep this as is.
The user-info-fetcher read its Keycloak/Entra client credentials and its OpenLDAP bind credentials without trimming, while the resource-info-fetcher trimmed its DataHub token. A Secret created with `echo` or `kubectl create secret --from-file` carries a trailing newline that is not part of the credential, and which was sent to the identity provider verbatim (inside the `Basic` header, or form-encoded as `%0A`), so authentication failed with an error that gave no hint at the cause.
Tokens and client secrets were plain Strings, so a `#[derive(Debug)]` on a struct holding one, a `?token` in a tracing call, or an `#[instrument]` that does not skip the right argument would write the credential to the log file the Vector agent ships off the node. Nothing did so today, but nothing stopped it either. Introduce `utils::secret::Secret`, which has no Display, renders as "[redacted]" in Debug and hands out its value only through `expose()`. Use it for the minted OAuth2 access token (including where it is cached and where it is passed to the backends), the DataHub personal access token, the Keycloak and Entra client secrets and the OpenLDAP bind password, so that leaking one now has to be deliberate.
The user-info-fetcher used moka's try_get_with, which does not insert on error, so while a lookup kept failing every single request queried the backend again. That is reachable by anyone who can name a user, and it is at its worst exactly when the backend is already in trouble. The resource-info-fetcher already solved this by caching failures with a short time-to-live of their own. Move that cache into info-fetcher-commons, generic over key, value and error type, and put both fetchers on it. The user-info-fetcher's backend dispatch moves into fetch_user_info so the cache loader is infallible, and its logging moves there with it: it previously logged from status_code(), once per rendered response, which with cached failures would log on every request that hits one. An unknown user now logs at debug rather than warn, as the caller can produce those at will.
main added `reconcile_exits_early_for_deleted_cluster` (#882), which builds a `Ctx`, while this branch added the `resource_info_fetcher_image` field to it. Neither side touches the other's lines, so the merge is clean textually but the result did not compile, which is what the CI cargo-udeps job tripped over before it got as far as looking for unused dependencies. Set the new field in that test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ECuWdW5czbvyp1WtEGz9r7
Maleware
reviewed
Aug 28, 2026
| .post(self.graphql_url.clone()) | ||
| // Authenticate with a DataHub Personal Access Token (a bearer JWT). DataHub's | ||
| // Metadata Service Authentication verifies it and resolves it to the token's actor. | ||
| .bearer_auth(self.token.expose()) |
Member
There was a problem hiding this comment.
This token only get read once at start up. Means rotating a expired token would only work if OPA pods get restarted.
IMO this should have a mechanism to reload this token without the need of an pod restart, or if not feasable, we should auto rolling restart to ensure tokens get distributed across all OPA pods without a user interacting with it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Part of #848
Yes, the diff looks big, but most of it are lockfiles and kuttl tests
CRD change
Basically the same as for the user-info-fetcher. The only difference is the
envfield, which is DataHub specific.Look at
extra/crds.yamlfor the concrete CRD changeAPI of RIF
The rego rules and HTTP API of RIF: https://github.com/stackabletech/opa-operator/blob/spike/rif/docs/modules/opa/pages/usage-guide/resource-info-fetcher.adoc
Definition of Done Checklist
Author
Reviewer
Acceptance
type/deprecationlabel & add to the deprecation scheduletype/experimentallabel & add to the experimental features trackerRelease notes
Added
For now only DataHub is supported.
Also, a rego-rule library has been added to make it easier to call resource-info-fetcher from within OPA.
The API (especially the response) might change in the future once more data catalogs are supported
Nightly docs
As soon as this PR is merged the docs will show up at https://docs.stackable.tech/home/nightly/opa/usage-guide/resource-info-fetcher/