Add pagination and sorting to image single table #5258

dvail · 2023-03-15T16:23:04Z

Description

Adds server side sorting and pagination to the image single page table.

Follow ups

Add appropriate sorting for all fields on the BE is complete and documented in the collab doc

Checklist

Investigated and inspected CI test results
Unit test and regression tests added
Evaluated and added CHANGELOG entry if required
Determined and documented upgrade steps
Documented user facing changes (create PR based on openshift/openshift-docs and merge into rhacs-docs)

If any of these don't apply, please comment below.

Testing Performed

Visit an image single page and verify that a single page worth's of data has loaded. The default sorting of the table should be by Severity, descending.

Use the pagination forward/back buttons to verify that pages of data are changed correctly.

Update the number of items per page and verify that the number of items is changed correctly.

Test that sorting by CVE, Severity, and CVE Status work correctly.

Verify that the CVEs by severity and CVEs by status cards have a total count matching that displayed at the top of the table.

openshift-ci · 2023-03-15T16:23:09Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

dvail · 2023-03-15T16:23:17Z

Current dependencies on/for this PR:

master
- PR Add pagination and sorting to image single table #5258 👈

This comment was auto-generated by Graphite.

roxbot · 2023-03-15T16:40:21Z

Images are ready for the commit at c65b3a8.

To use with deploy scripts, first export MAIN_IMAGE_TAG=3.74.x-415-gc65b3a8cd0.

dvail · 2023-03-15T19:39:26Z

ui/apps/platform/src/Containers/Vulnerabilities/WorkloadCves/ImageSingleVulnerabilities.tsx


-function severityCountsFromImageVulnerabilities(


This is from a proof-of-concept of client side pagination. Since we are not using client-side pagination, we can get this data directly from the query.

pedrottimark · 2023-03-16T20:19:57Z

ui/apps/platform/src/Containers/Vulnerabilities/WorkloadCves/ImageSingleVulnerabilities.tsx


    const [activeTabKey, setActiveTabKey] = useURLStringUnion('cveStatus', cveStatusTabValues);

    let mainContent: ReactNode | null = null;

+    const vulnerabilityData = data || previousData;


Is it possible to replace || with ?? nullish coalescing operator (what a name)

pedrottimark · 2023-03-16T20:23:51Z

ui/apps/platform/src/Containers/Vulnerabilities/WorkloadCves/ImageSingleVulnerabilities.tsx

        const hiddenSeverities = new Set<VulnerabilitySeverity>([]);
        const hiddenStatuses = new Set<FixableStatus>([]);


Can you share this at next team meeting?

Map and Set remove a security concern about overwriting Object properties.

Sure and I can share the useSet hook since it might be of use in other places in the app.

What's the security concern you are talking about in this context?

Whether possible (see below) or implausible, huge number of reported vulnerabilities are about possible object prototype pollution. Map and Set remove it as even a theoretical risk.

ljharb/qs#428

} else if (cleanRoot !== '__proto__') { obj[cleanRoot] = leaf; }

pedrottimark · 2023-03-16T20:35:18Z

.../platform/src/Containers/Vulnerabilities/WorkloadCves/SummaryCards/BySeveritySummaryCard.tsx

+    critical: 'CRITICAL_VULNERABILITY_SEVERITY',
+} as const;
+
+const severitiesCriticalToLow = ['critical', 'important', 'moderate', 'low'] as const;


Reader observations, not necessarily change request:

Make names more parallel severitiesCriticalToLow here and the following in hook?

export const imageVulnerabilityCounterKeys = ['low', 'moderate', 'important', 'critical'] as const;

Is the LowToCritical order ever used? At least some occurrences work either way, correct?

imageVulnerabilityCounterKeys.forEach(…)

severitiesCriticalToLow.forEach(…)

The idea here was to create a local variable that enforced the order specific to this component. I had thought about just making the default order in the hook CriticalToLow, but didn't want to have reliance on an order that is pretty much arbitrary in all other use cases.

pedrottimark · 2023-03-16T20:44:43Z

.../platform/src/Containers/Vulnerabilities/WorkloadCves/SummaryCards/BySeveritySummaryCard.tsx

-                        const hasNoResults = count === 0;
-                        const isHidden = hiddenSeverities.has(severity);
+                        const hasNoResults = count.total === 0;
+                        const vulnSeverity = vulnCounterToSeverity[severity];


Can you write a short comment (maybe above at vulnCounterToSeverity object) about the uses of the two severity keys?

Or, dare I ask, are the classic 'LOW_VULNERABILITY_SEVERITY' keys used in any query payloads or responses? At least in this immediate context, the keys seem to be under frontend control:

vulnerabilitySeverityLabels

SeverityIcons

There are definitely a handful of different representations of severity, both in this section and others. We're still deciding on how we want to standardize this data in this feature, at least as it pertains to mapping server responses to display values, etc.

Currently the API will return the LOW_VULNERABILITY_SEVERITY style keys in responses, and the BE team recommends using the long form in query payloads (even though not strictly necessary today).

I could alias the low/moderate/etc fields in the graphql resolver to this verbose key, which might simplify some UI code. Do you think semantically the two are equivalent?

Also, related, I found that we removed the UNKNOWN_VULNERABILITY_SEVERITY value from the front end type a while back. It looks like this is a valid response anywhere we could expect a severity from the BE though, and I've had a couple of cases of this in testing.

Do you see any reason to not add this back in? Possibly a discussion for the team meeting too.

That sounds familiar. Was me the we?

Almost certainly add it back, with discussion to remove any confusion within the team.

Can you search how much duplication of severity constants, icon, labels, and so on?

I have mixed feelings about pro and con:

Possibly move classic Vulnerability Management imports from global scope into container folder.

Encapsulate new 2.0 in its container folder with possibly duplication in case differences become needed.

There was some light discussion in this PR regarding UNKNOWN but I didn't see anything beyond that. I think adding it back will result in very few compile errors, which we can solve by adding empty strings in the worst case.

As for duplications:

It looks like the cve.proto.ts type VulnerabilitySeverity is referenced 24 times, mostly by other types/utils, which are then imported primarily (if not entirely) in VulnMgmt 1.0, Vuln Reporting 1.0, and Risk Acceptance.

We also have messages/common.ts with a const severityRatings that is used in Policies. This value uses the severity shorthand version. vulnSeverityLabels in the same file maps the long version to the shorthand, which I wouldn't really think is a problem but might be a good canonical value->display mapper.

severityColors.js has many different versions that map some severity to a color, I think mixing vuln severity with policy severity. It also contains a comment at the top // @TODO: Have one source of truth for severity colors from 4 years ago 😄

The new Workload CVEs intermixes the short style with the long style.

There are some .js files that use values like MODERATE_VULNERABILITY_SEVERITY and Moderate to refer to CVEs, but I'm less worried about those since they aren't typed and will either fall off with time, or become typed if they hang around.

There are also gql queries that reference resolvers like low, moderate, etc. that semantically seem like the same thing. We could alias those easily client side if we wanted, but we don't right now.

My general feeling is that "CVE Severity" is a concept that is used in enough places that it might be useful to keep it in a global types area. I'd be in favor of getting everything aligned with the current VulnerabilitySeverity since it is a direct mapping to the typed representation on the backend and would result in the least amount of moving code short-term. (Although too bad the API contract doesn't declare it, and we have to optimistically assume a string will be one of these values.)

Side note: Is types/node.proto.ts dead code? None of the types seem to be imported elsewhere. Transitively it looks like types/vulnerability.proto.ts might be dead code as well.

pedrottimark · 2023-03-16T20:46:25Z

...latform/src/Containers/Vulnerabilities/WorkloadCves/SummaryCards/CvesByStatusSummaryCard.tsx

+            imageVulnerabilityCounterKeys.forEach((key) => {
+                count += counts[key].fixable;
+            });
+            return `${count} vulnerabilities with available fixes`;


Is pluralize is relevant for vulnerabilities in these template literals?

pedrottimark

Another good-sized step forward. Some comments to consider.

openshift-ci · 2023-03-17T16:56:07Z

@dvail: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/gke-postgres-ui-e2e-tests	`c65b3a8`	link	false	`/test gke-postgres-ui-e2e-tests`
ci/prow/gke-ui-e2e-tests	`c65b3a8`	link	false	`/test gke-ui-e2e-tests`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-ci bot added the do-not-merge/work-in-progress label Mar 15, 2023

github-actions bot added the area/ui label Mar 15, 2023

dvail force-pushed the dv/ROX-15056-pagination-and-sorting-for-table branch 2 times, most recently from 770f3b1 to c6a3968 Compare March 15, 2023 19:26

dvail commented Mar 15, 2023

View reviewed changes

dvail added the epic/vm2.0/workload-cves label Mar 15, 2023

dvail requested review from alwayshooin and a team March 15, 2023 19:45

dvail marked this pull request as ready for review March 15, 2023 19:45

openshift-ci bot removed the do-not-merge/work-in-progress label Mar 15, 2023

dvail mentioned this pull request Mar 15, 2023

Add expandable section for image vulnerability components table #5218

Merged

5 tasks

pedrottimark reviewed Mar 16, 2023

View reviewed changes

pedrottimark approved these changes Mar 16, 2023

View reviewed changes

dvail added 2 commits March 17, 2023 11:40

Add pagination and sorting to image single table

d3e824e

PR feedback

c65b3a8

dvail force-pushed the dv/ROX-15056-pagination-and-sorting-for-table branch from c6a3968 to c65b3a8 Compare March 17, 2023 15:45

dvail merged commit 4cad570 into master Mar 17, 2023
55 checks passed

dvail deleted the dv/ROX-15056-pagination-and-sorting-for-table branch March 17, 2023 17:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pagination and sorting to image single table #5258

Add pagination and sorting to image single table #5258

dvail commented Mar 15, 2023 •

edited

openshift-ci bot commented Mar 15, 2023

dvail commented Mar 15, 2023

roxbot commented Mar 15, 2023 •

edited

dvail Mar 15, 2023

pedrottimark Mar 16, 2023

pedrottimark Mar 16, 2023

dvail Mar 17, 2023

pedrottimark Mar 17, 2023

pedrottimark Mar 16, 2023

dvail Mar 17, 2023

pedrottimark Mar 16, 2023

dvail Mar 17, 2023

dvail Mar 17, 2023

pedrottimark Mar 17, 2023

dvail Mar 17, 2023

pedrottimark Mar 16, 2023

pedrottimark left a comment

openshift-ci bot commented Mar 17, 2023

		const hiddenSeverities = new Set<VulnerabilitySeverity>([]);
		const hiddenStatuses = new Set<FixableStatus>([]);


		function severityCountsFromImageVulnerabilities(

Add pagination and sorting to image single table #5258

Add pagination and sorting to image single table #5258

Conversation

dvail commented Mar 15, 2023 • edited

Description

Follow ups

Checklist

Testing Performed

openshift-ci bot commented Mar 15, 2023

dvail commented Mar 15, 2023

roxbot commented Mar 15, 2023 • edited

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

pedrottimark left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Mar 17, 2023

dvail commented Mar 15, 2023 •

edited

roxbot commented Mar 15, 2023 •

edited