-
Notifications
You must be signed in to change notification settings - Fork 1.8k
C#: Automatically use configured private registry feeds #18850
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
Changes from 10 commits
6b2f348
63d5517
11efb55
726123c
0db6a26
6b15f77
a8dde15
9560593
b6c74fe
284f612
51874b8
7a92a72
d564529
92eab47
4448369
7cea2ad
d2b88ae
4d3b024
73ca2eb
be95d33
fe1c098
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,30 @@ | ||
using System; | ||
using System.Diagnostics; | ||
using System.Collections.Generic; | ||
using System.IO; | ||
using System.Security.Cryptography.X509Certificates; | ||
using Semmle.Util; | ||
using Semmle.Util.Logging; | ||
using Newtonsoft.Json; | ||
|
||
namespace Semmle.Extraction.CSharp.DependencyFetching | ||
{ | ||
public class DependabotProxy : IDisposable | ||
{ | ||
/// <summary> | ||
/// Represents configurations for package registries. | ||
/// </summary> | ||
public struct RegistryConfig | ||
{ | ||
/// <summary> | ||
/// The type of package registry. | ||
/// </summary> | ||
public string Type { get; set; } | ||
/// <summary> | ||
/// The URL of the package registry. | ||
/// </summary> | ||
public string URL { get; set; } | ||
} | ||
|
||
private readonly string host; | ||
private readonly string port; | ||
|
||
|
@@ -17,6 +33,10 @@ public class DependabotProxy : IDisposable | |
/// </summary> | ||
internal string Address { get; } | ||
/// <summary> | ||
/// The URLs of package registries that are configured for the proxy. | ||
/// </summary> | ||
internal HashSet<string> RegistryURLs { get; } | ||
/// <summary> | ||
/// The path to the temporary file where the certificate is stored. | ||
/// </summary> | ||
internal string? CertificatePath { get; private set; } | ||
|
@@ -67,6 +87,39 @@ public class DependabotProxy : IDisposable | |
result.Certificate = X509Certificate2.CreateFromPem(cert); | ||
} | ||
|
||
// Try to obtain the list of private registry URLs. | ||
var registryURLs = Environment.GetEnvironmentVariable(EnvironmentVariableNames.ProxyURLs); | ||
|
||
if (!string.IsNullOrWhiteSpace(registryURLs)) | ||
{ | ||
try | ||
{ | ||
// The value of the environment variable should be a JSON array of objects, such as: | ||
// [ { "type": "nuget_feed", "url": "https://nuget.pkg.github.com/org/index.json" } ] | ||
var array = JsonConvert.DeserializeObject<List<RegistryConfig>>(registryURLs); | ||
if (array != null) | ||
mbg marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we expect the array to be null? Should we log if it is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The main reason for the check is just to make sure we don't work with a |
||
{ | ||
foreach (RegistryConfig config in array) | ||
{ | ||
// The array contains all configured private registries, not just ones for C#. | ||
// We ignore the non-C# ones here. | ||
if (!config.Type.Equals("nuget_feed")) | ||
{ | ||
logger.LogDebug($"Ignoring registry at '{config.URL}' since it is not of type 'nuget_feed'."); | ||
continue; | ||
} | ||
|
||
logger.LogInfo($"Found private registry at '{config.URL}'"); | ||
result.RegistryURLs.Add(config.URL); | ||
} | ||
} | ||
} | ||
catch (JsonException ex) | ||
{ | ||
logger.LogError($"Unable to parse '{EnvironmentVariableNames.ProxyURLs}': {ex.Message}"); | ||
} | ||
} | ||
|
||
return result; | ||
} | ||
|
||
|
@@ -75,6 +128,7 @@ private DependabotProxy(string host, string port) | |
this.host = host; | ||
this.port = port; | ||
this.Address = $"http://{this.host}:{this.port}"; | ||
this.RegistryURLs = new HashSet<string>(); | ||
} | ||
|
||
public void Dispose() | ||
|
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -156,7 +156,7 @@ public HashSet<AssemblyLookupLocation> Restore() | |||
|
||||
var restoredProjects = RestoreSolutions(out var container); | ||||
var projects = fileProvider.Projects.Except(restoredProjects); | ||||
RestoreProjects(projects, out var containers); | ||||
RestoreProjects(projects, explicitFeeds, out var containers); | ||||
|
||||
var dependencies = containers.Flatten(container); | ||||
|
||||
|
@@ -260,8 +260,24 @@ private IEnumerable<string> RestoreSolutions(out DependencyContainer dependencie | |||
/// Populates dependencies with the relative paths to the assets files generated by the restore. | ||||
/// </summary> | ||||
/// <param name="projects">A list of paths to project files.</param> | ||||
private void RestoreProjects(IEnumerable<string> projects, out ConcurrentBag<DependencyContainer> dependencies) | ||||
private void RestoreProjects(IEnumerable<string> projects, HashSet<string>? configuredSources, out ConcurrentBag<DependencyContainer> dependencies) | ||||
{ | ||||
// Conservatively, we only set this to a non-null value if a Dependabot proxy is enabled. | ||||
// This ensures that we continue to get the old behaviour where feeds are taken from | ||||
// `nuget.config` files instead of the command-line arguments. | ||||
HashSet<string>? sources = null; | ||||
|
||||
if (this.dependabotProxy != null) | ||||
mbg marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
{ | ||||
// If the Dependabot proxy is configured, then our main goal is to make `dotnet` aware | ||||
// of the private registry feeds. However, since providing them as command-line arguments | ||||
// to `dotnet` ignores other feeds that may be configured, we also need to add the feeds | ||||
// we have discovered from analysing `nuget.config` files. | ||||
sources = configuredSources ?? new(); | ||||
sources.Add(PublicNugetOrgFeed); | ||||
this.dependabotProxy?.RegistryURLs.ForEach(url => sources.Add(url)); | ||||
mbg marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider constructing the "source" string at this location instead. Otherwise it will be constructed once for each project that is restored in the parallel restore loop. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in d564529 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To avoid timeouts, maybe there should be a feed validation check similar to: codeql/csharp/extractor/Semmle.Extraction.CSharp.DependencyFetching/NugetPackageRestorer.cs Line 692 in 72346cc
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in 4448369 |
||||
} | ||||
|
||||
var successCount = 0; | ||||
var nugetSourceFailures = 0; | ||||
ConcurrentBag<DependencyContainer> collectedDependencies = []; | ||||
|
@@ -276,7 +292,7 @@ private void RestoreProjects(IEnumerable<string> projects, out ConcurrentBag<Dep | |||
foreach (var project in projectGroup) | ||||
{ | ||||
logger.LogInfo($"Restoring project {project}..."); | ||||
var res = dotnet.Restore(new(project, PackageDirectory.DirInfo.FullName, ForceDotnetRefAssemblyFetching: true, TargetWindows: isWindows)); | ||||
var res = dotnet.Restore(new(project, PackageDirectory.DirInfo.FullName, ForceDotnetRefAssemblyFetching: true, sources?.ToList(), TargetWindows: isWindows)); | ||||
assets.AddDependenciesRange(res.AssetsFilePaths); | ||||
lock (sync) | ||||
{ | ||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any particular reason/benefit to declare this as a
struct
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No particular reason. I thought I was mirroring a similar setup elsewhere in the codebase, but happy to change it.