Skip to content

Commit 2f0d13c

Browse files
jpinzjpinz
committed
Address PR comments
1 parent 035bdc9 commit 2f0d13c

3 files changed

Lines changed: 93 additions & 29 deletions

File tree

‎docs/detectors/dockercompose.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ services:
3838

3939
### Variable Resolution
4040

41-
Images containing unresolved variables (e.g., `${TAG}` or `{{ .Values.tag }}`) are skipped to avoid reporting incomplete or incorrect references. The detector checks for `$`, `{`, or `}` characters in image references.
41+
Images containing unresolved variables (e.g., `${TAG}` or `${REGISTRY:-docker.io}`) are skipped to avoid reporting incomplete or incorrect references. The detector checks for `$`, `{`, or `}` characters in image references.
4242

4343
## Known limitations
4444

‎src/Microsoft.ComponentDetection.Detectors/dockerfile/DockerfileComponentDetector.cs‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
#nullable disable
1+
#nullable enable
22
namespace Microsoft.ComponentDetection.Detectors.Dockerfile;
33

44
using System;
@@ -35,7 +35,7 @@ public DockerfileComponentDetector(
3535

3636
public override string Id { get; } = "DockerReference";
3737

38-
public override IEnumerable<string> Categories => [Enum.GetName(typeof(DetectorClass), DetectorClass.DockerReference)];
38+
public override IEnumerable<string> Categories => [nameof(DetectorClass.DockerReference)];
3939

4040
public override IList<string> SearchPatterns { get; } = ["dockerfile", "dockerfile.*", "*.dockerfile"];
4141

@@ -83,12 +83,12 @@ private Task ParseDockerFileAsync(string fileContents, string fileLocation, ISin
8383
return Task.CompletedTask;
8484
}
8585

86-
private DockerReference ProcessDockerfileConstruct(DockerfileConstruct construct, char escapeChar, Dictionary<string, string> stageNameMap)
86+
private DockerReference? ProcessDockerfileConstruct(DockerfileConstruct construct, char escapeChar, Dictionary<string, string> stageNameMap)
8787
{
8888
try
8989
{
9090
var instructionKeyword = construct.Type;
91-
DockerReference baseImage = null;
91+
DockerReference? baseImage = null;
9292
if (instructionKeyword == ConstructType.Instruction)
9393
{
9494
var constructType = construct.GetType().Name;
@@ -114,10 +114,10 @@ private DockerReference ProcessDockerfileConstruct(DockerfileConstruct construct
114114
}
115115
}
116116

117-
private DockerReference ParseFromInstruction(DockerfileConstruct construct, char escapeChar, Dictionary<string, string> stageNameMap)
117+
private DockerReference? ParseFromInstruction(DockerfileConstruct construct, char escapeChar, Dictionary<string, string> stageNameMap)
118118
{
119119
var tokens = construct.Tokens.ToArray();
120-
var resolvedFromStatement = construct.ResolveVariables(escapeChar).TrimEnd();
120+
var resolvedFromStatement = construct.ResolveVariables(escapeChar)?.TrimEnd();
121121
var fromInstruction = (FromInstruction)construct;
122122
var reference = fromInstruction.ImageName;
123123
if (string.IsNullOrWhiteSpace(resolvedFromStatement) || string.IsNullOrEmpty(reference))
@@ -148,9 +148,9 @@ private DockerReference ParseFromInstruction(DockerfileConstruct construct, char
148148
return DockerReferenceUtility.TryParseImageReference(reference);
149149
}
150150

151-
private DockerReference ParseCopyInstruction(DockerfileConstruct construct, char escapeChar, Dictionary<string, string> stageNameMap)
151+
private DockerReference? ParseCopyInstruction(DockerfileConstruct construct, char escapeChar, Dictionary<string, string> stageNameMap)
152152
{
153-
var resolvedCopyStatement = construct.ResolveVariables(escapeChar).TrimEnd();
153+
var resolvedCopyStatement = construct.ResolveVariables(escapeChar)?.TrimEnd();
154154
var copyInstruction = (CopyInstruction)construct;
155155
var reference = copyInstruction.FromStageName;
156156
if (string.IsNullOrWhiteSpace(resolvedCopyStatement) || string.IsNullOrWhiteSpace(reference))

‎src/Microsoft.ComponentDetection.Detectors/helm/HelmComponentDetector.cs‎

Lines changed: 84 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,7 @@ namespace Microsoft.ComponentDetection.Detectors.Helm;
55
using System.Collections.Concurrent;
66
using System.Collections.Generic;
77
using System.IO;
8-
using System.Linq;
98
using System.Reactive.Linq;
10-
using System.Reactive.Threading.Tasks;
119
using System.Threading;
1210
using System.Threading.Tasks;
1311
using Microsoft.ComponentDetection.Common;
@@ -51,31 +49,97 @@ public override async Task<IndividualDetectorScanResult> ExecuteDetectorAsync(Sc
5149
return await base.ExecuteDetectorAsync(request, cancellationToken);
5250
}
5351

54-
protected override async Task<IObservable<ProcessRequest>> OnPrepareDetectionAsync(
52+
/// <summary>
53+
/// Pre-filters scan work to values files that are co-located with a Helm chart.
54+
///
55+
/// This is intentionally implemented as a streaming pipeline (instead of
56+
/// materializing all matching files with ToList) to reduce peak memory usage and
57+
/// start emitting work earlier on large repositories.
58+
///
59+
/// Enumeration order is not guaranteed, so values files may be seen before the
60+
/// corresponding Chart.yaml. To preserve correctness, values files are buffered
61+
/// per directory until a chart file for that directory is observed, then released.
62+
/// </summary>
63+
/// <returns>
64+
/// A prepared observable that emits only values-file requests belonging to
65+
/// directories that contain a Chart.yaml/Chart.yml.
66+
/// </returns>
67+
protected override Task<IObservable<ProcessRequest>> OnPrepareDetectionAsync(
5568
IObservable<ProcessRequest> processRequests,
5669
IDictionary<string, string> detectorArgs,
5770
CancellationToken cancellationToken = default)
5871
{
59-
// Materialize all matching files first so that chart directories are fully
60-
// known before any values file is decided on, regardless of enumeration order.
61-
var allRequests = await processRequests.ToList().ToTask(cancellationToken);
62-
63-
// Pass 1: record every directory that contains a Chart.yaml / Chart.yml.
64-
foreach (var request in allRequests)
72+
return Task.FromResult(Observable.Create<ProcessRequest>(observer =>
6573
{
66-
if (IsChartFile(Path.GetFileName(request.ComponentStream.Location)))
74+
// Buffer only values files whose directory has not yet produced a Chart file.
75+
var pendingValuesByDirectory = new Dictionary<string, List<ProcessRequest>>(StringComparer.OrdinalIgnoreCase);
76+
var gate = new object();
77+
78+
var subscription = processRequests.Subscribe(
79+
request =>
80+
{
81+
var fileName = Path.GetFileName(request.ComponentStream.Location);
82+
var directory = Path.GetDirectoryName(request.ComponentStream.Location) ?? string.Empty;
83+
84+
// Protect shared state because IObservable callbacks may arrive concurrently.
85+
lock (gate)
86+
{
87+
if (IsChartFile(fileName))
88+
{
89+
// Mark this directory as a Helm chart directory.
90+
this.helmChartDirectories.TryAdd(directory, true);
91+
92+
// Release any values files that arrived earlier for this directory.
93+
if (pendingValuesByDirectory.Remove(directory, out var pendingRequests))
94+
{
95+
foreach (var pendingRequest in pendingRequests)
96+
{
97+
observer.OnNext(pendingRequest);
98+
}
99+
}
100+
101+
return;
102+
}
103+
104+
if (!IsValuesFile(fileName))
105+
{
106+
// Ignore non-values files (Chart files are handled above).
107+
return;
108+
}
109+
110+
if (this.helmChartDirectories.ContainsKey(directory))
111+
{
112+
// Directory is already known to be a chart; emit immediately.
113+
observer.OnNext(request);
114+
return;
115+
}
116+
117+
// Chart file not seen yet for this directory; queue for later release.
118+
if (!pendingValuesByDirectory.TryGetValue(directory, out var pendingRequestsForDirectory))
119+
{
120+
pendingRequestsForDirectory = [];
121+
pendingValuesByDirectory[directory] = pendingRequestsForDirectory;
122+
}
123+
124+
pendingRequestsForDirectory.Add(request);
125+
}
126+
},
127+
observer.OnError,
128+
observer.OnCompleted);
129+
130+
var cancellationRegistration = cancellationToken.Register(() =>
67131
{
68-
this.helmChartDirectories.TryAdd(
69-
Path.GetDirectoryName(request.ComponentStream.Location)!, true);
70-
}
71-
}
132+
// Stop forwarding events if detection is cancelled.
133+
subscription.Dispose();
134+
observer.OnCompleted();
135+
});
72136

73-
// Pass 2: emit only the values files that sit in a known chart directory.
74-
return allRequests
75-
.Where(r =>
76-
IsValuesFile(Path.GetFileName(r.ComponentStream.Location)) &&
77-
this.helmChartDirectories.ContainsKey(Path.GetDirectoryName(r.ComponentStream.Location)!))
78-
.ToObservable();
137+
return () =>
138+
{
139+
cancellationRegistration.Dispose();
140+
subscription.Dispose();
141+
};
142+
}));
79143
}
80144

81145
protected override async Task OnFileFoundAsync(ProcessRequest processRequest, IDictionary<string, string> detectorArgs, CancellationToken cancellationToken = default)

0 commit comments

Comments
 (0)