ROX-9476 ROX-8533: Show health info of a local scanner by kovayur · Pull Request #1445 · stackrox/stackrox

kovayur · 2022-04-27T07:51:10Z

Description

Added local scanner health information to a cluster details page.

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 stackrox/openshift-docs and merge into rhacs-docs)

Testing Performed

Dev test on a local cluster

Screenshots

Secured cluster with `ROX_LOCAL_IMAGE_SCANNING_ENABLED=true`

Clusters screen:

Health Status widget:

Secured cluster with `ROX_LOCAL_IMAGE_SCANNING_ENABLED=false`

ghost · 2022-04-27T08:07:29Z

Tag for build #499964 is 3.69.x-541-gead56670fc.

💻 For deploying this image using the dev scripts, run the following first:

export MAIN_IMAGE_TAG='3.69.x-541-gead56670fc'

🕹️ A roxctl binary can be downloaded from the CircleCI artifacts.

RTann

Thank you for doing this! Just a few small things. Also, I'd recommend getting a reviewer from the UI team to take a look at the UI parts. Also, do you have screenshots of an example of this in action so we can see what it looks like?

RTann · 2022-04-28T17:08:28Z

CHANGELOG.md

 - ROX-9435: Updated dryrun API to generate preview violations for disabled policies
 - Support for security policies that do not have a policyVersion or have versions prior to 1.1 will be removed. If you have externally stored older policies, they cannot be imported.
 - ROX-10021: RHCOS node support is dropped until major improvements are made in ROX-8944.
+- ROX-8533: Show Cluster Health Info if Scanner is running on a secured cluster


I don't think this entry is needed since it's completely captured by a ticket. I tend to only add an entry if there is either no ticket and this adds something worth documenting (for the docs team) or there is a ticket and there is some nuance to the implementation that may not be clear in the ticket. I am guilty of the entry above this - ROX-10021: RHCOS node support is dropped until major improvements are made in ROX-8944., but the purpose of this entry it to let docs people know there are followup plans for this. Also, there is a followup entry which appends information about the UI (which is not really mentioned in the ticket) to the entry.

Thanks for the explanation. Entry removed.

RTann · 2022-04-28T17:21:58Z

proto/internalapi/central/cluster_status.proto

 message RawClusterHealthInfo {
    storage.CollectorHealthInfo collector_health_info                   = 1;
    storage.AdmissionControlHealthInfo admission_control_health_info    = 2;
+    storage.LocalScannerHealthInfo local_scanner_health_info            = 3;


Since I see it was adjusted in the proto below, should we align these variables, too?

Aligned in the whole file

RTann · 2022-04-28T17:23:33Z

proto/storage/cluster.proto


    HealthStatusLabel         overall_health_status             = 4 [(gogoproto.moretags) = "search:\"Cluster Status,store\""];
    HealthStatusLabel         admission_control_health_status   = 7 [(gogoproto.moretags) = "search:\"Admission Control Status,store\""];
+    HealthStatusLabel         local_scanner_health_status       = 11 [(gogoproto.moretags) = "search:\"Local Scanner Status,store\""];


can we add a comment to the top of this message to say which tag is next since the tags are out of order?

WDYT about adding a comment at the bottom of the post instead? Since we usually add new fields at the bottom, the comment will be more likely to be read.
Also, the comment placed at the top will be propagated to the swagger definitions.

RTann · 2022-04-28T17:25:04Z

sensor/kubernetes/clusterhealth/updater.go

+	if !env.LocalImageScanningEnabled.BooleanSetting() {
+		return nil
+	}
+	result := storage.LocalScannerHealthInfo{}


nit: var result storage.LocalScannerHealthInfo

RTann · 2022-04-28T17:26:08Z

sensor/kubernetes/clusterhealth/updater.go

+			TotalReadyAnalyzerPods: localScanner.Status.ReadyReplicas,
+		}
+	}
+	localScannerDb, err := u.client.AppsV1().Deployments(u.namespace).Get(u.ctx(), localScannerDbDeploymentName, metav1.GetOptions{})


nit: localScannerDB

RTann · 2022-04-28T17:26:45Z

sensor/kubernetes/clusterhealth/updater.go

 	admissionControlContainerName  = "admission-control"
+
+	localScannerDeploymentName   = "scanner"
+	localScannerDbDeploymentName = "scanner-db"


nit: localScannerDBDeploymentName

porridge

This looks awesome, but I think we should remove local in user-visible places, see inline.
Also a couple of questions:

did you perhaps manage to simulate a broken local scanner cert refresher component Start and see whether the error surfaces in the UI? Or is this PR just about pod health?
I also wanted to double-check what happens when local scanner is not supposed to be running? (Like in non-openshift clusters.) I think what should happen is that the scanner part of the health "pill" is simply not shown. A special sub-case is when secured cluster components run in the same cluster and namespace as central - in that case the local scanner does not get deployed, but there is a "central scanner" with the same name. We probably should not be showing its status together with other secured cluster components, right @RTann

porridge · 2022-04-29T06:52:47Z

CHANGELOG.md

 - ROX-9435: Updated dryrun API to generate preview violations for disabled policies
 - Support for security policies that do not have a policyVersion or have versions prior to 1.1 will be removed. If you have externally stored older policies, they cannot be imported.
 - ROX-10021: RHCOS node support is dropped until major improvements are made in ROX-8944.
+- ROX-8533: Show Cluster Health Info if Scanner is running on a secured cluster


porridge · 2022-04-29T06:59:18Z

pkg/search/options.go

 	SensorStatus           = newFieldLabel("Sensor Status")
 	CollectorStatus        = newFieldLabel("Collector Status")
 	AdmissionControlStatus = newFieldLabel("Admission Control Status")
+	LocalScannerStatus     = newFieldLabel("Local Scanner Status")


There was a product decision to show users just "Scanner", not "Local scanner" (here and elsewhere). I think the code-internal identifier names can stay as LocalScanner though.

Done as suggested.

It turned out to be a bit tricky because the label has to match the tag defined in the proto file. I am going to remove the word 'Local' from there.

porridge · 2022-04-29T07:01:41Z

proto/storage/cluster.proto

+        int32  total_desired_analyzer_pods = 1;
+    }
+    oneof total_ready_analyzer_pods_opt {
+        int32  total_ready_analyzer_pods = 2;
+    }
+    oneof total_desired_db_pods_opt {
+        int32 total_desired_db_pods = 3;
+    }
+    oneof total_ready_db_pods_opt {
+        int32  total_ready_db_pods = 4;


The spacing after int32 seems inconsistent.

porridge · 2022-04-29T07:02:55Z

proto/storage/cluster.proto

+
+    // Collection of errors that occurred while trying to obtain local scanner health info.
+    repeated string status_errors = 5;
+


Remove this blank line (and perhaps the one in AdmissionControlHealthInfo as well).

porridge · 2022-04-29T07:08:12Z

sensor/kubernetes/clusterhealth/updater.go

+		err = errors.Wrap(err, fmt.Sprintf("unable to find local scanner deployments in namespace %q", u.namespace))
+		result.StatusErrors = append(result.StatusErrors, fmt.Sprintf("unable to find local scanner deployments in namespace %q: %v", u.namespace, err))


Suggested change

err = errors.Wrap(err, fmt.Sprintf("unable to find local scanner deployments in namespace %q", u.namespace))

result.StatusErrors = append(result.StatusErrors, fmt.Sprintf("unable to find local scanner deployments in namespace %q: %v", u.namespace, err))

err = errors.Wrap(err, fmt.Sprintf("unable to find scanner deployment in namespace %q", u.namespace))

result.StatusErrors = append(result.StatusErrors, fmt.Sprintf("unable to find scanner deployment in namespace %q: %v", u.namespace, err))

Similar below for the DB case.

porridge · 2022-04-29T07:11:46Z

sensor/kubernetes/clusterhealth/updater.go

 	return &result
 }

+func (u *updaterImpl) getLocalScannerInfo() *storage.LocalScannerHealthInfo {


It's just itching to extract a generic function out of this once we have generics :-)

kovayur · 2022-04-29T15:12:06Z

Thank you for doing this! Just a few small things. Also, I'd recommend getting a reviewer from the UI team to take a look at the UI parts. Also, do you have screenshots of an example of this in action so we can see what it looks like?

@RTann Sure, I also added a few more screens to the description

kovayur · 2022-04-29T15:29:00Z

This looks awesome, but I think we should remove local in user-visible places, see inline. Also a couple of questions:

did you perhaps manage to simulate a broken local scanner cert refresher component Start and see whether the error surfaces in the UI? Or is this PR just about pod health?

To simplify the health check logic, I decided to leave only the health information of the scanner pods. If the cert refresher will not start successfully, the cert will expire and the corresponding pod will eventually become unhealthy. Of course, the indication of the component start failure will happen in advance and in some cases this may prevent an outage, but is it that critical?

I also wanted to double-check what happens when local scanner is not supposed to be running? (Like in non-openshift clusters.) I think what should happen is that the scanner part of the health "pill" is simply not shown. A special sub-case is when secured cluster components run in the same cluster and namespace as central - in that case the local scanner does not get deployed, but there is a "central scanner" with the same name. We probably should not be showing its status together with other secured cluster components, right @RTann

That's exactly how it has been implemented. The corresponding part of the health "pill" will not be shown if the local scanner is disabled (ROX_LOCAL_IMAGE_SCANNING_ENABLED=false). Thus, the mentioned special case works as expected (health info is not shown).

vjwilson

For the UI changes:

A couple of minor nits about code style.

More important is the lack of distinction in the design of "delayed" versus "unhealthy"

vjwilson · 2022-04-29T17:12:19Z

proto/internalapi/central/cluster_status.proto

+    storage.CollectorHealthInfo        collector_health_info            = 1;
    storage.AdmissionControlHealthInfo admission_control_health_info    = 2;
+    storage.ScannerHealthInfo          scanner_health_info              = 3;


Proto style reminds me of assembly ;)

LDX #$4000 BSR SAVEX * 7 CPU cycles (2 bytes) RTS * 5 CPU cycles (1 byte) * These two lines require 12 cycles and * 3 bytes ... SAVEX STX ,U++ RTS

vjwilson · 2022-04-29T17:21:57Z

ui/apps/platform/src/Containers/Clusters/cluster.helpers.tsx

+// Special case for Scanner when Sensor is UNHEALTHY or DELAYED.
+export const delayedScannerStatusStyle = {
+    Icon: InfoCircleIcon,
+    bgColor: 'bg-base-200',
+    fgColor: 'text-base-700',


This is a bit confusing.
The icon and color choice are neutral, which seems fair for "delayed" status, but unhealthy is represented in other widgets with yellow or red. Here's we're conflating the two.

nit: If we are going to conflate them, delayed doesn't convey that. Hard to come up with name that captures both nuances... "unsatisfactory" maybe?

This is used when the sensor status is delayed/unhealthy, not scanner.
When the sensor is unavailable, we can't distribute information about the status of the scanner, so we color it gray.
If the scanner is not available by itself, we color the status in red or green as in other cases.

TBH this was copy-pasted from the collector/admission controller health view, and it is probably worth using the same "delayed" style for all 3 cases, since they are exactly the same.

vjwilson · 2022-04-29T18:29:44Z

ui/apps/platform/src/Containers/Clusters/Components/Scanner/ScannerStatus.tsx

+import { DetailedTooltipOverlay, Tooltip } from '@stackrox/ui-components';
+import { ClusterHealthStatus } from '../../clusterTypes';
+import {
+    delayedScannerStatusStyle,
+    healthStatusStyles,
+    isDelayedSensorHealthStatus,
+} from '../../cluster.helpers';
+import HealthLabelWithDelayed from '../HealthLabelWithDelayed';
+import { getDistanceStrictAsPhrase } from '../../../../utils/dateUtils';


There are aliases for top-level directories, so you can avoid the deeply nested ancestor path for dateUtils by referencing it directly in the utils/ path.

(It also has to move up in the list, because of lint rule that "absolute" paths come first.)

Suggested change

import { DetailedTooltipOverlay, Tooltip } from '@stackrox/ui-components';

import { ClusterHealthStatus } from '../../clusterTypes';

import {

delayedScannerStatusStyle,

healthStatusStyles,

isDelayedSensorHealthStatus,

} from '../../cluster.helpers';

import HealthLabelWithDelayed from '../HealthLabelWithDelayed';

import { getDistanceStrictAsPhrase } from '../../../../utils/dateUtils';

import { DetailedTooltipOverlay, Tooltip } from '@stackrox/ui-components';

import { getDistanceStrictAsPhrase } from 'utils/dateUtils';

import { ClusterHealthStatus } from '../../clusterTypes';

import {

delayedScannerStatusStyle,

healthStatusStyles,

isDelayedSensorHealthStatus,

} from '../../cluster.helpers';

import HealthLabelWithDelayed from '../HealthLabelWithDelayed';

Thanks for the hint, fixed.

RTann

A few nits, but otherwise backend LGTM. Definitely make sure @porridge approves as well since I am probably not as familiar with the health status stuff

RTann · 2022-04-29T22:42:19Z

proto/storage/cluster.proto

    AdmissionControlHealthInfo admission_control_health_info    = 8;
+    // scanner_health_info is filled when the scanner is deployed on a secured cluster (so called "local scanner").
+    // Please do not confuse this with the default scanner deployment on a central cluster.
+    ScannerHealthInfo          scanner_health_info        = 10;


nit: I think the = 10 needs to be further to the right.

RTann · 2022-04-29T22:44:06Z

proto/storage/cluster.proto

+// derived from this data.
+// Aggregated scanner health status is not included because it is derived in central and not in the component that
+// first reports ScannerHealthInfo (sensor).
+message ScannerHealthInfo {


Is this also solely for the "local" Scanner? If so, can you add that comment here?

Comment added.

msugakov · 2022-05-02T08:26:23Z

ui/apps/platform/src/Containers/Clusters/Components/Scanner/ScannerStatusTotals.tsx

+            <tbody>
+                <tr className={trClassName} key="totalReadyPods">
+                    <th className={thClassName} scope="row">
+                        Scanner pods ready:


Suggestion: we can reduce verbosity and make things look a little nicer if we say Pods ready: instead of Scanner pods ready:, Version: instead of Collector version: and so on. What do you think?
It means you'd need to change titles for Collector and Admission Control but that shouldn't be hard.

Compare before

and after

Good idea. I was thinking about simplifying even more, like: "Replicas: 3/3". In any case, it seems that this is out of the scope of this PR.

porridge · 2022-05-05T10:31:00Z

did you perhaps manage to simulate a broken local scanner cert refresher component Start and see whether the error surfaces in the UI? Or is this PR just about pod health?

To simplify the health check logic, I decided to leave only the health information of the scanner pods. If the cert refresher will not start successfully, the cert will expire and the corresponding pod will eventually become unhealthy. Of course, the indication of the component start failure will happen in advance and in some cases this may prevent an outage, but is it that critical?

I'd say it's a prerequisite for closing ROX-9476 which was filed in response to my whining in that PR review.

I also wanted to double-check what happens when local scanner is not supposed to be running? (Like in non-openshift clusters.) I think what should happen is that the scanner part of the health "pill" is simply not shown. A special sub-case is when secured cluster components run in the same cluster and namespace as central - in that case the local scanner does not get deployed, but there is a "central scanner" with the same name. We probably should not be showing its status together with other secured cluster components, right @RTann

That's exactly how it has been implemented.

👌🏻

porridge

LGTM, but please don't let the bot close ROX-9476 yet as mentioned above.

kovayur force-pushed the yury/ROX-9476-sensor-error-handling-2 branch from 9b8538d to bfa0852 Compare April 27, 2022 11:45

github-actions bot added area/central area/sensor area/ui labels Apr 27, 2022

kovayur requested review from a team and RTann April 28, 2022 10:33

kovayur marked this pull request as ready for review April 28, 2022 10:34

kovayur added 3 commits April 28, 2022 12:42

ROX-9476 ROX-8533: Show health info of a local scanner

43dc6c9

Add border

2c0fd7b

ROX-8533: Add CHANGELOG entry

ae61e40

kovayur force-pushed the yury/ROX-9476-sensor-error-handling-2 branch from 3437b07 to ae61e40 Compare April 28, 2022 10:42

RTann reviewed Apr 28, 2022

View reviewed changes

porridge requested changes Apr 29, 2022

View reviewed changes

Address review comments

3bb3464

kovayur added 3 commits April 29, 2022 18:00

Remove the 'local' word from the proto definitions

156c21e

Remove the 'local' word from the proto definitions

73f016d

Remove the 'local' word from UI

a5a4077

kovayur requested a review from porridge April 29, 2022 16:56

vjwilson requested a review from a team April 29, 2022 17:02

vjwilson reviewed Apr 29, 2022

View reviewed changes

RTann approved these changes Apr 29, 2022

View reviewed changes

kovayur added 2 commits May 2, 2022 09:44

Address UI review comments

f231b3f

Address proto review comments

ead5667

msugakov reviewed May 2, 2022

View reviewed changes

porridge approved these changes May 5, 2022

View reviewed changes

kovayur merged commit addf58c into master May 5, 2022

kovayur deleted the yury/ROX-9476-sensor-error-handling-2 branch May 5, 2022 13:09


		// Collection of errors that occurred while trying to obtain local scanner health info.
		repeated string status_errors = 5;

		err = errors.Wrap(err, fmt.Sprintf("unable to find local scanner deployments in namespace %q", u.namespace))
		result.StatusErrors = append(result.StatusErrors, fmt.Sprintf("unable to find local scanner deployments in namespace %q: %v", u.namespace, err))

Conversation

kovayur commented Apr 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Testing Performed

Screenshots

Secured cluster with ROX_LOCAL_IMAGE_SCANNING_ENABLED=true

Secured cluster with ROX_LOCAL_IMAGE_SCANNING_ENABLED=false

Uh oh!

ghost commented Apr 27, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RTann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

porridge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kovayur commented Apr 29, 2022

Uh oh!

kovayur commented Apr 29, 2022

Uh oh!

vjwilson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kovayur commented Apr 27, 2022 •

edited

Loading

Secured cluster with `ROX_LOCAL_IMAGE_SCANNING_ENABLED=true`

Secured cluster with `ROX_LOCAL_IMAGE_SCANNING_ENABLED=false`

ghost commented Apr 27, 2022 •

edited by ghost

Loading

kovayur May 2, 2022 •

edited

Loading