-
Notifications
You must be signed in to change notification settings - Fork 231
Reduce throttling by adding distributed locks #3973
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| using System; | ||
|
|
||
| using Azure.Security.KeyVault.Secrets; | ||
|
|
||
| namespace Azure.Sdk.Tools.PipelineWitness.Configuration | ||
| { | ||
| public interface ISecretClientProvider | ||
| { | ||
| SecretClient GetSecretClient(Uri vaultUri); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| using System; | ||
| using System.Linq; | ||
| using System.Text.RegularExpressions; | ||
|
|
||
| using Microsoft.Extensions.Logging; | ||
| using Microsoft.Extensions.Options; | ||
|
|
||
| namespace Azure.Sdk.Tools.PipelineWitness.Configuration | ||
| { | ||
| public class PostConfigureKeyVaultSettings<T> : IPostConfigureOptions<T> where T : class | ||
| { | ||
| private static readonly Regex secretRegex = new Regex(@"(?<vault>https://.*?\.vault\.azure\.net)/secrets/(?<secret>.*)", RegexOptions.Compiled, TimeSpan.FromSeconds(5)); | ||
| private readonly ILogger logger; | ||
| private readonly ISecretClientProvider secretClientProvider; | ||
|
|
||
| public PostConfigureKeyVaultSettings(ILogger<PostConfigureKeyVaultSettings<T>> logger, ISecretClientProvider secretClientProvider) | ||
| { | ||
| this.logger = logger; | ||
| this.secretClientProvider = secretClientProvider; | ||
| } | ||
|
|
||
| public void PostConfigure(string name, T options) | ||
| { | ||
| var stringProperties = typeof(T) | ||
| .GetProperties() | ||
| .Where(x => x.PropertyType == typeof(string)); | ||
|
|
||
| foreach (var property in stringProperties) | ||
| { | ||
| var value = (string)property.GetValue(options); | ||
|
|
||
| if (value != null) | ||
| { | ||
| var match = secretRegex.Match(value); | ||
|
|
||
| if (match.Success) | ||
| { | ||
| var vaultUrl = match.Groups["vault"].Value; | ||
| var secretName = match.Groups["secret"].Value; | ||
|
|
||
| try | ||
| { | ||
| var secretClient = this.secretClientProvider.GetSecretClient(new Uri(vaultUrl)); | ||
| this.logger.LogInformation("Replacing setting property {PropertyName} with value from secret {SecretUrl}", property.Name, value); | ||
|
|
||
| var response = secretClient.GetSecret(secretName); | ||
| var secret = response.Value; | ||
|
|
||
| property.SetValue(options, secret.Value); | ||
| } | ||
| catch (Exception exception) | ||
| { | ||
| this.logger.LogError(exception, "Unable to read secret {SecretName} from vault {VaultUrl}", secretName, vaultUrl); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| using System; | ||
|
|
||
| using Azure.Core; | ||
| using Azure.Security.KeyVault.Secrets; | ||
|
|
||
| namespace Azure.Sdk.Tools.PipelineWitness.Configuration | ||
| { | ||
| public class SecretClientProvider : ISecretClientProvider | ||
| { | ||
| private readonly TokenCredential tokenCredential; | ||
|
|
||
| public SecretClientProvider(TokenCredential tokenCredential) | ||
| { | ||
| this.tokenCredential = tokenCredential; | ||
| } | ||
|
|
||
| public SecretClient GetSecretClient(Uri vaultUri) | ||
| { | ||
| return new SecretClient(vaultUri, this.tokenCredential); | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| using System.Threading; | ||
| using System.Threading.Tasks; | ||
|
|
||
| using Azure.Sdk.Tools.PipelineWitness.Configuration; | ||
|
|
||
| using Microsoft.Azure.Cosmos; | ||
| using Microsoft.Extensions.Hosting; | ||
| using Microsoft.Extensions.Options; | ||
|
|
||
| namespace Azure.Sdk.Tools.PipelineWitness | ||
| { | ||
| internal class CosmosDatabaseInitializer : IHostedService | ||
| { | ||
| private readonly CosmosClient cosmosClient; | ||
| private readonly IOptions<PipelineWitnessSettings> options; | ||
|
|
||
| public CosmosDatabaseInitializer(CosmosClient cosmosClient, IOptions<PipelineWitnessSettings> options) | ||
| { | ||
| this.cosmosClient = cosmosClient; | ||
| this.options = options; | ||
| } | ||
|
|
||
| public async Task StartAsync(CancellationToken cancellationToken) | ||
| { | ||
| var settings = this.options.Value; | ||
|
|
||
| Database database = await this.cosmosClient.CreateDatabaseIfNotExistsAsync(settings.CosmosDatabase, cancellationToken: cancellationToken); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you spin up multiple pipeline witness instances, is this going to be a safe concurrent operation? Or would most instances just fail and restart?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe concurrent CreateIfNotExist calls result in success because they eat conflicts as proof of existence. In a perfect world, the database and container preexist the app and the app uses a database level principal with only document crud access.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense, I would say we should have a follow-up to move the database initialization into the buildout bicep templates so the principal permissions can be limited as you suggest. |
||
|
|
||
| await database.CreateContainerIfNotExistsAsync( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above re: concurrent. |
||
| new ContainerProperties | ||
| { | ||
| Id = settings.CosmosAsyncLockContainer, | ||
| PartitionKeyPath = "/id", | ||
| }, | ||
| cancellationToken: cancellationToken); | ||
| } | ||
|
|
||
| public Task StopAsync(CancellationToken cancellationToken) | ||
| { | ||
| return Task.CompletedTask; | ||
| } | ||
| } | ||
| } | ||
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.
At the risk of premature optimization, is it worth doing this async? Startup time could get high if there are a lot of secrets + client instantiations.
Uh oh!
There was an error while loading. Please reload this page.
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.
OptionsManager caches each named instance of IOptions
https://github.com/dotnet/runtime/blob/main/src/libraries/Microsoft.Extensions.Options/src/OptionsManager.cs
I think this will prevent PostConfig from running multiple times.
Within a single pass, I could group on vault and fan out on secrets per vault.