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
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
src/Microsoft.ComponentDetection.Detectors/helm/HelmComponentDetector.cs
Outdated
Show resolved
Hide resolved
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:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR expands Component Detection’s container-image discovery by introducing new DefaultOff detectors for Docker Compose, Helm values, and Kubernetes manifests, and consolidates “skip unresolved template variables” logic into a shared Docker reference utility.
Changes:
- Added new detectors:
DockerComposeComponentDetector,HelmComponentDetector, andKubernetesComponentDetectorto parse YAML and registerDockerReferenceComponentusages. - Added shared
DockerReferenceUtility.HasUnresolvedVariableshelper and updated the existing Dockerfile detector to use it. - Added documentation and unit tests covering the new detectors (plus Dockerfile detector coverage).
Show a summary per file
| File | Description |
|---|---|
| test/Microsoft.ComponentDetection.Detectors.Tests/KubernetesComponentDetectorTests.cs | New unit tests for Kubernetes YAML image extraction. |
| test/Microsoft.ComponentDetection.Detectors.Tests/HelmComponentDetectorTests.cs | New unit tests for Helm values image extraction + chart co-location behavior. |
| test/Microsoft.ComponentDetection.Detectors.Tests/DockerfileComponentDetectorTests.cs | New unit tests for Dockerfile image parsing and variable-skipping behavior. |
| test/Microsoft.ComponentDetection.Detectors.Tests/DockerComposeComponentDetectorTests.cs | New unit tests for Docker Compose service image extraction. |
| src/Microsoft.ComponentDetection.Orchestrator/Extensions/ServiceCollectionExtensions.cs | Registers the new detectors in DI. |
| src/Microsoft.ComponentDetection.Detectors/kubernetes/KubernetesComponentDetector.cs | New Kubernetes detector implementation. |
| src/Microsoft.ComponentDetection.Detectors/helm/HelmComponentDetector.cs | New Helm detector implementation with two-pass preparation. |
| src/Microsoft.ComponentDetection.Detectors/dockerfile/DockerfileComponentDetector.cs | Switches unresolved-variable detection to shared helper; removes local regex. |
| src/Microsoft.ComponentDetection.Detectors/dockercompose/DockerComposeComponentDetector.cs | New Docker Compose detector implementation. |
| src/Microsoft.ComponentDetection.Contracts/DetectorClass.cs | Adds new detector classes for DockerCompose/Helm/Kubernetes. |
| src/Microsoft.ComponentDetection.Common/DockerReference/DockerReferenceUtility.cs | Adds HasUnresolvedVariables helper. |
| docs/detectors/README.md | Adds detector docs links/entries for Docker Compose, Helm, Kubernetes. |
| docs/detectors/kubernetes.md | New Kubernetes detector documentation. |
| docs/detectors/helm.md | New Helm detector documentation. |
| docs/detectors/dockercompose.md | New Docker Compose detector documentation. |
| .gitignore | Ignores .nuget/ directory. |
Copilot's findings
- Files reviewed: 15/16 changed files
- Comments generated: 2
Comment on lines
+37
to
+39
| "Chart.yaml", "Chart.yml", | ||
| "chart.yaml", "chart.yml", | ||
| "*values*.yaml", "*values*.yml", |
There was a problem hiding this comment.
SearchPatterns uses *values*.yaml / *values*.yml, but the repo’s PathUtilityService.MatchesPattern only supports * as a leading or trailing wildcard; mid-string * is treated literally, so values files (e.g., values.yaml, values.production.yaml) will never be discovered in real scans and the detector will effectively do no work. Consider switching to supported patterns (e.g., *.yaml/*.yml with IsValuesFile filtering) or extending the matcher to support contains patterns.
Suggested change
| "Chart.yaml", "Chart.yml", | |
| "chart.yaml", "chart.yml", | |
| "*values*.yaml", "*values*.yml", | |
| "*.yaml", "*.yml", |
Comment on lines
+30
to
+36
| public override IList<string> SearchPatterns { get; } = | ||
| [ | ||
| "docker-compose.yml", "docker-compose.yaml", | ||
| "docker-compose.*.yml", "docker-compose.*.yaml", | ||
| "compose.yml", "compose.yaml", | ||
| "compose.*.yml", "compose.*.yaml", | ||
| ]; |
There was a problem hiding this comment.
SearchPatterns contains docker-compose.*.yml / compose.*.yaml variants, but the repo’s PathUtilityService.MatchesPattern only supports * at the beginning or end of a pattern. As written, override/env-specific compose files like docker-compose.override.yml or compose.prod.yaml won’t be discovered in real scans. Use patterns the matcher can handle (e.g., docker-compose.* / compose.*) or broaden to *.yml/*.yaml and filter by filename.
JamieMagee
reviewed
Apr 4, 2026
| @@ -0,0 +1,145 @@ | |||
| #nullable disable | |||
Member
There was a problem hiding this comment.
Enable nullability for new files please
JamieMagee
reviewed
Apr 4, 2026
Comment on lines
+37
to
+38
| "Chart.yaml", "Chart.yml", | ||
| "chart.yaml", "chart.yml", |
Member
There was a problem hiding this comment.
Are the lowercase chart.yaml / chart.yml entries here for Linux case-sensitivity? IsChartFile() already does OrdinalIgnoreCase, so once a file is matched by the walker, it'll be handled correctly. The question is whether the walker's glob is case-sensitive.
JamieMagee
reviewed
Apr 4, 2026
Comment on lines
+19
to
+29
| private static readonly HashSet<string> KubernetesKinds = new(StringComparer.OrdinalIgnoreCase) | ||
| { | ||
| "Pod", | ||
| "Deployment", | ||
| "StatefulSet", | ||
| "DaemonSet", | ||
| "ReplicaSet", | ||
| "Job", | ||
| "CronJob", | ||
| "ReplicationController", | ||
| }; |
Member
There was a problem hiding this comment.
Missing PodTemplate from the KubernetesKinds set. It's not common but it does embed a pod spec with container images.
JamieMagee
reviewed
Apr 4, 2026
| } | ||
|
|
||
| [TestMethod] | ||
| public async Task TestHelm_ValuesFileObservedBeforeChartYamlAsync() |
Member
There was a problem hiding this comment.
This passes because both files default to tmp as their directory. They're co-located by coincidence, not by test design.
I'd add a test where Chart.yaml is in /tmp/chartA/ and values.yaml is in /tmp/chartB/ (via the fileLocation parameter) to actually verify the co-location filtering rejects mismatched directories. TestHelm_ValuesWithoutChartYamlSkippedAsync tests "no chart at all" but not "chart exists in a different directory."
JamieMagee
reviewed
Apr 4, 2026
| } | ||
| } | ||
|
|
||
| private void TryRegisterImageReference(string imageReference, ISingleFileComponentRecorder recorder, string fileLocation) |
Member
There was a problem hiding this comment.
TryRegisterImageReference is copy-pasted across all three detectors. Consider pulling it into a shared static method on DockerReferenceUtility. Something like TryParseImageReference(string) -> DockerReference? that handles the unresolved-variable check, parsing, and exception swallowing. The Dockerfile detector does the same work inline too, so it'd consolidate four copies.
Member
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