[SDKMigration]Attempt to fix missing authentication. #10245
Conversation
ryuyu
changed the title
|
Please add related workitem if you have one. |
| @@ -12,6 +12,8 @@ | |||
| using NuGet.Services.Metadata.Catalog.Helpers; | |||
| using NuGet.Services.Metadata.Catalog.Persistence; | |||
|
|
|||
| using Storage = NuGet.Services.Metadata.Catalog.Persistence.Storage; | |||
There was a problem hiding this comment.
It seems, you don't need this because you're using fully qualified namespace.
There was a problem hiding this comment.
I took a quick look in the project and found that we actually already had to make this disambiguation before, so i'm going to update these to use the existing alias instead (CatalogStorage)
ryuyu
dismissed stale reviews from erdembayar, dannyjdev, and advay26
via
f90a326
| @@ -92,7 +94,7 @@ private static ICloudBlobDirectory GetCloudBlobDirectoryUri(Uri storageBaseUri) | |||
|
|
|||
| var blobEndpoint = new Uri(storageBaseUri.GetComponents(UriComponents.SchemeAndServer, UriFormat.Unescaped)); | |||
| // Create BlobServiceClient with anonymous credentials | |||
| var blobServiceClient = new BlobServiceClient(blobEndpoint); | |||
| var blobServiceClient = new BlobServiceClientFactory(blobEndpoint, new DefaultAzureCredential()); | |||
There was a problem hiding this comment.
Is there a usage of this method that requires the MSI client id? or did you just add the DefaultAzureCredential for testing?
There was a problem hiding this comment.
As far as we (@advay26 and I) could tell, this particular code path is only called in icons.
The original code doesn't pass any authentication information at all, so at this point, we are unsure exactly what it does/how it worked/works.
DefaultAzureCredential was put here as from what I could see, it is likely the "closest" alternative.
For whomever migrates icons later, assuming that this doesn't behave as expected, may need to enhance BlobServiceClientFactory to be able to return a no auth client.
There was a problem hiding this comment.
I'm actually migrating icons, so I can verify when I start my tests for that job.
There was a problem hiding this comment.
From what I understand, DefaultAzureCredential picks up the MSI that is assigned to the VM, so it should hopefully just work for Icons too. I'm not sure what happens if we have a VM with multiple MSIs assigned to it
When a set of non null blob options are passed through to src/Catalog/Persistence/CloudBlobDirectoryWrapper.cs, the BlobServiceClient is recreated with the correct options to create the ContainerClient (containerClient inherits blob options from the creating service client).
Unfortunately, this is created without authentication information, and the authentication information is not available here.
This change attempts to "work around" this issue by instead passing through a serviceClientFactory so we can recreate with whatever options we need whenever we want.
Note that this adds a reference from NuGet.Service.Storage to Catalog project, which has its own Storage implementation. This requires us to disambiguate Storage (between NuGet.Services.Storage and catalog.persistence.Storage).
Things we tried that didn't work:
Related to https://github.com/NuGet/Engineering/issues/5584