Conversation
sbernauer
marked this pull request as ready for review
July 30, 2026 13:49
`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.
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.
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.
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.
`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.
review: fixes for spike/rif branch
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
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/