From c5d44ad0dd437e7f05fa8d8662b3d6620b3c2597 Mon Sep 17 00:00:00 2001 From: Case Wylie Date: Wed, 5 Mar 2025 08:58:05 -0500 Subject: [PATCH 1/4] choreL WIP Signed-off-by: Case Wylie --- src/lib/processors/mutate-processor.test.ts | 184 ++++++++++++++++++-- 1 file changed, 170 insertions(+), 14 deletions(-) diff --git a/src/lib/processors/mutate-processor.test.ts b/src/lib/processors/mutate-processor.test.ts index 06dc5e12..f90e7501 100644 --- a/src/lib/processors/mutate-processor.test.ts +++ b/src/lib/processors/mutate-processor.test.ts @@ -5,15 +5,40 @@ import { beforeEach, describe, expect, it, jest } from "@jest/globals"; import { clone } from "ramda"; import { ModuleConfig } from "../types"; import { PeprMutateRequest } from "../mutate-request"; -import * as sut from "./mutate-processor"; import { AdmissionRequest, Binding, MutateAction } from "../types"; import { Event, Operation } from "../enums"; import { convertFromBase64Map, convertToBase64Map, base64Encode } from "../utils"; -import { GenericClass, KubernetesObject } from "kubernetes-fluent-client"; +import { GenericClass, KubernetesObject, GroupVersionKind } from "kubernetes-fluent-client"; import { MutateResponse } from "../k8s"; import { OnError } from "../../cli/init/enums"; -import { updateResponsePatchAndWarnings } from "./mutate-processor"; +import { updateResponsePatchAndWarnings, Bindable, reencodeData, mutateProcessor, updateStatus, logMutateErrorMessage, decodeData, processRequest } from "./mutate-processor"; import { Operation as JSONPatchOperation } from "fast-json-patch"; +import { Capability } from "../core/capability"; +import { MeasureWebhookTimeout } from "../telemetry/webhookTimeouts"; +import * as utils from "../utils"; +import { shouldSkipRequest } from "../filter/filter"; + +jest.mock("../telemetry/logger", () => ({ + info: jest.fn(), + debug: jest.fn(), + error: jest.fn(), +})); + +jest.mock("../telemetry/metrics", () => ({ + metricsCollector: { + addCounter: jest.fn(), + incCounter: jest.fn(), + }, + MeasureWebhookTimeout: jest.fn(), +})); + +jest.mock("../telemetry/timeUtils", () => ({ + getNow: jest.fn(() => 1000), +})); + +jest.mock("../filter/filter", () => ({ + shouldSkipRequest: jest.fn(), +})); jest.mock("../utils"); const mockConvertFromBase64Map = jest.mocked(convertFromBase64Map); @@ -60,7 +85,7 @@ describe("updateStatus", () => { const status = "test-status"; const annote = `${defaultModuleConfig.uuid}.pepr.dev/${name}`; - const result = sut.updateStatus(defaultModuleConfig, name, defaultPeprMutateRequest(), status); + const result = updateStatus(defaultModuleConfig, name, defaultPeprMutateRequest(), status); expect(result.HasAnnotation(annote)).toBe(true); expect(result.Raw.metadata?.annotations?.[annote]).toBe(status); @@ -77,7 +102,7 @@ describe("updateStatus", () => { const name = "capa"; const annote = `${defaultModuleConfig.uuid}.pepr.dev/${name}`; - const result = sut.updateStatus( + const result = updateStatus( defaultModuleConfig, name, defaultPeprMutateRequest(testAdmissionRequest), @@ -96,7 +121,7 @@ describe("logMutateErrorMessage", () => { ["", "An error occurred with the mutate action."], ["[object Object]", "An error occurred with the mutate action."], ])("given error '%s', returns '%s'", (err, res) => { - const result = sut.logMutateErrorMessage(new Error(err)); + const result = logMutateErrorMessage(new Error(err)); expect(result).toBe(res); }); }); @@ -119,7 +144,7 @@ describe("decodeData", () => { }; const testPeprMutateRequest = defaultPeprMutateRequest(testAdmissionRequest); - const { skipped, wrapped } = sut.decodeData(testPeprMutateRequest); + const { skipped, wrapped } = decodeData(testPeprMutateRequest); expect(mockConvertFromBase64Map.mock.calls.length).toBe(1); expect(mockConvertFromBase64Map.mock.calls[0].at(0)).toBe(testPeprMutateRequest.Raw); @@ -138,7 +163,7 @@ describe("decodeData", () => { }; const testPeprMutateRequest = defaultPeprMutateRequest(testAdmissionRequest); - const { skipped, wrapped } = sut.decodeData(testPeprMutateRequest); + const { skipped, wrapped } = decodeData(testPeprMutateRequest); expect(mockConvertFromBase64Map.mock.calls.length).toBe(0); expect(skipped).toEqual([]); @@ -159,7 +184,7 @@ describe("reencodeData", () => { }; const testPeprMutateRequest = defaultPeprMutateRequest(testAdmissionRequest); - const transformed = sut.reencodeData(testPeprMutateRequest, skipped); + const transformed = reencodeData(testPeprMutateRequest, skipped); expect(mockConvertToBase64Map.mock.calls.length).toBe(0); expect(transformed).toEqual(testAdmissionRequest.object); @@ -177,7 +202,7 @@ describe("reencodeData", () => { }; const testPeprMutateRequest = defaultPeprMutateRequest(testAdmissionRequest); - const transformed = sut.reencodeData(testPeprMutateRequest, skipped); + const transformed = reencodeData(testPeprMutateRequest, skipped); expect(mockConvertToBase64Map.mock.calls.length).toBe(1); expect(mockConvertToBase64Map.mock.calls[0].at(0)).toEqual(testPeprMutateRequest.Raw); @@ -206,7 +231,7 @@ const defaultBinding: Binding = { mutateCallback: jest.fn() as jest.Mocked>, }; -const defaultBindable: sut.Bindable = { +const defaultBindable: Bindable = { req: defaultAdmissionRequest, config: defaultModuleConfig, name: "test-name", @@ -226,7 +251,7 @@ describe("processRequest", () => { const testMutateResponse = clone(defaultMutateResponse); const annote = `${defaultModuleConfig.uuid}.pepr.dev/${defaultBindable.name}`; - const result = await sut.processRequest(defaultBindable, testPeprMutateRequest, testMutateResponse); + const result = await processRequest(defaultBindable, testPeprMutateRequest, testMutateResponse); expect(result).toEqual({ wrapped: testPeprMutateRequest, response: testMutateResponse }); expect(result.wrapped.Raw.metadata?.annotations).toBeDefined(); @@ -250,7 +275,7 @@ describe("processRequest", () => { const testMutateResponse = clone(defaultMutateResponse); const annote = `${defaultModuleConfig.uuid}.pepr.dev/${defaultBindable.name}`; - const result = await sut.processRequest(testBindable, testPeprMutateRequest, testMutateResponse); + const result = await processRequest(testBindable, testPeprMutateRequest, testMutateResponse); expect(result).toEqual({ wrapped: testPeprMutateRequest, response: testMutateResponse }); expect(result.wrapped.Raw.metadata?.annotations).toBeDefined(); @@ -275,7 +300,7 @@ describe("processRequest", () => { const testMutateResponse = clone(defaultMutateResponse); const annote = `${defaultModuleConfig.uuid}.pepr.dev/${defaultBindable.name}`; - const result = await sut.processRequest(testBindable, testPeprMutateRequest, testMutateResponse); + const result = await processRequest(testBindable, testPeprMutateRequest, testMutateResponse); expect(result).toEqual({ wrapped: testPeprMutateRequest, response: testMutateResponse }); expect(result.wrapped.Raw.metadata?.annotations).toBeDefined(); @@ -312,3 +337,134 @@ describe("updateResponsePatchAndWarnings", () => { expect(localMutateResponse.warnings).not.toBeDefined(); }); }); + +describe("mutateProcessor", () => { + let config: ModuleConfig; + beforeEach(()=>{ + jest.clearAllMocks(); + config = { + webhookTimeout: 11, + uuid: "some-uuid", + alwaysIgnore: {}, + }; + }) + + it("should measure if a timeout occurred based on webhookTimeout", async () => { + const capability = new Capability({ + name: "test", + description: "test", + }); + + const req = defaultAdmissionRequest; + const reqMetadata = {}; + + const spyStart = jest.spyOn(MeasureWebhookTimeout.prototype, "start"); + + await mutateProcessor(config, [capability], req, reqMetadata); + + expect(spyStart).toHaveBeenCalledWith(config.webhookTimeout); + spyStart.mockRestore(); + }); + + // it("should call convertFromBase64Map if the kind is a Secret", async () => { + // const capability = new Capability({ + // name: "test", + // description: "test", + // }); + // const testGroupVersionKind: GroupVersionKind = { + // kind: "Secret", + // version: "v1", + // group: "", + // }; + // const req: AdmissionRequest = { ...defaultAdmissionRequest, kind: testGroupVersionKind }; + // const reqMetadata = {}; + + // const spyConvert = jest.spyOn(utils, "convertFromBase64Map"); + + // await mutateProcessor(config, [capability], req, reqMetadata); + + // expect(spyConvert).toHaveBeenCalled(); + // spyConvert.mockRestore(); + // }); + it("should stop the timer after processing", async () => { + const capability = new Capability({ + name: "test", + description: "test", + }); + + const req = defaultAdmissionRequest; + const reqMetadata = {}; + + const spyStop = jest.spyOn(MeasureWebhookTimeout.prototype, "stop"); + + await mutateProcessor(config, [capability], req, reqMetadata); + + expect(spyStop).toHaveBeenCalled(); + spyStop.mockRestore(); + }); + + // it("should skip bindings that do not have validateCallback", async () => { + // config = { + // webhookTimeout: 10, + // uuid: "some-uuid", + // alwaysIgnore: {}, + // }; + + // const capability = new Capability({ + // name: "test", + // description: "test", + // bindings: [ + // { + // isValidate: true, + // validateCallback: undefined, + // }, + // ], + // } as unknown as Capability); + + // const req = testAdmissionRequest; + // const reqMetadata = {}; + + // // This rule is skipped because we cannot mock this function globally as it is tested above + // // eslint-disable-next-line @typescript-eslint/no-require-imports + // const spyProcessRequest = jest.spyOn(require("./validate-processor"), "processRequest"); + + // await validateProcessor(config, [capability], req, reqMetadata); + + // expect(spyProcessRequest).not.toHaveBeenCalled(); + + // spyProcessRequest.mockRestore(); + // }); + + // it("should skip bindings if shouldSkipRequest returns a reason", async () => { + // config = { + // webhookTimeout: 10, + // uuid: "some-uuid", + // alwaysIgnore: {}, + // }; + + // const capability = new Capability({ + // name: "test", + // description: "test", + // bindings: [ + // { + // isValidate: true, + // validateCallback: jest.fn(), + // }, + // ], + // } as unknown as Capability); + + // const req = testAdmissionRequest; + // const reqMetadata = {}; + + // // This rule is skipped because we cannot mock this function globally as it is tested above + // // eslint-disable-next-line @typescript-eslint/no-require-imports + // const spyProcessRequest = jest.spyOn(require("./validate-processor"), "processRequest"); + // (shouldSkipRequest as jest.Mock).mockReturnValue("Skip reason"); + + // await validateProcessor(config, [capability], req, reqMetadata); + + // expect(spyProcessRequest).not.toHaveBeenCalled(); + + // spyProcessRequest.mockRestore(); + // }); +}); From 13fe2c0e0aec66b039618df0e2bd38ffce7d0bdd Mon Sep 17 00:00:00 2001 From: Case Wylie Date: Wed, 5 Mar 2025 13:19:37 -0500 Subject: [PATCH 2/4] chore: mutate-processor calls time start and stop Signed-off-by: Case Wylie --- src/lib/processors/mutate-processor.test.ts | 112 ++++---------------- 1 file changed, 20 insertions(+), 92 deletions(-) diff --git a/src/lib/processors/mutate-processor.test.ts b/src/lib/processors/mutate-processor.test.ts index f90e7501..c7163308 100644 --- a/src/lib/processors/mutate-processor.test.ts +++ b/src/lib/processors/mutate-processor.test.ts @@ -8,15 +8,22 @@ import { PeprMutateRequest } from "../mutate-request"; import { AdmissionRequest, Binding, MutateAction } from "../types"; import { Event, Operation } from "../enums"; import { convertFromBase64Map, convertToBase64Map, base64Encode } from "../utils"; -import { GenericClass, KubernetesObject, GroupVersionKind } from "kubernetes-fluent-client"; +import { GenericClass, KubernetesObject } from "kubernetes-fluent-client"; import { MutateResponse } from "../k8s"; import { OnError } from "../../cli/init/enums"; -import { updateResponsePatchAndWarnings, Bindable, reencodeData, mutateProcessor, updateStatus, logMutateErrorMessage, decodeData, processRequest } from "./mutate-processor"; +import { + updateResponsePatchAndWarnings, + Bindable, + reencodeData, + mutateProcessor, + updateStatus, + logMutateErrorMessage, + decodeData, + processRequest, +} from "./mutate-processor"; import { Operation as JSONPatchOperation } from "fast-json-patch"; import { Capability } from "../core/capability"; import { MeasureWebhookTimeout } from "../telemetry/webhookTimeouts"; -import * as utils from "../utils"; -import { shouldSkipRequest } from "../filter/filter"; jest.mock("../telemetry/logger", () => ({ info: jest.fn(), @@ -340,14 +347,14 @@ describe("updateResponsePatchAndWarnings", () => { describe("mutateProcessor", () => { let config: ModuleConfig; - beforeEach(()=>{ + beforeEach(() => { jest.clearAllMocks(); config = { webhookTimeout: 11, uuid: "some-uuid", alwaysIgnore: {}, }; - }) + }); it("should measure if a timeout occurred based on webhookTimeout", async () => { const capability = new Capability({ @@ -366,31 +373,17 @@ describe("mutateProcessor", () => { spyStart.mockRestore(); }); - // it("should call convertFromBase64Map if the kind is a Secret", async () => { - // const capability = new Capability({ - // name: "test", - // description: "test", - // }); - // const testGroupVersionKind: GroupVersionKind = { - // kind: "Secret", - // version: "v1", - // group: "", - // }; - // const req: AdmissionRequest = { ...defaultAdmissionRequest, kind: testGroupVersionKind }; - // const reqMetadata = {}; - - // const spyConvert = jest.spyOn(utils, "convertFromBase64Map"); - - // await mutateProcessor(config, [capability], req, reqMetadata); - - // expect(spyConvert).toHaveBeenCalled(); - // spyConvert.mockRestore(); - // }); it("should stop the timer after processing", async () => { const capability = new Capability({ name: "test", description: "test", - }); + bindings: [ + { + isMutate: true, + mutateCallback: jest.fn(), + }, + ], + } as unknown as Capability); const req = defaultAdmissionRequest; const reqMetadata = {}; @@ -402,69 +395,4 @@ describe("mutateProcessor", () => { expect(spyStop).toHaveBeenCalled(); spyStop.mockRestore(); }); - - // it("should skip bindings that do not have validateCallback", async () => { - // config = { - // webhookTimeout: 10, - // uuid: "some-uuid", - // alwaysIgnore: {}, - // }; - - // const capability = new Capability({ - // name: "test", - // description: "test", - // bindings: [ - // { - // isValidate: true, - // validateCallback: undefined, - // }, - // ], - // } as unknown as Capability); - - // const req = testAdmissionRequest; - // const reqMetadata = {}; - - // // This rule is skipped because we cannot mock this function globally as it is tested above - // // eslint-disable-next-line @typescript-eslint/no-require-imports - // const spyProcessRequest = jest.spyOn(require("./validate-processor"), "processRequest"); - - // await validateProcessor(config, [capability], req, reqMetadata); - - // expect(spyProcessRequest).not.toHaveBeenCalled(); - - // spyProcessRequest.mockRestore(); - // }); - - // it("should skip bindings if shouldSkipRequest returns a reason", async () => { - // config = { - // webhookTimeout: 10, - // uuid: "some-uuid", - // alwaysIgnore: {}, - // }; - - // const capability = new Capability({ - // name: "test", - // description: "test", - // bindings: [ - // { - // isValidate: true, - // validateCallback: jest.fn(), - // }, - // ], - // } as unknown as Capability); - - // const req = testAdmissionRequest; - // const reqMetadata = {}; - - // // This rule is skipped because we cannot mock this function globally as it is tested above - // // eslint-disable-next-line @typescript-eslint/no-require-imports - // const spyProcessRequest = jest.spyOn(require("./validate-processor"), "processRequest"); - // (shouldSkipRequest as jest.Mock).mockReturnValue("Skip reason"); - - // await validateProcessor(config, [capability], req, reqMetadata); - - // expect(spyProcessRequest).not.toHaveBeenCalled(); - - // spyProcessRequest.mockRestore(); - // }); }); From 32b2b741f0baeef427966a47f87075b918d39e4a Mon Sep 17 00:00:00 2001 From: Case Wylie Date: Wed, 5 Mar 2025 15:21:50 -0500 Subject: [PATCH 3/4] chore: move around files in order to mock Signed-off-by: Case Wylie --- src/lib/decode-utils.test.ts | 122 +++++++++++++++++ src/lib/decode-utils.ts | 31 +++++ src/lib/processors/mutate-processor.test.ts | 137 +++++--------------- src/lib/processors/mutate-processor.ts | 50 +++---- 4 files changed, 211 insertions(+), 129 deletions(-) create mode 100644 src/lib/decode-utils.test.ts create mode 100644 src/lib/decode-utils.ts diff --git a/src/lib/decode-utils.test.ts b/src/lib/decode-utils.test.ts new file mode 100644 index 00000000..e2eb3827 --- /dev/null +++ b/src/lib/decode-utils.test.ts @@ -0,0 +1,122 @@ +import { beforeEach, describe, expect, it, jest } from "@jest/globals"; +import { AdmissionRequest } from "./types"; +import { convertFromBase64Map, convertToBase64Map } from "./utils"; +import { Operation } from "./enums"; +import { PeprMutateRequest } from "./mutate-request"; +import { decodeData, reencodeData } from "./decode-utils"; + +jest.mock("./utils"); + +const defaultPeprMutateRequest = (admissionRequest = defaultAdmissionRequest) => + new PeprMutateRequest(admissionRequest); + +const mockConvertToBase64Map = jest.mocked(convertToBase64Map); +const mockConvertFromBase64Map = jest.mocked(convertFromBase64Map); + +const defaultAdmissionRequest: AdmissionRequest = { + uid: "uid", + kind: { + kind: "kind", + group: "group", + version: "version", + }, + resource: { + group: "group", + version: "version", + resource: "resource", + }, + name: "", + object: { + metadata: { + name: "create-me", + }, + }, + operation: Operation.CREATE, + userInfo: {}, +}; + +describe("decodeData", () => { + const skips = ["convert", "From", "Base64", "Map"]; + + beforeEach(() => { + mockConvertFromBase64Map.mockClear(); + mockConvertFromBase64Map.mockImplementation(() => skips); + }); + + it("returns skips if required & given a Secret", () => { + const testAdmissionRequest = { + ...defaultAdmissionRequest, + kind: { + kind: "Secret", + version: "v1", + group: "", + }, + }; + const testPeprMutateRequest = defaultPeprMutateRequest(testAdmissionRequest); + + const { skipped, wrapped } = decodeData(testPeprMutateRequest); + + expect(mockConvertFromBase64Map.mock.calls.length).toBe(1); + expect(mockConvertFromBase64Map.mock.calls[0].at(0)).toBe(testPeprMutateRequest.Raw); + expect(skipped).toBe(skips); + expect(wrapped).toBe(testPeprMutateRequest); + }); + + it("returns no skips when given a non-Secret", () => { + const testAdmissionRequest = { + ...defaultAdmissionRequest, + kind: { + kind: "NotASecret", + version: "v1", + group: "", + }, + }; + const testPeprMutateRequest = defaultPeprMutateRequest(testAdmissionRequest); + + const { skipped, wrapped } = decodeData(testPeprMutateRequest); + + expect(mockConvertFromBase64Map.mock.calls.length).toBe(0); + expect(skipped).toEqual([]); + expect(wrapped).toBe(testPeprMutateRequest); + }); +}); + +describe("reencodeData", () => { + it("returns unchanged content when given non-secret", () => { + const skipped = ["convert", "To", "Base64", "Map"]; + const testAdmissionRequest = { + ...defaultAdmissionRequest, + kind: { + kind: "NotASecret", + version: "v1", + group: "", + }, + }; + const testPeprMutateRequest = defaultPeprMutateRequest(testAdmissionRequest); + + const transformed = reencodeData(testPeprMutateRequest, skipped); + + expect(mockConvertToBase64Map.mock.calls.length).toBe(0); + expect(transformed).toEqual(testAdmissionRequest.object); + }); + + it("returns modified content when given a secret and skips", () => { + const skipped = ["convert", "To", "Base64", "Map"]; + const testAdmissionRequest = { + ...defaultAdmissionRequest, + kind: { + kind: "Secret", + version: "v1", + group: "", + }, + }; + const testPeprMutateRequest = defaultPeprMutateRequest(testAdmissionRequest); + + const transformed = reencodeData(testPeprMutateRequest, skipped); + + expect(mockConvertToBase64Map.mock.calls.length).toBe(1); + expect(mockConvertToBase64Map.mock.calls[0].at(0)).toEqual(testPeprMutateRequest.Raw); + expect(mockConvertToBase64Map.mock.calls[0].at(1)).toBe(skipped); + expect(transformed).toEqual(testPeprMutateRequest.Raw); + }); +}); diff --git a/src/lib/decode-utils.ts b/src/lib/decode-utils.ts new file mode 100644 index 00000000..0a364999 --- /dev/null +++ b/src/lib/decode-utils.ts @@ -0,0 +1,31 @@ +import { convertFromBase64Map, convertToBase64Map } from "./utils"; +import { kind, KubernetesObject } from "kubernetes-fluent-client"; +import { PeprMutateRequest } from "./mutate-request"; +import { clone } from "ramda"; + +export function decodeData(wrapped: PeprMutateRequest): { + skipped: string[]; + wrapped: PeprMutateRequest; +} { + let skipped: string[] = []; + + const isSecret = wrapped.Request.kind.version === "v1" && wrapped.Request.kind.kind === "Secret"; + if (isSecret) { + // convertFromBase64Map modifies it's arg rather than returing a mod'ed copy (ye olde side-effect special, blerg) + skipped = convertFromBase64Map(wrapped.Raw as unknown as kind.Secret); + } + + return { skipped, wrapped }; +} + +export function reencodeData(wrapped: PeprMutateRequest, skipped: string[]): KubernetesObject { + const transformed = clone(wrapped.Raw); + + const isSecret = wrapped.Request.kind.version === "v1" && wrapped.Request.kind.kind === "Secret"; + if (isSecret) { + // convertToBase64Map modifies it's arg rather than returing a mod'ed copy (ye olde side-effect special, blerg) + convertToBase64Map(transformed as unknown as kind.Secret, skipped); + } + + return transformed; +} diff --git a/src/lib/processors/mutate-processor.test.ts b/src/lib/processors/mutate-processor.test.ts index c7163308..589ea835 100644 --- a/src/lib/processors/mutate-processor.test.ts +++ b/src/lib/processors/mutate-processor.test.ts @@ -7,23 +7,26 @@ import { ModuleConfig } from "../types"; import { PeprMutateRequest } from "../mutate-request"; import { AdmissionRequest, Binding, MutateAction } from "../types"; import { Event, Operation } from "../enums"; -import { convertFromBase64Map, convertToBase64Map, base64Encode } from "../utils"; +import { base64Encode } from "../utils"; import { GenericClass, KubernetesObject } from "kubernetes-fluent-client"; import { MutateResponse } from "../k8s"; import { OnError } from "../../cli/init/enums"; import { updateResponsePatchAndWarnings, Bindable, - reencodeData, mutateProcessor, updateStatus, logMutateErrorMessage, - decodeData, processRequest, } from "./mutate-processor"; import { Operation as JSONPatchOperation } from "fast-json-patch"; import { Capability } from "../core/capability"; import { MeasureWebhookTimeout } from "../telemetry/webhookTimeouts"; +import { decodeData } from "../decode-utils"; + +jest.mock("../decode-utils", () => ({ + decodeData: jest.fn(), +})); jest.mock("../telemetry/logger", () => ({ info: jest.fn(), @@ -48,8 +51,6 @@ jest.mock("../filter/filter", () => ({ })); jest.mock("../utils"); -const mockConvertFromBase64Map = jest.mocked(convertFromBase64Map); -const mockConvertToBase64Map = jest.mocked(convertToBase64Map); const defaultModuleConfig: ModuleConfig = { uuid: "test-uuid", @@ -83,6 +84,10 @@ const defaultPeprMutateRequest = (admissionRequest = defaultAdmissionRequest) => beforeEach(() => { jest.resetAllMocks(); + (decodeData as jest.Mock).mockReturnValue({ + wrapped: { mockWrapped: true }, + skipped: [], + }); }); describe("updateStatus", () => { @@ -133,91 +138,6 @@ describe("logMutateErrorMessage", () => { }); }); -describe("decodeData", () => { - const skips = ["convert", "From", "Base64", "Map"]; - - beforeEach(() => { - mockConvertFromBase64Map.mockImplementation(() => skips); - }); - - it("returns skips if required & given a Secret", () => { - const testAdmissionRequest = { - ...defaultAdmissionRequest, - kind: { - kind: "Secret", - version: "v1", - group: "", - }, - }; - const testPeprMutateRequest = defaultPeprMutateRequest(testAdmissionRequest); - - const { skipped, wrapped } = decodeData(testPeprMutateRequest); - - expect(mockConvertFromBase64Map.mock.calls.length).toBe(1); - expect(mockConvertFromBase64Map.mock.calls[0].at(0)).toBe(testPeprMutateRequest.Raw); - expect(skipped).toBe(skips); - expect(wrapped).toBe(testPeprMutateRequest); - }); - - it("returns no skips when given a non-Secret", () => { - const testAdmissionRequest = { - ...defaultAdmissionRequest, - kind: { - kind: "NotASecret", - version: "v1", - group: "", - }, - }; - const testPeprMutateRequest = defaultPeprMutateRequest(testAdmissionRequest); - - const { skipped, wrapped } = decodeData(testPeprMutateRequest); - - expect(mockConvertFromBase64Map.mock.calls.length).toBe(0); - expect(skipped).toEqual([]); - expect(wrapped).toBe(testPeprMutateRequest); - }); -}); - -describe("reencodeData", () => { - it("returns unchanged content when given non-secret", () => { - const skipped = ["convert", "To", "Base64", "Map"]; - const testAdmissionRequest = { - ...defaultAdmissionRequest, - kind: { - kind: "NotASecret", - version: "v1", - group: "", - }, - }; - const testPeprMutateRequest = defaultPeprMutateRequest(testAdmissionRequest); - - const transformed = reencodeData(testPeprMutateRequest, skipped); - - expect(mockConvertToBase64Map.mock.calls.length).toBe(0); - expect(transformed).toEqual(testAdmissionRequest.object); - }); - - it("returns modified content when given a secret and skips", () => { - const skipped = ["convert", "To", "Base64", "Map"]; - const testAdmissionRequest = { - ...defaultAdmissionRequest, - kind: { - kind: "Secret", - version: "v1", - group: "", - }, - }; - const testPeprMutateRequest = defaultPeprMutateRequest(testAdmissionRequest); - - const transformed = reencodeData(testPeprMutateRequest, skipped); - - expect(mockConvertToBase64Map.mock.calls.length).toBe(1); - expect(mockConvertToBase64Map.mock.calls[0].at(0)).toEqual(testPeprMutateRequest.Raw); - expect(mockConvertToBase64Map.mock.calls[0].at(1)).toBe(skipped); - expect(transformed).toEqual(testPeprMutateRequest.Raw); - }); -}); - const defaultBinding: Binding = { event: Event.CREATE, model: {} as GenericClass, @@ -347,6 +267,7 @@ describe("updateResponsePatchAndWarnings", () => { describe("mutateProcessor", () => { let config: ModuleConfig; + let capability: Capability; beforeEach(() => { jest.clearAllMocks(); config = { @@ -354,14 +275,17 @@ describe("mutateProcessor", () => { uuid: "some-uuid", alwaysIgnore: {}, }; - }); - - it("should measure if a timeout occurred based on webhookTimeout", async () => { - const capability = new Capability({ + capability = new Capability({ name: "test", description: "test", }); + (decodeData as jest.Mock).mockReturnValue({ + wrapped: { mockWrapped: true }, + skipped: [], + }); + }); + it("should measure if a timeout occurred based on webhookTimeout", async () => { const req = defaultAdmissionRequest; const reqMetadata = {}; @@ -374,17 +298,6 @@ describe("mutateProcessor", () => { }); it("should stop the timer after processing", async () => { - const capability = new Capability({ - name: "test", - description: "test", - bindings: [ - { - isMutate: true, - mutateCallback: jest.fn(), - }, - ], - } as unknown as Capability); - const req = defaultAdmissionRequest; const reqMetadata = {}; @@ -395,4 +308,18 @@ describe("mutateProcessor", () => { expect(spyStop).toHaveBeenCalled(); spyStop.mockRestore(); }); + + it("should call decodeData", async () => { + const req = defaultAdmissionRequest; + const reqMetadata = {}; + + (decodeData as jest.Mock).mockReturnValue({ + wrapped: { mockWrapped: true }, + skipped: [], + }); + + await mutateProcessor(config, [capability], req, reqMetadata); + + expect(decodeData).toHaveBeenCalled(); + }); }); diff --git a/src/lib/processors/mutate-processor.ts b/src/lib/processors/mutate-processor.ts index 4d1788be..a3f949b4 100644 --- a/src/lib/processors/mutate-processor.ts +++ b/src/lib/processors/mutate-processor.ts @@ -2,8 +2,7 @@ // SPDX-FileCopyrightText: 2023-Present The Pepr Authors import jsonPatch from "fast-json-patch"; -import { kind, KubernetesObject } from "kubernetes-fluent-client"; -import { clone } from "ramda"; +import { KubernetesObject } from "kubernetes-fluent-client"; import { MeasureWebhookTimeout } from "../telemetry/webhookTimeouts"; import { Capability } from "../core/capability"; import { shouldSkipRequest } from "../filter/filter"; @@ -12,11 +11,13 @@ import { AdmissionRequest, Binding } from "../types"; import Log from "../telemetry/logger"; import { ModuleConfig } from "../types"; import { PeprMutateRequest } from "../mutate-request"; -import { base64Encode, convertFromBase64Map, convertToBase64Map } from "../utils"; +import { base64Encode } from "../utils"; import { OnError } from "../../cli/init/enums"; import { resolveIgnoreNamespaces } from "../assets/webhooks"; import { Operation } from "fast-json-patch"; import { WebhookType } from "../enums"; +import { decodeData, reencodeData } from "../decode-utils"; + export interface Bindable { req: AdmissionRequest; config: ModuleConfig; @@ -60,32 +61,32 @@ export function logMutateErrorMessage(e: Error): string { } } -export function decodeData(wrapped: PeprMutateRequest): { - skipped: string[]; - wrapped: PeprMutateRequest; -} { - let skipped: string[] = []; +// export function decodeData(wrapped: PeprMutateRequest): { +// skipped: string[]; +// wrapped: PeprMutateRequest; +// } { +// let skipped: string[] = []; - const isSecret = wrapped.Request.kind.version === "v1" && wrapped.Request.kind.kind === "Secret"; - if (isSecret) { - // convertFromBase64Map modifies it's arg rather than returing a mod'ed copy (ye olde side-effect special, blerg) - skipped = convertFromBase64Map(wrapped.Raw as unknown as kind.Secret); - } +// const isSecret = wrapped.Request.kind.version === "v1" && wrapped.Request.kind.kind === "Secret"; +// if (isSecret) { +// // convertFromBase64Map modifies it's arg rather than returing a mod'ed copy (ye olde side-effect special, blerg) +// skipped = convertFromBase64Map(wrapped.Raw as unknown as kind.Secret); +// } - return { skipped, wrapped }; -} +// return { skipped, wrapped }; +// } -export function reencodeData(wrapped: PeprMutateRequest, skipped: string[]): KubernetesObject { - const transformed = clone(wrapped.Raw); +// export function reencodeData(wrapped: PeprMutateRequest, skipped: string[]): KubernetesObject { +// const transformed = clone(wrapped.Raw); - const isSecret = wrapped.Request.kind.version === "v1" && wrapped.Request.kind.kind === "Secret"; - if (isSecret) { - // convertToBase64Map modifies it's arg rather than returing a mod'ed copy (ye olde side-effect special, blerg) - convertToBase64Map(transformed as unknown as kind.Secret, skipped); - } +// const isSecret = wrapped.Request.kind.version === "v1" && wrapped.Request.kind.kind === "Secret"; +// if (isSecret) { +// // convertToBase64Map modifies it's arg rather than returing a mod'ed copy (ye olde side-effect special, blerg) +// convertToBase64Map(transformed as unknown as kind.Secret, skipped); +// } - return transformed; -} +// return transformed; +// } export async function processRequest( bindable: Bindable, @@ -196,6 +197,7 @@ export async function mutateProcessor( // If no capability matched the request, exit early if (bindables.length === 0) { Log.info(reqMetadata, `No matching actions found`); + webhookTimer.stop(); return response; } From f8ceb43c39edf98e58c1869738e577c28409fdc4 Mon Sep 17 00:00:00 2001 From: Case Wylie Date: Thu, 6 Mar 2025 11:19:42 -0500 Subject: [PATCH 4/4] chore: address PR comments Signed-off-by: Case Wylie --- src/lib/assets/defaultTestObjects.ts | 19 ++++++++++- src/lib/{ => processors}/decode-utils.test.ts | 33 ++++--------------- src/lib/{ => processors}/decode-utils.ts | 4 +-- src/lib/processors/mutate-processor.test.ts | 4 +-- src/lib/processors/mutate-processor.ts | 29 +--------------- 5 files changed, 29 insertions(+), 60 deletions(-) rename src/lib/{ => processors}/decode-utils.test.ts (84%) rename src/lib/{ => processors}/decode-utils.ts (90%) diff --git a/src/lib/assets/defaultTestObjects.ts b/src/lib/assets/defaultTestObjects.ts index adec2f99..884c9a5e 100644 --- a/src/lib/assets/defaultTestObjects.ts +++ b/src/lib/assets/defaultTestObjects.ts @@ -1,8 +1,25 @@ -import { GenericClass } from "kubernetes-fluent-client"; +import { GenericClass, GroupVersionKind } from "kubernetes-fluent-client"; import { Event } from "../enums"; import { Binding, CapabilityExport } from "../types"; import { defaultFilters } from "../filter/adjudicators/defaultTestObjects"; import { V1PolicyRule as PolicyRule } from "@kubernetes/client-node"; +import { AdmissionRequest, GroupVersionResource } from "../types"; +import { Operation } from "../enums"; + +export const createMockAdmissionRequest = ( + kind: GroupVersionKind = { kind: "kind", group: "group", version: "version" }, + resource: GroupVersionResource = { group: "group", version: "version", resource: "resource" }, + object: { metadata: { name: string } } = { metadata: { name: "create-me" } }, + operation: Operation = Operation.CREATE, +): AdmissionRequest => ({ + uid: "uid", + kind, + resource, + name: "", + object, + operation, + userInfo: {}, +}); export const createMockRbacRule = ( apiGroups: string[] = ["pepr.dev"], diff --git a/src/lib/decode-utils.test.ts b/src/lib/processors/decode-utils.test.ts similarity index 84% rename from src/lib/decode-utils.test.ts rename to src/lib/processors/decode-utils.test.ts index e2eb3827..6be065c2 100644 --- a/src/lib/decode-utils.test.ts +++ b/src/lib/processors/decode-utils.test.ts @@ -1,11 +1,12 @@ import { beforeEach, describe, expect, it, jest } from "@jest/globals"; -import { AdmissionRequest } from "./types"; -import { convertFromBase64Map, convertToBase64Map } from "./utils"; -import { Operation } from "./enums"; -import { PeprMutateRequest } from "./mutate-request"; +import { convertFromBase64Map, convertToBase64Map } from "../utils"; +import { PeprMutateRequest } from "../mutate-request"; import { decodeData, reencodeData } from "./decode-utils"; +import { createMockAdmissionRequest } from "../assets/defaultTestObjects"; -jest.mock("./utils"); +jest.mock("../utils"); + +const defaultAdmissionRequest = createMockAdmissionRequest(); const defaultPeprMutateRequest = (admissionRequest = defaultAdmissionRequest) => new PeprMutateRequest(admissionRequest); @@ -13,28 +14,6 @@ const defaultPeprMutateRequest = (admissionRequest = defaultAdmissionRequest) => const mockConvertToBase64Map = jest.mocked(convertToBase64Map); const mockConvertFromBase64Map = jest.mocked(convertFromBase64Map); -const defaultAdmissionRequest: AdmissionRequest = { - uid: "uid", - kind: { - kind: "kind", - group: "group", - version: "version", - }, - resource: { - group: "group", - version: "version", - resource: "resource", - }, - name: "", - object: { - metadata: { - name: "create-me", - }, - }, - operation: Operation.CREATE, - userInfo: {}, -}; - describe("decodeData", () => { const skips = ["convert", "From", "Base64", "Map"]; diff --git a/src/lib/decode-utils.ts b/src/lib/processors/decode-utils.ts similarity index 90% rename from src/lib/decode-utils.ts rename to src/lib/processors/decode-utils.ts index 0a364999..54a6e8be 100644 --- a/src/lib/decode-utils.ts +++ b/src/lib/processors/decode-utils.ts @@ -1,6 +1,6 @@ -import { convertFromBase64Map, convertToBase64Map } from "./utils"; +import { convertFromBase64Map, convertToBase64Map } from "../utils"; import { kind, KubernetesObject } from "kubernetes-fluent-client"; -import { PeprMutateRequest } from "./mutate-request"; +import { PeprMutateRequest } from "../mutate-request"; import { clone } from "ramda"; export function decodeData(wrapped: PeprMutateRequest): { diff --git a/src/lib/processors/mutate-processor.test.ts b/src/lib/processors/mutate-processor.test.ts index 589ea835..4967fe04 100644 --- a/src/lib/processors/mutate-processor.test.ts +++ b/src/lib/processors/mutate-processor.test.ts @@ -22,9 +22,9 @@ import { import { Operation as JSONPatchOperation } from "fast-json-patch"; import { Capability } from "../core/capability"; import { MeasureWebhookTimeout } from "../telemetry/webhookTimeouts"; -import { decodeData } from "../decode-utils"; +import { decodeData } from "./decode-utils"; -jest.mock("../decode-utils", () => ({ +jest.mock("./decode-utils", () => ({ decodeData: jest.fn(), })); diff --git a/src/lib/processors/mutate-processor.ts b/src/lib/processors/mutate-processor.ts index 3c5c7ed6..a35b55dc 100644 --- a/src/lib/processors/mutate-processor.ts +++ b/src/lib/processors/mutate-processor.ts @@ -16,7 +16,7 @@ import { OnError } from "../../cli/init/enums"; import { resolveIgnoreNamespaces } from "../assets/webhooks"; import { Operation } from "fast-json-patch"; import { WebhookType } from "../enums"; -import { decodeData, reencodeData } from "../decode-utils"; +import { decodeData, reencodeData } from "./decode-utils"; export interface Bindable { req: AdmissionRequest; @@ -61,33 +61,6 @@ export function logMutateErrorMessage(e: Error): string { } } -// export function decodeData(wrapped: PeprMutateRequest): { -// skipped: string[]; -// wrapped: PeprMutateRequest; -// } { -// let skipped: string[] = []; - -// const isSecret = wrapped.Request.kind.version === "v1" && wrapped.Request.kind.kind === "Secret"; -// if (isSecret) { -// // convertFromBase64Map modifies it's arg rather than returing a mod'ed copy (ye olde side-effect special, blerg) -// skipped = convertFromBase64Map(wrapped.Raw as unknown as kind.Secret); -// } - -// return { skipped, wrapped }; -// } - -// export function reencodeData(wrapped: PeprMutateRequest, skipped: string[]): KubernetesObject { -// const transformed = clone(wrapped.Raw); - -// const isSecret = wrapped.Request.kind.version === "v1" && wrapped.Request.kind.kind === "Secret"; -// if (isSecret) { -// // convertToBase64Map modifies it's arg rather than returing a mod'ed copy (ye olde side-effect special, blerg) -// convertToBase64Map(transformed as unknown as kind.Secret, skipped); -// } - -// return transformed; -// } - export async function processRequest( bindable: Bindable, wrapped: PeprMutateRequest,