fix: exemption race conditions

src/pepr/operator/controllers/exemptions/exemptions.ts

@@ -1,9 +1,14 @@
 import { Log } from "pepr";
-import { policies } from "../../../policies/index";
-import { ExemptionElement, Matcher, Policy, UDSExemption } from "../../crd";
-
-type StoredMatcher = Matcher & { owner: string };
-type PolicyMap = Map<Policy, StoredMatcher[]>;
+import { PolicyMap, StoredMatcher } from "../../../policies";
+import { ExemptionElement, Policy, UDSExemption } from "../../crd";
+
+export enum WatchPhase {
+  Added = "ADDED",
+  Modified = "MODIFIED",
+  Deleted = "DELETED",
+  Bookmark = "BOOKMARK",
+  Error = "ERROR",
+}
 
 const isSame = (a: StoredMatcher, b: StoredMatcher) => {
   return (
@@ -89,35 +94,18 @@ function deleteIfMatchersRemoved(
   }
 }
 
-// Iterate through local Map and update Store
-function updateStore(policyMap: PolicyMap) {
-  const { Store } = policies;
-  for (const [policy, matchers] of policyMap.entries()) {
-    Log.debug(`Updating uds policy ${policy} exemptions: ${JSON.stringify(matchers)}`);
-    Store.setItem(policy, JSON.stringify(matchers));
-  }
-}
-
-function setupPolicyMap() {
-  const { Store } = policies;
-  const policyMap: PolicyMap = new Map();
-  const policyList = Object.values(Policy);
-
-  for (const p of policyList) {
-    policyMap.set(p, JSON.parse(Store.getItem(p) || "[]"));
-  }
-
-  return { policyMap, policyList };
-}
-
 // Add Exemptions to Pepr store as "policy": "[{...matcher, owner: uid}]"
 //(Performance Optimization) Use local map to do aggregation before updating store
-export function processExemptions(exempt: UDSExemption) {
-  const { policyMap, policyList } = setupPolicyMap();
+export function processExemptions(
+  exempt: UDSExemption,
+  phase: WatchPhase,
+  exemptionMap: Map<Policy, StoredMatcher[]>,
+) {
   const currExemptMatchers: StoredMatcher[] = [];
   const ownerId = exempt.metadata?.uid || "";
 
   // Iterate through all policies -- important for removing exemptions if CR is updated
+  const policyList = Object.values(Policy);
   for (const p of policyList) {
     for (const e of exempt.spec?.exemptions ?? []) {
       currExemptMatchers.push({
@@ -126,35 +114,33 @@ export function processExemptions(exempt: UDSExemption) {
       });
 
       // Add if exemption has this policy in its list
-      addIfIncludesPolicy(p, policyMap, e, ownerId);
+      addIfIncludesPolicy(p, exemptionMap, e, ownerId);
 
       // Check if matcher no longer has this policy from previous CR version
-      deleteIfPolicyRemoved(p, policyMap, e, ownerId);
+      deleteIfPolicyRemoved(p, exemptionMap, e, ownerId);
     }
 
     // Check if policy should no longer have this matcher from previous CR version
-    deleteIfMatchersRemoved(p, policyMap, currExemptMatchers, ownerId);
+    deleteIfMatchersRemoved(p, exemptionMap, currExemptMatchers, ownerId);
   }
 
-  updateStore(policyMap);
+  if (phase === WatchPhase.Deleted) {
+    removeExemptions(exempt, exemptionMap);
+  }
 }
 
 //(Performance Optimization) Use local map to do aggregation before updating store
-export function removeExemptions(exempt: UDSExemption) {
-  const { policyMap } = setupPolicyMap();
-
+export function removeExemptions(exempt: UDSExemption, exemptionMap: Map<Policy, StoredMatcher[]>) {
   Log.debug(`Removing policy exemptions for ${exempt.metadata?.name}`);
 
   // Loop through exemptions and remove matchers from policies in the local map
   for (const e of exempt.spec?.exemptions ?? []) {
     for (const p of e.policies) {
-      const matchers = policyMap.get(p) || [];
+      const matchers = exemptionMap.get(p) || [];
       const filteredList = matchers.filter(m => {
         if (!isSame(m, { ...e.matcher, owner: exempt.metadata?.uid || "" })) return m;
       });
-      policyMap.set(p, filteredList);
+      exemptionMap.set(p, filteredList);
     }
   }
-
-  updateStore(policyMap);
 }

src/pepr/operator/crd/generated/exemption-v1alpha1.ts

@@ -4,7 +4,6 @@ import { GenericKind, RegisterKind } from "kubernetes-fluent-client";
 
 export class Exemption extends GenericKind {
   spec?: Spec;
-  status?: Status;
 }
 
 export interface Spec {
@@ -64,18 +63,6 @@ export enum Policy {
   RestrictVolumeTypes = "RestrictVolumeTypes",
 }
 
-export interface Status {
-  observedGeneration?: number;
-  phase?: Phase;
-  titles?: string[];
-}
-
-export enum Phase {
-  Failed = "Failed",
-  Pending = "Pending",
-  Ready = "Ready",
-}
-
 RegisterKind(Exemption, {
   group: "uds.dev",
   version: "v1alpha1",

src/pepr/operator/crd/index.ts

@@ -12,8 +12,6 @@ export {
 } from "./generated/package-v1alpha1";
 
 export {
-  Phase as ExemptPhase,
-  Status as ExemptStatus,
   ExemptionElement,
   Matcher,
   Kind as MatcherKind,

src/pepr/operator/crd/sources/exemption/v1alpha1.ts

@@ -4,51 +4,10 @@ export const v1alpha1: V1CustomResourceDefinitionVersion = {
   name: "v1alpha1",
   served: true,
   storage: true,
-  additionalPrinterColumns: [
-    {
-      name: "Status",
-      type: "string",
-      description: "The status of the exemption",
-      jsonPath: ".status.phase",
-    },
-    {
-      name: "Exemptions",
-      type: "string",
-      description: "Titles of the exemptions",
-      jsonPath: ".status.titles",
-    },
-    {
-      name: "Age",
-      type: "date",
-      description: "The age of the exemption",
-      jsonPath: ".metadata.creationTimestamp",
-    },
-  ],
-  subresources: {
-    status: {},
-  },
   schema: {
     openAPIV3Schema: {
       type: "object",
       properties: {
-        status: {
-          type: "object",
-          properties: {
-            observedGeneration: {
-              type: "integer",
-            },
-            phase: {
-              enum: ["Pending", "Ready", "Failed"],
-              type: "string",
-            },
-            titles: {
-              type: "array",
-              items: {
-                type: "string",
-              },
-            },
-          },
-        } as V1JSONSchemaProps,
         spec: {
           type: "object",
           properties: {

src/pepr/operator/index.ts

@@ -3,7 +3,6 @@ import { a } from "pepr";
 import { When } from "./common";
 
 // Controller imports
-import { removeExemptions } from "./controllers/exemptions/exemptions";
 import { cleanupNamespace } from "./controllers/istio/injection";
 import { purgeSSOClients } from "./controllers/keycloak/client-sync";
 import {
@@ -14,11 +13,11 @@ import {
 
 // CRD imports
 import { UDSExemption, UDSPackage } from "./crd";
-import { exemptValidator } from "./crd/validators/exempt-validator";
 import { validator } from "./crd/validators/package-validator";
 
 // Reconciler imports
-import { exemptReconciler } from "./reconcilers/exempt-reconciler";
+import { startExemptionWatch } from "../policies";
+import { exemptValidator } from "./crd/validators/exempt-validator";
 import { packageReconciler } from "./reconcilers/package-reconciler";
 
 // Export the operator capability for registration in the root pepr.ts
@@ -61,8 +60,8 @@ When(UDSPackage)
   // Enqueue the package for processing
   .Reconcile(packageReconciler);
 
-//Watch for changes to the UDSExemption CRD and cleanup exemptions in policies Store
-When(UDSExemption).IsDeleted().Watch(removeExemptions);
+// Watch for Exemptions and validate
+When(UDSExemption).IsCreatedOrUpdated().Validate(exemptValidator);
 
-// Watch for changes to the UDSExemption CRD to enqueue an exemption for processing
-When(UDSExemption).IsCreatedOrUpdated().Validate(exemptValidator).Reconcile(exemptReconciler);
+// KFC watch for exemptions and update in-memory map
+void startExemptionWatch();

src/pepr/operator/reconcilers/index.spec.ts

@@ -4,7 +4,7 @@ import { K8s, Log, kind } from "pepr";
 
 import { Mock } from "jest-mock";
 import { handleFailure, shouldSkip, updateStatus, writeEvent } from ".";
-import { ExemptStatus, Phase, PkgStatus, UDSExemption, UDSPackage } from "../crd";
+import { Phase, PkgStatus, UDSPackage } from "../crd";
 
 jest.mock("pepr", () => ({
   K8s: jest.fn(),
@@ -77,17 +77,6 @@ describe("updateStatus", () => {
       status,
     });
   });
-
-  it("should update the status of an exemption", async () => {
-    const cr = { kind: "Exemption", metadata: { name: "test", namespace: "default" } };
-    const status = { phase: Phase.Ready };
-    await updateStatus(cr as GenericKind, status as ExemptStatus);
-    expect(K8s).toHaveBeenCalledWith(UDSExemption);
-    expect(PatchStatus).toHaveBeenCalledWith({
-      metadata: { name: "test", namespace: "default" },
-      status,
-    });
-  });
 });
 
 describe("writeEvent", () => {
@@ -149,7 +138,7 @@ describe("handleFailure", () => {
   it("should handle a 404 error", async () => {
     const err = { status: 404, message: "Not found" };
     const cr = { metadata: { namespace: "default", name: "test" } };
-    await handleFailure(err, cr as UDSPackage | UDSExemption);
+    await handleFailure(err, cr as UDSPackage);
     expect(Log.warn).toHaveBeenCalledWith({ err }, "Package metadata seems to have been deleted");
     expect(Create).not.toHaveBeenCalled();
   });
@@ -161,7 +150,7 @@ describe("handleFailure", () => {
       apiVersion: "v1",
       metadata: { namespace: "default", name: "test", generation: 1, uid: "1" },
     };
-    await handleFailure(err, cr as UDSPackage | UDSExemption);
+    await handleFailure(err, cr as UDSPackage);
     expect(Log.error).toHaveBeenCalledWith({ err }, "Error configuring default/test");
 
     expect(Create).toHaveBeenCalledWith({

src/pepr/operator/reconcilers/index.ts

@@ -1,7 +1,7 @@
 import { GenericKind } from "kubernetes-fluent-client";
 import { K8s, Log, kind } from "pepr";
 
-import { ExemptStatus, Phase, PkgStatus, UDSExemption, UDSPackage } from "../crd";
+import { Phase, PkgStatus, UDSPackage } from "../crd";
 import { Status } from "../crd/generated/package-v1alpha1";
 
 const uidSeen = new Set<string>();
@@ -12,7 +12,7 @@ const uidSeen = new Set<string>();
  * @param cr The custom resource to check
  * @returns true if the CRD is pending or the current generation has been processed
  */
-export function shouldSkip(cr: UDSExemption | UDSPackage) {
+export function shouldSkip(cr: GenericKind) {
   const isPending = cr.status?.phase === Phase.Pending;
   const isCurrentGeneration = cr.metadata?.generation === cr.status?.observedGeneration;
 
@@ -41,12 +41,11 @@ export function shouldSkip(cr: UDSExemption | UDSPackage) {
  * @param cr The custom resource to update
  * @param status The new status
  */
-export async function updateStatus(cr: GenericKind, status: PkgStatus | ExemptStatus) {
-  const model = cr.kind === "Package" ? UDSPackage : UDSExemption;
+export async function updateStatus(cr: UDSPackage, status: PkgStatus) {
   Log.debug(cr.metadata, `Updating status to ${status.phase}`);
 
   // Update the status of the CRD
-  await K8s(model).PatchStatus({
+  await K8s(UDSPackage).PatchStatus({
     metadata: {
       name: cr.metadata!.name,
       namespace: cr.metadata!.namespace,
@@ -62,7 +61,7 @@ export async function updateStatus(cr: GenericKind, status: PkgStatus | ExemptSt
  * @param message A human-readable message for the event
  * @param type The type of event to write
  */
-export async function writeEvent(cr: GenericKind, event: Partial<kind.CoreEvent>) {
+export async function writeEvent(cr: UDSPackage, event: Partial<kind.CoreEvent>) {
   Log.debug(cr.metadata, `Writing event: ${event.message}`);
 
   await K8s(kind.CoreEvent).Create({
@@ -93,10 +92,7 @@ export async function writeEvent(cr: GenericKind, event: Partial<kind.CoreEvent>
  * @param err The error-like object
  * @param cr The custom resource that failed
  */
-export async function handleFailure(
-  err: { status: number; message: string },
-  cr: UDSPackage | UDSExemption,
-) {
+export async function handleFailure(err: { status: number; message: string }, cr: UDSPackage) {
   const metadata = cr.metadata!;
   const identifier = `${metadata.namespace}/${metadata.name}`;
 

src/pepr/policies/exemptions/index.ts

@@ -1,7 +1,7 @@
 import { KubernetesObject } from "kubernetes-fluent-client";
 import { Log, PeprMutateRequest, PeprValidateRequest } from "pepr";
+import { policyExemptionMap } from "..";
 import { Policy } from "../../operator/crd";
-import { Store } from "../common";
 
 /**
  * Check a resource against an exemption list for use by the validation action.
@@ -14,7 +14,7 @@ export function isExempt<T extends KubernetesObject>(
   request: PeprValidateRequest<T> | PeprMutateRequest<T>,
   policy: Policy,
 ) {
-  const exemptList = JSON.parse(Store.getItem(policy) || "[]");
+  const exemptList = policyExemptionMap.get(policy) || [];
 
   // Loop through the exempt list
   for (const exempt of exemptList) {