-
Notifications
You must be signed in to change notification settings - Fork 474
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
Organize dashboard config to use strongly typed options, support primary/secondary API keys and rotation #3119
Conversation
70b3366
to
0d5aa50
Compare
11f8303
to
8badf94
Compare
Review please 🙏 |
c5e3c1a
to
12de1fb
Compare
public string? StoreName { get; set; } | ||
public StoreLocation? Location { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@drewnoakes I changed StoreName and Location just properties on the cert options.
Couple of reasons:
- This matches Kestrel endpoint config for HTTPS.
- Having store name and location in a nested
KeyStore
section seems odd when the subject isn't. And FilePath/Password isn't nested under a file section. Simpler to just have a flat list of properties.
@eerhardt we should use the config binder source gen and config schema generator, right? |
.AddScheme<OtlpCompositeAuthenticationHandlerOptions, OtlpCompositeAuthenticationHandler>(OtlpCompositeAuthenticationDefaults.AuthenticationScheme, o => { }) | ||
.AddScheme<OtlpApiKeyAuthenticationHandlerOptions, OtlpApiKeyAuthenticationHandler>(OtlpApiKeyAuthenticationDefaults.AuthenticationScheme, o => { }) | ||
.AddScheme<OtlpConnectionAuthenticationHandlerOptions, OtlpConnectionAuthenticationHandler>(OtlpConnectionAuthenticationDefaults.AuthenticationScheme, o => { }) | ||
.AddCertificate(options => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not important for this PR but it would look nicer if these were hidden in a helper extension method. Sorta like AddCertifcate is 😄
@@ -119,7 +118,7 @@ GrpcChannel CreateChannel() | |||
ClientCertificates = certificates | |||
}; | |||
|
|||
configuration.Bind("ResourceServiceClient:Ssl", httpHandler.SslOptions); | |||
configuration.Bind("Dashboard:ResourceServiceClient:Ssl", httpHandler.SslOptions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why doesn't this come from options? It's because we're binding this type directly...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's used as a simple way to set TLS config in tests without having to explicitly add more config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should own all of the options types and copy settings over explicitly. Less fragile. Can be follow up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about continuing to do this, but not advertising it in docs? In other words, it's an internal implementaiton detail. A private API.
Then we add new options if people ask for them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we preserve the schema?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand. What do you mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You said don't document it, I assume that to mean, we don't document it because we might change it later so it would be a breaking change if we do.
I'm asking if we know what the existing schema is, and can we preserve it to avoid breaking people.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the public getters and setters on https://learn.microsoft.com/en-us/dotnet/api/system.net.security.sslclientauthenticationoptions?view=net-8.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm worried about this type adding something then suddenly config binding breaks. Or we switch to the source generator then it breaks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not public API so we can switch the type behind the scenes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left feedback, but this LGTM
/backport to release/8.0-preview5 |
Started backporting to release/8.0-preview5: https://github.com/dotnet/aspire/actions/runs/8437452822 |
I would use the config binder source gen, yes. It's main benefit is trimming and AOT, but it also helps with startup perf because you don't need to use reflection to inspect the Options properties and set their values. The reflection based ConfigBinder is notoriously slow.
The config schema generator would have less value here because we don't ship a NuGet package for the Dashboard. Today, the only thing consuming these schemas is we put them in the NuGet packages, and when someone adds a PackageRef VS will pick it up and combine it in the appsettings.json schema. But here there is no flow like that. It would be useful for documentation purposes, or if we wanted to ship the config schema some other way. |
Fixes #2996
DOTNET_DASHBOARD_OTLP_ENDPOINT_URL
is set toDashboard:Otlp:EndpointUrl
.IOptionsMonitor<T>
to get latest keys after config changesDOTNET_DASHBOARD_CONFIG_FILE_PATH
which configs a location to load JSON from a file. This can be used in cloud hosting scenarios to point at a mounted volume. Changes to the file can be applied to dashboard without restarting the app (for API key rotation without process restart)TODO:
Resource service endpoint URL doesn't use new config yetUpdate dashboard readmeRemove old codeMicrosoft Reviewers: Open in CodeFlow