Skip to content
This repository was archived by the owner on Aug 28, 2024. It is now read-only.

Deliver multiple KeyVault support#880

Merged
saragluna merged 11 commits intomicrosoft:masterfrom
mnriem:keyvault-multiple-vaults
May 18, 2020
Merged

Deliver multiple KeyVault support#880
saragluna merged 11 commits intomicrosoft:masterfrom
mnriem:keyvault-multiple-vaults

Conversation

@mnriem
Copy link
Collaborator

@mnriem mnriem commented May 4, 2020

Summary

Delivers support for multiple keyvaults

Issue Type

  • New Feature

Starter Names

  • key vault spring boot starter

Additional Information

Copy link
Contributor

@saragluna saragluna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. We'd better also change the document to demonstrate how to use multiple KeyVault, and clarify the precedence of them when containing the same key.
  2. I'm not sure which pattern is better to organize the KeyVault properties:
  • The pattern used in this PR, <keyvault-name>.azure.keyvault.xxx, will keep the implementation neat.
  • The pattern like this azure.keyvault.<keyvault-name>.xxx will make the properties organized in one unified namespace.

@mnriem
Copy link
Collaborator Author

mnriem commented May 6, 2020

When you say the document which one do you mean?

On the order yes we need to clarify that the order of the .names attribute determines the order in which the respective KeyVaults are consulted.

On the naming convention I opted for the key vault name at the beginning as it was easier/cleaner in the implementation to add it, but I have no preference. So if it needs to be changed just say the word.

@saragluna
Copy link
Contributor

Sorry for not mentioning where to add them. A section describing the new feature added to the README.md will be enough.

If the <keyvault-name> has no usage other than distinguishing different configurations, we'd better use array to list these configurations, in which case the order could be expressed by their order showing up in the array.

If we want to use the <keyvault-name> pattern, I think azure.keyvault.<keyvault-name>.xxx is better even though the implementation isn't clean/compact.

@mnriem
Copy link
Collaborator Author

mnriem commented May 7, 2020

OK I will change it to use to the azure.keyvault.<keyvault-name>.xxx syntax. I will rename the .names property to .order as it expresses what it is used for from the customers perspective, it is required however as it is also used to determine whether or not to enable multiple key vault support or not.

@saragluna
Copy link
Contributor

There're some unit tests failed.

When telemetry is enabled, an HTTP request will be sent to URL `https://dc.services.visualstudio.com/v2/track`. So please make sure it's not blocked by your firewall.
Find more information about Azure Service Privacy Statement, please check [Microsoft Online Services Privacy Statement](https://www.microsoft.com/en-us/privacystatement/OnlineServices/Default.aspx).

## Multiple Key Vault support
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the user also sets the azure.keyvault.uri, what's the order between them?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The order would be dependent on the order in the file, but I feel we should caution a customer against doing that. If they opt for multiple key vaults they should only use it that way so the order is guaranteed. I will add this to the documentation.

…figuration if multiple key vaults are to be used
@mnriem mnriem marked this pull request as ready for review May 14, 2020 20:26
@saragluna saragluna merged commit 194b454 into microsoft:master May 18, 2020
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.

2 participants