Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[NuGet Symbol Server] Add Symbol push validation support #6276

Merged
merged 13 commits into from
Aug 9, 2018
16 changes: 16 additions & 0 deletions src/NuGetGallery.Core/Entities/IPackageEntity.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

namespace NuGetGallery
{
/// <summary>
/// The common entity type to be shared by <see cref="Package"/> and <see cref="SymbolPackage"/>
/// This allows us the generic type instantiation in dependency injection for the commonly required code.
/// </summary>
public interface IPackageEntity
{
string Id { get; }

string Version { get; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any shared code paths as part of this PR that would require NormalizedVersion?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not the code I am touching. I wouldn't refactor much for anything else right now.

}
}
4 changes: 3 additions & 1 deletion src/NuGetGallery.Core/Entities/Package.cs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ namespace NuGetGallery
{
[DisplayColumn("Title")]
public class Package
: IEntity
: IEntity, IPackageEntity
{

#pragma warning disable 618 // TODO: remove Package.Authors completely once production services definitely no longer need it
Expand Down Expand Up @@ -243,5 +243,7 @@ public Package()
public virtual Certificate Certificate { get; set; }

public virtual ICollection<SymbolPackage> SymbolPackages { get; set; }

public string Id => PackageRegistration.Id;
}
}
7 changes: 5 additions & 2 deletions src/NuGetGallery.Core/Entities/SymbolPackage.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,11 @@

using System;
using System.ComponentModel.DataAnnotations;
using System.ComponentModel.DataAnnotations.Schema;

namespace NuGetGallery
{
public class SymbolPackage
: IEntity, IEquatable<SymbolPackage>
: IEntity, IPackageEntity, IEquatable<SymbolPackage>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Outside the scope of this PR... but why does SymbolPackage implement IEquatable, and Package does not? Is there functionality only specific to symbols that requires this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{
public int Key { get; set; }

Expand Down Expand Up @@ -51,6 +50,10 @@ public class SymbolPackage
/// </summary>
public byte[] RowVersion { get; set; }

public string Id => Package?.Id;

public string Version => Package?.Version;

public bool Equals(SymbolPackage other)
{
return other.Key == Key;
Expand Down
5 changes: 3 additions & 2 deletions src/NuGetGallery.Core/NuGetGallery.Core.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@
<Compile Include="Entities\AccountDelete.cs" />
<Compile Include="Entities\MembershipRequest.cs" />
<Compile Include="Entities\OrganizationMigrationRequest.cs" />
<Compile Include="Entities\IPackageEntity.cs" />
<Compile Include="Entities\SymbolPackage.cs" />
<Compile Include="Entities\PackageDelete.cs" />
<Compile Include="Entities\EmailMessage.cs" />
Expand Down Expand Up @@ -254,10 +255,10 @@
<Version>4.8.0-preview4.5287</Version>
</PackageReference>
<PackageReference Include="NuGet.Services.Validation">
<Version>2.26.0-master-33196</Version>
<Version>2.28.0-master-37018</Version>
</PackageReference>
<PackageReference Include="NuGet.Services.Validation.Issues">
<Version>2.26.0-master-33196</Version>
<Version>2.28.0-master-37018</Version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a stable version to update to?

Copy link
Contributor Author

@shishirx34 shishirx34 Aug 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see one in manage nuget packages.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe our (undocumented) process for ServerCommon is to kick off a stable build when making changes there. I had similiar feedback in another PR, which is why I mention it.

</PackageReference>
<PackageReference Include="NuGet.Versioning">
<Version>4.8.0-preview4.5287</Version>
Expand Down
57 changes: 50 additions & 7 deletions src/NuGetGallery/App_Start/DefaultDependenciesModule.cs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,14 @@ namespace NuGetGallery
{
public class DefaultDependenciesModule : Module
{
public static class BindingKeys
{
public const string PackageValidationTopic = "PackageValidationBindingKey";
public const string SymbolsPackageValidationTopic = "SymbolsPackageValidationBindingKey";
public const string PackageValidationEnqueuer = "PackageValidationEnqueuerBindingKey";
public const string SymbolsPackageValidationEnqueuer = "SymbolsPackageValidationEnqueuerBindingKey";
}

[System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Maintainability", "CA1502:CyclomaticComplexity", Justification = "This code is more maintainable in the same function.")]
protected override void Load(ContainerBuilder builder)
{
Expand Down Expand Up @@ -468,36 +476,71 @@ private static DbConnection CreateDbConnection(ISqlConnectionFactory connectionF
.RegisterType<ServiceBusMessageSerializer>()
.As<IServiceBusMessageSerializer>();

// We need to setup two enqueuers for Package validation and symbol validation each publishes
// to a different topic for validation.
builder
.RegisterType<PackageValidationEnqueuer>()
.WithParameter(new ResolvedParameter(
(pi, ctx) => pi.ParameterType == typeof(ITopicClient),
(pi, ctx) => ctx.ResolveKeyed<ITopicClient>(BindingKeys.PackageValidationTopic)))
.Keyed<IPackageValidationEnqueuer>(BindingKeys.PackageValidationEnqueuer)
.As<IPackageValidationEnqueuer>();

builder
.RegisterType<PackageValidationEnqueuer>()
.WithParameter(new ResolvedParameter(
(pi, ctx) => pi.ParameterType == typeof(ITopicClient),
(pi, ctx) => ctx.ResolveKeyed<ITopicClient>(BindingKeys.SymbolsPackageValidationTopic)))
.Keyed<IPackageValidationEnqueuer>(BindingKeys.SymbolsPackageValidationEnqueuer)
.As<IPackageValidationEnqueuer>();

if (configuration.Current.AsynchronousPackageValidationEnabled)
{
ConfigureValidationEntitiesContext(builder, diagnostics, configuration, secretInjector);

builder
.RegisterType<AsynchronousPackageValidationInitiator>()
.As<IPackageValidationInitiator>();
.Register(c => {
return new AsynchronousPackageValidationInitiator<Package>(
c.ResolveKeyed<IPackageValidationEnqueuer>(BindingKeys.PackageValidationEnqueuer),
c.Resolve<IAppConfiguration>(),
c.Resolve<IDiagnosticsService>());
}).As<IPackageValidationInitiator<Package>>();

builder
.Register(c => {
return new AsynchronousPackageValidationInitiator<SymbolPackage>(
c.ResolveKeyed<IPackageValidationEnqueuer>(BindingKeys.SymbolsPackageValidationEnqueuer),
c.Resolve<IAppConfiguration>(),
c.Resolve<IDiagnosticsService>());
}).As<IPackageValidationInitiator<SymbolPackage>>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shishirx34 @joelverhagen

So was ResolveKeyed the solution to get DI registrations to work for the specific generic type args?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed instantiating the AsynchronousPackageValidationInitiator and ImmediateValidationInitiator to be Generic which was causing the problem. See the code below of RegisterGenerics which sets up the DI appropriately.


// we retrieve the values here (on main thread) because otherwise it would run in another thread
// and potentially cause a deadlock on async operation.
var validationConnectionString = configuration.ServiceBus.Validation_ConnectionString;
var validationTopicName = configuration.ServiceBus.Validation_TopicName;
var symbolsValidationConnectionString = configuration.ServiceBus.SymbolsValidation_ConnectionString;
var symbolsValidationTopicName = configuration.ServiceBus.SymbolsValidation_TopicName;

builder
.Register(c => new TopicClientWrapper(validationConnectionString, validationTopicName))
.As<ITopicClient>()
.SingleInstance()
.Keyed<ITopicClient>(BindingKeys.PackageValidationTopic)
.OnRelease(x => x.Close());

builder
.Register(c => new TopicClientWrapper(
validationConnectionString,
validationTopicName))
.Register(c => new TopicClientWrapper(symbolsValidationConnectionString, symbolsValidationTopicName))
.As<ITopicClient>()
.SingleInstance()
.Keyed<ITopicClient>(BindingKeys.SymbolsPackageValidationTopic)
.OnRelease(x => x.Close());
}
else
{
// This will register all the instances of ImmediatePackageValidator<T> as IPackageValidationInitiator<T> where T is a typeof(IPackageEntity)
builder
.RegisterType<ImmediatePackageValidator>()
.As<IPackageValidationInitiator>();
.RegisterGeneric(typeof(ImmediatePackageValidator<>))
.As(typeof(IPackageValidationInitiator<>));
}

builder.RegisterType<ValidationAdminService>()
Expand Down
14 changes: 14 additions & 0 deletions src/NuGetGallery/Configuration/IServiceBusConfiguration.cs
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,19 @@ public interface IServiceBusConfiguration
/// time as the <see cref="Validation_ConnectionString"/>.
/// </summary>
string Validation_TopicName { get; set; }

/// <summary>
/// The connection string to use when connecting to the symbols validation topic. This connection string should not
/// contain the topic name as the name is explicitly specified by <see cref="SymbolsValidation_TopicName"/>. This
/// connection string only needs to have the "Send" privilege. This topic is used for requesting asynchronous
/// validation of symbol packages.
/// </summary>
string SymbolsValidation_ConnectionString { get; set; }

/// <summary>
/// The name of the Azure Service Bus topic to send validation messages to. This topic name is used at the same
/// time as the <see cref="SymbolsValidation_ConnectionString"/>.
/// </summary>
string SymbolsValidation_TopicName { get; set; }
}
}
6 changes: 6 additions & 0 deletions src/NuGetGallery/Configuration/ServiceBusConfiguration.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,11 @@ public class ServiceBusConfiguration : IServiceBusConfiguration

[DisplayName("Validation.TopicName")]
public string Validation_TopicName { get; set; }

[DisplayName("SymbolsValidation.ConnectionString")]
public string SymbolsValidation_ConnectionString { get; set; }

[DisplayName("SymbolsValidation.TopicName")]
public string SymbolsValidation_TopicName { get; set; }
}
}
24 changes: 0 additions & 24 deletions src/NuGetGallery/NuGetGallery.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -2190,21 +2190,9 @@
<PackageReference Include="Newtonsoft.Json">
<Version>9.0.1</Version>
</PackageReference>
<PackageReference Include="NuGet.Common">
<Version>4.8.0-preview4.5287</Version>
</PackageReference>
<PackageReference Include="NuGet.Configuration">
<Version>4.8.0-preview4.5287</Version>
</PackageReference>
<PackageReference Include="NuGet.Frameworks">
<Version>4.8.0-preview4.5287</Version>
</PackageReference>
<PackageReference Include="NuGet.Packaging">
<Version>4.8.0-preview4.5287</Version>
</PackageReference>
<PackageReference Include="NuGet.Packaging.Core">
<Version>4.8.0-preview4.5287</Version>
</PackageReference>
<PackageReference Include="NuGet.Protocol">
<Version>4.8.0-preview4.5287</Version>
</PackageReference>
Expand All @@ -2220,21 +2208,9 @@
<PackageReference Include="NuGet.Services.Platform.Client">
<Version>3.0.29-r-master</Version>
</PackageReference>
<PackageReference Include="NuGet.Services.ServiceBus">
<Version>2.26.0-master-33196</Version>
</PackageReference>
<PackageReference Include="NuGet.Services.Sql">
<Version>2.27.0</Version>
</PackageReference>
<PackageReference Include="NuGet.Services.Validation">
<Version>2.26.0-master-33196</Version>
</PackageReference>
<PackageReference Include="NuGet.Services.Validation.Issues">
<Version>2.26.0-master-33196</Version>
</PackageReference>
<PackageReference Include="NuGet.Versioning">
<Version>4.8.0-preview4.5287</Version>
</PackageReference>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I assume these dependencies are pulled in from Gallery.Core?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

<PackageReference Include="Owin">
<Version>1.0.0</Version>
</PackageReference>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,10 @@ namespace NuGetGallery
/// Initiates asynchronous validation on a package by enqueuing a message containing the package identity and a new
/// <see cref="Guid"/>. The <see cref="Guid"/> represents a unique validation request.
/// </summary>
public class AsynchronousPackageValidationInitiator : IPackageValidationInitiator
public class AsynchronousPackageValidationInitiator<TPackageEntity> : IPackageValidationInitiator<TPackageEntity>
where TPackageEntity: IPackageEntity
{
private readonly IPackageValidationEnqueuer _enqueuer;
private readonly IPackageValidationEnqueuer _validationEnqueuer;
private readonly IAppConfiguration _appConfiguration;
private readonly IDiagnosticsSource _diagnosticsSource;

Expand All @@ -24,36 +25,51 @@ public class AsynchronousPackageValidationInitiator : IPackageValidationInitiato
IAppConfiguration appConfiguration,
IDiagnosticsService diagnosticsService)
{
_enqueuer = enqueuer ?? throw new ArgumentNullException(nameof(enqueuer));
_validationEnqueuer = enqueuer ?? throw new ArgumentNullException(nameof(enqueuer));
_appConfiguration = appConfiguration ?? throw new ArgumentNullException(nameof(appConfiguration));

if (diagnosticsService == null)
{
throw new ArgumentNullException(nameof(IDiagnosticsService));
}

_diagnosticsSource = diagnosticsService.SafeGetSource(nameof(AsynchronousPackageValidationInitiator));
_diagnosticsSource = diagnosticsService.SafeGetSource(nameof(AsynchronousPackageValidationInitiator<TPackageEntity>));
}

public async Task<PackageStatus> StartValidationAsync(Package package)
public async Task<PackageStatus> StartValidationAsync(TPackageEntity package)
{
if (_appConfiguration.ReadOnlyMode)
{
throw new ReadOnlyModeException(Strings.CannotEnqueueDueToReadOnly);
}

ValidatingType validatingType;
if (package is Package)
{
validatingType = ValidatingType.Package;
}
else if (package is SymbolPackage)
{
validatingType = ValidatingType.SymbolPackage;
}
else
{
throw new ArgumentException($"Unknown IPackageEntity type: {nameof(package)}");
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why add ValidatingType to Package and SymbolPackage, but not IPackageEntity? Seems like it complicates the code here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see @skofman1's comment. I think it doesn't make sense to put the ValidatingType on the entity.


var data = new PackageValidationMessageData(
package.PackageRegistration.Id,
package.Id,
package.Version,
Guid.NewGuid());
Guid.NewGuid(),
validatingType);

var activityName = $"Enqueuing asynchronous package validation: " +
$"{data.PackageId} {data.PackageVersion} ({data.ValidationTrackingId})";
$"{data.PackageId} {data.PackageVersion} {data.ValidatingType} ({data.ValidationTrackingId})";
using (_diagnosticsSource.Activity(activityName))
{
var postponeProcessingTill = DateTimeOffset.UtcNow + _appConfiguration.AsynchronousPackageValidationDelay;

await _enqueuer.StartValidationAsync(data, postponeProcessingTill);
await _validationEnqueuer.StartValidationAsync(data, postponeProcessingTill);
}

if (_appConfiguration.BlockingAsynchronousPackageValidationEnabled)
Expand Down