Add Storage DataProtection integration package #9961

pakrym · 2020-02-13T21:00:48Z

This migrates existing dataprotection key storage extension to track 2 (https://www.nuget.org/packages/Microsoft.AspNetCore.DataProtection.AzureStorage/)

tg-msft · 2020-02-14T18:00:42Z

Should this be Azure.Storage.Blobs.AspNetCore.DataProtection or maybe something more like Azure.AspNetCore.DataProtection.Blobs? +@KrzysztofCwalina

pakrym · 2020-02-14T18:17:45Z

Naming is something I'm very open to changing. I'm trying to get the package building and ship a preview out of our repo so partners can start using it. APIs and and names are up for discussion.

...age.Blobs.AspNetCore.DataProtection/src/Azure.Storage.Blobs.AspNetCore.DataProtection.csproj

eng/.docsettings.yml

tg-msft · 2020-02-14T18:19:10Z

sdk/storage/Azure.Storage.Blobs.AspNetCore.DataProtection/src/AzureBlobXmlRepository.cs

+            // returns a TimeSpan in the range [0.8, 1.0) * ConflictBackoffPeriod
+            // not used for crypto purposes
+            var multiplier = 0.8 + (_random.NextDouble() * 0.2);
+            return (int) (multiplier * ConflictBackoffPeriod.Ticks);


I feel like this is the third copy of this code. Might be worth filing a bug to create a shared source version of it.

I know we have exponential backoff in couple places but haven't seen random back off anywhere else. Do you have an example?

I was thinking of https://github.com/Azure/azure-sdk-for-net/blob/master/sdk/eventhub/Azure.Messaging.EventHubs.Shared/src/Core/BasicRetryPolicy.cs#L115, but that is sufficiently more complicated.

sdk/storage/Azure.Storage.Blobs.AspNetCore.DataProtection/src/AzureBlobXmlRepository.cs

tg-msft · 2020-02-14T18:33:56Z

...orage.Blobs.AspNetCore.DataProtection/src/AzureStorageBlobDataProtectionBuilderExtensions.cs

+            // The SAS token is present in the query string.
+            if (uriBuilder.Sas == null)
+            {
+                client = new BlobClient(blobUri, new DefaultAzureCredential());


I guess there's no meaningful case where this would ever be a public access blob, right? That's the only other case where the Sas would be null with a shared key.

Would it actually be nicer to throw if there's no Sas and make users supply the DefaultAzureCredential themselves in the other overload that takes a token credential? I don't have a good feel for how much magic is the right amount of magic in the ASP.NET world.

tg-msft · 2020-02-14T18:35:45Z

...orage.Blobs.AspNetCore.DataProtection/src/AzureStorageBlobDataProtectionBuilderExtensions.cs

+        /// <remarks>
+        /// The container referenced by <paramref name="blobUri"/> must already exist.
+        /// </remarks>
+        public static IDataProtectionBuilder PersistKeysToAzureBlobStorage(this IDataProtectionBuilder builder, Uri blobUri, TokenCredential tokenCredential)


I find it a little confusing that we've got helpers for Sas/TokenCredential but not SharedKeyCredential/ConnectionString. I get that users could instantiate it themselves and pass a BlobClient in to the other overload, but I'm not sure it will be as obvious to others. I'm guessing this is a design decision to guide people toward better auth choices? I think it's fine to Preview but would be worth looking to see if people struggle here.

sdk/storage/Azure.Storage.Blobs.AspNetCore.DataProtection/tests/MockResponse.cs

tg-msft · 2020-02-14T18:41:59Z

sdk/storage/Azure.Storage.Blobs.AspNetCore.DataProtection/tests/AzureBlobXmlRepositoryTests.cs

+        }
+
+        [Test]
+        public void StoreUpdatesWhenExistsAndNewerExists()


Can we add one to test what happens when a blob does exist there but it's not ASP.NET keys?

tg-msft · 2020-02-14T18:44:35Z

I feel like we should structure the naming as Azure.AspNetCore.* because this is more relevant to ASP.NET users than other Storage users.

pakrym · 2020-02-14T18:46:14Z

I feel like we should structure the naming as Azure.AspNetCore.* because this is more relevant to ASP.NET users than other Storage users.

There are other packages that need to have Extension in the name and I'm worried that Azure.Extensions.* might be a little to generic.

I like that the names are way shorted the way you propose.

heaths

Signing off with comments. Only reviewed general and KV files.

heaths · 2020-02-14T22:15:39Z

...Extensions.Configuration/src/Azure.Security.KeyVault.Secrets.Extensions.Configuration.csproj

    <PackageTags>$(PackageTags);azure;keyvault</PackageTags>
    <Version>1.0.0-preview.1</Version>
    <EnableApiCompat>false</EnableApiCompat>
+    <IsExtensionClientLibrary>true</IsExtensionClientLibrary>


Why on the test projects?

Good point, I'll move some packages under IsTestProject condition instead.

heaths · 2020-02-14T22:17:21Z

eng/Packages.Data.props

+    <PackageReference Update="Azure.Storage.Blobs" Version="12.0.0" />
+  </ItemGroup>
+
+  <ItemGroup Condition="'$(IsExtensionClientLibrary)' == 'true'">


Doesn't this increase the risk of dependencies getting out of date? I don't see any benefit here since the compiler will elide any references that aren't needed and, in fact, these don't actually add package references anyway - just update existing ones with a version.

This condition is to prevent "normal" azure SDKs from taking dependency on "extensions" packages that are intended only for a few selected plugin packages.

pakrym · 2020-02-16T19:37:29Z

Filed #10003 to follow up on extension methods.

weshaggard · 2020-02-18T17:03:32Z

eng/Packages.Data.props

+    <PackageReference Update="Microsoft.DotNet.GenAPI" Version="5.0.0-beta.19552.1" />
+
+    <!-- Azure SDK packages -->
+    <PackageReference Update="Azure.Storage.Blobs" Version="12.0.0" />


This is already in the larger package list and set to 12.2.0.

Not sure what's up with this. I just moved this reference around.

weshaggard · 2020-02-18T17:05:56Z

eng/Packages.Data.props

  </ItemGroup>

  <!-- Track 2 specific versions -->
  <ItemGroup Condition="'$(IsClientLibrary)' == 'true'">


All of the extension are already in the general item group above as well. While this override some it doesn't eliminate others. Part my issue with this change is trying to avoid duplicate versions across our different package sets. Perhaps we should figure out a way to ensure we don't have duplicate items across the different package types where there are different conflicting versions. Ideally we shouldn't find a package listed more then once in this file.

Let me see who references those and try to clean them up.

They might be referenced by the track 1 libraries. At a min we should probably make the lists mutually exclusive (i.e. make the larger top list '$(IsClientLibrary)' != 'true').

Making them exclusive might involve a lot of churn that I don't want to include into this PR. I moved all Extensions references to test project only group.

This file is also used as the baseline for ApiCompat.

We might need to start using a separate file for ApiCompat.

KrzysztofCwalina · 2020-02-18T22:03:10Z

Should this be Azure.Storage.Blobs.AspNetCore.DataProtection or maybe something more like Azure.AspNetCore.DataProtection.Blobs?

I think Azure.AspNet.DataProtection.Blobs is better. The feature is only interesting to people who use ASP.NET, and so it should be grouped in an "asp.net" group. I don't think we should sprinkle these types all over Azure SDK namespaces.

pakrym · 2020-02-18T22:40:12Z

I think Azure.AspNet.DataProtection.Blobs is better. The feature is only interesting to people who use ASP.NET, and so it should be grouped in an "asp.net" group. I don't think we should sprinkle these types all over Azure SDK namespaces.

Sounds good, but I'd like to change one this and use AspNetCore instead of AspNet as it's more consistent with other such libraries.

pakrym · 2020-02-19T00:55:20Z

/azp run net - core - ci

azure-pipelines · 2020-02-19T00:55:32Z

Azure Pipelines successfully started running 1 pipeline(s).

pakrym added 2 commits February 13, 2020 12:58

Add Storage DataProtection integration package

bc1899f

Add exclusion

e97701b

pakrym marked this pull request as ready for review February 13, 2020 22:50

pakrym requested review from JoshLove-msft, KrzysztofCwalina, amnguye, chidozieononiwu, danieljurek, mitchdenny, seanmcc-msft, tg-msft and weshaggard as code owners February 13, 2020 22:50

Merge remote-tracking branch 'azure/master' into pakrym/dp-storage

84e492e

tg-msft approved these changes Feb 14, 2020

View reviewed changes

pakrym added 2 commits February 14, 2020 11:28

First pass of FB

dfbb2bd

Strict version management

11f85f4

pakrym requested review from heaths and schaabs as code owners February 14, 2020 20:03

heaths approved these changes Feb 14, 2020

View reviewed changes

Adjust version condition

42d162f

weshaggard reviewed Feb 18, 2020

View reviewed changes

pakrym added 2 commits February 18, 2020 09:23

Cleanup more extensins

4b02535

1.1

3507e2c

pakrym added 3 commits February 18, 2020 14:51

Azure.AspNetCore.DataProtection

10f2a8b

fixes

ab1949f

More fixes

ba52e21

pakrym mentioned this pull request Feb 19, 2020

Adjust keyvault extensions package names #10034

Merged

pakrym merged commit df0806d into Azure:master Feb 19, 2020

Add Storage DataProtection integration package #9961

Add Storage DataProtection integration package #9961

Uh oh!

Conversation

pakrym commented Feb 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tg-msft commented Feb 14, 2020

Uh oh!

pakrym commented Feb 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tg-msft commented Feb 14, 2020

Uh oh!

pakrym commented Feb 14, 2020

Uh oh!

heaths left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pakrym commented Feb 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weshaggard Feb 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KrzysztofCwalina commented Feb 18, 2020

Uh oh!

pakrym commented Feb 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pakrym commented Feb 19, 2020

Uh oh!

azure-pipelines bot commented Feb 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

pakrym commented Feb 13, 2020 •

edited

Loading

pakrym commented Feb 14, 2020 •

edited

Loading

weshaggard Feb 18, 2020 •

edited

Loading

pakrym commented Feb 18, 2020 •

edited

Loading