Change dashboard API to default to off#15154
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 15154Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 15154" |
There was a problem hiding this comment.
Pull request overview
This PR changes the Dashboard Telemetry API to default to disabled so /api/telemetry/* endpoints are only registered when users explicitly opt in.
Changes:
- Flip API enablement checks to treat
Api.Enabled == nullas disabled viaGetValueOrDefault(). - Update docs/comments to reflect the new default (
false). - Update integration tests/helpers to explicitly enable the API by default and add coverage for
null/true/falseenablement.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Aspire.Dashboard.Tests/Integration/TelemetryApiTests.cs | Removes redundant disabled-API test now covered elsewhere. |
| tests/Aspire.Dashboard.Tests/Integration/StartupTests.cs | Adds theory test covering enabled null/true/false and warning behavior. |
| tests/Aspire.Dashboard.Tests/Integration/IntegrationTestHelpers.cs | Forces API enabled in default test configuration to keep existing tests working. |
| src/Aspire.Dashboard/DashboardWebApplication.cs | Updates unsecured API warning gate to treat null as disabled. |
| src/Aspire.Dashboard/DashboardEndpointsBuilder.cs | Updates endpoint mapping gate to treat null as disabled and adjusts comment. |
| src/Aspire.Dashboard/Configuration/DashboardOptions.cs | Updates XML doc to state API defaults to disabled. |
| src/Aspire.Dashboard/Components/Layout/MainLayout.razor.cs | Updates UI warning gate to treat null as disabled. |
🎬 CLI E2E Test RecordingsThe following terminal recordings are available for commit
📹 Recordings uploaded automatically from CI run #22985573073 |
mitchdenny
left a comment
mitchdenny
left a comment
There was a problem hiding this comment.
LGTM! Clean, security-positive change with good test coverage.
Nit (non-blocking): The new field DashboardApiEnabledEnvName breaks the naming convention of all other ConfigName fields, which consistently use just a Name suffix (e.g., DashboardAIDisabledName, DashboardFrontendUrlName). The "Env" infix was needed to avoid colliding with the pre-existing DashboardApiEnabledName, but it reads a bit inconsistently. Something like DashboardAspireApiEnabledName might better convey what distinguishes it (the ASPIRE_-prefixed env var path vs the binding-style path).
|
The transient CI rerun workflow requested reruns for the following jobs after analyzing the failed attempt.
|
Description
Change the dashboard Telemetry API to default to disabled (
false) instead of enabled.Previously, the
Api.Enabledoption defaulted totruewhen not explicitly configured. This change flips the default so the API endpoints (/api/telemetry/*) are not registered unless the user explicitly opts in by settingApi.Enabled = true.Key changes:
ApiOptions.Enableddefault fromtruetofalsein documentation and behavior.GetValueOrDefault()(which returnsfalsefornull) instead of== false/== truechecks.ApiEnabled_ReturnsExpectedStatusAndWarningtheory test coveringnull,true, andfalsescenarios.GetSpans_ApiDisabled_Returns404test (covered by the new theory).Checklist
<remarks />and<code />elements on your triple slash comments?aspire.devissue: