Migrate StatusAggregator job to MSI storage authentication #10328
Conversation
| @@ -35,7 +35,7 @@ public Task CreateIfNotExistsAsync() | |||
| public async Task<T> RetrieveAsync<T>(string rowKey) | |||
| where T : class, ITableEntity | |||
| { | |||
| return (await _table.GetEntityAsync<T>(TablePartitionKeys.Get<T>(), rowKey)) as T; | |||
| return (await _table.GetEntityAsync<T>(TablePartitionKeys.Get<T>(), rowKey))?.Value as T; | |||
There was a problem hiding this comment.
This was broken since #10106
| { | ||
| var blob = _container.GetBlobClient(name); | ||
| return blob.UploadAsync(contents); | ||
| using (var stream = new MemoryStream(Encoding.UTF8.GetBytes(contents))) | ||
| { |
There was a problem hiding this comment.
Necessary to send content to correct method overload, otherwise an illegal character exception is thrown.
drewgillies
changed the title
| return new Uri(tempClient.Uri.GetLeftPart(UriPartial.Path)); | ||
| } | ||
|
|
||
| public static Uri GetPrimaryBlobServiceUri(string storageConnectionString) => GetPrimaryServiceUri(storageConnectionString); |
There was a problem hiding this comment.
Why need this function? can we cal 0GetPrimaryServiceUri directly?
There was a problem hiding this comment.
+1, then we don't need mostly cosmetic changes in src/NuGet.Jobs.Common/StorageAccountExtensions.cs
| public static Uri GetPrimaryBlobServiceUri(string storageConnectionString) => GetPrimaryServiceUri(storageConnectionString); | ||
|
|
||
|
|
||
| public static Uri GetPrimaryTableServiceUri(string storageConnectionString) |
There was a problem hiding this comment.
Seems unused. Do we really need it?
| { | ||
| var blob = _container.GetBlobClient(name); | ||
| return blob.UploadAsync(contents); | ||
| using (var stream = new MemoryStream(Encoding.UTF8.GetBytes(contents))) |
There was a problem hiding this comment.
Could this Encoding.UTF8.GetBytes safely handle data? If by “string” you mean we’ve forced arbitrary binary data into a .NET string (which can happen in edge cases, like reading a file with non-text bytes and interpreting them as characters), you can run into trouble.
There was a problem hiding this comment.
UTF-8 expects valid Unicode code points. If your data wasn’t actually text to begin with, we can end up with corruption or exceptions.
| /// </summary> | ||
| public string StorageAccount { get; set; } | ||
| public string PrimaryStorageBlobEndpoint { get; set; } |
There was a problem hiding this comment.
Do we need to make corresponding changes to NuGet.Deployment to match this change?
Addresses: https://github.com/NuGet/Engineering/issues/5447
This is 90% @advay26 (maybe more) and the rest me. :)
This change updates StatusAggregator to use MSIs for table storage access.