Add component detectors for Docker Compose, Helm, and Kubernetes#1759
Open
Add component detectors for Docker Compose, Helm, and Kubernetes#1759
Conversation
|
👋 Hi! It looks like you modified some files in the
If none of the above scenarios apply, feel free to ignore this comment 🙂 |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds new YAML-based detectors to Component Detection so Docker image references can be discovered from Docker Compose, Helm values, and Kubernetes manifests, plus updates orchestration wiring and expands detector test coverage.
Changes:
- Introduce new
DockerCompose,Helm, andKubernetesdetectors that parse YAML and registerDockerReferenceComponentusages. - Register the new detectors in Orchestrator DI and extend
DetectorClasswith new categories. - Add MSTest coverage for the new detectors and add/expand tests for the existing Dockerfile detector.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/Microsoft.ComponentDetection.Detectors.Tests/KubernetesComponentDetectorTests.cs | Adds unit tests for K8s YAML image extraction and non-K8s YAML ignoring behavior. |
| test/Microsoft.ComponentDetection.Detectors.Tests/HelmComponentDetectorTests.cs | Adds unit tests for Helm values.yaml image patterns (scalar, structured, digest, sequences). |
| test/Microsoft.ComponentDetection.Detectors.Tests/DockerfileComponentDetectorTests.cs | Adds Dockerfile detector tests for FROM, multi-stage, digests, COPY --from, and file patterns. |
| test/Microsoft.ComponentDetection.Detectors.Tests/DockerComposeComponentDetectorTests.cs | Adds tests for Compose service images, overrides, digests, and build-only services. |
| src/Microsoft.ComponentDetection.Orchestrator/Extensions/ServiceCollectionExtensions.cs | Registers the three new detectors with DI so they can be executed by the orchestrator. |
| src/Microsoft.ComponentDetection.Detectors/kubernetes/KubernetesComponentDetector.cs | New detector implementation for parsing Kubernetes manifests and extracting container images. |
| src/Microsoft.ComponentDetection.Detectors/helm/HelmComponentDetector.cs | New detector implementation for parsing Helm values.yaml and extracting image references. |
| src/Microsoft.ComponentDetection.Detectors/dockercompose/DockerComposeComponentDetector.cs | New detector implementation for parsing Compose YAML and extracting service image: references. |
| src/Microsoft.ComponentDetection.Contracts/DetectorClass.cs | Adds new detector classes/categories for Docker Compose, Helm, and Kubernetes. |
Comments suppressed due to low confidence (5)
src/Microsoft.ComponentDetection.Detectors/kubernetes/KubernetesComponentDetector.cs:104
- This file is compiled with nullable reference types enabled, but
apiVersion/kindare declared as non-nullablestringand initialized tonull, which will fail the build with warnings-as-errors. Make thesestring?(and similarly update any other locals/returns that can be null).
private bool IsKubernetesManifest(YamlMappingNode rootMapping)
{
string apiVersion = null;
string kind = null;
foreach (var entry in rootMapping.Children)
{
var key = (entry.Key as YamlScalarNode)?.Value;
if (string.Equals(key, "apiVersion", StringComparison.OrdinalIgnoreCase))
{
apiVersion = (entry.Value as YamlScalarNode)?.Value;
}
else if (string.Equals(key, "kind", StringComparison.OrdinalIgnoreCase))
{
kind = (entry.Value as YamlScalarNode)?.Value;
}
src/Microsoft.ComponentDetection.Detectors/kubernetes/KubernetesComponentDetector.cs:217
GetMappingChildreturnsnullwhen the key isn't present, but the return type is non-nullableYamlMappingNode. With nullable enabled + warnings-as-errors this will fail compilation. Change the return type toYamlMappingNode?(and update call sites accordingly).
private static YamlMappingNode GetMappingChild(YamlMappingNode parent, string key)
{
foreach (var entry in parent.Children)
{
if (entry.Key is YamlScalarNode scalarKey && string.Equals(scalarKey.Value, key, StringComparison.OrdinalIgnoreCase))
{
return entry.Value as YamlMappingNode;
}
}
return null;
src/Microsoft.ComponentDetection.Detectors/kubernetes/KubernetesComponentDetector.cs:230
GetSequenceChildreturnsnullwhen the key isn't present, but the return type is non-nullableYamlSequenceNode. With nullable enabled + warnings-as-errors this will fail compilation. Change the return type toYamlSequenceNode?(and update call sites accordingly).
private static YamlSequenceNode GetSequenceChild(YamlMappingNode parent, string key)
{
foreach (var entry in parent.Children)
{
if (entry.Key is YamlScalarNode scalarKey && string.Equals(scalarKey.Value, key, StringComparison.OrdinalIgnoreCase))
{
return entry.Value as YamlSequenceNode;
}
}
return null;
src/Microsoft.ComponentDetection.Detectors/helm/HelmComponentDetector.cs:135
- This method initializes non-nullable
stringlocals (repository,tag,digest,registry) tonull. With nullable enabled + TreatWarningsAsErrors, this will fail the build. Usestring?for these locals (and keep the subsequent null/whitespace checks).
private void TryRegisterStructuredImageReference(YamlMappingNode imageMapping, ISingleFileComponentRecorder recorder, string fileLocation)
{
string repository = null;
string tag = null;
string digest = null;
string registry = null;
src/Microsoft.ComponentDetection.Detectors/dockercompose/DockerComposeComponentDetector.cs:137
GetMappingChildreturnsnullwhen the key isn't present, but the return type is non-nullableYamlMappingNode. With nullable enabled + warnings-as-errors this will fail compilation. Change the return type toYamlMappingNode?and update call sites accordingly.
private static YamlMappingNode GetMappingChild(YamlMappingNode parent, string key)
{
foreach (var entry in parent.Children)
{
if (entry.Key is YamlScalarNode scalarKey && string.Equals(scalarKey.Value, key, StringComparison.OrdinalIgnoreCase))
{
return entry.Value as YamlMappingNode;
}
}
return null;
|
|
||
| public override int Version => 1; | ||
|
|
||
| public override IEnumerable<string> Categories => [Enum.GetName(typeof(DetectorClass), DetectorClass.Kubernetes)]; |
There was a problem hiding this comment.
Enum.GetName(...) returns string? under nullable reference types. Returning it directly here can fail compilation (and can propagate null into Categories). Use the null-forgiving operator (!) or an alternate non-null approach (e.g., nameof(DetectorClass.Kubernetes)).
This issue also appears in the following locations of the same file:
- line 88
- line 206
- line 219
Suggested change
| public override IEnumerable<string> Categories => [Enum.GetName(typeof(DetectorClass), DetectorClass.Kubernetes)]; | |
| public override IEnumerable<string> Categories => [nameof(DetectorClass.Kubernetes)]; |
|
|
||
| public override int Version => 1; | ||
|
|
||
| public override IEnumerable<string> Categories => [Enum.GetName(typeof(DetectorClass), DetectorClass.Helm)]; |
There was a problem hiding this comment.
Enum.GetName(...) returns string? under nullable reference types. Returning it directly here can fail compilation and can propagate null into Categories. Use ! (as some other detectors do) or another guaranteed-non-null approach (e.g., nameof(DetectorClass.Helm)).
This issue also appears on line 129 of the same file.
Suggested change
| public override IEnumerable<string> Categories => [Enum.GetName(typeof(DetectorClass), DetectorClass.Helm)]; | |
| public override IEnumerable<string> Categories => [nameof(DetectorClass.Helm)]; |
Comment on lines
+36
to
+41
|
|
||
| public override IEnumerable<ComponentType> SupportedComponentTypes => [ComponentType.DockerReference]; | ||
|
|
||
| public override int Version => 1; | ||
|
|
||
| public override IEnumerable<string> Categories => [Enum.GetName(typeof(DetectorClass), DetectorClass.DockerCompose)]; |
There was a problem hiding this comment.
Enum.GetName(...) returns string? under nullable reference types. Returning it directly here can fail compilation and can propagate null into Categories. Use the null-forgiving operator (!) or an alternate non-null approach.
This issue also appears on line 126 of the same file.
src/Microsoft.ComponentDetection.Detectors/kubernetes/KubernetesComponentDetector.cs
Show resolved
Hide resolved
src/Microsoft.ComponentDetection.Detectors/kubernetes/KubernetesComponentDetector.cs
Show resolved
Hide resolved
Comment on lines
+50
to
+72
| this.Logger.LogInformation("Discovered Helm file: {Location}", file.Location); | ||
|
|
||
| string contents; | ||
| using (var reader = new StreamReader(file.Stream)) | ||
| { | ||
| contents = await reader.ReadToEndAsync(cancellationToken); | ||
| } | ||
|
|
||
| var yaml = new YamlStream(); | ||
| yaml.Load(new StringReader(contents)); | ||
|
|
||
| if (yaml.Documents.Count == 0) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| var fileName = Path.GetFileName(file.Location); | ||
|
|
||
| if (fileName.Contains("values", StringComparison.OrdinalIgnoreCase) && | ||
| (fileName.EndsWith(".yaml", StringComparison.OrdinalIgnoreCase) || fileName.EndsWith(".yml", StringComparison.OrdinalIgnoreCase))) | ||
| { | ||
| this.ExtractImageReferencesFromValues(yaml, singleFileComponentRecorder, file.Location); | ||
| } |
There was a problem hiding this comment.
This method parses YAML for every matched file (including Chart.yaml/Chart.yml) even though only *values*.(yml|yaml) files are actually processed for image extraction. Since the filename decision is based solely on file.Location, you can short-circuit before reading/parsing the stream for non-values files to avoid unnecessary work and noise in logs.
src/Microsoft.ComponentDetection.Detectors/dockercompose/DockerComposeComponentDetector.cs
Show resolved
Hide resolved
…d improve logging
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/Microsoft.ComponentDetection.Detectors/kubernetes/KubernetesComponentDetector.cs:86
- The exception handler logs "Skipping non-Kubernetes YAML file" for any exception, but exceptions here primarily represent YAML parse/processing failures (a file can be a Kubernetes manifest but malformed). Consider adjusting the message to reflect parse errors (or splitting try/catch so non-Kubernetes vs parse-failure are distinguished) to avoid misleading diagnostics.
catch (Exception e)
{
// Not all YAML files are Kubernetes manifests; silently skip parse errors
this.Logger.LogDebug(e, "Skipping non-Kubernetes YAML file: {Location}", file.Location);
}
src/Microsoft.ComponentDetection.Detectors/helm/HelmComponentDetector.cs
Show resolved
Hide resolved
…prove error logging. And address SA1204
Comment on lines
+125
to
+138
| private void TryRegisterImageReference(string imageReference, ISingleFileComponentRecorder recorder, string fileLocation) | ||
| { | ||
| try | ||
| { | ||
| var dockerRef = DockerReferenceUtility.ParseFamiliarName(imageReference); | ||
| if (dockerRef != null) | ||
| { | ||
| recorder.RegisterUsage(new DetectedComponent(dockerRef.ToTypedDockerReferenceComponent())); | ||
| } | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| this.Logger.LogWarning(e, "Failed to parse image reference '{ImageReference}' in {Location}", imageReference, fileLocation); | ||
| } |
There was a problem hiding this comment.
Docker Compose files commonly use variable interpolation in image (e.g., ${REGISTRY}/app:${TAG}). DockerReferenceUtility.ParseFamiliarName throws on these, which will generate warnings for otherwise valid compose files. Consider skipping registration when the image reference contains unresolved variables (similar to DockerfileComponentDetector.HasUnresolvedVariables) instead of logging a warning for each occurrence.
Comment on lines
+195
to
+208
| private void TryRegisterImageReference(string imageReference, ISingleFileComponentRecorder recorder, string fileLocation) | ||
| { | ||
| try | ||
| { | ||
| var dockerRef = DockerReferenceUtility.ParseFamiliarName(imageReference); | ||
| if (dockerRef != null) | ||
| { | ||
| recorder.RegisterUsage(new DetectedComponent(dockerRef.ToTypedDockerReferenceComponent())); | ||
| } | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| this.Logger.LogWarning(e, "Failed to parse image reference '{ImageReference}' in {Location}", imageReference, fileLocation); | ||
| } |
There was a problem hiding this comment.
Helm values often use templated/variable image strings (e.g., ${REGISTRY} or {{ .Values.image.tag }} via preprocessing). Passing these through DockerReferenceUtility.ParseFamiliarName will throw and can create noisy warning logs. Consider detecting unresolved variables/templates in imageReference and skipping registration (without logging) to match the Dockerfile detector’s behavior.
Comment on lines
+228
to
+241
| private void TryRegisterImageReference(string imageReference, ISingleFileComponentRecorder recorder, string fileLocation) | ||
| { | ||
| try | ||
| { | ||
| var dockerRef = DockerReferenceUtility.ParseFamiliarName(imageReference); | ||
| if (dockerRef != null) | ||
| { | ||
| recorder.RegisterUsage(new DetectedComponent(dockerRef.ToTypedDockerReferenceComponent())); | ||
| } | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| this.Logger.LogWarning(e, "Failed to parse image reference '{ImageReference}' in {Location}", imageReference, fileLocation); | ||
| } |
There was a problem hiding this comment.
Kubernetes manifests frequently use variable placeholders in image: (e.g., ${IMAGE}) via kustomize/CI substitution. DockerReferenceUtility.ParseFamiliarName throws on these, so this will log warnings for many real-world manifests. Consider skipping image references containing unresolved variables/templates (rather than logging) before calling ParseFamiliarName, consistent with DockerfileComponentDetector.HasUnresolvedVariables.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces new default-off detectors to scan Docker image references from Docker Compose, Helm values, and Kubernetes manifests, wiring them into the orchestrator and extending shared Docker reference handling.
Changes:
- Added new detectors:
DockerComposeComponentDetector,HelmComponentDetector, andKubernetesComponentDetectorto discover Docker image references in YAML-based configs. - Added a shared
DockerReferenceUtility.HasUnresolvedVariableshelper to skip image references containing unresolved placeholders. - Added unit tests for the new detectors and for the existing Dockerfile detector; registered new detectors in DI and extended
DetectorClass.
Show a summary per file
| File | Description |
|---|---|
| test/Microsoft.ComponentDetection.Detectors.Tests/KubernetesComponentDetectorTests.cs | Adds unit tests for Kubernetes image extraction scenarios. |
| test/Microsoft.ComponentDetection.Detectors.Tests/HelmComponentDetectorTests.cs | Adds unit tests for Helm values image discovery patterns. |
| test/Microsoft.ComponentDetection.Detectors.Tests/DockerfileComponentDetectorTests.cs | Adds unit tests for Dockerfile image reference detection (including placeholder skipping). |
| test/Microsoft.ComponentDetection.Detectors.Tests/DockerComposeComponentDetectorTests.cs | Adds unit tests for Docker Compose image extraction. |
| src/Microsoft.ComponentDetection.Orchestrator/Extensions/ServiceCollectionExtensions.cs | Registers the new detectors in orchestrator DI. |
| src/Microsoft.ComponentDetection.Detectors/kubernetes/KubernetesComponentDetector.cs | Implements Kubernetes YAML detector for container image references. |
| src/Microsoft.ComponentDetection.Detectors/helm/HelmComponentDetector.cs | Implements Helm values YAML detector for image references. |
| src/Microsoft.ComponentDetection.Detectors/dockercompose/DockerComposeComponentDetector.cs | Implements Docker Compose YAML detector for service images. |
| src/Microsoft.ComponentDetection.Contracts/DetectorClass.cs | Adds DockerCompose, Helm, and Kubernetes detector classes. |
| src/Microsoft.ComponentDetection.Common/DockerReference/DockerReferenceUtility.cs | Adds shared unresolved-variable detection helper for Docker references. |
| .gitignore | Ignores .nuget/ directory. |
Copilot's findings
Comments suppressed due to low confidence (2)
src/Microsoft.ComponentDetection.Common/DockerReference/DockerReferenceUtility.cs:48
HasUnresolvedVariablesis a public utility but will throw a NullReferenceException ifreferenceis null. Consider returning false for null/empty inputs (or throwing an ArgumentNullException consistently) so callers don’t crash unexpectedly.
public static bool HasUnresolvedVariables(string reference) =>
reference.IndexOfAny(['$', '{', '}']) >= 0;
src/Microsoft.ComponentDetection.Detectors/helm/HelmComponentDetector.cs:200
- The detector skips unresolved variable placeholders via
DockerReferenceUtility.HasUnresolvedVariables, but there’s no unit test covering this Helm behavior (e.g.,{{ .Values.image.tag }}) even though it’s called out in the PR description. Add a test to prevent regressions.
{
if (DockerReferenceUtility.HasUnresolvedVariables(imageReference))
{
return;
}
- Files reviewed: 10/11 changed files
- Comments generated: 5
Comment on lines
+41
to
+44
| public override string Id => "Kubernetes"; | ||
|
|
||
| public override IList<string> SearchPatterns { get; } = ["*.yaml", "*.yml"]; | ||
|
|
There was a problem hiding this comment.
SearchPatterns matches all *.yaml / *.yml files, which can be extremely broad and lead to a lot of unnecessary file reads/parses in repos with many YAML files. Consider narrowing patterns (or adding a cheaper filename-based prefilter) to reduce I/O and CPU cost.
Comment on lines
+41
to
+45
| /// <summary> | ||
| /// Returns true if the reference contains unresolved variable placeholders (e.g., ${VAR}, {{ .Values.tag }}). | ||
| /// Such references should be skipped before calling <see cref="ParseFamiliarName"/> or <see cref="ParseQualifiedName"/>. | ||
| /// </summary> | ||
| /// <param name="reference">The image reference string to check.</param> |
There was a problem hiding this comment.
PR description says unresolved-variable skipping logic was consolidated across all Docker-related detectors, but DockerfileComponentDetector still has its own regex-based HasUnresolvedVariables helper and does not use this new shared method. Either update the Dockerfile detector to use DockerReferenceUtility.HasUnresolvedVariables or adjust the PR description to match the implemented scope.
This issue also appears on line 47 of the same file.
Comment on lines
+126
to
+130
| { | ||
| if (DockerReferenceUtility.HasUnresolvedVariables(imageReference)) | ||
| { | ||
| return; | ||
| } |
There was a problem hiding this comment.
The detector skips unresolved variable placeholders via DockerReferenceUtility.HasUnresolvedVariables, but there’s no unit test covering this Docker Compose behavior (e.g., ${REGISTRY}/app:${TAG}) even though it’s called out in the PR description. Add a test to prevent regressions.
Comment on lines
+229
to
+233
| { | ||
| if (DockerReferenceUtility.HasUnresolvedVariables(imageReference)) | ||
| { | ||
| return; | ||
| } |
There was a problem hiding this comment.
The detector skips unresolved variable placeholders via DockerReferenceUtility.HasUnresolvedVariables, but there’s no unit test covering this Kubernetes behavior (e.g., ${REGISTRY}/app:${TAG}) even though it’s called out in the PR description. Add a test to prevent regressions.
src/Microsoft.ComponentDetection.Detectors/helm/HelmComponentDetector.cs
Show resolved
Hide resolved
Member
Author
|
@copilot apply changes based on the comments in this thread Note the response I left about the values/chart yaml files as well |
…Compose, Helm, and Kubernetes when specified.
…-location check, add unresolved variable tests Agent-Logs-Url: https://github.com/microsoft/component-detection/sessions/cb526852-cfd1-4b40-936c-c1b2a2dfa371 Co-authored-by: jpinz <8357054+jpinz@users.noreply.github.com>
Contributor
Applied in commit
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds new DefaultOff container-focused detectors (Docker Compose, Helm values, Kubernetes manifests) to discover Docker image references from YAML, plus introduces a shared “Containers” category to enable them via --DetectorCategories.
Changes:
- Added
DockerComposeComponentDetector,HelmComponentDetector, andKubernetesComponentDetectorand registered them in orchestrator DI. - Expanded
DetectorClasswithDockerCompose,Helm,Kubernetes, andContainers, and updated restriction logic/docs to support enabling DefaultOff detectors by category. - Added new unit tests and detector documentation pages for the new detectors (plus Dockerfile detector tests).
Show a summary per file
| File | Description |
|---|---|
| test/Microsoft.ComponentDetection.Orchestrator.Tests/Services/DetectorRestrictionServiceTests.cs | Adds tests validating DefaultOff detectors can be included via category restrictions. |
| test/Microsoft.ComponentDetection.Detectors.Tests/KubernetesComponentDetectorTests.cs | New test coverage for Kubernetes YAML image extraction scenarios. |
| test/Microsoft.ComponentDetection.Detectors.Tests/HelmComponentDetectorTests.cs | New test coverage for Helm values.yaml image extraction scenarios. |
| test/Microsoft.ComponentDetection.Detectors.Tests/DockerfileComponentDetectorTests.cs | Adds unit tests for Dockerfile image reference parsing behavior. |
| test/Microsoft.ComponentDetection.Detectors.Tests/DockerComposeComponentDetectorTests.cs | New test coverage for Docker Compose image extraction scenarios. |
| src/Microsoft.ComponentDetection.Orchestrator/Services/DetectorRestrictionService.cs | Updates restriction logic to include DefaultOff detectors when filtering by categories. |
| src/Microsoft.ComponentDetection.Orchestrator/Extensions/ServiceCollectionExtensions.cs | Registers the new detectors in DI. |
| src/Microsoft.ComponentDetection.Detectors/kubernetes/KubernetesComponentDetector.cs | Implements Kubernetes YAML detector for container image references. |
| src/Microsoft.ComponentDetection.Detectors/helm/HelmComponentDetector.cs | Implements Helm values YAML detector for image references. |
| src/Microsoft.ComponentDetection.Detectors/dockerfile/DockerfileComponentDetector.cs | Adds “Containers” category to Dockerfile detector. |
| src/Microsoft.ComponentDetection.Detectors/dockercompose/DockerComposeComponentDetector.cs | Implements Docker Compose YAML detector for service image references. |
| src/Microsoft.ComponentDetection.Contracts/DetectorClass.cs | Adds new detector categories: DockerCompose/Helm/Kubernetes/Containers. |
| src/Microsoft.ComponentDetection.Common/DockerReference/DockerReferenceUtility.cs | Adds shared HasUnresolvedVariables helper for skipping templated refs. |
| docs/detectors/README.md | Lists the new detectors in detector documentation index. |
| docs/detectors/kubernetes.md | Adds Kubernetes detector documentation. |
| docs/detectors/helm.md | Adds Helm detector documentation. |
| docs/detectors/dockerfile.md | Notes container-category enabling path for Dockerfile detector. |
| docs/detectors/dockercompose.md | Adds Docker Compose detector documentation. |
| docs/detector-arguments.md | Documents that category selection enables matching DefaultOff detectors; adds Containers to supported categories list. |
| .gitignore | Ignores local .nuget/ folder. |
Copilot's findings
- Files reviewed: 19/20 changed files
- Comments generated: 2
Comment on lines
+59
to
+77
| @@ -68,7 +68,13 @@ public IEnumerable<IComponentDetector> ApplyRestrictions(DetectorRestrictions re | |||
| } | |||
|
|
|||
| return false; | |||
| }).ToList(); | |||
| } | |||
|
|
|||
| detectors = detectors.Where(CategoryFilter).ToList(); | |||
|
|
|||
| // Also include DefaultOff detectors whose categories match | |||
| detectors = detectors.Union(defaultOffDetectors.Where(CategoryFilter)).ToList(); | |||
|
|
|||
There was a problem hiding this comment.
--DetectorsFilter (AllowedDetectorIds) is applied before category filtering, but the new Union(defaultOffDetectors.Where(CategoryFilter)) can re-introduce DefaultOff detectors even when a user explicitly provided a detector allow-list. If both AllowedDetectorIds and AllowedDetectorCategories are set, this effectively turns the result into a union rather than an intersection. Consider applying the AllowedDetectorIds filter after category/DefaultOff expansion (or applying it to the DefaultOff set as well) and add a regression test covering the combined options.
Comment on lines
+41
to
+48
| /// <summary> | ||
| /// Returns true if the reference contains unresolved variable placeholders (e.g., ${VAR}, {{ .Values.tag }}). | ||
| /// Such references should be skipped before calling <see cref="ParseFamiliarName"/> or <see cref="ParseQualifiedName"/>. | ||
| /// </summary> | ||
| /// <param name="reference">The image reference string to check.</param> | ||
| /// <returns><c>true</c> if the reference contains variable placeholder characters; otherwise <c>false</c>.</returns> | ||
| public static bool HasUnresolvedVariables(string reference) => | ||
| reference.IndexOfAny(['$', '{', '}']) >= 0; |
There was a problem hiding this comment.
This new helper duplicates the Dockerfile detector’s existing HasUnresolvedVariables implementation (Regex("[${}]")). To actually consolidate the logic across Docker-related detectors (as described in the PR), consider switching the Dockerfile detector to call this shared helper and removing the duplicate method so the criteria can’t drift over time.
Member
Author
|
@copilot apply changes based on the comments in this thread |
…tionService Agent-Logs-Url: https://github.com/microsoft/component-detection/sessions/a9f4d3d4-b85a-4e4f-b83f-20bf4462eb02 Co-authored-by: jpinz <8357054+jpinz@users.noreply.github.com>
Agent-Logs-Url: https://github.com/microsoft/component-detection/sessions/a9f4d3d4-b85a-4e4f-b83f-20bf4462eb02 Co-authored-by: jpinz <8357054+jpinz@users.noreply.github.com>
Contributor
Applied in commit
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
…r unresolved variable checks
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends Component Detection’s container scanning capabilities by introducing new DefaultOff detectors for Docker Compose, Helm, and Kubernetes YAML, consolidating “unresolved variable” skipping logic, and adding a Containers detector category to enable all container-related detectors together. It also fixes detector restriction behavior so that category expansion respects an explicit detector ID allow-list (intersection instead of union).
Changes:
- Added new DefaultOff detectors:
DockerComposeComponentDetector,HelmComponentDetector,KubernetesComponentDetector, and registered them in orchestrator DI. - Added
DetectorClass.Containers(and related per-detector categories) and updated CLI/docs accordingly. - Centralized unresolved-variable skipping with
DockerReferenceUtility.HasUnresolvedVariablesand updated Dockerfile detector + added tests.
Show a summary per file
| File | Description |
|---|---|
| test/Microsoft.ComponentDetection.Orchestrator.Tests/Services/DetectorRestrictionServiceTests.cs | Adds test coverage for category-based enabling of DefaultOff detectors and for the allow-list intersection fix. |
| test/Microsoft.ComponentDetection.Detectors.Tests/KubernetesComponentDetectorTests.cs | Adds unit tests for Kubernetes YAML image extraction and skipping unresolved variables. |
| test/Microsoft.ComponentDetection.Detectors.Tests/HelmComponentDetectorTests.cs | Adds unit tests for Helm values parsing patterns (direct/structured images, digests, skipping). |
| test/Microsoft.ComponentDetection.Detectors.Tests/DockerfileComponentDetectorTests.cs | Adds unit tests for Dockerfile detector behavior including unresolved-variable skipping and digest handling. |
| test/Microsoft.ComponentDetection.Detectors.Tests/DockerComposeComponentDetectorTests.cs | Adds unit tests for Docker Compose YAML image extraction and skipping unresolved variables. |
| src/Microsoft.ComponentDetection.Orchestrator/Services/DetectorRestrictionService.cs | Fixes restriction logic so DefaultOff detectors introduced via category expansion are constrained by AllowedDetectorIds when provided. |
| src/Microsoft.ComponentDetection.Orchestrator/Extensions/ServiceCollectionExtensions.cs | Registers the new detectors in DI. |
| src/Microsoft.ComponentDetection.Detectors/kubernetes/KubernetesComponentDetector.cs | New Kubernetes YAML detector that extracts container image references. |
| src/Microsoft.ComponentDetection.Detectors/helm/HelmComponentDetector.cs | New Helm detector intended to parse values files when a Chart file is co-located. |
| src/Microsoft.ComponentDetection.Detectors/dockerfile/DockerfileComponentDetector.cs | Updates categories to include Containers and switches unresolved-variable detection to shared utility. |
| src/Microsoft.ComponentDetection.Detectors/dockercompose/DockerComposeComponentDetector.cs | New Docker Compose detector that extracts service image: references. |
| src/Microsoft.ComponentDetection.Contracts/DetectorClass.cs | Adds DockerCompose, Helm, Kubernetes, and Containers detector classes/categories. |
| src/Microsoft.ComponentDetection.Common/DockerReference/DockerReferenceUtility.cs | Adds HasUnresolvedVariables helper used across container-related detectors. |
| docs/detectors/README.md | Documents the new detectors in the detector docs index. |
| docs/detectors/kubernetes.md | Adds Kubernetes detector documentation. |
| docs/detectors/helm.md | Adds Helm detector documentation. |
| docs/detectors/dockerfile.md | Updates Dockerfile docs to mention enabling via --DetectorCategories Containers. |
| docs/detectors/dockercompose.md | Adds Docker Compose detector documentation. |
| docs/detector-arguments.md | Updates --DetectorCategories documentation to mention DefaultOff category enabling and Containers. |
| .gitignore | Ignores .nuget/. |
Copilot's findings
- Files reviewed: 19/20 changed files
- Comments generated: 6
src/Microsoft.ComponentDetection.Detectors/helm/HelmComponentDetector.cs
Show resolved
Hide resolved
Comment on lines
+65
to
+74
| // Only process values files — and only when co-located with a Chart.yaml in the same directory. | ||
| if (!IsValuesFile(fileName)) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| if (!this.helmChartDirectories.ContainsKey(directory)) | ||
| { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Values files are only processed if helmChartDirectories already contains the directory, but file enumeration order isn’t guaranteed. If a values file is observed before its Chart file, it will be skipped and never re-processed, causing missed detections. Consider checking for a co-located Chart file on demand (e.g., via file existence) or building the chart-directory set up-front in OnPrepareDetectionAsync before processing values files.
Member
Author
There was a problem hiding this comment.
@copilot apply changes based on this feedback
Contributor
There was a problem hiding this comment.
Applied in commit 7f08f43. Replaced the single-pass OnFileFoundAsync approach with a two-pass OnPrepareDetectionAsync (following the pattern used by GoComponentDetector): all matching files are materialized first, chart directories are identified in pass 1, then only values files from those directories are emitted for processing in pass 2. This eliminates the ordering dependency entirely. Added TestHelm_ValuesFileObservedBeforeChartYamlAsync to cover the values-before-chart enumeration scenario.
src/Microsoft.ComponentDetection.Detectors/dockercompose/DockerComposeComponentDetector.cs
Show resolved
Hide resolved
Agent-Logs-Url: https://github.com/microsoft/component-detection/sessions/cb0747c1-3fa9-4eaf-b0b3-3c3cde4e0f34 Co-authored-by: jpinz <8357054+jpinz@users.noreply.github.com>
Contributor
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request adds new detectors for Docker Compose, Helm, and Kubernetes YAML files, enabling the system to automatically discover and register Docker image references found in these types of configuration files. It also updates the service registration and the
DetectorClassenum to support these new detectors.It also adds tests for the new detectors plus the Dockerfile one.
Image references containing unresolved variable placeholders (e.g.,
${REGISTRY}/app:${TAG},{{ .Values.image.tag }}) are silently skipped in all four Docker-related detectors (DockerCompose, Helm, Kubernetes, and Dockerfile) to avoid noisy warnings for otherwise-valid real-world manifests. A sharedDockerReferenceUtility.HasUnresolvedVariableshelper was added toMicrosoft.ComponentDetection.Commonto consolidate this logic, and all detectors—including the existingDockerfileComponentDetector—now use it.The Helm detector uses the combination of a
Chart.yaml/Chart.ymlfile and a*values*.(yaml|yml)file in the same directory to identify Helm charts. Values files found without a co-located Chart file are skipped, avoiding false positives from unrelatedvalues.yamlfiles in non-Helm projects. The co-location check is implemented via a two-passOnPrepareDetectionAsync: all matching files are materialized first, chart directories are identified in pass 1, then only values files from those directories are emitted for processing in pass 2. This eliminates any dependency on file enumeration order, so values files are correctly detected regardless of whether they are observed before or after their co-locatedChart.yaml.A
Containersdetector category was added toDetectorClass, allowing all container-related DefaultOff detectors (Dockerfile, DockerCompose, Helm, Kubernetes) to be enabled together via--DetectorCategories Containers.A bug in
DetectorRestrictionServicewas also fixed: when bothAllowedDetectorIdsandAllowedDetectorCategoriesare specified, DefaultOff detectors introduced via category expansion are now also constrained by the ID allow-list, producing an intersection rather than an unintended union.New Detector Checklist
IDefaultOffComponentDetector