Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add pagination and sorting to image single table #5258

Merged
merged 2 commits into from Mar 17, 2023
Merged

Add pagination and sorting to image single table #5258

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d3e824e
Add pagination and sorting to image single table
dvail Mar 13, 2023
c65b3a8
PR feedback
dvail Mar 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
102 changes: 48 additions & 54 deletions ui/apps/platform/src/Containers/Vulnerabilities/WorkloadCves/ImageSingleVulnerabilities.tsx
View file
Open in desktop
Expand Up @@ -11,6 +11,7 @@ import {
GridItem,
Label,
PageSection,
Pagination,
pluralize,
Spinner,
Split,
Expand All @@ -28,52 +29,24 @@ import { VulnerabilitySeverity } from 'types/cve.proto';
import { getAxiosErrorMessage } from 'utils/responseErrorUtils';
import useURLStringUnion from 'hooks/useURLStringUnion';
import useURLSearch from 'hooks/useURLSearch';
import useURLPagination from 'hooks/useURLPagination';
import useURLSort, { UseURLSortProps } from 'hooks/useURLSort';
import { getHasSearchApplied } from 'utils/searchUtils';
import { cveStatusTabValues, FixableStatus } from './types';
import WorkloadTableToolbar from './WorkloadTableToolbar';
import BySeveritySummaryCard from './SummaryCards/BySeveritySummaryCard';
import CvesByStatusSummaryCard from './SummaryCards/CvesByStatusSummaryCard';
import SingleEntityVulnerabilitiesTable from './Tables/SingleEntityVulnerabilitiesTable';
import useImageVulnerabilities, {
ImageVulnerabilitiesResponse,
} from './hooks/useImageVulnerabilities';
import { ImageDetailsResponse } from './hooks/useImageDetails';
import useImageVulnerabilities from './hooks/useImageVulnerabilities';

function severityCountsFromImageVulnerabilities(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

imageVulnerabilities: ImageVulnerabilitiesResponse['image']['imageVulnerabilities']
): Record<VulnerabilitySeverity, number> {
const severityCounts = {
LOW_VULNERABILITY_SEVERITY: 0,
MODERATE_VULNERABILITY_SEVERITY: 0,
IMPORTANT_VULNERABILITY_SEVERITY: 0,
CRITICAL_VULNERABILITY_SEVERITY: 0,
};

imageVulnerabilities.forEach(({ severity }) => {
severityCounts[severity] += 1;
});

return severityCounts;
}

function statusCountsFromImageVulnerabilities(
imageVulnerabilities: ImageVulnerabilitiesResponse['image']['imageVulnerabilities']
): Record<FixableStatus, number> {
const statusCounts = {
Fixable: 0,
'Not fixable': 0,
};

imageVulnerabilities.forEach(({ isFixable }) => {
if (isFixable) {
statusCounts.Fixable += 1;
} else {
statusCounts['Not fixable'] += 1;
}
});

return statusCounts;
}
const defaultSortOptions: UseURLSortProps = {
sortFields: ['CVE', 'Severity', 'Fixable'],
defaultSortOption: {
field: 'Severity',
direction: 'desc',
},
};

export type ImageSingleVulnerabilitiesProps = {
imageId: string;
Expand All @@ -82,13 +55,23 @@ export type ImageSingleVulnerabilitiesProps = {

function ImageSingleVulnerabilities({ imageId, imageData }: ImageSingleVulnerabilitiesProps) {
const { searchFilter } = useURLSearch();
const { page, perPage, setPage, setPerPage } = useURLPagination(50);
// TODO Need to reset current page at the same time sorting changes
const { sortOption, getSortParams } = useURLSort(defaultSortOptions);
// TODO Still need to properly integrate search filter with query
const { data, loading, error } = useImageVulnerabilities(imageId, {});
const pagination = {
offset: (page - 1) * perPage,
limit: perPage,
sortOption,
};
const { data, previousData, loading, error } = useImageVulnerabilities(imageId, {}, pagination);

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

let mainContent: ReactNode | null = null;

const vulnerabilityData = data ?? previousData;

if (error) {
mainContent = (
<Bullseye>
Expand All @@ -102,34 +85,35 @@ function ImageSingleVulnerabilities({ imageId, imageData }: ImageSingleVulnerabi
</EmptyState>
</Bullseye>
);
} else if (loading && !data) {
} else if (loading && !vulnerabilityData) {
mainContent = (
<Bullseye>
<Spinner isSVG />
</Bullseye>
);
} else if (data) {
const vulnerabilities = data.image.imageVulnerabilities;
const severityCounts = severityCountsFromImageVulnerabilities(vulnerabilities);
const cveStatusCounts = statusCountsFromImageVulnerabilities(vulnerabilities);
} else if (vulnerabilityData) {
const vulnerabilities = vulnerabilityData.image.imageVulnerabilities;

// TODO Integrate these with page search filters
const hiddenSeverities = new Set<VulnerabilitySeverity>([]);
const hiddenStatuses = new Set<FixableStatus>([]);
Comment on lines 98 to 99
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you share this at next team meeting?

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}


const totalVulnerabilityCount = vulnerabilityData.image.imageVulnerabilityCounter.all.total;

mainContent = (
<>
<div className="pf-u-px-lg pf-u-pb-lg">
<Grid hasGutter>
<GridItem sm={12} md={6} xl2={4}>
<BySeveritySummaryCard
title="CVEs by severity"
severityCounts={severityCounts}
severityCounts={vulnerabilityData.image.imageVulnerabilityCounter}
hiddenSeverities={hiddenSeverities}
/>
</GridItem>
<GridItem sm={12} md={6} xl2={4}>
<CvesByStatusSummaryCard
cveStatusCounts={cveStatusCounts}
cveStatusCounts={vulnerabilityData.image.imageVulnerabilityCounter}
hiddenStatuses={hiddenStatuses}
/>
</GridItem>
Expand All @@ -141,12 +125,7 @@ function ImageSingleVulnerabilities({ imageId, imageData }: ImageSingleVulnerabi
<SplitItem isFilled>
<Flex alignContent={{ default: 'alignContentCenter' }}>
<Title headingLevel="h2">
{pluralize(
data.image.imageVulnerabilities.length,
'result',
'results'
)}{' '}
found
{pluralize(totalVulnerabilityCount, 'result', 'results')} found
</Title>
{getHasSearchApplied(searchFilter) && (
<Label isCompact color="blue" icon={<InfoCircleIcon />}>
Expand All @@ -155,11 +134,26 @@ function ImageSingleVulnerabilities({ imageId, imageData }: ImageSingleVulnerabi
)}
</Flex>
</SplitItem>
<SplitItem>TODO Pagination</SplitItem>
<SplitItem>
<Pagination
isCompact
itemCount={totalVulnerabilityCount}
page={page}
perPage={perPage}
onSetPage={(_, newPage) => setPage(newPage)}
onPerPageSelect={(_, newPerPage) => {
if (totalVulnerabilityCount < (page - 1) * newPerPage) {
setPage(1);
}
setPerPage(newPerPage);
}}
/>
</SplitItem>
</Split>
<SingleEntityVulnerabilitiesTable
image={imageData}
imageVulnerabilities={data.image.imageVulnerabilities}
imageVulnerabilities={vulnerabilities}
getSortParams={getSortParams}
/>
</div>
</>
Expand Down
35 changes: 22 additions & 13 deletions ...atform/src/Containers/Vulnerabilities/WorkloadCves/SummaryCards/BySeveritySummaryCard.tsx
View file
Open in desktop
Expand Up @@ -5,19 +5,26 @@ import SeverityIcons from 'Components/PatternFly/SeverityIcons';

import { VulnerabilitySeverity } from 'types/cve.proto';
import { vulnerabilitySeverityLabels } from 'messages/common';
import { SVGIconProps } from '@patternfly/react-icons/dist/esm/createIcon';
import {
ImageVulnerabilityCounter,
ImageVulnerabilityCounterKey,
} from '../hooks/useImageVulnerabilities';

export type BySeveritySummaryCardProps = {
title: string;
severityCounts: Record<VulnerabilitySeverity, number>;
severityCounts: ImageVulnerabilityCounter;
hiddenSeverities: Set<VulnerabilitySeverity>;
};

const severitiesCriticalToLow = [
'CRITICAL_VULNERABILITY_SEVERITY',
'IMPORTANT_VULNERABILITY_SEVERITY',
'MODERATE_VULNERABILITY_SEVERITY',
'LOW_VULNERABILITY_SEVERITY',
] as const;
const vulnCounterToSeverity: Record<ImageVulnerabilityCounterKey, VulnerabilitySeverity> = {
low: 'LOW_VULNERABILITY_SEVERITY',
moderate: 'MODERATE_VULNERABILITY_SEVERITY',
important: 'IMPORTANT_VULNERABILITY_SEVERITY',
critical: 'CRITICAL_VULNERABILITY_SEVERITY',
} as const;

const severitiesCriticalToLow = ['critical', 'important', 'moderate', 'low'] as const;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reader observations, not necessarily change request:

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

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

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

    • imageVulnerabilityCounterKeys.forEach(…)
    • severitiesCriticalToLow.forEach(…)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


const disabledColor100 = 'var(--pf-global--disabled-color--100)';
const disabledColor200 = 'var(--pf-global--disabled-color--200)';
Expand All @@ -34,11 +41,12 @@ function BySeveritySummaryCard({
<Grid className="pf-u-pl-sm">
{severitiesCriticalToLow.map((severity) => {
const count = severityCounts[severity];
const hasNoResults = count === 0;
const isHidden = hiddenSeverities.has(severity);
const hasNoResults = count.total === 0;
const vulnSeverity = vulnCounterToSeverity[severity];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const isHidden = hiddenSeverities.has(vulnSeverity);

let textColor = '';
let text = `${count} ${vulnerabilitySeverityLabels[severity]}`;
let text = `${count.total} ${vulnerabilitySeverityLabels[vulnSeverity]}`;

if (isHidden) {
textColor = disabledColor100;
Expand All @@ -48,16 +56,17 @@ function BySeveritySummaryCard({
text = 'No results';
}

const Icon = SeverityIcons[severity];
const Icon: React.FC<SVGIconProps> | undefined =
SeverityIcons[vulnSeverity];

return (
<GridItem key={severity} span={6}>
<GridItem key={vulnSeverity} span={6}>
<Flex
className="pf-u-pt-sm"
spaceItems={{ default: 'spaceItemsSm' }}
alignItems={{ default: 'alignItemsCenter' }}
>
<Icon color={hasNoResults ? textColor : undefined} />
{Icon && <Icon color={hasNoResults ? textColor : undefined} />}
<Text style={{ color: textColor }}>{text}</Text>
</Flex>
</GridItem>
Expand Down
35 changes: 29 additions & 6 deletions ...form/src/Containers/Vulnerabilities/WorkloadCves/SummaryCards/CvesByStatusSummaryCard.tsx
View file
Open in desktop
@@ -1,11 +1,24 @@
import React from 'react';
import { Card, CardTitle, CardBody, Flex, Text, Grid, GridItem } from '@patternfly/react-core';
import {
Card,
CardTitle,
CardBody,
Flex,
Grid,
GridItem,
pluralize,
Text,
} from '@patternfly/react-core';

import { CheckCircleIcon, ExclamationCircleIcon } from '@patternfly/react-icons';
import { FixableStatus } from '../types';
import {
ImageVulnerabilityCounter,
imageVulnerabilityCounterKeys,
} from '../hooks/useImageVulnerabilities';

export type CvesByStatusSummaryCardProps = {
cveStatusCounts: Record<FixableStatus, number | 'hidden'>;
cveStatusCounts: ImageVulnerabilityCounter;
hiddenStatuses: Set<FixableStatus>;
};

Expand All @@ -14,15 +27,25 @@ const statusDisplays = [
status: 'Fixable',
Icon: CheckCircleIcon,
iconColor: 'var(--pf-global--success-color--100)',
text: (counts: CvesByStatusSummaryCardProps['cveStatusCounts']) =>
`${counts.Fixable} vulnerabilities with available fixes`,
text: (counts: ImageVulnerabilityCounter) => {
let count = 0;
imageVulnerabilityCounterKeys.forEach((key) => {
count += counts[key].fixable;
});
return `${pluralize(count, 'vulnerability', 'vulnerabilities')} with available fixes`;
},
},
{
status: 'Not fixable',
Icon: ExclamationCircleIcon,
iconColor: 'var(--pf-global--danger-color--100)',
text: (counts: CvesByStatusSummaryCardProps['cveStatusCounts']) =>
`${counts['Not fixable']} vulnerabilities without fixes`,
text: (counts: ImageVulnerabilityCounter) => {
let count = 0;
imageVulnerabilityCounterKeys.forEach((key) => {
count += counts[key].total - counts[key].fixable;
});
return `${count} vulnerabilities without fixes`;
},
},
] as const;

Expand Down
10 changes: 7 additions & 3 deletions ...m/src/Containers/Vulnerabilities/WorkloadCves/Tables/SingleEntityVulnerabilitiesTable.tsx
View file
Open in desktop
Expand Up @@ -17,6 +17,7 @@ import SeverityIcons from 'Components/PatternFly/SeverityIcons';
import useSet from 'hooks/useSet';
import { vulnerabilitySeverityLabels } from 'messages/common';
import { getDistanceStrictAsPhrase } from 'utils/dateUtils';
import { UseURLSortResult } from 'hooks/useURLSort';
import { ImageVulnerabilitiesResponse } from '../hooks/useImageVulnerabilities';
import { getEntityPagePath } from '../searchUtils';
import ImageComponentsTable from './ImageComponentsTable';
Expand All @@ -25,21 +26,24 @@ import { ImageDetailsResponse } from '../hooks/useImageDetails';
export type SingleEntityVulnerabilitiesTableProps = {
image: ImageDetailsResponse['image'] | undefined;
imageVulnerabilities: ImageVulnerabilitiesResponse['image']['imageVulnerabilities'];
getSortParams: UseURLSortResult['getSortParams'];
};

function SingleEntityVulnerabilitiesTable({
image,
imageVulnerabilities,
getSortParams,
}: SingleEntityVulnerabilitiesTableProps) {
const expandedRowSet = useSet<string>();
return (
<TableComposable>
<Thead>
<Tr>
<Th>{/* Header for expanded column */}</Th>
<Th>CVE</Th>
<Th>Severity</Th>
<Th>CVE status</Th>
<Th sort={getSortParams('CVE')}>CVE</Th>
<Th sort={getSortParams('Severity')}>Severity</Th>
<Th sort={getSortParams('Fixable')}>CVE Status</Th>
{/* TODO Add sorting for these columns once aggregate sorting is available in BE */}
<Th>Affected components</Th>
<Th>First discovered</Th>
</Tr>
Expand Down