feat(config): Configuration Endpoint - ConfigurationProvider #14237
Conversation
github-actions
bot
added
product
|
✅ Meticulous spotted 0 visual differences across 1393 screens tested: view results. Meticulous evaluated ~9 hours of user flows against your PR. Expected differences? Click here. Last updated for commit f847596. This comment will update as new commits are pushed. |
Bundle ReportChanges will decrease total bundle size by 82 bytes (-0.0%) ⬇️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: datahub-react-web-esmAssets Changed:
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
metadata-io/src/main/java/com/linkedin/metadata/system_info/SystemInfoService.java
Show resolved
Hide resolved
| private boolean isAllowedProperty(String key) { | ||
| String lowerKey = key.toLowerCase(); | ||
| return ALLOWED_PATTERNS.stream().anyMatch(pattern -> pattern.matcher(lowerKey).find()) | ||
| || SENSITIVE_PATTERNS.stream().noneMatch(lowerKey::endsWith); |
There was a problem hiding this comment.
I copied the sensitive detection logic from the original PR, however I wonder how much we are concerned about avoiding exposing sensitive data.
I think the most safe is approach is the explicit allow list, when the element is not in the allow list it would be redacted. This allow list should not use wildcards (since with the wildcards it is easier to miss and leak sensitive data. At the same time there are almost 700 items in the config, so this explicit allowlist would be pretty large.
@david-leifker and @RyanHolstien - what are your thoughts on this?
There was a problem hiding this comment.
Yeah, the problem with an allow list is that we'll never keep it up to date. I was trying to balance the maintenance of an allow list with some kind of rational default.
While the endpoint is behind a restricted endpoint, the use-case is for support requests. We'd like to request the dump of system information from a customers' system for debugging and prevent leaking sensitive information to us.
There was a problem hiding this comment.
Ryan and I discussed this and we should add a unit test that explicitly will require properties to be tagged as sensitive or not (and check that the sensitive ones get redacted)
...io/src/main/java/com/linkedin/metadata/system_info/collectors/SpringComponentsCollector.java
Show resolved
Hide resolved
...io/src/main/java/com/linkedin/metadata/system_info/collectors/SpringComponentsCollector.java
Show resolved
Hide resolved
datahub-cyborg
bot
added
pending-submitter-merge
and removed
needs-review
…NAGE_SYSTEM_OPERATIONS_PRIVILEGE
| return token_data["accessToken"], token_data["metadata"]["id"] | ||
|
|
||
|
|
||
| def test_system_info_authenticated_non_admin_user_returns_403(): |
There was a problem hiding this comment.
@RyanHolstien I added this test to make sure users without MANAGE_SYSTEM_OPERATIONS_PRIVILEGE cannot access the system-info endpoints. the test does create_user and remove_user in finally so normally there should not be users left after running this test.
the pattern is similar to what privileges_and_test_user_setup from test_privileges.py (for example) is doing
|
|
||
| This directory contains end-to-end smoke tests for DataHub functionality. These tests can be run locally for faster development and debugging compared to the full CI pipeline. | ||
|
|
||
| > 📖 **Official Documentation**: For additional setup details and troubleshooting, see the [Running smoke tests locally](https://www.notion.so/acryldata/Running-smoke-tests-locally-23ffc6a6427780df8f22dd0a4e57d793) guide. |
There was a problem hiding this comment.
Can't link out to internal Notion doc on OSS repo
There was a problem hiding this comment.
oops, missed this comment - I will do another pr to remove it
| "neo4j.maxConnectionPoolSize", | ||
| "neo4j.maxTransactionRetryTime", | ||
| "neo4j.uri", | ||
| "neo4j.username", |
There was a problem hiding this comment.
Waffling on whether or not usernames should be considered sensitive, they aren't really, but might get some pushback with them. Wdyt?
There was a problem hiding this comment.
this is the non sensitive list, so they are not considered sensitive right now, it makes sense to me. David's pr also had them as such
…/datahub into sa-config-PFP-1193-david
| @Slf4j | ||
| @Component | ||
| @RequiredArgsConstructor | ||
| public class SpringComponentsCollector { |
There was a problem hiding this comment.
I removed fetchRemoteComponentInfo - it is not used and it was adding to non covered lines.
it is still here
Based on the original PR #13933 by @david-leifker, this implementation provides a focused subset of system information functionality, specifically extracting configuration data from ConfigurationProvider and Spring application context details. While David's comprehensive PR included storage systems, Kubernetes, and Kafka collectors with extensive infrastructure dependencies, this implementation streamlines the scope to focus on core system configuration and Spring component status (GMS, MAE Consumer, MCE Consumer) with a new dedicated /openapi/v1/system-info endpoint family. The implementation preserves the hierarchical JSON structure from the original design, includes robust property filtering for sensitive data redaction, and maintains the parallel execution architecture, but excludes the broader infrastructure monitoring capabilities to provide a more targeted solution for configuration visibility and component health reporting. MAE/MCE consumer info currently uses placeholder data to avoid circular dependency issues in embedded deployment mode, with the foundation prepared for future deployment mode detection as envisioned in the original PR.