Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix configuration binding #5173

Merged
merged 3 commits into from
Aug 6, 2024

Conversation

drewnoakes
Copy link
Member

@drewnoakes drewnoakes commented Aug 5, 2024

The dashboard supports the use of client certificates for authorization to the resource service. This requires a custom resource server, and we're not aware of any that use this feature.

Throughout the Aspire documentation, we advertise values like Dashboard:ResourceServiceClient:ClientCertificate:Source to configure certificates, however these values will not bind to the documented configuration values. The CLR property was ClientCertificates (plural), not the documented ClientCertificate (singular).

This change makes the dashboard respect the documented configuration names.

We don't have any automated tests here yet. We have manual test instructions in #4626 for vendors to use, which assume this fix has been applied.

Microsoft Reviewers: Open in CodeFlow

The dashboard supports the use of client certificates for authorization to the resource service. This requires a custom resource server, and we're not aware of any that use this feature.

Throughout the Aspire documentation, we advertise values like `Dashboard:ResourceServiceClient:ClientCertificates:Source` to configure certificates, however these values will not bind to the documented configuration values. The CLR property was `ClientCertificates` (plural), not the documented `ClientCertificate` (singular).

This change makes the dashboard respect the documented configuration names.
@JamesNK
Copy link
Member

JamesNK commented Aug 5, 2024

Throughout the Aspire documentation, we advertise values like Dashboard:ResourceServiceClient:ClientCertificates:Source to configure certificates, however these values will not bind to the documented configuration values.

nit: Typo in the issue description. Dashboard:ResourceServiceClient:ClientCertificate:Source is what is in the docs.

@JamesNK
Copy link
Member

JamesNK commented Aug 5, 2024

I assume fixing #4626 involves creating integration tests?

@JamesNK
Copy link
Member

JamesNK commented Aug 5, 2024

An end-to-end test with client cert auth would be useful.

However, to simply test the configuration works as expected you could write an integration test like here:

await using var app = IntegrationTestHelpers.CreateDashboardWebApplication(_testOutputHelper, config =>
{
config[DashboardConfigNames.DashboardFrontendAuthModeName.ConfigKey] = FrontendAuthMode.BrowserToken.ToString();
config[DashboardConfigNames.DashboardFrontendBrowserTokenName.ConfigKey] = apiKey;
});
await app.StartAsync();

The test passes the config value when the dashboard starts up, then gets the dashboard options from the running app and assert that the expected property is set on the options.

var options = app.DashboardOptionsMonitor.Value;
Assert.Equals("...", options.ResourceServiceClient.ClientCertificate.Source);

Consider doing adding the test in this PR or at least in an immediate follow up PR.

Edit: There is a whole file full of tests like these. Add it here: https://github.com/dotnet/aspire/blob/02a0bc6fef3f2353b306f3ada63445f3bf133b83/tests/Aspire.Dashboard.Tests/Integration/StartupTests.cs

@drewnoakes
Copy link
Member Author

I assume fixing #4626 involves creating integration tests?

Ultimately, but for now Bala's team is going to do manual testing, following the steps outlined in #4626 (comment).

you could write an integration test like here

I added one, though it wouldn't have caught the bug this PR fixes.

@JamesNK
Copy link
Member

JamesNK commented Aug 6, 2024

I don't think you can say a test wouldn't have helped here. If the config key in the test didn't set data onto the options because of a bad property name, the test would have caught the bug. And having all the configs as constants in one place is useful when writing docs because you can double check config names in the docs against the config names in the list.

@drewnoakes
Copy link
Member Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@drewnoakes drewnoakes merged commit 1397d5b into dotnet:main Aug 6, 2024
11 checks passed
@drewnoakes drewnoakes deleted the dev/drnoakes/fix-config-binding branch August 6, 2024 09:52
@github-actions github-actions bot locked and limited conversation to collaborators Sep 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants