Conversation
sbernauer
marked this pull request as ready for review
July 30, 2026 13:49
I also had to refactor the crd mod to be only pulled in via lib.rs to fix clippy warnings
maltesander
reviewed
Aug 11, 2026
maltesander
left a comment
Member
There was a problem hiding this comment.
Claude reproduced a bigger security issue (policies can pass if backend is gone).
Silent fail-open when the backend is unavailable
| Backend state | allow if tag==public |
deny if tag==pii |
|---|---|---|
| healthy | deny | deny ✅ |
| PAT rejected (401) | deny | allow ❌ |
| GMS scaled to 0 | deny | allow ❌ |
All three containers report ready=true throughout. There is no readiness probe on the sidecar.
- Repro:
kubectl -n $NS scale deploy datahub-datahub-gms --replicas=0, wait 60s for the cache to drain, query any policy that excludes a tag. - Fix: add a readiness probe in
controller/build/resource/daemonset/resource_info_fetcher.rs
Tags and domains are not inherited from the parent container
Tagging schema tpch.sf1 with pii leaves child table nation reporting tags:[public], domain:null. A deny if pii policy allows it. DataHub exposes the parent in the same query (container { tags domain }), so this is one field away.
- Repro:
addTag/setDomainon the schema container URN, wait out the 60 s TTL, query the table. - Fix: add
container { tags {...Tags} domain {...Domain} }to the fragment atgraphql.rs:30, or state the non-inheritance explicitly in the docs.
20 tasks
Maleware
requested changes
Aug 13, 2026
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.
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.
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.
`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.
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`.
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.
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.
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.
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.
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.
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.
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.
`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
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/