Sast rescoring update #270

TuanAnh17N · 2024-11-27T15:55:03Z

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Release note:

test/test_rescoring.py

rescore/model.py

rescore/utility.py

cm06/scan.py

rescore/model.py

config.py

features/__init__.py

cm06/scan.py

config.py

rescore/model.py

zkdev · 2024-12-12T08:39:32Z

rescore/utility.py

+    return None
+
+
+def rescore_sast_severity(


this function should work for all types of rescoring-rule no?
can we merge / factor out with existing bdba rescoring?

it seems you decided against my suggestion, which of course is fine, but could you please elaborate what lead to this decision?

looking only at the PR I do not see a good reason against 🤔

test/test_rescoring.py

rescore/model.py

cm06/scan.py

rescore/utility.py

ccwienk · 2024-12-17T11:42:11Z

@TuanAnh17N : I assume test-errors stem from dependency against (unreleased) changes in cc-utils? If so, then I suggest to first merge and release change for cc-utils + rerun tests here to be sure.

cm06/scan.py

features/__init__.py

test/test_rescoring.py

zkdev · 2024-12-20T08:18:01Z

cm06/scan.py

+    sast_config: config.SASTConfig,
+    component_descriptor_lookup: cnudie.retrieve.ComponentDescriptorLookupById,
+) -> list[str]:
+    if sast_config.component_version:


mv to caller, this function should only get versions from time range -> SoC
also, can we please use a more meaningful function name, e.g. version_from_time_range?

zkdev · 2024-12-20T08:18:44Z

cm06/scan.py

+    if sast_config.component_version:
+        return [sast_config.component_version]
+
+    version_lookup = lookups.init_version_lookup()


rather pass in version-lookup (dependency injection)

also: consistency (why pass-in component-descriptor-lookup, but not version-lookup)

zkdev · 2024-12-20T08:19:37Z

cm06/scan.py

+        if not (pv := versionutil.parse_to_semver(
+            version=v,
+            invalid_semver_ok=False,
+        )).prerelease and not pv.build


version filtering is good, but I suggest to at least make it configurable via function parameters

I think we have a util-function for that in cc-util's version.py (at least is_final comes to my mind)

zkdev · 2024-12-20T08:21:04Z

cm06/scan.py

+
+    versions = [
+        version for version
+        in filter_by_date_range(versions)


rather use built-in filter function

zkdev · 2024-12-20T08:23:04Z

cm06/scan.py

+            prune_unique=True,
+        ):
+            if not cnode.component.sources:
+                continue


please add a brief comment explaining why this case is skipped to help our future selves x)

also: why not simply use note_filter.cnudie.iter.Filter.sources?`

zkdev · 2024-12-20T08:25:13Z

cm06/scan.py

+                all_findings_for_rescoring = []
+
+                source_node = cnudie.iter.SourceNode(
+                    path=(cnudie.iter.NodePathEntry(cnode.component),),


rather directly iter source nodes (node_filter in cnudie.iter.iter)

also, if constructing source_node manually, make sure to give the full path and not only the parent component

I think in this context, if is not a good idea to manually construct sourcenodes.

zkdev · 2024-12-20T08:27:31Z

cm06/scan.py

+                    ),
+                    time_now=time_now,
+                )
+                new_metadata.extend(rescored_metadata)


TIL: list.extend resolves a generator :-)

sure - it accepts any iterable (which includes generators)

zkdev · 2024-12-20T08:29:31Z

rescore/utility.py

+    return None
+
+
+def rescore_sast_severity(


it seems you decided against my suggestion, which of course is fine, but could you please elaborate what lead to this decision?

looking only at the PR I do not see a good reason against 🤔

ccwienk · 2024-12-20T10:16:26Z

cm06/scan.py

+    if sast_config.component_version:
+        return [sast_config.component_version]
+
+    version_lookup = lookups.init_version_lookup()


also: consistency (why pass-in component-descriptor-lookup, but not version-lookup)

ccwienk · 2024-12-20T10:17:28Z

cm06/scan.py

+        if not (pv := versionutil.parse_to_semver(
+            version=v,
+            invalid_semver_ok=False,
+        )).prerelease and not pv.build


I think we have a util-function for that in cc-util's version.py (at least is_final comes to my mind)

ccwienk · 2024-12-20T10:19:14Z

cm06/scan.py

+            prune_unique=True,
+        ):
+            if not cnode.component.sources:
+                continue


also: why not simply use note_filter.cnudie.iter.Filter.sources?`

ccwienk · 2024-12-20T10:20:01Z

cm06/scan.py

+                all_findings_for_rescoring = []
+
+                source_node = cnudie.iter.SourceNode(
+                    path=(cnudie.iter.NodePathEntry(cnode.component),),


I think in this context, if is not a good idea to manually construct sourcenodes.

ccwienk · 2024-12-20T10:20:27Z

cm06/scan.py

+                    ),
+                    time_now=time_now,
+                )
+                new_metadata.extend(rescored_metadata)


sure - it accepts any iterable (which includes generators)

ccwienk · 2024-12-20T10:24:06Z

rescore/utility.py

+    elif rescoring_rule.rescore is rescore.model.Rescore.TO_NONE:
+        return github.compliance.model.Severity.NONE.name
+
+    raise NotImplementedError(rescoring_rule.rescore)


nit: while we do use NIE for that in our "heritage" codebase, it is actually not intended to be used for this purpose (python-documentation states NIE is intended for being raised by (abstract) methods that were not implemented). rather use ValueError

ccwienk · 2024-12-20T10:26:00Z

rescore/utility.py

+    time_now: datetime.datetime
+) -> typing.Generator[dso.model.ArtefactMetadata, None, None]:
+    for finding in findings:
+        matching_rule = next(


I really do not like this pattern. how about rather doing sth. like

try: mr = next(...) except StopIteration: continue

In my opinion, this makes it a lot more obvious what is being done.

ccwienk · 2024-12-20T10:27:21Z

rescore/utility.py

+) -> typing.Generator[dso.model.ArtefactMetadata, None, None]:
+    for finding in findings:
+        matching_rule = next(
+            matching_sast_rescore_rule(


btw: is this function intended to yield more than one rule (judging from the name, I would assume: no)? if not, why not have the function either return a rule or None. This would make it less messy to use.

ccwienk · 2024-12-20T10:27:42Z

rescore/utility.py

+        new_severity = rescore_sast_severity(
+            rescoring_rule=matching_rule,
+        )
+        if finding.data.severity == new_severity:


if severity is an enum (did not check), use is for comparison.

ccwienk · 2024-12-20T10:28:15Z

rescore/utility.py

+                severity=new_severity,
+                user=user,
+                matching_rules=[matching_rule.name],
+                comment='Automatically rescored based on rules.',


include rulename(s) (+ ruleset-name) in comment?

TuanAnh17N requested a review from a team as a code owner November 27, 2024 15:55

zkdev requested changes Nov 28, 2024

View reviewed changes

ccwienk requested changes Nov 29, 2024

View reviewed changes

cm06/scan.py Outdated Show resolved Hide resolved

cm06/scan.py Outdated Show resolved Hide resolved

cm06/scan.py Outdated Show resolved Hide resolved

cm06/scan.py Outdated Show resolved Hide resolved

rescore/model.py Outdated Show resolved Hide resolved

TuanAnh17N force-pushed the sast-rescoring-update branch from 79f7529 to 5106da9 Compare December 2, 2024 16:08

ocm-ci-robot-0 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Dec 2, 2024

TuanAnh17N requested review from zkdev and ccwienk December 2, 2024 16:10

8R0WNI3 requested changes Dec 3, 2024

View reviewed changes

TuanAnh17N force-pushed the sast-rescoring-update branch from 5106da9 to a3c0361 Compare December 10, 2024 19:01

ocm-ci-robot-0 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Dec 10, 2024

ccwienk requested changes Dec 11, 2024

View reviewed changes

TuanAnh17N force-pushed the sast-rescoring-update branch from a3c0361 to 2f3b3ba Compare December 11, 2024 16:23

ocm-ci-robot-0 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 11, 2024

TuanAnh17N requested a review from 8R0WNI3 December 11, 2024 16:24

ocm-ci-robot-0 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 11, 2024

zkdev requested changes Dec 12, 2024

View reviewed changes

8R0WNI3 requested changes Dec 12, 2024

View reviewed changes

8R0WNI3 mentioned this pull request Dec 12, 2024

Introduce sastfinding as a new finding type gardener/cc-utils#1086

Merged

TuanAnh17N force-pushed the sast-rescoring-update branch from 2f3b3ba to 9895c5f Compare December 16, 2024 14:54

ocm-ci-robot-0 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Dec 16, 2024

TuanAnh17N requested a review from zkdev December 16, 2024 15:17

zkdev requested changes Dec 16, 2024

View reviewed changes

cm06/scan.py Outdated Show resolved Hide resolved

rescore/utility.py Outdated Show resolved Hide resolved

rescore/utility.py Outdated Show resolved Hide resolved

TuanAnh17N force-pushed the sast-rescoring-update branch from 9895c5f to 13f4f76 Compare December 16, 2024 16:44

ocm-ci-robot-0 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 16, 2024

ocm-ci-robot-0 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Dec 16, 2024

TuanAnh17N requested a review from zkdev December 16, 2024 16:48

TuanAnh17N force-pushed the sast-rescoring-update branch from 13f4f76 to 2bf0120 Compare December 17, 2024 09:10

ocm-ci-robot-0 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Dec 17, 2024

ccwienk requested changes Dec 17, 2024

View reviewed changes

cm06/scan.py Outdated Show resolved Hide resolved

cm06/scan.py Outdated Show resolved Hide resolved

features/__init__.py Outdated Show resolved Hide resolved

test/test_rescoring.py Outdated Show resolved Hide resolved

TuanAnh17N added 10 commits December 19, 2024 18:36

Extend unit-testing for sast

57678fe

Add support for sast rescoring deserialization

2db4ee3

Introduce sast rescoring

ecf4bc9

Add cm06 scan result uploading and automatic sast rescoring

fcbeee0

Integrate k8s config handling

3fd7959

Refactor rescore functions

f32ee5e

Introduce cm06 config dataclass and deserialisation function

221aaab

Refactor unit tests

a146b4c

Refactor SAST worker: modularize functions, update config, use new rules

baab207

introduce source-level handling and simplify version retrieval logic

4ca5d08

TuanAnh17N force-pushed the sast-rescoring-update branch from 2bf0120 to 4ca5d08 Compare December 19, 2024 17:53

ocm-ci-robot-0 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Dec 19, 2024

zkdev requested changes Dec 20, 2024

View reviewed changes

ccwienk requested changes Dec 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sast rescoring update #270

Sast rescoring update #270

TuanAnh17N commented Nov 27, 2024

zkdev Dec 12, 2024

zkdev Dec 20, 2024

ccwienk commented Dec 17, 2024

zkdev Dec 20, 2024

zkdev Dec 20, 2024

ccwienk Dec 20, 2024

zkdev Dec 20, 2024

ccwienk Dec 20, 2024

zkdev Dec 20, 2024

zkdev Dec 20, 2024

ccwienk Dec 20, 2024

zkdev Dec 20, 2024

ccwienk Dec 20, 2024

zkdev Dec 20, 2024

ccwienk Dec 20, 2024

zkdev Dec 20, 2024

ccwienk Dec 20, 2024

ccwienk Dec 20, 2024

ccwienk Dec 20, 2024

ccwienk Dec 20, 2024

ccwienk Dec 20, 2024

ccwienk Dec 20, 2024

ccwienk Dec 20, 2024

ccwienk Dec 20, 2024

ccwienk Dec 20, 2024

ccwienk Dec 20, 2024

Sast rescoring update #270

Are you sure you want to change the base?

Sast rescoring update #270

Conversation

TuanAnh17N commented Nov 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccwienk commented Dec 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment