Enable vault name mapping and error suppression #1231

jamesmcroft · 2024-02-09T07:43:31Z

Description

Enable vault name mapping and error suppression. This PR supersedes #1051 including the original changes from @yash-nisar

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: #887

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

Code compiles correctly
Created/updated tests
Extended the documentation

philliphoff · 2024-02-14T20:54:58Z

src/Dapr.Extensions.Configuration/DaprSecretDescriptor.cs

+        public bool IsRequired { get; }
+
+        /// <summary>
+        /// SecretKey value is mapping value to Vault Name. If The application's Secret Name and Secret in the


Nit: the description could be more agnostic as to the backend service. That is, it may be something other than Azure Key Vault. The description for SecretName could be improved to better highlight the differences between the two.

I've updated the descriptions on the properties in the descriptor to be more reflective of their usage. Please let me know if this now makes sense.

philliphoff · 2024-02-14T21:01:20Z

src/Dapr.Extensions.Configuration/DaprSecretStoreConfigurationProvider.cs

+                            throw;
+                        }
+
+                        Console.WriteLine($"'{secretDescriptor.SecretName}' doesn't exists in Key Vault but because " +


I know that execution happens before the application host is built, so logging isn't generally available, but I'm still not certain we should be writing to the console here. I've seen other solutions where a "cached" logger provider will replay logs once the logging manager is available. I'm not saying that this PR should include such a logger implementation, but only to consider that perhaps this configuration provider accept an optional logger. The user then has the ability to get at the messages (with their own suitable logger) if/when they choose.

Alternatively, we could just decide that the benefit of the statement is not worth the infrastructure required to log it. The user has already explicitly indicated that it's optional therefore shouldn't be surprised if no value is returned by the configuration.

I agree with your second comment, this Console.WriteLine is redundant and I have removed this.

philliphoff · 2024-02-14T21:02:35Z

src/Dapr.Extensions.Configuration/DaprSecretStoreConfigurationProvider.cs


                    foreach (var key in result.Keys)
                    {
                        if (data.ContainsKey(key))
                        {
-                            throw new InvalidOperationException($"A duplicate key '{key}' was found in the secret store '{store}'. Please remove any duplicates from your secret store.");
+                            throw new InvalidOperationException($"A duplicate key '{key}' was found in the " +


Nit: was there a specific reason to split this up aside from, perhaps, overall line length (which I wouldn't be concerned with for a string)?

I have reverted this change, not sure how this occurred.

philliphoff · 2024-02-14T21:08:58Z

src/Dapr.Extensions.Configuration/DaprSecretDescriptor.cs

        /// </summary>
-        public DaprSecretDescriptor(string secretName, IReadOnlyDictionary<string, string> metadata)
+        public DaprSecretDescriptor(string secretName, IReadOnlyDictionary<string, string> metadata, bool isRequired, string secretKey)


Consider making isRequired and secretKey both optional arguments, to help reduce the explosion of constructor overloads as well as, say, allow users to only specify the secretKey but not have to then care about the isRequired argument. We might also consider doing the same for metadata.

Ideally these would all (save for secretName) bet init properties on the class, but that's a larger breaking change (depending on how many people actually specify metdata with each secret).

Removed the constructor overload bloat and consolidated this change into the existing 2 constructors using optional arguments.

philliphoff

Overall the approach is sound, but I had some feedback on the additional constructors and console statements.

codecov · 2024-02-14T21:13:37Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (817b60d) 67.20% compared to head (57dd847) 67.28%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1231      +/-   ##
==========================================
+ Coverage   67.20%   67.28%   +0.08%     
==========================================
  Files         174      174              
  Lines        5991     6006      +15     
  Branches      668      670       +2     
==========================================
+ Hits         4026     4041      +15     
  Misses       1798     1798              
  Partials      167      167

Flag	Coverage Δ
net6	`67.26% <100.00%> (+0.08%)`	⬆️
net7	`67.26% <100.00%> (+0.08%)`	⬆️
net8	`67.26% <100.00%> (+0.08%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jamesmcroft · 2024-02-15T13:46:51Z

Thanks for the review @philliphoff. I will review your comments and make any amendments

…ontract serialization framework. (dapr#1222) Signed-off-by: Whit Waldo <whit.waldo@innovian.net> Signed-off-by: James Croft <jamz_c@hotmail.co.uk>

Signed-off-by: Remco Blok <remco.blok@resilientenergy.com> Co-authored-by: Remco Blok <remco.blok@resilientenergy.com> Co-authored-by: Phillip Hoff <phillip@orst.edu> Signed-off-by: James Croft <jamz_c@hotmail.co.uk>

Signed-off-by: Yash Nisar <yashnisar@microsoft.com> Signed-off-by: James Croft <jamz_c@hotmail.co.uk>

… map Signed-off-by: James Croft <jamz_c@hotmail.co.uk>

Signed-off-by: James Croft <jamz_c@hotmail.co.uk>

* Added method to DaprClient and GRPC implementation to call cryptography proto endpoints Signed-off-by: Whit Waldo <whit.waldo@innovian.net> * First pass at implementing all exposed Cryptography methods on Go interface Signed-off-by: Whit Waldo <whit.waldo@innovian.net> * Added examples for Cryptography block Signed-off-by: Whit Waldo <whit.waldo@innovian.net> * Added missing copyright statements Signed-off-by: Whit Waldo <whit.waldo@innovian.net> * Updated to properly support Crypto API this time Signed-off-by: Whit Waldo <whit.waldo@innovian.net> * Added copyright statements Signed-off-by: Whit Waldo <whit.waldo@innovian.net> * Removed deprecated examples as the subtle APIs are presently disabled Signed-off-by: Whit Waldo <whit.waldo@innovian.net> * Updated example to reflect new API shape Signed-off-by: Whit Waldo <whit.waldo@innovian.net> * Updated example and readme Signed-off-by: Whit Waldo <whit.waldo@innovian.net> * Added overloads for encrypting/decrypting streams instead of just fixed byte arrays. Added example demonstrating the same encrypting a file via a FileStream and decrypting from a MemoryStream. Signed-off-by: Whit Waldo <whit.waldo@innovian.net> * Added some unit tests to pair with the implementation Signed-off-by: Whit Waldo <whit.waldo@innovian.net> * Added null check for the stream argument Signed-off-by: Whit Waldo <whit.waldo@innovian.net> * Changed case of the arguments as they should read "plaintext" and not "plainText" Signed-off-by: Whit Waldo <whit.waldo@innovian.net> * Reduced number of encryption implementations by just wrapping byte array into memory stream Signed-off-by: Whit Waldo <whit.waldo@innovian.net> * Constrainted returned member types per review suggestion Signed-off-by: Whit Waldo <whit.waldo@innovian.net> * Updated methods to use ReadOnlyMemory<byte> instead of byte[] - updated implementations to use low-allocation spans where possible (though ToArray is necessary to wrap with MemoryStream). Signed-off-by: Whit Waldo <whit.waldo@innovian.net> * Updated to use encryption/decryption options instead of lots of method overload variations. Simplified gRPC implementation to use fewer methods. Applied argument name updates applied previously (plainText -> plaintext). Signed-off-by: Whit Waldo <whit.waldo@innovian.net> * Updated tests Signed-off-by: Whit Waldo <whit.waldo@innovian.net> * Removed unused reference Signed-off-by: Whit Waldo <whit.waldo@innovian.net> * Updated examples to reflect new method shapes. Downgraded package to .net 6 instead of .net 8 per review suggestion. Signed-off-by: Whit Waldo <whit.waldo@innovian.net> * Updated to reflect non-aliased values per review suggestion Signed-off-by: Whit Waldo <whit.waldo@innovian.net> * Update to ensure that both send/receive streams run at the same time instead of sequentially. Signed-off-by: Whit Waldo <whit.waldo@innovian.net> * Updated to support streamed results in addition to fixed byte arrays. Refactored implementation to minimize duplicative code. Signed-off-by: Whit Waldo <whit.waldo@innovian.net> * Updated example to fix compile issue Signed-off-by: Whit Waldo <whit.waldo@innovian.net> * Removed encrypt/decrypt methods that accepted streams and returned ReadOnlyMemory<byte>. Marked implementations that use this on the gRPC class as private instead. Signed-off-by: Whit Waldo <whit.waldo@innovian.net> * Added missing Obsolete attributes on Encrypt/Decrypt methods. Added overloads on decrypt methods that do not require a DecryptionOptions to be passed in. Signed-off-by: Whit Waldo <whit.waldo@innovian.net> * Updated encrypt/decrypt options so the streaming block size no longer uses a uint. Added validation in its place to ensure the value provided is never less than or equal to 0. Signed-off-by: Whit Waldo <whit.waldo@innovian.net> * Updated how validation works in the options to accommodate lack of the shorter variation in .NET 6 Signed-off-by: Whit Waldo <whit.waldo@innovian.net> * Updated names of encrypt/decrypt streaming methods so everything uses just EncryptAsync or DecryptAsync Signed-off-by: Whit Waldo <whit.waldo@innovian.net> * Fixed regression that would have prevented data from being sent entirely to the sidecar. Also simplified operation per suggestion in review. Signed-off-by: Whit Waldo <whit.waldo@innovian.net> * Updated examples to reflect changed API Signed-off-by: Whit Waldo <whit.waldo@innovian.net> * Updated so IAsyncEnumerable methods (encrypt and decrypt) return IAsyncEnumerable<ReadOnlyMemory<byte>> instead of IAsyncEnumerable<byte[]>. Signed-off-by: Whit Waldo <whit.waldo@innovian.net> * Updated example to reflect change from IAsyncEnumerable<byte> to IAsyncEnumerable<ReadOnlyMemory<byte>> Signed-off-by: Whit Waldo <whit.waldo@innovian.net> * Avoiding allocation by using MemoryMarshal instead of .ToArray() to create MemoryStream from ReadOnlyMemory<byte>. Signed-off-by: Whit Waldo <whit.waldo@innovian.net> * Performance updates to minimize unnecessary byte array copies and eliminate unnecessary allocations. Signed-off-by: Whit Waldo <whit.waldo@innovian.net> * Removed unnecessary return from SendPlaintextStreamAsync and SendCiphertextStreamAsync methods Signed-off-by: Whit Waldo <whit.waldo@innovian.net> * Updated exception text to be more specific as to what's wrong with the input value. Signed-off-by: Whit Waldo <whit.waldo@innovian.net> * Minor tweak to prefer using using a Memory Signed-off-by: Whit Waldo <whit.waldo@innovian.net> * Deduplicated some of the Decrypt methods, simplifying the implementation Signed-off-by: Whit Waldo <whit.waldo@innovian.net> * Eliminated duplicate encryption method, simplifying implementation Signed-off-by: Whit Waldo <whit.waldo@innovian.net> * Updated to eliminate an unnecessary `await` and `async foreach`. Signed-off-by: Whit Waldo <whit.waldo@innovian.net> * Updated stream example to reflect the changes to the API shape Signed-off-by: Whit Waldo <whit.waldo@innovian.net> * Added notes about operations with stream-based data Signed-off-by: Whit Waldo <whit.waldo@innovian.net> --------- Signed-off-by: Whit Waldo <whit.waldo@innovian.net> Signed-off-by: James Croft <jamz_c@hotmail.co.uk>

Signed-off-by: James Croft Signed-off-by: James Croft <jamz_c@hotmail.co.uk>

Signed-off-by: James Croft <jamz_c@hotmail.co.uk>

jamesmcroft requested review from a team as code owners February 9, 2024 07:43

jamesmcroft force-pushed the vault-name-mapping branch 2 times, most recently from d665cad to bed18d5 Compare February 9, 2024 07:48

philliphoff reviewed Feb 14, 2024

View reviewed changes

philliphoff requested changes Feb 14, 2024

View reviewed changes

WhitWaldo and others added 8 commits February 16, 2024 11:53

Added documentation detailing how serialization works using the DataC…

840a763

…ontract serialization framework. (dapr#1222) Signed-off-by: Whit Waldo <whit.waldo@innovian.net> Signed-off-by: James Croft <jamz_c@hotmail.co.uk>

Weakly typed actor polymorphic and null responses (dapr#1214)

6337367

Signed-off-by: Remco Blok <remco.blok@resilientenergy.com> Co-authored-by: Remco Blok <remco.blok@resilientenergy.com> Co-authored-by: Phillip Hoff <phillip@orst.edu> Signed-off-by: James Croft <jamz_c@hotmail.co.uk>

Enable vault name mapping and error suppression

4e2443c

Signed-off-by: Yash Nisar <yashnisar@microsoft.com> Signed-off-by: James Croft <jamz_c@hotmail.co.uk>

Add additional secret descriptor constructor for required without key…

2b4b44d

… map Signed-off-by: James Croft <jamz_c@hotmail.co.uk>

Update configuration load exception rethrow to match rules

fefdfcb

Signed-off-by: James Croft <jamz_c@hotmail.co.uk>

Add tests for required/not required exception handling

4adc7be

Signed-off-by: James Croft <jamz_c@hotmail.co.uk>

Update DaprSecretDescriptor constructors and documentation

4a0a1e7

Signed-off-by: James Croft Signed-off-by: James Croft <jamz_c@hotmail.co.uk>

jamesmcroft force-pushed the vault-name-mapping branch from 006dc28 to 4a0a1e7 Compare February 16, 2024 11:54

jamesmcroft and others added 2 commits February 16, 2024 11:54

Merge branch 'master' into vault-name-mapping

0b55a35

Remove DaprSecretStoreConfigurationProvider Console.WriteLine

76f8a35

Signed-off-by: James Croft <jamz_c@hotmail.co.uk>

jamesmcroft requested a review from philliphoff February 16, 2024 12:16

philliphoff approved these changes Feb 16, 2024

View reviewed changes

Merge branch 'master' into vault-name-mapping

57dd847

philliphoff merged commit 034de3e into dapr:master Feb 16, 2024
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable vault name mapping and error suppression #1231

Enable vault name mapping and error suppression #1231

jamesmcroft commented Feb 9, 2024

philliphoff Feb 14, 2024

jamesmcroft Feb 16, 2024

philliphoff Feb 14, 2024

jamesmcroft Feb 16, 2024

philliphoff Feb 14, 2024

jamesmcroft Feb 16, 2024

philliphoff Feb 14, 2024

jamesmcroft Feb 16, 2024

philliphoff left a comment

codecov bot commented Feb 14, 2024 •

edited

jamesmcroft commented Feb 15, 2024

Enable vault name mapping and error suppression #1231

Enable vault name mapping and error suppression #1231

Conversation

jamesmcroft commented Feb 9, 2024

Description

Issue reference

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philliphoff left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 14, 2024 • edited

Codecov Report

jamesmcroft commented Feb 15, 2024

codecov bot commented Feb 14, 2024 •

edited