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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 13 commits into from
Aug 9, 2018
Merged

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

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
242351e
Add symbol validation service
shishirx34 Aug 3, 2018
5004758
Merge remote-tracking branch 'origin/dev' into symbol-push-validation
shishirx34 Aug 6, 2018
517d47a
Add validation interfaces
shishirx34 Aug 6, 2018
20f3d00
Update NuGet.Services.Validation with issues and new message data type
shishirx34 Aug 6, 2018
0eb9568
Add autofac di magic for symbol validation
shishirx34 Aug 7, 2018
ade5f9f
nit fix
shishirx34 Aug 7, 2018
60e5433
Add tests
shishirx34 Aug 7, 2018
f72c497
Refactor and DI changes
shishirx34 Aug 7, 2018
743e2ca
Address feedback
shishirx34 Aug 8, 2018
a908c03
Address feedback
shishirx34 Aug 8, 2018
e04a585
nit fix and refactor
shishirx34 Aug 8, 2018
e3bf803
remove validating type
shishirx34 Aug 8, 2018
ffb555c
move binding keys
shishirx34 Aug 8, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 16 additions & 0 deletions src/NuGetGallery.Core/Entities/IPackageEntity.cs
View file
Open in desktop
@@ -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.

using NuGet.Services.Validation;

namespace NuGetGallery
{
public interface IPackageEntity
Copy link
Member

Choose a reason for hiding this comment

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

Consider IPackageEntity : IEntity, to restrict usage of this interface to entity types.

Copy link
Member

Choose a reason for hiding this comment

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

Consider adding doc comments to specify the need for this type -- to reuse code between nupkg and snupkg packages.

{
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.


ValidatingType Type { get; }
Copy link
Member

Choose a reason for hiding this comment

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

Is ValidatingType an enum that should be added with this PR? Do you need it - why not just check instance type?

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 it is an enum. This completely removes the dependency of checking the type in initiator. Also, it is going to be specific to each IPackageEntity so it makes sense to put it in here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Consider renaming this to "PackageType". Validation type is not an attribute of the package, it's a behavior derived from the type of the package.

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 am reusing this from the Validations side for consistency, we use this to identify the package. While I agree with you, I do not think we should diverge for this enum. I'll alias it here I guess for ease.

}
}
7 changes: 6 additions & 1 deletion src/NuGetGallery.Core/Entities/Package.cs
View file
Open in desktop
Expand Up @@ -5,12 +5,13 @@
using System.Collections.Generic;
using System.ComponentModel.DataAnnotations;
using System.ComponentModel.DataAnnotations.Schema;
using NuGet.Services.Validation;

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 +244,9 @@ public Package()
public virtual Certificate Certificate { get; set; }

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

public string Id => PackageRegistration.Id;

public ValidatingType Type => ValidatingType.Package;
Copy link
Member

Choose a reason for hiding this comment

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

Type could be confused with GetType(). Consider renaming to ValidatingType.

Is this used in this PR? Would remove if not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removing this.

}
}
10 changes: 8 additions & 2 deletions src/NuGetGallery.Core/Entities/SymbolPackage.cs
View file
Open in desktop
Expand Up @@ -3,12 +3,12 @@

using System;
using System.ComponentModel.DataAnnotations;
using System.ComponentModel.DataAnnotations.Schema;
using NuGet.Services.Validation;

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.

See: https://github.com/NuGet/NuGetGallery/pull/6276/files#diff-c93f5b23258b3e89d85636deb43d0236R70

{
public int Key { get; set; }

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

public string Id => Package?.Id;

public string Version => Package?.Version;

public ValidatingType Type => ValidatingType.SymbolPackage;
Copy link
Member

Choose a reason for hiding this comment

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

See comment above (Package.cs).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will be deleted. missed it.


public bool Equals(SymbolPackage other)
{
return other.Key == Key;
Expand Down
5 changes: 3 additions & 2 deletions src/NuGetGallery.Core/NuGetGallery.Core.csproj
View file
Open in desktop
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 •
edited

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
View file
Open in desktop
Expand Up @@ -39,6 +39,14 @@

namespace NuGetGallery
{
public static class BindingKeys
Copy link
Member

Choose a reason for hiding this comment

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

PackageValidationBindingKeys? I think BindingKeys may be too general given this is nested in the DefaultDependenciesModule class.

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 think we should keep it as a general class for binding constants, going forward use it for any DI setup that requires binding. This isn't specific for PackageValidation.

Copy link
Member

Choose a reason for hiding this comment

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

Common coding standard I've seen is to keep 1:1 relation between classes and files, with names matching.

Given this, would consider moving BindingKeys into separate file or nesting under DefaultDependenciesModule class.

{
public const string PackageValidationTopic = "PackageValidationBindingKey";
public const string SymbolsPackageValidationTopic = "SymbolsPackageValidationBindingKey";
public const string PackageValidationEnqueuer = "PackageValidationEnqueuerBindingKey";
public const string SymbolsPackageValidationEnqueuer = "SymbolsPackageValidationEnqueuerBindingKey";
}

public class DefaultDependenciesModule : Module
{
[System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Maintainability", "CA1502:CyclomaticComplexity", Justification = "This code is more maintainable in the same function.")]
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
19 changes: 10 additions & 9 deletions src/NuGetGallery/Services/AsynchronousPackageValidationInitiator.cs
View file
Open in desktop
Expand Up @@ -13,9 +13,9 @@ 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<TPackage> : IPackageValidationInitiator<TPackage> where TPackage: IPackageEntity
Copy link
Member

Choose a reason for hiding this comment

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

Can we wrap this line at where?

{
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does it have to be generic?

Copy link
Member

Choose a reason for hiding this comment

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

My suggestion... see my comment here

My concern was that you could create a snupkg initiator with an (invalid) API to initiate nupkg validation.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not as familiar w/ symbols work though, so feel free to suggest something different. Just thinking this could help with code reuse.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@agr - code reusability for pushing to Package validation topic and symbol validation topic. The DI initializes these generic types to be used in Validation service.

Copy link
Contributor

Choose a reason for hiding this comment

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

TPackageEntity is not used for anything in this class. IPackageValidationInitiator can be made non-generic with Task<PackageStatus> StartValidationAsync(IPackageEntity package) method instead and then the code would still work unless I am missing something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Talked and resolved this query offline.

Copy link
Contributor

Choose a reason for hiding this comment

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

Talked offline: this makes DI setup quite complicated, so disregard.

private readonly IPackageValidationEnqueuer _enqueuer;
private readonly IPackageValidationEnqueuer _validationEnqueuer;
private readonly IAppConfiguration _appConfiguration;
private readonly IDiagnosticsSource _diagnosticsSource;

Expand All @@ -24,36 +24,37 @@ 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<TPackage>));
}

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

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

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