From b7fe919c21cb997c0368503cd55c2724ee2fff87 Mon Sep 17 00:00:00 2001 From: Andy Godish Date: Tue, 13 Aug 2024 08:11:30 -0600 Subject: [PATCH 1/6] added validator for checking that each array item in 'monitors' has a unique desc --- .../operator/crd/validators/package-validator.ts | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/pepr/operator/crd/validators/package-validator.ts b/src/pepr/operator/crd/validators/package-validator.ts index a7684c10a..d675ec2af 100644 --- a/src/pepr/operator/crd/validators/package-validator.ts +++ b/src/pepr/operator/crd/validators/package-validator.ts @@ -115,5 +115,20 @@ export async function validator(req: PeprValidateRequest) { } } + const monitors = pkg.spec?.monitor ?? []; + + // Ensure the descriptions are generating unique names + const monitorDescriptions = new Set(); + + for (const monitor of monitors) { + const description = sanitizeResourceName(monitor.description || "null"); + + if (monitorDescriptions.has(description)) { + return req.Deny(`The description "${description}" is not unique`); + } + + monitorDescriptions.add(description); + } + return req.Approve(); } From ee3b54fe79d449a3dc8e15a9847f9b167c71beee Mon Sep 17 00:00:00 2001 From: Andy Godish Date: Tue, 13 Aug 2024 08:56:02 -0600 Subject: [PATCH 2/6] added partial monitorList array input to makeMockReq and fixed subequent missing parameters in existing calls to makeMockReq --- .../crd/validators/package-validator.spec.ts | 42 ++++++++++++++++--- .../crd/validators/package-validator.ts | 2 +- 2 files changed, 37 insertions(+), 7 deletions(-) diff --git a/src/pepr/operator/crd/validators/package-validator.spec.ts b/src/pepr/operator/crd/validators/package-validator.spec.ts index daa3d7035..d46396e08 100644 --- a/src/pepr/operator/crd/validators/package-validator.spec.ts +++ b/src/pepr/operator/crd/validators/package-validator.spec.ts @@ -1,6 +1,6 @@ import { afterEach, describe, expect, it, jest } from "@jest/globals"; import { PeprValidateRequest } from "pepr"; -import { Gateway, Expose, UDSPackage, Allow, Sso, Direction, RemoteGenerated, Protocol } from ".."; +import { Gateway, Expose, UDSPackage, Allow, Sso, Direction, RemoteGenerated, Protocol, Monitor } from ".."; import { validator } from "./package-validator"; const makeMockReq = ( @@ -8,6 +8,7 @@ const makeMockReq = ( exposeList: Partial[], allowList: Partial[], ssoClients: Partial[], + monitorList: Partial[], ) => { const defaultPkg: UDSPackage = { metadata: { @@ -20,6 +21,7 @@ const makeMockReq = ( allow: [], }, sso: [], + monitor: [], }, }; @@ -46,6 +48,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(), @@ -59,13 +71,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); }); @@ -83,6 +95,7 @@ describe("Test validation of Exemption CRs", () => { ], [], [], + [], ); await validator(mockReq); expect(mockReq.Deny).toHaveBeenCalledTimes(1); @@ -101,6 +114,7 @@ describe("Test validation of Exemption CRs", () => { ], [], [], + [], ); await validator(mockReq); expect(mockReq.Deny).toHaveBeenCalledTimes(1); @@ -119,6 +133,7 @@ describe("Test validation of Exemption CRs", () => { ], [], [], + [], ); await validator(mockReq); expect(mockReq.Deny).toHaveBeenCalledTimes(1); @@ -137,6 +152,7 @@ describe("Test validation of Exemption CRs", () => { ], [], [], + [], ); await validator(mockReq); expect(mockReq.Deny).toHaveBeenCalledTimes(1); @@ -155,13 +171,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); }); @@ -177,6 +194,7 @@ describe("Test validation of Exemption CRs", () => { }, ], [], + [], ); await validator(mockReq); expect(mockReq.Deny).toHaveBeenCalledTimes(1); @@ -193,19 +211,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); }); @@ -220,6 +239,7 @@ describe("Test validation of Exemption CRs", () => { secretName: "HELLO_KITTEH", }, ], + [], ); await validator(mockReq); expect(mockReq.Deny).toHaveBeenCalledTimes(1); @@ -235,6 +255,7 @@ describe("Test validation of Exemption CRs", () => { redirectUris: undefined, }, ], + [], ); await validator(mockReq); expect(mockReq.Deny).toHaveBeenCalledTimes(1); @@ -251,6 +272,7 @@ describe("Test validation of Exemption CRs", () => { redirectUris: undefined, }, ], + [], ); await validator(mockReq); expect(mockReq.Approve).toHaveBeenCalledTimes(1); @@ -268,6 +290,7 @@ describe("Test validation of Exemption CRs", () => { standardFlowEnabled: true, }, ], + [], ); await validator(mockReq); expect(mockReq.Deny).toHaveBeenCalledTimes(1); @@ -286,6 +309,7 @@ describe("Test validation of Exemption CRs", () => { secret: "app-client-secret", }, ], + [], ); await validator(mockReq); expect(mockReq.Deny).toHaveBeenCalledTimes(1); @@ -304,6 +328,7 @@ describe("Test validation of Exemption CRs", () => { secretName: "app-k8s-secret", }, ], + [], ); await validator(mockReq); expect(mockReq.Deny).toHaveBeenCalledTimes(1); @@ -322,6 +347,7 @@ describe("Test validation of Exemption CRs", () => { secretTemplate: {}, }, ], + [], ); await validator(mockReq); expect(mockReq.Deny).toHaveBeenCalledTimes(1); @@ -340,6 +366,7 @@ describe("Test validation of Exemption CRs", () => { enableAuthserviceSelector: {}, }, ], + [], ); await validator(mockReq); expect(mockReq.Deny).toHaveBeenCalledTimes(1); @@ -358,6 +385,7 @@ describe("Test validation of Exemption CRs", () => { protocol: Protocol.Saml, }, ], + [], ); await validator(mockReq); expect(mockReq.Deny).toHaveBeenCalledTimes(1); @@ -374,6 +402,7 @@ describe("Test validation of Exemption CRs", () => { standardFlowEnabled: false, }, ], + [], ); await validator(mockReq); expect(mockReq.Deny).toHaveBeenCalledTimes(1); @@ -391,6 +420,7 @@ describe("Test validation of Exemption CRs", () => { standardFlowEnabled: false, }, ], + [], ); await validator(mockReq); expect(mockReq.Approve).toHaveBeenCalledTimes(1); diff --git a/src/pepr/operator/crd/validators/package-validator.ts b/src/pepr/operator/crd/validators/package-validator.ts index d675ec2af..4b13fa97e 100644 --- a/src/pepr/operator/crd/validators/package-validator.ts +++ b/src/pepr/operator/crd/validators/package-validator.ts @@ -124,7 +124,7 @@ export async function validator(req: PeprValidateRequest) { const description = sanitizeResourceName(monitor.description || "null"); if (monitorDescriptions.has(description)) { - return req.Deny(`The description "${description}" is not unique`); + return req.Deny(`The sanitized description "${description}" is not unique`); } monitorDescriptions.add(description); From 6a30f219b7cd029a9e7fe8a8ce153f483140d4e9 Mon Sep 17 00:00:00 2001 From: Andy Godish Date: Tue, 13 Aug 2024 09:36:54 -0600 Subject: [PATCH 3/6] added unit tests for non-unique monitor descriptions --- .../crd/validators/package-validator.spec.ts | 32 +++++++++++++++++-- .../crd/validators/package-validator.ts | 6 ++-- 2 files changed, 33 insertions(+), 5 deletions(-) diff --git a/src/pepr/operator/crd/validators/package-validator.spec.ts b/src/pepr/operator/crd/validators/package-validator.spec.ts index d46396e08..8060698f6 100644 --- a/src/pepr/operator/crd/validators/package-validator.spec.ts +++ b/src/pepr/operator/crd/validators/package-validator.spec.ts @@ -1,6 +1,16 @@ import { afterEach, describe, expect, it, jest } from "@jest/globals"; import { PeprValidateRequest } from "pepr"; -import { Gateway, Expose, UDSPackage, Allow, Sso, Direction, RemoteGenerated, Protocol, Monitor } from ".."; +import { + Gateway, + Expose, + UDSPackage, + Allow, + Sso, + Direction, + RemoteGenerated, + Protocol, + Monitor, +} from ".."; import { validator } from "./package-validator"; const makeMockReq = ( @@ -53,7 +63,7 @@ const makeMockReq = ( description: "Default Monitor", portName: "http-metrics", selector: {}, - targetPort: 8080, + targetPort: 8080, }; defaultPkg.spec!.monitor?.push({ ...defaultMonitor, ...monitor }); } @@ -425,4 +435,22 @@ describe("Test validation of Exemption CRs", () => { await validator(mockReq); expect(mockReq.Approve).toHaveBeenCalledTimes(1); }); + + it("denies monitors with undefined descriptions", async () => { + const mockReq = makeMockReq({}, [], [], [], [{}, {}]); + await validator(mockReq); + expect(mockReq.Deny).toHaveBeenCalledTimes(1); + }); + + 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 index 4b13fa97e..81b29db70 100644 --- a/src/pepr/operator/crd/validators/package-validator.ts +++ b/src/pepr/operator/crd/validators/package-validator.ts @@ -116,17 +116,17 @@ export async function validator(req: PeprValidateRequest) { } const monitors = pkg.spec?.monitor ?? []; - + // Ensure the descriptions are generating unique names const monitorDescriptions = new Set(); for (const monitor of monitors) { const description = sanitizeResourceName(monitor.description || "null"); - + if (monitorDescriptions.has(description)) { return req.Deny(`The sanitized description "${description}" is not unique`); } - + monitorDescriptions.add(description); } From cd1f9c3e0797235132cc5ec39f8b27d6d38f0ef3 Mon Sep 17 00:00:00 2001 From: Andy Godish Date: Tue, 20 Aug 2024 21:46:23 -0600 Subject: [PATCH 4/6] updated validator to reflect the same logic as the name generator function --- .../crd/validators/package-validator.spec.ts | 27 +++++++++---------- .../crd/validators/package-validator.ts | 12 ++++----- 2 files changed, 19 insertions(+), 20 deletions(-) diff --git a/src/pepr/operator/crd/validators/package-validator.spec.ts b/src/pepr/operator/crd/validators/package-validator.spec.ts index 8060698f6..7c63b73b0 100644 --- a/src/pepr/operator/crd/validators/package-validator.spec.ts +++ b/src/pepr/operator/crd/validators/package-validator.spec.ts @@ -1,16 +1,6 @@ import { afterEach, describe, expect, it, jest } from "@jest/globals"; import { PeprValidateRequest } from "pepr"; -import { - Gateway, - Expose, - UDSPackage, - Allow, - Sso, - Direction, - RemoteGenerated, - Protocol, - Monitor, -} from ".."; +import { Gateway, Expose, UDSPackage, Allow, Sso, Direction, RemoteGenerated, Protocol, Monitor } from ".." import { validator } from "./package-validator"; const makeMockReq = ( @@ -436,10 +426,19 @@ describe("Test validation of Exemption CRs", () => { expect(mockReq.Approve).toHaveBeenCalledTimes(1); }); - it("denies monitors with undefined descriptions", async () => { - const mockReq = makeMockReq({}, [], [], [], [{}, {}]); + 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(1); + expect(mockReq.Deny).toHaveBeenCalledTimes(0); }); it("denies monitors that do not have unique descriptions", async () => { diff --git a/src/pepr/operator/crd/validators/package-validator.ts b/src/pepr/operator/crd/validators/package-validator.ts index 81b29db70..13ad3bbdd 100644 --- a/src/pepr/operator/crd/validators/package-validator.ts +++ b/src/pepr/operator/crd/validators/package-validator.ts @@ -117,17 +117,17 @@ export async function validator(req: PeprValidateRequest) { const monitors = pkg.spec?.monitor ?? []; - // Ensure the descriptions are generating unique names - const monitorDescriptions = new Set(); + // Ensure serviceMonitors use a unique description or selector/portName used for generating the resource name + const monitorNames = new Set(); for (const monitor of monitors) { - const description = sanitizeResourceName(monitor.description || "null"); + const monitorName = sanitizeResourceName(monitor.description || `${Object.values(monitor.selector)}-${monitor.portName}`); - if (monitorDescriptions.has(description)) { - return req.Deny(`The sanitized description "${description}" is not unique`); + if (monitorNames.has(monitorName)) { + return req.Deny(`A serviceMonitor resource generated from the Package, ${pkg.metadata?.name} contains a duplicate name, please provide a unique description for each item in the monitor array`); } - monitorDescriptions.add(description); + monitorNames.add(monitorName); } return req.Approve(); From 2fd8ede9ad201955c33148003ba85e24d1460b4f Mon Sep 17 00:00:00 2001 From: Andy Godish Date: Tue, 20 Aug 2024 22:12:49 -0600 Subject: [PATCH 5/6] caught up branch to main, resolved merge conflicts and added in monitor array to test mocks --- .../operator/crd/validators/package-validator.spec.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/pepr/operator/crd/validators/package-validator.spec.ts b/src/pepr/operator/crd/validators/package-validator.spec.ts index 22add07b1..663569c4e 100644 --- a/src/pepr/operator/crd/validators/package-validator.spec.ts +++ b/src/pepr/operator/crd/validators/package-validator.spec.ts @@ -439,6 +439,7 @@ describe("Test validation of Exemption CRs", () => { }, }, ], + [], ); await validator(mockReq); expect(mockReq.Deny).toHaveBeenCalledTimes(1); @@ -455,6 +456,7 @@ describe("Test validation of Exemption CRs", () => { enableAuthserviceSelector: undefined, // explicitly undefined }, ], + [], ); await validator(mockReq); expect(mockReq.Approve).toHaveBeenCalledTimes(1); @@ -478,6 +480,7 @@ describe("Test Allowed SSO Client Attributes", () => { }, }, ], + [], ); await validator(mockReq); expect(mockReq.Deny).toHaveBeenCalledTimes(1); @@ -504,6 +507,7 @@ describe("Test Allowed SSO Client Attributes", () => { }, }, ], + [], ); await validator(mockReq); expect(mockReq.Approve).toHaveBeenCalledTimes(1); @@ -522,6 +526,7 @@ describe("Test Allowed SSO Client Attributes", () => { }, }, ], + [], ); await validator(mockReq); expect(mockReq.Deny).toHaveBeenCalledTimes(1); @@ -540,13 +545,14 @@ 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); }); @@ -584,4 +590,3 @@ describe("Test proper generation of a unique name for service monitors", () => { expect(mockReq.Deny).toHaveBeenCalledTimes(1); }); }); -}); From 8f5fcbc386fd7acf87ec1008db813e202155cff1 Mon Sep 17 00:00:00 2001 From: Andy Godish Date: Tue, 20 Aug 2024 22:17:55 -0600 Subject: [PATCH 6/6] uds run lint-check after CI fail --- .../crd/validators/package-validator.spec.ts | 14 ++++++++++++-- .../operator/crd/validators/package-validator.ts | 8 ++++++-- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/src/pepr/operator/crd/validators/package-validator.spec.ts b/src/pepr/operator/crd/validators/package-validator.spec.ts index 663569c4e..476bcea38 100644 --- a/src/pepr/operator/crd/validators/package-validator.spec.ts +++ b/src/pepr/operator/crd/validators/package-validator.spec.ts @@ -1,6 +1,16 @@ import { afterEach, describe, expect, it, jest } from "@jest/globals"; import { PeprValidateRequest } from "pepr"; -import { Allow, Direction, Expose, Gateway, Monitor, Protocol, RemoteGenerated, Sso, UDSPackage } from ".."; +import { + Allow, + Direction, + Expose, + Gateway, + Monitor, + Protocol, + RemoteGenerated, + Sso, + UDSPackage, +} from ".."; import { validator } from "./package-validator"; const makeMockReq = ( @@ -562,7 +572,7 @@ 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( {}, diff --git a/src/pepr/operator/crd/validators/package-validator.ts b/src/pepr/operator/crd/validators/package-validator.ts index 27888f0d1..3ff046d10 100644 --- a/src/pepr/operator/crd/validators/package-validator.ts +++ b/src/pepr/operator/crd/validators/package-validator.ts @@ -147,10 +147,14 @@ export async function validator(req: PeprValidateRequest) { const monitorNames = new Set(); for (const monitor of monitors) { - const monitorName = sanitizeResourceName(monitor.description || `${Object.values(monitor.selector)}-${monitor.portName}`); + const monitorName = sanitizeResourceName( + monitor.description || `${Object.values(monitor.selector)}-${monitor.portName}`, + ); if (monitorNames.has(monitorName)) { - return req.Deny(`A serviceMonitor resource generated from the Package, ${pkg.metadata?.name} contains a duplicate name, please provide a unique description for each item in the monitor array`); + return req.Deny( + `A serviceMonitor resource generated from the Package, ${pkg.metadata?.name} contains a duplicate name, please provide a unique description for each item in the monitor array`, + ); } monitorNames.add(monitorName);