Conversation
|
Because we had it yesterday with OpenLineage: I believe it could make sense to add a "path" or "endpoint" field. Just in case DataHub is deployed behind some proxy. :// |
Good point! For consistency reasons with UIF and AuthenticationClass I went with |
Maleware
left a comment
There was a problem hiding this comment.
Just two typos, thanks you! Test passes locally, thus LTGTM otherwise.
| } | ||
|
|
||
| /// Generates the trivial `From<Params> for ResourceInfoRequest` conversions, so each HTTP handler | ||
| /// can turn its deserialized query parameters into a [`ResourceInfoRequest`] via `.into()`. Adding a |
There was a problem hiding this comment.
| /// can turn its deserialized query parameters into a [`ResourceInfoRequest`] via `.into()`. Adding a | |
| /// can turn its deserialized query parameters into a [`ResourceInfoRequest`] via `.from()`. Adding a |
There was a problem hiding this comment.
https://doc.rust-lang.org/std/convert/trait.From.html
From automatically implements Into, so the caller can pick.
In
Into<ResourceInfoRequest>. In general I feel in this situation a into feels more natural than a from
Co-authored-by: Maximilian Wittich <56642549+Maleware@users.noreply.github.com>
|
Please vote on the CRD change as well as the rego rule API on this comment. Feel free to also look at the JSON API, but I'd say that's an implementation detail we are allowed to change in the future |
maltesander
left a comment
There was a problem hiding this comment.
First round, did not check docs etc. properly yet and not testing yet.
| let mut client_builder = ClientBuilder::new(); | ||
| client_builder = utils::tls::configure_reqwest(&config.tls, client_builder) | ||
| .await | ||
| .context(ConfigureTlsSnafu)?; | ||
| let http_client = client_builder.build().context(ConstructHttpClientSnafu)?; |
There was a problem hiding this comment.
This uses request client builder and does not have any timeouts set.
Check the UIF backends and please use utils::http::client_builder() instead.
| let resource_info_cache = { | ||
| Cache::builder() | ||
| .name("resource-info") | ||
| .time_to_live(*config.cache.entry_time_to_live) | ||
| .build() | ||
| }; |
There was a problem hiding this comment.
The sidecar has 128MB memory and this cache can grow forever. UIF is limited/keyed on usernames from a directory, so this is limited. Set a max capacity (or make it configurable)?
| // Trim trailing whitespace/newlines so the value is safe to use in an HTTP header. | ||
| let token = tokio::fs::read_to_string(&token_path) |
There was a problem hiding this comment.
This is never refreshed? Will go stale and produce 401 until restart?
| fragment ResourceInfo on Entity { | ||
| # 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. | ||
| dataProducts: relationships(input: {types: ["DataProductContains"], direction: INCOMING, count: 10}) { |
There was a problem hiding this comment.
What if there is something belonging to 11 data products? Why limit to 10? This will hard fail policies that should succeed. Can we paginate that or warn (if a much higher limit than 10 (e.g. 1000)) is reached somehow?
| @@ -0,0 +1,179 @@ | |||
| = Resource info fetcher | |||
There was a problem hiding this comment.
Not referenced in nav.adoc?
xref:opa:usage-guide/resource-info-fetcher.adoc[]
| /// | ||
| /// [`urn_for_request`]: crate::backend::data_hub::resource_to_urn_mapping::urn_for_request | ||
| #[derive(Debug, Clone, PartialEq, Eq, Hash)] | ||
| pub enum ResourceInfoRequest { |
There was a problem hiding this comment.
The instance is always required. Datahub (defaults) leave that out and a trino table will become e.g. urn:li:dataset:(urn:li:dataPlatform:trino,tpch.sf1.customer,PROD) and the tableResourceInfo always builds {instance}.tpch.sf1.customer which will be empty and therefore fail a correct rego?
Can we make instance optional and skip?
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/