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

Loads secrets in parallel #944

Merged
merged 4 commits into from Jan 15, 2019
Merged

Loads secrets in parallel #944

merged 4 commits into from Jan 15, 2019

Conversation

pakrym
Copy link

@pakrym pakrym commented Jan 14, 2019

Fixes: #910

@pakrym pakrym requested a review from davidfowl January 14, 2019 19:55
@pakrym pakrym changed the title Pakrym/load secrets in parallel Loads secrets in parallel Jan 14, 2019
@pakrym
Copy link
Author

pakrym commented Jan 14, 2019

This is technically a breaking change. We now require thread safety from customer provided IKeyVaultSecretManager implementations where it wasn't required before.

On the other hand it's inlikely that IKeyVaultSecretManager implementations would contain any non-thread safe state considering it's purpose is filtering and transforming secret names into configuration key names.

    /// <summary>
    /// The <see cref="IKeyVaultSecretManager"/> instance used to control secret loading.
    /// </summary>
    public interface IKeyVaultSecretManager
    {
        /// <summary>
        /// Checks if <see cref="SecretItem"/> value should be retrieved.
        /// </summary>
        /// <param name="secret">The <see cref="SecretItem"/> instance.</param>
        /// <returns><code>true</code> is secrets value should be loaded, otherwise <code>false</code>.</returns>
        bool Load(SecretItem secret);

        /// <summary>
        /// Maps secret to a configuration key.
        /// </summary>
        /// <param name="secret">The <see cref="SecretBundle"/> instance.</param>
        /// <returns>Configuration key name to store secret value.</returns>
        string GetKey(SecretBundle secret);
    }

/cc @muratg @Eilon

@Eilon
Copy link
Member

Eilon commented Jan 15, 2019

Out of curiosity, with this change, what is the time delta in the scenario of ~79 secrets being loaded?

@Eilon
Copy link
Member

Eilon commented Jan 15, 2019

As far as breaking changes go, I agree with the assessment. Can we update the doc comments for the affected types/methods to indicate the thread safety requirements?

@pakrym
Copy link
Author

pakrym commented Jan 15, 2019

Not a breaking change anymore. All IKeyVaultSecretManager interactions are done on the same thread now.

@pakrym
Copy link
Author

pakrym commented Jan 15, 2019

@Eilon our connection to KeyVault is pretty good so on my machine with 100 secrets load time went from 11 to 7 sec. I would imagine much better results on machines with slower access to keyvault.

@pakrym
Copy link
Author

pakrym commented Jan 15, 2019

@gfoidl thank you for the review!

@pakrym pakrym merged commit 46c65b8 into master Jan 15, 2019
@JunTaoLuo JunTaoLuo deleted the pakrym/load-secrets-in-parallel branch February 2, 2019 02:36
@dotnet dotnet locked as resolved and limited conversation to collaborators May 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants