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 <[email protected]> Signed-off-by: James Croft <[email protected]>

Signed-off-by: Remco Blok <[email protected]> Co-authored-by: Remco Blok <[email protected]> Co-authored-by: Phillip Hoff <[email protected]> Signed-off-by: James Croft <[email protected]>

Signed-off-by: Yash Nisar <[email protected]> Signed-off-by: James Croft <[email protected]>

… map Signed-off-by: James Croft <[email protected]>

Signed-off-by: James Croft <[email protected]>

* Added method to DaprClient and GRPC implementation to call cryptography proto endpoints Signed-off-by: Whit Waldo <[email protected]> * First pass at implementing all exposed Cryptography methods on Go interface Signed-off-by: Whit Waldo <[email protected]> * Added examples for Cryptography block Signed-off-by: Whit Waldo <[email protected]> * Added missing copyright statements Signed-off-by: Whit Waldo <[email protected]> * Updated to properly support Crypto API this time Signed-off-by: Whit Waldo <[email protected]> * Added copyright statements Signed-off-by: Whit Waldo <[email protected]> * Removed deprecated examples as the subtle APIs are presently disabled Signed-off-by: Whit Waldo <[email protected]> * Updated example to reflect new API shape Signed-off-by: Whit Waldo <[email protected]> * Updated example and readme Signed-off-by: Whit Waldo <[email protected]> * 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 <[email protected]> * Added some unit tests to pair with the implementation Signed-off-by: Whit Waldo <[email protected]> * Added null check for the stream argument Signed-off-by: Whit Waldo <[email protected]> * Changed case of the arguments as they should read "plaintext" and not "plainText" Signed-off-by: Whit Waldo <[email protected]> * Reduced number of encryption implementations by just wrapping byte array into memory stream Signed-off-by: Whit Waldo <[email protected]> * Constrainted returned member types per review suggestion Signed-off-by: Whit Waldo <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * Updated tests Signed-off-by: Whit Waldo <[email protected]> * Removed unused reference Signed-off-by: Whit Waldo <[email protected]> * Updated examples to reflect new method shapes. Downgraded package to .net 6 instead of .net 8 per review suggestion. Signed-off-by: Whit Waldo <[email protected]> * Updated to reflect non-aliased values per review suggestion Signed-off-by: Whit Waldo <[email protected]> * Update to ensure that both send/receive streams run at the same time instead of sequentially. Signed-off-by: Whit Waldo <[email protected]> * Updated to support streamed results in addition to fixed byte arrays. Refactored implementation to minimize duplicative code. Signed-off-by: Whit Waldo <[email protected]> * Updated example to fix compile issue Signed-off-by: Whit Waldo <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * Updated how validation works in the options to accommodate lack of the shorter variation in .NET 6 Signed-off-by: Whit Waldo <[email protected]> * Updated names of encrypt/decrypt streaming methods so everything uses just EncryptAsync or DecryptAsync Signed-off-by: Whit Waldo <[email protected]> * 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 <[email protected]> * Updated examples to reflect changed API Signed-off-by: Whit Waldo <[email protected]> * Updated so IAsyncEnumerable methods (encrypt and decrypt) return IAsyncEnumerable<ReadOnlyMemory<byte>> instead of IAsyncEnumerable<byte[]>. Signed-off-by: Whit Waldo <[email protected]> * Updated example to reflect change from IAsyncEnumerable<byte> to IAsyncEnumerable<ReadOnlyMemory<byte>> Signed-off-by: Whit Waldo <[email protected]> * Avoiding allocation by using MemoryMarshal instead of .ToArray() to create MemoryStream from ReadOnlyMemory<byte>. Signed-off-by: Whit Waldo <[email protected]> * Performance updates to minimize unnecessary byte array copies and eliminate unnecessary allocations. Signed-off-by: Whit Waldo <[email protected]> * Removed unnecessary return from SendPlaintextStreamAsync and SendCiphertextStreamAsync methods Signed-off-by: Whit Waldo <[email protected]> * Updated exception text to be more specific as to what's wrong with the input value. Signed-off-by: Whit Waldo <[email protected]> * Minor tweak to prefer using using a Memory Signed-off-by: Whit Waldo <[email protected]> * Deduplicated some of the Decrypt methods, simplifying the implementation Signed-off-by: Whit Waldo <[email protected]> * Eliminated duplicate encryption method, simplifying implementation Signed-off-by: Whit Waldo <[email protected]> * Updated to eliminate an unnecessary `await` and `async foreach`. Signed-off-by: Whit Waldo <[email protected]> * Updated stream example to reflect the changes to the API shape Signed-off-by: Whit Waldo <[email protected]> * Added notes about operations with stream-based data Signed-off-by: Whit Waldo <[email protected]> --------- Signed-off-by: Whit Waldo <[email protected]> Signed-off-by: James Croft <[email protected]>

Signed-off-by: James Croft Signed-off-by: James Croft <[email protected]>

Signed-off-by: James Croft <[email protected]>

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 <[email protected]> Signed-off-by: James Croft <[email protected]>

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

6337367

Signed-off-by: Remco Blok <[email protected]> Co-authored-by: Remco Blok <[email protected]> Co-authored-by: Phillip Hoff <[email protected]> Signed-off-by: James Croft <[email protected]>

Enable vault name mapping and error suppression

4e2443c

Signed-off-by: Yash Nisar <[email protected]> Signed-off-by: James Croft <[email protected]>

Add additional secret descriptor constructor for required without key…

2b4b44d

… map Signed-off-by: James Croft <[email protected]>

Update configuration load exception rethrow to match rules

fefdfcb

Signed-off-by: James Croft <[email protected]>

Add tests for required/not required exception handling

4adc7be

Signed-off-by: James Croft <[email protected]>

Update DaprSecretDescriptor constructors and documentation

4a0a1e7

Signed-off-by: James Croft Signed-off-by: James Croft <[email protected]>

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 <[email protected]>

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

Loading

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 Loading

Codecov Report

jamesmcroft commented Feb 15, 2024

codecov bot commented Feb 14, 2024 •

edited

Loading