fix: validate unique names for monitors (#666)

## Description Added a new piece to the pepr operator's validator function. The new functionality loops through the Package CR's monitor array, adding each description field (or a dummy string in the event of no provided description) to a set, failing given a duplicate. The sanitizeResourceName() function was used in the validator since its also used downstream to create the ServiceMonitor resource name based on the string from the monitor.description field. ... ## Related Issue Fixes #656 ## Type of change - [x] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Other (security config, docs update, etc) ## Checklist before merging - [x] Test, docs, adr added or updated as needed - [x] [Contributor Guide](https://github.com/defenseunicorns/uds-template-capability/blob/main/CONTRIBUTING.md) followed --------- Co-authored-by: Micah Nagel <[email protected]>
defenseunicorns · Jan 6, 2025 · 80e28c1 · 80e28c1
1 parent 5a46e41
commit 80e28c1
Show file tree

Hide file tree

Showing 2 changed files with 120 additions and 7 deletions.
diff --git a/src/pepr/operator/crd/validators/package-validator.spec.ts b/src/pepr/operator/crd/validators/package-validator.spec.ts
@@ -5,14 +5,25 @@
 
 import { afterEach, describe, expect, it, jest } from "@jest/globals";
 import { PeprValidateRequest } from "pepr";
-import { Allow, Direction, Expose, Gateway, Protocol, RemoteGenerated, Sso, UDSPackage } from "..";
+import {
+  Allow,
+  Direction,
+  Expose,
+  Gateway,
+  Monitor,
+  Protocol,
+  RemoteGenerated,
+  Sso,
+  UDSPackage,
+} from "..";
 import { validator } from "./package-validator";
 
 const makeMockReq = (
   pkg: Partial<UDSPackage>,
   exposeList: Partial<Expose>[],
   allowList: Partial<Allow>[],
   ssoClients: Partial<Sso>[],
+  monitorList: Partial<Monitor>[],
 ) => {
   const defaultPkg: UDSPackage = {
     metadata: {
@@ -25,6 +36,7 @@ const makeMockReq = (
         allow: [],
       },
       sso: [],
+      monitor: [],
     },
   };
 
@@ -51,6 +63,16 @@ const makeMockReq = (
     defaultPkg.spec!.sso?.push({ ...defaultClient, ...client });
   }
 
+  for (const monitor of monitorList) {
+    const defaultMonitor: Monitor = {
+      description: "Default Monitor",
+      portName: "http-metrics",
+      selector: {},
+      targetPort: 8080,
+    };
+    defaultPkg.spec!.monitor?.push({ ...defaultMonitor, ...monitor });
+  }
+
   return {
     Raw: { ...defaultPkg, ...pkg },
     Approve: jest.fn(),
@@ -64,13 +86,13 @@ describe("Test validation of Exemption CRs", () => {
   });
 
   it("allows packages that have no issues", async () => {
-    const mockReq = makeMockReq({}, [{}], [{}], [{}]);
+    const mockReq = makeMockReq({}, [{}], [{}], [{}], [{}]);
     await validator(mockReq);
     expect(mockReq.Approve).toHaveBeenCalledTimes(1);
   });
 
   it("denies system namespaces", async () => {
-    const mockReq = makeMockReq({ metadata: { namespace: "kube-system" } }, [], [], []);
+    const mockReq = makeMockReq({ metadata: { namespace: "kube-system" } }, [], [], [], []);
     await validator(mockReq);
     expect(mockReq.Deny).toHaveBeenCalledTimes(1);
   });
@@ -88,6 +110,7 @@ describe("Test validation of Exemption CRs", () => {
       ],
       [],
       [],
+      [],
     );
     await validator(mockReq);
     expect(mockReq.Deny).toHaveBeenCalledTimes(1);
@@ -106,6 +129,7 @@ describe("Test validation of Exemption CRs", () => {
       ],
       [],
       [],
+      [],
     );
     await validator(mockReq);
     expect(mockReq.Deny).toHaveBeenCalledTimes(1);
@@ -124,6 +148,7 @@ describe("Test validation of Exemption CRs", () => {
       ],
       [],
       [],
+      [],
     );
     await validator(mockReq);
     expect(mockReq.Deny).toHaveBeenCalledTimes(1);
@@ -142,6 +167,7 @@ describe("Test validation of Exemption CRs", () => {
       ],
       [],
       [],
+      [],
     );
     await validator(mockReq);
     expect(mockReq.Deny).toHaveBeenCalledTimes(1);
@@ -160,13 +186,14 @@ describe("Test validation of Exemption CRs", () => {
       ],
       [],
       [],
+      [],
     );
     await validator(mockReq);
     expect(mockReq.Deny).toHaveBeenCalledTimes(1);
   });
 
   it("denies virtual services that are the same name", async () => {
-    const mockReq = makeMockReq({}, [{}, {}], [], []);
+    const mockReq = makeMockReq({}, [{}, {}], [], [], []);
     await validator(mockReq);
     expect(mockReq.Deny).toHaveBeenCalledTimes(1);
   });
@@ -182,6 +209,7 @@ describe("Test validation of Exemption CRs", () => {
         },
       ],
       [],
+      [],
     );
     await validator(mockReq);
     expect(mockReq.Deny).toHaveBeenCalledTimes(1);
@@ -198,19 +226,20 @@ describe("Test validation of Exemption CRs", () => {
         },
       ],
       [],
+      [],
     );
     await validator(mockReq);
     expect(mockReq.Deny).toHaveBeenCalledTimes(1);
   });
 
   it("denies network policies that are the same name", async () => {
-    const mockReq = makeMockReq({}, [], [{}, {}], []);
+    const mockReq = makeMockReq({}, [], [{}, {}], [], []);
     await validator(mockReq);
     expect(mockReq.Deny).toHaveBeenCalledTimes(1);
   });
 
   it("denies clients with clientIDs that are not unique", async () => {
-    const mockReq = makeMockReq({}, [], [], [{}, {}]);
+    const mockReq = makeMockReq({}, [], [], [{}, {}], []);
     await validator(mockReq);
     expect(mockReq.Deny).toHaveBeenCalledTimes(1);
   });
@@ -225,6 +254,7 @@ describe("Test validation of Exemption CRs", () => {
           secretName: "HELLO_KITTEH",
         },
       ],
+      [],
     );
     await validator(mockReq);
     expect(mockReq.Deny).toHaveBeenCalledTimes(1);
@@ -240,6 +270,7 @@ describe("Test validation of Exemption CRs", () => {
           redirectUris: undefined,
         },
       ],
+      [],
     );
     await validator(mockReq);
     expect(mockReq.Deny).toHaveBeenCalledTimes(1);
@@ -256,6 +287,7 @@ describe("Test validation of Exemption CRs", () => {
           redirectUris: undefined,
         },
       ],
+      [],
     );
     await validator(mockReq);
     expect(mockReq.Approve).toHaveBeenCalledTimes(1);
@@ -273,6 +305,7 @@ describe("Test validation of Exemption CRs", () => {
           standardFlowEnabled: true,
         },
       ],
+      [],
     );
     await validator(mockReq);
     expect(mockReq.Deny).toHaveBeenCalledTimes(1);
@@ -289,6 +322,7 @@ describe("Test validation of Exemption CRs", () => {
           serviceAccountsEnabled: true,
         },
       ],
+      [],
     );
     await validator(mockReq);
     expect(mockReq.Deny).toHaveBeenCalledTimes(1);
@@ -305,6 +339,7 @@ describe("Test validation of Exemption CRs", () => {
           serviceAccountsEnabled: true,
         },
       ],
+      [],
     );
     await validator(mockReq);
     expect(mockReq.Deny).toHaveBeenCalledTimes(1);
@@ -323,6 +358,7 @@ describe("Test validation of Exemption CRs", () => {
           secret: "app-client-secret",
         },
       ],
+      [],
     );
     await validator(mockReq);
     expect(mockReq.Deny).toHaveBeenCalledTimes(1);
@@ -341,6 +377,7 @@ describe("Test validation of Exemption CRs", () => {
           secretName: "app-k8s-secret",
         },
       ],
+      [],
     );
     await validator(mockReq);
     expect(mockReq.Deny).toHaveBeenCalledTimes(1);
@@ -359,6 +396,7 @@ describe("Test validation of Exemption CRs", () => {
           secretTemplate: {},
         },
       ],
+      [],
     );
     await validator(mockReq);
     expect(mockReq.Deny).toHaveBeenCalledTimes(1);
@@ -377,6 +415,7 @@ describe("Test validation of Exemption CRs", () => {
           enableAuthserviceSelector: {},
         },
       ],
+      [],
     );
     await validator(mockReq);
     expect(mockReq.Deny).toHaveBeenCalledTimes(1);
@@ -395,6 +434,7 @@ describe("Test validation of Exemption CRs", () => {
           protocol: Protocol.Saml,
         },
       ],
+      [],
     );
     await validator(mockReq);
     expect(mockReq.Deny).toHaveBeenCalledTimes(1);
@@ -411,6 +451,7 @@ describe("Test validation of Exemption CRs", () => {
           standardFlowEnabled: false,
         },
       ],
+      [],
     );
     await validator(mockReq);
     expect(mockReq.Deny).toHaveBeenCalledTimes(1);
@@ -428,6 +469,7 @@ describe("Test validation of Exemption CRs", () => {
           standardFlowEnabled: false,
         },
       ],
+      [],
     );
     await validator(mockReq);
     expect(mockReq.Approve).toHaveBeenCalledTimes(1);
@@ -444,6 +486,7 @@ describe("Test validation of Exemption CRs", () => {
           standardFlowEnabled: false,
         },
       ],
+      [],
     );
     await validator(mockReq);
     expect(mockReq.Approve).toHaveBeenCalledTimes(1);
@@ -462,6 +505,7 @@ describe("Test validation of Exemption CRs", () => {
           },
         },
       ],
+      [],
     );
     await validator(mockReq);
     expect(mockReq.Deny).toHaveBeenCalledTimes(1);
@@ -478,6 +522,7 @@ describe("Test validation of Exemption CRs", () => {
           enableAuthserviceSelector: undefined, // explicitly undefined
         },
       ],
+      [],
     );
     await validator(mockReq);
     expect(mockReq.Approve).toHaveBeenCalledTimes(1);
@@ -501,6 +546,7 @@ describe("Test Allowed SSO Client Attributes", () => {
           },
         },
       ],
+      [],
     );
     await validator(mockReq);
     expect(mockReq.Deny).toHaveBeenCalledTimes(1);
@@ -535,6 +581,7 @@ describe("Test Allowed SSO Client Attributes", () => {
           },
         },
       ],
+      [],
     );
     await validator(mockReq);
     expect(mockReq.Approve).toHaveBeenCalledTimes(1);
@@ -553,6 +600,7 @@ describe("Test Allowed SSO Client Attributes", () => {
           },
         },
       ],
+      [],
     );
     await validator(mockReq);
     expect(mockReq.Deny).toHaveBeenCalledTimes(1);
@@ -571,14 +619,48 @@ describe("Test Allowed SSO Client Attributes", () => {
           attributes: {},
         },
       ],
+      [],
     );
     await validator(mockReq);
     expect(mockReq.Approve).toHaveBeenCalledTimes(1);
   });
 
   it("allows clients with no attributes defined", async () => {
-    const mockReq = makeMockReq({}, [], [], [{}]);
+    const mockReq = makeMockReq({}, [], [], [{}], []);
     await validator(mockReq);
     expect(mockReq.Approve).toHaveBeenCalledTimes(1);
   });
 });
+
+describe("Test proper generation of a unique name for service monitors", () => {
+  afterEach(() => {
+    jest.resetAllMocks();
+  });
+
+  it("given an undefined description, a unique serviceMonitor name should be generated using the selector and portName fields", async () => {
+    const mockReq = makeMockReq(
+      {},
+      [],
+      [],
+      [],
+      [
+        { description: undefined, portName: "http-foo", selector: { key: "foo" } },
+        { description: undefined, portName: "http-bar", selector: { key: "bar" } },
+      ],
+    );
+    await validator(mockReq);
+    expect(mockReq.Deny).toHaveBeenCalledTimes(0);
+  });
+
+  it("denies monitors that do not have unique descriptions", async () => {
+    const mockReq = makeMockReq(
+      {},
+      [],
+      [],
+      [],
+      [{ description: "Metrics" }, { description: "Metrics" }],
+    );
+    await validator(mockReq);
+    expect(mockReq.Deny).toHaveBeenCalledTimes(1);
+  });
+});
diff --git a/src/pepr/operator/crd/validators/package-validator.ts b/src/pepr/operator/crd/validators/package-validator.ts
@@ -7,8 +7,10 @@ import { PeprValidateRequest } from "pepr";
 
 import { Gateway, Protocol, UDSPackage } from "..";
 import { generateVSName } from "../../controllers/istio/virtual-service";
+import { generateMonitorName } from "../../controllers/monitoring/common";
 import { generateName } from "../../controllers/network/generate";
 import { sanitizeResourceName } from "../../controllers/utils";
+import { Kind } from "../../crd/generated/package-v1alpha1";
 import { migrate } from "../migrate";
 
 const invalidNamespaces = ["kube-system", "kube-public", "_unknown_", "pepr-system"];
@@ -186,5 +188,34 @@ export async function validator(req: PeprValidateRequest<UDSPackage>) {
     }
   }
 
+  const monitors = pkg.spec?.monitor ?? [];
+
+  // Ensure service and pod monitors use a unique description or selector/portName used for generating the resource name
+  const podMonitorNames = new Set<string>();
+  const svcMonitorNames = new Set<string>();
+
+  for (const monitor of monitors) {
+    const monitorName = generateMonitorName(pkgName, monitor);
+    if (monitor.kind === Kind.PodMonitor) {
+      if (podMonitorNames.has(monitorName)) {
+        return req.Deny(
+          `The combination of characteristics of this monitor entry would create a duplicate PodMonitor. ` +
+            `Verify you do not have duplicate values, or add a unique "description" field for this monitor. ` +
+            `The duplicate rule would be named "${monitorName}".`,
+        );
+      }
+      podMonitorNames.add(monitorName);
+    } else {
+      if (svcMonitorNames.has(monitorName)) {
+        return req.Deny(
+          `The combination of characteristics of this monitor entry would create a duplicate ServiceMonitor. ` +
+            `Verify you do not have duplicate values, or add a unique "description" field for this monitor. ` +
+            `The duplicate rule would be named "${monitorName}".`,
+        );
+      }
+      svcMonitorNames.add(monitorName);
+    }
+  }
+
   return req.Approve();
 }