chl-wxp commented on PR #10838:
URL: https://github.com/apache/seatunnel/pull/10838#issuecomment-4637934335
> Thanks for the update. I re-reviewed the latest head from scratch again,
tracing the current dynamic-metadata path from Jetty servlet registration into
the Hazelcast datasource store and back into connector config resolution.
>
> What this PR is trying to solve:
>
> * User pain: users want datasource connection properties to be registered
once and then referenced from jobs by `metadata_datasource_id`.
> * Fix approach: add a built-in `dynamic` metadata provider for Zeta,
persist datasource definitions in Hazelcast, and expose REST CRUD endpoints for
datasource management.
> * One-line summary: the feature direction is useful, but the current
management surface is still not safe enough to merge.
>
> Runtime path checked: Jetty startup -> `JettyService.createJettyServer()`
-> register `DynamicMetadataDataSourceServlet` ->
`DynamicMetadataDataSourceService` CRUD over `engine_metadataDatasource` ->
`MetadataProviderManager.resolveDataSourceConfigs()` ->
`DynamicMetadataProvider.datasourceMap()` -> merge datasource properties into
connector config.
>
> I still see two blockers on the current head:
>
> 1. +1 to @davidzollo's earlier write-surface point. `JettyService` mounts
the datasource CRUD servlet whenever `metadata.enabled=true` and
`kind=dynamic`, and `DynamicMetadataDataSourceService` then directly creates /
updates / deletes cluster-wide datasource definitions. That means enabling
dynamic metadata also exposes a management write surface that can change what
future jobs connect to, but there is still no dedicated safety gate here beyond
the general HTTP surface. For something that stores and rewrites datasource
credentials / endpoints, that is not a safe enough default boundary.
> 2. Secret masking is still not safe by default. In
`DynamicMetadataDataSourceService.convertToJson(...)`, properties are only
masked when the key matches `sensitive_keys`, but `parseSensitiveKeys(...)`
returns an empty set when the option is not configured. In other words, the
default GET/list path still returns plaintext `password` / token-style values
if the deployer does not remember to configure masking explicitly.
>
> CI note: the current `Build` check is also red on the fork run (`Dead
links` plus several broad integration jobs), but the two items above are
already sufficient blockers even before the CI red status.
>
> Conclusion: please address those blockers first, then I am happy to
re-review the latest head.
Thanks for the re-review.
I understand the concern that dynamic metadata datasource APIs are more
sensitive than ordinary read-only monitoring APIs, because they can manage
datasource
definitions used by later jobs.
However, I do not think this PR should introduce a separate authentication
requirement or endpoint-specific auth policy for `/metadata/datasource*`.
SeaTunnel currently uses a single REST security model: the whole REST
service is protected by the existing global `enable-basic-auth` option when
operators choose to
enable it. Other REST APIs that can affect runtime behavior, such as job
submit / stop / configuration-related operations, also rely on the same global
REST security
boundary. Making only dynamic metadata datasource CRUD require a different
mandatory auth rule would make the REST API security model inconsistent and
reduce
deployment flexibility.
This feature is also not exposed by default. The datasource CRUD endpoints
are registered only when the user explicitly enables the dynamic metadata
provider with:
```yaml
metadata:
enabled: true
kind: dynamic
So the current behavior is an explicit opt-in feature under the existing
REST management surface. For production deployments, the right expectation
should be
documented clearly: operators should enable the existing REST basic auth
or protect the REST service through their normal network / gateway controls. I
can strengthen
the docs to make that operational requirement explicit, but I do not think
adding another auth mechanism or silently disabling these endpoints when global
basic auth
is off is the right design for this PR.
For the sensitive_keys point, the current behavior is intentional: masking
is controlled by user configuration. If metadata.<kind>.sensitive_keys is
unset, no fields
are masked. This keeps the behavior explicit and avoids pretending that a
small built-in default list covers all secret-like keys used by different
connectors, such as
token, access_key, secret, private_key, etc.
I also noticed the docs previously said password and secret_key are masked
by default, which did not match the implementation. I have updated the REST API
docs to
remove that claim and added an example showing how to configure:
metadata:
enabled: true
kind: dynamic
dynamic:
sensitive_keys:
- password
- secret_key
So from my perspective:
1. The auth model should stay consistent with SeaTunnel's existing global
REST basic-auth model.
2. sensitive_keys is an optional explicit masking configuration, not a
default masking feature.
3. The docs should clearly state both points, and I am happy to keep
improving that documentation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
