feat: allow to remove fields from vulnerability reports and omit empty fields#2854
feat: allow to remove fields from vulnerability reports and omit empty fields#2854sathieu wants to merge 5 commits intoaquasecurity:mainfrom
Conversation
be96b31 to
9251aa4
Compare
|
Hi @sathieu |
9251aa4 to
25fc308
Compare
| value := !strings.HasPrefix(field, "-") | ||
| field = strings.TrimPrefix(field, "-") |
There was a problem hiding this comment.
i think we should run strings.TrimSpace before these calls to support a case -PublishedDate.
There was a problem hiding this comment.
also it'd be great to have a test for this case.
There was a problem hiding this comment.
Added strings.TrimSpace before removing prefix, with various tests: " -Resource,- InstalledVersion, - FixedVersion, - PublishedDate,[...]"
| FixedVersion: true, | ||
| PublishedDate: true, | ||
| LastModifiedDate: true, | ||
| Severity: true, |
There was a problem hiding this comment.
there is a concern about Severity. it may affect on Summary.
so I suggest don't allow to exclude it. because I don't know a case when we need to exclude Severity. may be I miss something
WDYT?
There was a problem hiding this comment.
Severity is now always kept (also in CRD, no omitempty).
| Title bool | ||
| PrimaryLink bool | ||
| Score bool | ||
| PURL bool |
There was a problem hiding this comment.
about PURL.
It seems PURL is too common name, because actually we can manage PkgPURL only, can't we?
|
@sathieu Thank you very much for your effort and for submitting this PR — it’s greatly appreciated. I’ve left a few inline comments to discuss some ideas. I also have a small concern regarding backward compatibility. In particular, clients that previously relied on the presence of certain fields (even when they were empty) now need to be prepared for those fields to be omitted entirely. Specifically, the handling of nil vs empty slices has changed: This is likely not a critical issue, but it would be good to get feedback from the community and confirm that this change won’t cause unexpected behavior for existing consumers. |
25fc308 to
caa0cb0
Compare
| value := !strings.HasPrefix(field, "-") | ||
| field = strings.TrimPrefix(field, "-") |
There was a problem hiding this comment.
Added strings.TrimSpace before removing prefix, with various tests: " -Resource,- InstalledVersion, - FixedVersion, - PublishedDate,[...]"
| FixedVersion: true, | ||
| PublishedDate: true, | ||
| LastModifiedDate: true, | ||
| Severity: true, |
There was a problem hiding this comment.
Severity is now always kept (also in CRD, no omitempty).
| Title bool | ||
| PrimaryLink bool | ||
| Score bool | ||
| PURL bool |
caa0cb0 to
8da4c79
Compare
8da4c79 to
d9d4dd8
Compare
There was a problem hiding this comment.
Pull request overview
This PR makes vulnerability reports more size-efficient and configurable to mitigate etcd max request size issues by allowing selected vulnerability fields to be omitted and by not serializing empty fields. It introduces a configuration-based mechanism to disable specific core vulnerability fields, updates the CRDs and docs accordingly, and ensures webhook and plugin behavior remains consistent with the new schema.
Changes:
- Extend
trivy.additionalVulnerabilityReportFieldsto support disabling core vulnerability fields (e.g.,Resource,InstalledVersion,FixedVersion, dates, title, primary link, score, PURL), with new default behavior keeping them enabled. - Make several
Vulnerabilityfields optional (omitempty) and adjust the Trivy plugin’s mapping from scan results to respect the new configuration, omitting disabled fields and not emitting empty values. - Update CRDs, Helm charts, static manifests, docs, and tests (Trivy config tests, plugin tests, and webhook reporter tests) to align with the new behavior.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
pkg/vulnerabilityreport/io.go |
Introduces the expanded AdditionalFields struct, a NewDefaultAdditionalFields helper, and updates GetVulnerabilitiesFromScanResult to conditionally populate vulnerability fields based on config and omit unset date and PURL fields. |
pkg/plugins/trivy/config.go |
Changes GetAdditionalVulnerabilityReportFields to start from defaults and then enable/disable flags based on a comma-separated, optionally --prefixed configuration list. |
pkg/plugins/trivy/config_test.go |
Expands tests to validate default behavior, additive configuration, full removal, and mixed add/remove scenarios for additionalVulnerabilityReportFields, asserting each AdditionalFields flag individually. |
pkg/plugins/trivy/plugin.go |
Refactors ParseReportData to compute addFields once and pass it into GetVulnerabilitiesFromScanResult, ensuring all results use the same configured field set. |
pkg/plugins/trivy/plugin_test.go |
Keeps ParseReportData behavior under existing configurations validated via sampleVulnerabilityReport / sampleExposedSecretReport, ensuring the new conditional field logic does not regress expected reports. |
pkg/apis/aquasecurity/v1alpha1/vulnerability_types.go |
Marks multiple Vulnerability fields (resource, versions, dates, title, links, target) as omitempty, enabling the API to omit them when unset and reducing serialized size. |
pkg/webhook/webhookreporter_test.go |
Updates the webhook payload expectation for vulnerability reports to reflect that only non-empty, non-omitted fields appear in the JSON body. |
docs/docs/vulnerability-scanning/trivy.md |
Documents that additionalVulnerabilityReportFields now supports both adding extra fields and removing core ones via --prefixed names, listing the supported removable field identifiers. |
deploy/static/trivy-operator.yaml |
Adjusts the static VulnerabilityReport and ClusterVulnerabilityReport CRD schemas to drop core vulnerability fields from the required list, matching their new optional nature. |
deploy/helm/crds/aquasecurity.github.io_vulnerabilityreports.yaml |
Updates the Helm CRD for namespaced vulnerability reports to make the same fields optional in the schema. |
deploy/helm/crds/aquasecurity.github.io_clustervulnerabilityreports.yaml |
Updates the Helm CRD for cluster-scoped vulnerability reports to mirror the optional field behavior. |
deploy/helm/values.yaml |
Extends the Helm values description of trivy.additionalVulnerabilityReportFields to document both additive fields and removable fields with a leading -. |
deploy/helm/README.md |
Mirrors the values.yaml description in the chart README to explain how to disable specific vulnerability fields via trivy.additionalVulnerabilityReportFields. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
@sathieu I think we certainly need to version this. It will be backwards breaking change to my understanding (removing existing fields). As you mentioned, we also will need a webhook to convert the resources on the fly. |
d9d4dd8 to
5269b3d
Compare
|
@afdesk @simar7 This PR starts to be too big. Here is what I have done:
I don't know how to handle the last item. Proposal:
WDYT? Can we mandate cert-manager for the static installation? How to handle OLM installs? |
Description
I propose two changes to limit the problems of etcd max size reached (#757):
Before:
After first commit (and
trivy.additionalVulnerabilityReportFields=-Resource,-InstalledVersion,-FixedVersion,-PublishedDate,-LastModifiedDate,-Title,-PrimaryLink,-Score,-PURL):After both commits (and
trivy.additionalVulnerabilityReportFields=-<same>):Notes:
Related issues
Checklist