fix: managing cache state is now synchronous

ivanstanev · ivanstanev · commit 614fc72fa60a · 2022-11-30T15:02:07.000Z
We no longer use async when managing cache state because we suspect it spins the event loop and causes race conditions where the same image is scanned multiple times even though it should have been cached the first time.

Also, add debug logging to inspect the data in the cache.
diff --git a/src/scanner/index.ts b/src/scanner/index.ts
@@ -134,8 +134,8 @@ export async function scanImagesAndSendResults(
 
   // All workloads are identical, pick the first one
   const workload = workloadMetadata[0];
-  const workloadState = await getWorkloadAlreadyScanned(workload);
-  const imageState = await getWorkloadImageAlreadyScanned(
+  const workloadState = getWorkloadAlreadyScanned(workload);
+  const imageState = getWorkloadImageAlreadyScanned(
     workload,
     workload.imageName,
     workload.imageId,
diff --git a/src/state.ts b/src/state.ts
@@ -2,6 +2,7 @@ import { KubernetesObject, V1Namespace } from '@kubernetes/client-node';
 import LruCache from 'lru-cache';
 
 import { config } from './common/config';
+import { logger } from './common/logger';
 import { extractNamespaceName } from './supervisor/watchers/internal-namespaces';
 
 const imagesLruCacheOptions: LruCache.Options<string, Set<string>> = {
@@ -42,43 +43,62 @@ function getWorkloadImageAlreadyScannedKey(
   return `${workload.uid}/${imageName}`;
 }
 
-export async function getWorkloadAlreadyScanned(
+export function getWorkloadAlreadyScanned(
   workload: WorkloadAlreadyScanned,
-): Promise<string | undefined> {
+): string | undefined {
   const key = workload.uid;
   return state.workloadsAlreadyScanned.get(key);
 }
 
-export async function setWorkloadAlreadyScanned(
+export function setWorkloadAlreadyScanned(
   workload: WorkloadAlreadyScanned,
   revision: string,
-): Promise<boolean> {
+): boolean {
   const key = workload.uid;
   return state.workloadsAlreadyScanned.set(key, revision);
 }
 
-export async function deleteWorkloadAlreadyScanned(
+export function deleteWorkloadAlreadyScanned(
   workload: WorkloadAlreadyScanned,
-): Promise<void> {
+): void {
   const key = workload.uid;
   state.workloadsAlreadyScanned.del(key);
 }
 
-export async function getWorkloadImageAlreadyScanned(
+export function getWorkloadImageAlreadyScanned(
   workload: WorkloadAlreadyScanned,
   imageName: string,
   imageId: string,
-): Promise<string | undefined> {
+): string | undefined {
+  const cachedImages = state.imagesAlreadyScanned.dump().map((entry) => {
+    const values = new Array<string>();
+    for (const v in entry.v.values()) {
+      values.push(v);
+    }
+    return `entry: ${entry.e}, key: ${entry.k}; values: ${values.join(',')}`;
+  });
+  logger.debug(
+    { 'kubernetes-monitor': { cachedImages } },
+    'images in the cache',
+  );
+
   const key = getWorkloadImageAlreadyScannedKey(workload, imageName);
   const hasImageId = state.imagesAlreadyScanned.get(key)?.has(imageId);
-  return hasImageId ? imageId : undefined;
+  const response = hasImageId ? imageId : undefined;
+  if (response !== undefined) {
+    logger.debug(
+      { 'kubernetes-monitor': { imageId } },
+      'image already exists in cache',
+    );
+  }
+  return response;
 }
 
-export async function setWorkloadImageAlreadyScanned(
+export function setWorkloadImageAlreadyScanned(
   workload: WorkloadAlreadyScanned,
   imageName: string,
   imageId: string,
-): Promise<boolean> {
+): boolean {
   const key = getWorkloadImageAlreadyScannedKey(workload, imageName);
   const images = state.imagesAlreadyScanned.get(key);
   if (images !== undefined) {
@@ -92,9 +112,9 @@ export async function setWorkloadImageAlreadyScanned(
   return true;
 }
 
-export async function deleteWorkloadImagesAlreadyScanned(
+export function deleteWorkloadImagesAlreadyScanned(
   workload: WorkloadImagesAlreadyScanned,
-): Promise<void> {
+): void {
   for (const imageId of workload.imageIds) {
     const key = getWorkloadImageAlreadyScannedKey(workload, imageId);
     state.imagesAlreadyScanned.del(key);
diff --git a/src/supervisor/watchers/handlers/argo-rollout.ts b/src/supervisor/watchers/handlers/argo-rollout.ts
@@ -116,16 +116,14 @@ export async function argoRolloutWatchHandler(
   const workloadAlreadyScanned =
     kubernetesObjectToWorkloadAlreadyScanned(rollout);
   if (workloadAlreadyScanned !== undefined) {
-    await Promise.all([
-      deleteWorkloadAlreadyScanned(workloadAlreadyScanned),
-      deleteWorkloadImagesAlreadyScanned({
-        ...workloadAlreadyScanned,
-        imageIds: rollout.spec.template.spec.containers
-          .filter((container) => container.image !== undefined)
-          .map((container) => container.image!),
-      }),
-      deleteWorkloadFromScanQueue(workloadAlreadyScanned),
-    ]);
+    deleteWorkloadAlreadyScanned(workloadAlreadyScanned);
+    deleteWorkloadImagesAlreadyScanned({
+      ...workloadAlreadyScanned,
+      imageIds: rollout.spec.template.spec.containers
+        .filter((container) => container.image !== undefined)
+        .map((container) => container.image!),
+    });
+    deleteWorkloadFromScanQueue(workloadAlreadyScanned);
   }
 
   const workloadName = rollout.metadata.name || FALSY_WORKLOAD_NAME_MARKER;
diff --git a/src/supervisor/watchers/handlers/cron-job.ts b/src/supervisor/watchers/handlers/cron-job.ts
@@ -110,16 +110,14 @@ export async function cronJobWatchHandler(
   const workloadAlreadyScanned =
     kubernetesObjectToWorkloadAlreadyScanned(cronJob);
   if (workloadAlreadyScanned !== undefined) {
-    await Promise.all([
-      deleteWorkloadAlreadyScanned(workloadAlreadyScanned),
-      deleteWorkloadImagesAlreadyScanned({
-        ...workloadAlreadyScanned,
-        imageIds: cronJob.spec.jobTemplate.spec.template.spec.containers
-          .filter((container) => container.image !== undefined)
-          .map((container) => container.image!),
-      }),
-      deleteWorkloadFromScanQueue(workloadAlreadyScanned),
-    ]);
+    deleteWorkloadAlreadyScanned(workloadAlreadyScanned);
+    deleteWorkloadImagesAlreadyScanned({
+      ...workloadAlreadyScanned,
+      imageIds: cronJob.spec.jobTemplate.spec.template.spec.containers
+        .filter((container) => container.image !== undefined)
+        .map((container) => container.image!),
+    });
+    deleteWorkloadFromScanQueue(workloadAlreadyScanned);
   }
 
   const workloadName = cronJob.metadata.name || FALSY_WORKLOAD_NAME_MARKER;
diff --git a/src/supervisor/watchers/handlers/daemon-set.ts b/src/supervisor/watchers/handlers/daemon-set.ts
@@ -64,16 +64,14 @@ export async function daemonSetWatchHandler(
   const workloadAlreadyScanned =
     kubernetesObjectToWorkloadAlreadyScanned(daemonSet);
   if (workloadAlreadyScanned !== undefined) {
-    await Promise.all([
-      deleteWorkloadAlreadyScanned(workloadAlreadyScanned),
-      deleteWorkloadImagesAlreadyScanned({
-        ...workloadAlreadyScanned,
-        imageIds: daemonSet.spec.template.spec.containers
-          .filter((container) => container.image !== undefined)
-          .map((container) => container.image!),
-      }),
-      deleteWorkloadFromScanQueue(workloadAlreadyScanned),
-    ]);
+    deleteWorkloadAlreadyScanned(workloadAlreadyScanned);
+    deleteWorkloadImagesAlreadyScanned({
+      ...workloadAlreadyScanned,
+      imageIds: daemonSet.spec.template.spec.containers
+        .filter((container) => container.image !== undefined)
+        .map((container) => container.image!),
+    });
+    deleteWorkloadFromScanQueue(workloadAlreadyScanned);
   }
 
   const workloadName = daemonSet.metadata.name || FALSY_WORKLOAD_NAME_MARKER;
diff --git a/src/supervisor/watchers/handlers/deployment-config.ts b/src/supervisor/watchers/handlers/deployment-config.ts
@@ -116,16 +116,14 @@ export async function deploymentConfigWatchHandler(
   const workloadAlreadyScanned =
     kubernetesObjectToWorkloadAlreadyScanned(deploymentConfig);
   if (workloadAlreadyScanned !== undefined) {
-    await Promise.all([
-      deleteWorkloadAlreadyScanned(workloadAlreadyScanned),
-      deleteWorkloadImagesAlreadyScanned({
-        ...workloadAlreadyScanned,
-        imageIds: deploymentConfig.spec.template.spec.containers
-          .filter((container) => container.image !== undefined)
-          .map((container) => container.image!),
-      }),
-      deleteWorkloadFromScanQueue(workloadAlreadyScanned),
-    ]);
+    deleteWorkloadAlreadyScanned(workloadAlreadyScanned);
+    deleteWorkloadImagesAlreadyScanned({
+      ...workloadAlreadyScanned,
+      imageIds: deploymentConfig.spec.template.spec.containers
+        .filter((container) => container.image !== undefined)
+        .map((container) => container.image!),
+    });
+    deleteWorkloadFromScanQueue(workloadAlreadyScanned);
   }
 
   const workloadName =
diff --git a/src/supervisor/watchers/handlers/deployment.ts b/src/supervisor/watchers/handlers/deployment.ts
@@ -64,16 +64,14 @@ export async function deploymentWatchHandler(
   const workloadAlreadyScanned =
     kubernetesObjectToWorkloadAlreadyScanned(deployment);
   if (workloadAlreadyScanned !== undefined) {
-    await Promise.all([
-      deleteWorkloadAlreadyScanned(workloadAlreadyScanned),
-      deleteWorkloadImagesAlreadyScanned({
-        ...workloadAlreadyScanned,
-        imageIds: deployment.spec.template.spec.containers
-          .filter((container) => container.image !== undefined)
-          .map((container) => container.image!),
-      }),
-      deleteWorkloadFromScanQueue(workloadAlreadyScanned),
-    ]);
+    deleteWorkloadAlreadyScanned(workloadAlreadyScanned);
+    deleteWorkloadImagesAlreadyScanned({
+      ...workloadAlreadyScanned,
+      imageIds: deployment.spec.template.spec.containers
+        .filter((container) => container.image !== undefined)
+        .map((container) => container.image!),
+    });
+    deleteWorkloadFromScanQueue(workloadAlreadyScanned);
   }
 
   const workloadName = deployment.metadata.name || FALSY_WORKLOAD_NAME_MARKER;
diff --git a/src/supervisor/watchers/handlers/job.ts b/src/supervisor/watchers/handlers/job.ts
@@ -58,16 +58,14 @@ export async function jobWatchHandler(job: V1Job): Promise<void> {
 
   const workloadAlreadyScanned = kubernetesObjectToWorkloadAlreadyScanned(job);
   if (workloadAlreadyScanned !== undefined) {
-    await Promise.all([
-      deleteWorkloadAlreadyScanned(workloadAlreadyScanned),
-      deleteWorkloadImagesAlreadyScanned({
-        ...workloadAlreadyScanned,
-        imageIds: job.spec.template.spec.containers
-          .filter((container) => container.image !== undefined)
-          .map((container) => container.image!),
-      }),
-      deleteWorkloadFromScanQueue(workloadAlreadyScanned),
-    ]);
+    deleteWorkloadAlreadyScanned(workloadAlreadyScanned);
+    deleteWorkloadImagesAlreadyScanned({
+      ...workloadAlreadyScanned,
+      imageIds: job.spec.template.spec.containers
+        .filter((container) => container.image !== undefined)
+        .map((container) => container.image!),
+    });
+    deleteWorkloadFromScanQueue(workloadAlreadyScanned);
   }
 
   const workloadName = job.metadata.name || FALSY_WORKLOAD_NAME_MARKER;
diff --git a/src/supervisor/watchers/handlers/pod.ts b/src/supervisor/watchers/handlers/pod.ts
@@ -30,7 +30,7 @@ export async function handleReadyPod(
 ): Promise<void> {
   const workloadToScan: IWorkload[] = [];
   for (const workload of workloadMetadata) {
-    const scanned = await getWorkloadImageAlreadyScanned(
+    const scanned = getWorkloadImageAlreadyScanned(
       workload,
       workload.imageName,
       workload.imageId,
@@ -49,7 +49,7 @@ export async function handleReadyPod(
       { workloadToScan, imageId: workload.imageId },
       'image has not been scanned',
     );
-    await setWorkloadImageAlreadyScanned(
+    setWorkloadImageAlreadyScanned(
       workload,
       workload.imageName,
       workload.imageId,
@@ -147,12 +147,12 @@ export async function podWatchHandler(pod: V1Pod): Promise<void> {
     const workloadRevision = workloadMember.revision
       ? workloadMember.revision.toString()
       : '';
-    const scannedRevision = await getWorkloadAlreadyScanned(workloadMember);
+    const scannedRevision = getWorkloadAlreadyScanned(workloadMember);
     const isRevisionDifferent =
       scannedRevision === undefined ||
       Number(workloadRevision) > Number(scannedRevision);
     if (isRevisionDifferent) {
-      await setWorkloadAlreadyScanned(workloadMember, workloadRevision);
+      setWorkloadAlreadyScanned(workloadMember, workloadRevision);
       await sendWorkloadMetadata(workloadMetadataPayload);
     }
 
@@ -169,16 +169,14 @@ export async function podDeletedHandler(pod: V1Pod): Promise<void> {
 
   const workloadAlreadyScanned = kubernetesObjectToWorkloadAlreadyScanned(pod);
   if (workloadAlreadyScanned !== undefined) {
-    await Promise.all([
-      deleteWorkloadAlreadyScanned(workloadAlreadyScanned),
-      deleteWorkloadImagesAlreadyScanned({
-        ...workloadAlreadyScanned,
-        imageIds: pod.spec.containers
-          .filter((container) => container.image !== undefined)
-          .map((container) => container.image!),
-      }),
-      deleteWorkloadFromScanQueue(workloadAlreadyScanned),
-    ]);
+    deleteWorkloadAlreadyScanned(workloadAlreadyScanned);
+    deleteWorkloadImagesAlreadyScanned({
+      ...workloadAlreadyScanned,
+      imageIds: pod.spec.containers
+        .filter((container) => container.image !== undefined)
+        .map((container) => container.image!),
+    });
+    deleteWorkloadFromScanQueue(workloadAlreadyScanned);
   }
 
   const workloadName = pod.metadata.name || FALSY_WORKLOAD_NAME_MARKER;
diff --git a/src/supervisor/watchers/handlers/queue.ts b/src/supervisor/watchers/handlers/queue.ts
@@ -58,7 +58,7 @@ async function queueWorkerWorkloadScan(
       ...workloadMetadata[0],
       imageIds,
     };
-    await deleteWorkloadImagesAlreadyScanned(workload);
+    deleteWorkloadImagesAlreadyScanned(workload);
   }
 }
 
diff --git a/src/supervisor/watchers/handlers/replica-set.ts b/src/supervisor/watchers/handlers/replica-set.ts
@@ -65,16 +65,14 @@ export async function replicaSetWatchHandler(
   const workloadAlreadyScanned =
     kubernetesObjectToWorkloadAlreadyScanned(replicaSet);
   if (workloadAlreadyScanned !== undefined) {
-    await Promise.all([
-      deleteWorkloadAlreadyScanned(workloadAlreadyScanned),
-      deleteWorkloadImagesAlreadyScanned({
-        ...workloadAlreadyScanned,
-        imageIds: replicaSet.spec.template.spec.containers
-          .filter((container) => container.image !== undefined)
-          .map((container) => container.image!),
-      }),
-      deleteWorkloadFromScanQueue(workloadAlreadyScanned),
-    ]);
+    deleteWorkloadAlreadyScanned(workloadAlreadyScanned);
+    deleteWorkloadImagesAlreadyScanned({
+      ...workloadAlreadyScanned,
+      imageIds: replicaSet.spec.template.spec.containers
+        .filter((container) => container.image !== undefined)
+        .map((container) => container.image!),
+    });
+    deleteWorkloadFromScanQueue(workloadAlreadyScanned);
   }
 
   const workloadName = replicaSet.metadata.name || FALSY_WORKLOAD_NAME_MARKER;
diff --git a/src/supervisor/watchers/handlers/replication-controller.ts b/src/supervisor/watchers/handlers/replication-controller.ts
@@ -73,16 +73,14 @@ export async function replicationControllerWatchHandler(
     replicationController,
   );
   if (workloadAlreadyScanned !== undefined) {
-    await Promise.all([
-      deleteWorkloadAlreadyScanned(workloadAlreadyScanned),
-      deleteWorkloadImagesAlreadyScanned({
-        ...workloadAlreadyScanned,
-        imageIds: replicationController.spec.template.spec.containers
-          .filter((container) => container.image !== undefined)
-          .map((container) => container.image!),
-      }),
-      deleteWorkloadFromScanQueue(workloadAlreadyScanned),
-    ]);
+    deleteWorkloadAlreadyScanned(workloadAlreadyScanned);
+    deleteWorkloadImagesAlreadyScanned({
+      ...workloadAlreadyScanned,
+      imageIds: replicationController.spec.template.spec.containers
+        .filter((container) => container.image !== undefined)
+        .map((container) => container.image!),
+    });
+    deleteWorkloadFromScanQueue(workloadAlreadyScanned);
   }
 
   const workloadName =
diff --git a/src/supervisor/watchers/handlers/stateful-set.ts b/src/supervisor/watchers/handlers/stateful-set.ts
@@ -64,16 +64,14 @@ export async function statefulSetWatchHandler(
   const workloadAlreadyScanned =
     kubernetesObjectToWorkloadAlreadyScanned(statefulSet);
   if (workloadAlreadyScanned !== undefined) {
-    await Promise.all([
-      deleteWorkloadAlreadyScanned(workloadAlreadyScanned),
-      deleteWorkloadImagesAlreadyScanned({
-        ...workloadAlreadyScanned,
-        imageIds: statefulSet.spec.template.spec.containers
-          .filter((container) => container.image !== undefined)
-          .map((container) => container.image!),
-      }),
-      deleteWorkloadFromScanQueue(workloadAlreadyScanned),
-    ]);
+    deleteWorkloadAlreadyScanned(workloadAlreadyScanned);
+    deleteWorkloadImagesAlreadyScanned({
+      ...workloadAlreadyScanned,
+      imageIds: statefulSet.spec.template.spec.containers
+        .filter((container) => container.image !== undefined)
+        .map((container) => container.image!),
+    });
+    deleteWorkloadFromScanQueue(workloadAlreadyScanned);
   }
 
   const workloadName = statefulSet.metadata.name || FALSY_WORKLOAD_NAME_MARKER;
diff --git a/test/unit/scanner/scan-results-caching.spec.ts b/test/unit/scanner/scan-results-caching.spec.ts

Original file line number	Diff line number	Diff line change
`@@ -58,7 +58,7 @@ async function queueWorkerWorkloadScan(`
`58`	`58`	`...workloadMetadata[0],`
`59`	`59`	`imageIds,`
`60`	`60`	`};`
`61`		`- await deleteWorkloadImagesAlreadyScanned(workload);`
	`61`	`+ deleteWorkloadImagesAlreadyScanned(workload);`
`62`	`62`	`}`
`63`	`63`	`}`
`64`	`64`