Closed dwertent closed 7 months ago
PR Description updated to latest commit (https://github.com/kubescape/kubevuln/commit/8a9b27b7a2201e942620fab22a5a2a6077a62151)
⏱️ Estimated effort to review [1-5] | 3, because the PR involves multiple changes across different files, including error handling, logic for handling partial artifacts in SBOMs, and updates to unit tests. The complexity is moderate due to the handling of specific error conditions and the interaction with storage systems. |
🧪 Relevant tests | Yes |
🔍 Possible issues | Hardcoded Version Check: The check for a specific SBOM creator version ("v0.101.1") in `validateSBOMp` function is hardcoded, which might lead to maintenance issues when versions change. |
Error Handling Consistency: The new error `ErrSBOMWithPartialArtifacts` is handled differently in various parts of the code, which could lead to inconsistent behavior under different conditions. | |
🔒 Security concerns | No |
Category | Suggestions |
Enhancement |
Improve the clarity and helpfulness of the error message for
___
**Consider using a more descriptive error message for |
Enhance logging by adding more contextual information.___ **Use structured logging to include more context in the log message, such as the error typeor other relevant details.** [core/services/scan.go [240-241]](https://github.com/kubescape/kubevuln/pull/224/files#diff-85deac1fd3ad15a30ddc8a15245049faf294923a8058adfb7c47994034b662d8R240-R241) ```diff -logger.L().Ctx(ctx).Warning("error getting relevant SBOM", helpers.Error(err), - helpers.String("instanceID", workload.InstanceID)) +logger.L().Ctx(ctx).Warning("Failed to retrieve SBOM", helpers.Error(err), helpers.String("instanceID", workload.InstanceID), helpers.String("errorType", fmt.Sprintf("%T", err))) ``` | |
Maintainability |
Refactor nested loops into a separate function to enhance code readability.___ **Refactor the nested loop for updating artifacts to a separate function to improvereadability and maintainability.** [core/services/scan.go [231-237]](https://github.com/kubescape/kubevuln/pull/224/files#diff-85deac1fd3ad15a30ddc8a15245049faf294923a8058adfb7c47994034b662d8R231-R237) ```diff -for i := range sbomp.Content.Artifacts { - for j := range sbom.Content.Artifacts { - if sbomp.Content.Artifacts[i].ID == sbom.Content.Artifacts[j].ID { - sbomp.Content.Artifacts[i] = sbom.Content.Artifacts[j] - break - } - } -} +updateArtifacts(sbomp, sbom) ``` |
Best practice |
Replace hardcoded version string with a constant to improve code maintainability.___ **Avoid hardcoding the version string in the conditional check. Consider defining it as aconstant or retrieving it from a configuration.** [repositories/apiserver.go [825-826]](https://github.com/kubescape/kubevuln/pull/224/files#diff-dcc3484e8a0759bddcff34049e6114ed941f40176d0f3c1e4a84841dfabf6403R825-R826) ```diff -if manifest.Spec.Metadata.Tool.Version == "v0.101.1" { // hard coded version. We have a specific workaround for this version +const specificVersion = "v0.101.1" +if manifest.Spec.Metadata.Tool.Version == specificVersion { return domain.ErrSBOMWithPartialArtifacts } ``` |
Bug |
Improve error handling by returning the actual error instead of
___
**Instead of returning |
Summary:
User description
Overview
This would enable us to release the next chart without requiring restarts for generating new filtered SBOMs
Type
bug_fix, enhancement
Description
ErrSBOMWithPartialArtifacts
to handle SBOMs that contain only partial artifacts.Changes walkthrough
scan.go
Introduce Error Handling for Partial Artifacts in SBOMs
core/domain/scan.go
ErrSBOMWithPartialArtifacts
to handle cases of SBOMswith partial artifacts.
scan.go
Enhance SBOM Handling During CVE Scans
core/services/scan.go
scanning.
other errors.
apiserver.go
Update SBOM Validation and Retrieval Logic
repositories/apiserver.go
validation function.
GetSBOMp
function to handle partial artifacts errorsdifferently.
apiserver_test.go
Unit Tests for SBOM Validation
repositories/apiserver_test.go - Added unit tests for validating SBOMs with partial artifacts.