Add publishing to the dotnetbuilds storage account alongside dotnetcli#7877
Conversation
| var path = TestInputs.GetFullPath("Symbol"); | ||
| var buildAsset = new Dictionary<string, HashSet<Asset>>(); | ||
| var publish = task.HandleSymbolPublishingAsync(path, MsdlToken, SymWebToken, "", false, buildAsset, null, path); | ||
| await task.HandleSymbolPublishingAsync(path, MsdlToken, SymWebToken, "", false, buildAsset, null, path); |
There was a problem hiding this comment.
That's... troubling. :-)
|
|
||
| protected AssetPublisher(TaskLoggingHelper log) | ||
| { | ||
| Log = log; |
There was a problem hiding this comment.
Why not just let the base classes handle this if they want? Rather than forcing everyone to call base(log)?
This abstract class isn't really adding any functionality that an interface with the single "PublishAssetAsync" doesn't provide.
There was a problem hiding this comment.
All of the implementations needed it, and calling base(log) isn't really difficult. An interface would play nicer with DI if we do it in the future, so I will change it.
| { | ||
| Log.LogError( | ||
| $"Azure DevOps NuGetFeed was not in the expected format '{PublishingConstants.AzDoNuGetFeedPattern}'"); | ||
| return; |
There was a problem hiding this comment.
You can... return from a constructor? Is that... wise? This feels like it should throw, since nothing later has anything like "did the complete normally" which is a weird thing to have to ask.
There was a problem hiding this comment.
Yea, you can return. This is this way because it was that way where I moved it from, but it should 100% be a throw, I will change it.
|
|
||
| protected override void Dispose(bool disposing) | ||
| { | ||
| _httpClient?.Dispose(); |
There was a problem hiding this comment.
This needs to only happen in "disposing==true". If it's fale, the _httpClient might already be GC'd. (antoher reason maybe having the base class isn't a good idea.
| { | ||
| var localMD5 = CalculateMD5(file); | ||
| var blobMD5 = Convert.ToBase64String(properties.ContentHash); | ||
| return blobMD5.Equals(localMD5, StringComparison.OrdinalIgnoreCase); |
There was a problem hiding this comment.
Technically this isn't correct. Just because two files have the same MD5 has doesn't mean they are the same file.
But if the risk of false passes is acceptable, that's fine. Though a comment explaining why a false positive here is considered "ok" is a good idea.
There was a problem hiding this comment.
I'm not 100% sure why it was considered okay, but I assume we didn't want publishing to take hours comparing every file byte-for-byte. This is mostly copied from the place it was originally. @mmitche do we care about these collisions?
There was a problem hiding this comment.
Right, that was the intention. Avoid very costly download and comparisons. This was based on the content hash being available. That said, I am not sure whether it's ever actually been reliably available. I'm okay with removing this code, or perhaps sticking a bit of telemetry to determine whether we actually hit this code path
| } | ||
|
|
||
| int bytesPerMegabyte = 1 * 1024 * 1024; | ||
| if (properties.ContentLength < bytesPerMegabyte) |
There was a problem hiding this comment.
Do we need this special case? The other code basically degenerates into exactly this in this case. Seems like it's better to only have 1 thing to maintain/test. :-)
There was a problem hiding this comment.
This is just another thing I moved and fixed up a bit. I can remove this case.
| }; | ||
| } | ||
|
|
||
| private string FixUpSasTokenForMSBuildEscaping(string uriWithSas) |
There was a problem hiding this comment.
This somewhat concerning... why are we accepting incorrect inputs? Why aren't people escaping things correctly?
This also hard codes a lot of assumptions about SAS's. (like that "sig" always comes before "se"), which seems really fragile.
There was a problem hiding this comment.
This is fixing up msbuild property escaping issues. If you assign a value containing url encoded things to a property, msbuild will un-encode them before passing them to a task. This is fine for things that aren't already urls, but this value is a url with %2B and %2F all over the query string.
Would re-escaping everything after the ? be less fragile?
There was a problem hiding this comment.
ITaskItem2 does proper escaping, I believe (https://docs.microsoft.com/en-us/dotnet/api/microsoft.build.framework.itaskitem2?view=msbuild-16-netcore). Could we somehow utilize that?
There was a problem hiding this comment.
As noted above, we should use base64 SAS for this, like we do for the read SAS's that we use to access internally built runtimes.
| { | ||
| return null; | ||
| } | ||
| uriWithSas = TransformSection(uriWithSas, "sig=", "&se=", section => section.Replace("+", "%2B").Replace("/", "%2F").Replace("=", "%3D")); |
There was a problem hiding this comment.
Can't we just "end" at "&", aka "fixup this parameter""?
There was a problem hiding this comment.
Is there a reason not to "fix" all the parameters?
There was a problem hiding this comment.
Probably not, I will do that.
There was a problem hiding this comment.
I have to parse the query string to do this. I was trying to avoid that.
There was a problem hiding this comment.
ugh.... HttpUtility.ParseQueryString does partial re-encoding, so this is way more work. The real solution is to not have this publishing stuff be an msbuild task, it gains nothing from msbuild other than logging.
|
I am oof tomorrow but back Monday. Is really like a chance to test this in the staging pipeline before it goes in, so please give me a chance to review before it goes in! |
|
I should be able to take a look at this on Monday |
|
I am having a hard time parsing out some of the logic without like.... stepping through it, so I will just ask: In the staging pipeline, we use the feed overrides so that we can publish to dotnetstage (like you are publishing to dotnetbuilds in this change). We want to only publish to dotnetstage/the feed overrides in the staging pipeline. How does your change manage that? Is there a way to tell it "if AllowFeedOverrides is true, do NOT upload to dotnetbuilds"? If there isn't, there needs to be, or our logic will begin failing, trying to publish newly signed blobs that have already been published as unsigned to dotnetbuilds, which would be a conflict. Oh, I think I'm seeing how you're doing this now: you only add in the dotnetbuilds publishing if the feed matches the "default" feed. So if someone has passed in an override, it will not match the PublishingConstants feed uri, and it won't add dotnetbuilds. Is that correct? |
|
Yes, that is correct. I add the dotnetbuilds things when we are publishing to the standard place. |
| { | ||
| case FeedType.AzDoNugetFeed: | ||
| return new AzureDevOpsNugetFeedAssetPublisher(_log, feedConfig.TargetURL, feedConfig.Token, task); | ||
| case FeedType.AzureStorageFeed: |
There was a problem hiding this comment.
I believe this can be removed (maybe just stick an NYI in the case). We should not be publishing using Sleet any longer.
There was a problem hiding this comment.
FeedType.AzureStorageFeed == publishing to dotnetcli, I don't think we can remove that. Unless the code does very different things than from what I interpreted.
There was a problem hiding this comment.
Hmm..I thought that was "AzureStorageContainer".
There was a problem hiding this comment.
AzureStorageContainer is the new one I made, that uses a SAS token to publish, AzureStorageFeed is the old thing that looks and smells like sleet, but just strips the index.json off the end and publishes blobs like normal. It uses the account key.
There was a problem hiding this comment.
Okay. Can you do a quick refactor on the AzureStorageFeed enum name to be more indicative of its purpose?
|
|
||
| _httpClient = new HttpClient(new HttpClientHandler {CheckCertificateRevocationList = true}) | ||
| { | ||
| Timeout = TimeSpan.FromSeconds(300), |
There was a problem hiding this comment.
Can you hoist this constant out (and other similar constants). We've had to update these a few times in the past and its never easy to actually find which ones to change.
| return; | ||
| } | ||
|
|
||
| using var packageReader = new PackageArchiveReader(file); |
There was a problem hiding this comment.
You should wrap this {} and scope appropriately. I seem to remember changing this specifically a while back for either performance or correctness reasons. IIRC, PackageArchiveReader took an exclusive lock on the file, and depending on disposal time (since in this configuration the scope goes to the end of the method), we got occasional lock issues.
Anyways, scoping down to the minimal required scope is also good practice.
| var blobClient = CreateBlobClient(blobPath); | ||
| if (!options.AllowOverwrite && await blobClient.ExistsAsync()) | ||
| { | ||
| if (options.PassIfExistingItemIdentical) |
There was a problem hiding this comment.
I think this option can be just removed at this point.
There was a problem hiding this comment.
Its set to true in all cases I can see, so I don't see how we can remove it.
There was a problem hiding this comment.
I mean remove the conditional altogether. I don't see us ever passing false for this.
| DotNetBuildsPublicUri, | ||
| FeedType.AzureStorageContainer, | ||
| null, | ||
| latestLinkShortUrlPrefix: LatestLinkShortUrlPrefix, |
There was a problem hiding this comment.
I think this will lead to the aka.ms link pointing to either dotnetcli or dotnetbuilds, depending on what finishes first.
| /// </summary> | ||
| public class TargetFeedConfig | ||
| { | ||
| public string SafeTargetURL => new UriBuilder(TargetURL) {Query = "", Fragment = ""}.Uri.AbsoluteUri; |
There was a problem hiding this comment.
Can you add a comment as to what "SafeTargetUrl" means and why it's configured this way? What problem does this solve?
There was a problem hiding this comment.
It strips the secret part of the url so we can log it, I will add a comment.
| UseStreamingPublishing="$(UseStreamingPublishing)" | ||
| StreamingPublishingMaxClients="$(StreamingPublishingMaxClients)" | ||
| NonStreamingPublishingMaxClients="$(NonStreamingPublishingMaxClients)"/> | ||
| NonStreamingPublishingMaxClients="$(NonStreamingPublishingMaxClients)" |
There was a problem hiding this comment.
When the feed overrides are present, we should write a test that ensures that an asset is only pushed to a target endpoint once. In the new configuration here, there are multiple target feed configs for an installer blob, for instance. That might cause a bad interaction when the target is overridden to be a specific feed.
Yes, it looks like it. It has a tweak I added though to skip the dotnetcli publishing part so I didn't mess up normal stuff with my changes. |
If you do it with arcade (promote arcade with your changes), you can publish to general testing, and not mess anything up. I'd honestly like to see what happens when it publishes to both. |
| public const string SnupkgPackageSuffix = ".snupkg"; | ||
| public const string PackageSuffix = ".nupkg"; | ||
| public const string PackagesCategory = "PACKAGE"; | ||
| public static TimeSpan NugetFeedPublisherHttpClientTimeout => TimeSpan.FromSeconds(300); |
There was a problem hiding this comment.
| public static TimeSpan NugetFeedPublisherHttpClientTimeout => TimeSpan.FromSeconds(300); | |
| public readonly static TimeSpan NugetFeedPublisherHttpClientTimeout = TimeSpan.FromSeconds(300); |
|
This build has these changes running in a full flow https://dev.azure.com/dnceng/internal/_build/results?buildId=1360392&view=logs&j=ba23343f-f710-5af9-782d-5bd26b102304&t=6e277ba4-1c1e-552d-b96f-db0aeb4be20a |
|
it only actually published one thing to dotnetbuilds because arcade doesn't produce any non-packages, but it did publish the manifest there. |
| AkaMSTenant = this.AkaMSTenant, | ||
| BuildQuality = this.BuildQuality, | ||
| }; | ||
| Log.LogError("V2 Publishing is no longer supported."); |
| { | ||
| (TargetFeedContentType.Package, FeedDotNet31InternalShipping, AssetSelection.ShippingOnly), | ||
| (TargetFeedContentType.Package, FeedDotNet31InternalTransport, AssetSelection.NonShippingOnly), | ||
| (InstallersAndSymbols, FeedInternalForInstallers), |
There was a problem hiding this comment.
We might need to tweak darc for publishing the internal symbols to the storage account. Issue would be something like it not appending a SAS token when trying to access them.
The thing to test is probably to assign a random, previously unpromoted build of arcade to the "General Testing Internal" channel, then use darc gather-drop on it.
There was a problem hiding this comment.
I have one ready, i'll run that.
| ImmutableList<string> filesToExclude = null, | ||
| bool flatten = true) | ||
| : base(isInternalBuild, isStableBuild, repositoryName, commitSha, azureStorageTargetFeedPAT, publishInstallersAndChecksums, installersTargetStaticFeed, installersAzureAccountKey, checksumsTargetStaticFeed, checksumsAzureAccountKey, azureDevOpsStaticShippingFeed, azureDevOpsStaticTransportFeed, azureDevOpsStaticSymbolsFeed, latestLinkShortUrlPrefix, azureDevOpsFeedsKey) | ||
| : base(isInternalBuild, isStableBuild, repositoryName, commitSha, null, publishInstallersAndChecksums, null, null, null, null, null, null, null, latestLinkShortUrlPrefix, null) |
There was a problem hiding this comment.
Can you refactor this to use named arguments? The number of arguments and number of nulls makes a silly mistake easy. Note that there is an quick-refactor for this in VS.
| private static string ConvertFromBase64(string value) | ||
| { | ||
| if (string.IsNullOrEmpty(InstallersAzureAccountKey)) | ||
| if (value == null) |
There was a problem hiding this comment.
Why the use of == null vs. IsNullOrEmpty?
There was a problem hiding this comment.
(and if it's some subtle difference in behavior, it's worth a comment)
There was a problem hiding this comment.
No real reason, I will change it.
| flatten: Flatten)); | ||
| foreach (var type in spec.ContentTypes) | ||
| { | ||
| if (!PublishInstallersAndChecksums) |
There was a problem hiding this comment.
PublishInstallersAndChecksums should be assumed to be always true now
| : (sasUri != null ? FeedType.AzureStorageContainer : FeedType.AzureStorageFeed); | ||
| yield return new TargetFeedConfig( | ||
| type, | ||
| sasUri ?? feed, |
There was a problem hiding this comment.
I think this might be wrong? Second parameter is the target url, which makes sense for feed but not for sasUri, which I assume is just the suffix.
There was a problem hiding this comment.
sasUri is the entire uri with sas token.
| String.Equals(InstallersFeed, config.InstallersFeed, StringComparison.OrdinalIgnoreCase) && | ||
| string.Equals(AkaMSChannelName, config.AkaMSChannelName, StringComparison.OrdinalIgnoreCase) && | ||
| TargetFeeds.Count == config.TargetFeeds.Count && | ||
| TargetFeeds.Zip(config.TargetFeeds, (l, r) => l.Equals(r)).All(b => b) && |
There was a problem hiding this comment.
That's cool, I didn't know about this method.
There was a problem hiding this comment.
its even easier if this didn't have to work on net472, There is a nicer overload of Zip on newer frameworks.
mmitche
left a comment
There was a problem hiding this comment.
Approved with a few nits. Love the refactor. Before checking in, I would verify the following:
- That checking in won't immediately break publishing - This shouldn't happen because there aren't changes that are outside of the arcade SDK and associated packages (like templates), but I would take this branch and attempt to run a simple promotion against it. Publishing behavior should not change.
- Do tests (if you haven't already) against a branch of arcade that has its arcade updated to an official build of this branch.
@epananth Is the expert on testing publishing and can also help out if you have questions.
|
My latest test publishing run is here https://dev.azure.com/dnceng/internal/_build/results?buildId=1385970&view=logs&j=ba23343f-f710-5af9-782d-5bd26b102304&t=6e277ba4-1c1e-552d-b96f-db0aeb4be20a Which tests are you refering to? The xunit ones? |
That kind of test. |
|
Those tests have all been run successfully. |
michellemcdaniel
left a comment
There was a problem hiding this comment.
This passed in the test staging pipeline, so I'm happy
|
It looks like this breaks Arcade-validation's official build when it's trying to validate that using the newer Arcade SDK would not break publishing. https://dev.azure.com/dnceng/internal/_build/results?buildId=1402340&view=results which tried to run this promotion pipeline run: https://dev.azure.com/dnceng/internal/_build/results?buildId=1403439&view=results is failing with a 403. Is there something that the repo needs to pipe through? |
|
I also saw this error yesterday when I was trying to promote runtime build -> https://dev.azure.com/dnceng/internal/_build/results?buildId=1402675&view=results |
dotnet/core-eng#14095
To double check: