From 4e8549a43bb911072bd0316f7dace7172e5eb1a0 Mon Sep 17 00:00:00 2001 From: Case Wylie Date: Wed, 5 Mar 2025 11:29:48 -0500 Subject: [PATCH 1/5] chore: increase coverage in validate processor (#1891) ## Description We need to increase coverage in validateProcessor. Ensure that: - the requests will be skipped if the filter criteria is wrong - the requests are measured in terms of the webhookTimeout - secrets are converted from base64. ## Related Issue Fixes #1890 Relates to # ## Type of change - [ ] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [x] Other (security config, docs update, etc) ## Checklist before merging - [x] Unit, [Journey](https://github.com/defenseunicorns/pepr/tree/main/journey), [E2E Tests](https://github.com/defenseunicorns/pepr-excellent-examples), [docs](https://github.com/defenseunicorns/pepr/tree/main/docs), [adr](https://github.com/defenseunicorns/pepr/tree/main/adr) added or updated as needed - [x] [Contributor Guide Steps](https://docs.pepr.dev/main/contribute/#submitting-a-pull-request) followed Signed-off-by: Case Wylie Co-authored-by: Sam Mayer --- src/lib/processors/validate-processor.test.ts | 164 +++++++++++++++++- 1 file changed, 161 insertions(+), 3 deletions(-) diff --git a/src/lib/processors/validate-processor.test.ts b/src/lib/processors/validate-processor.test.ts index f54cdfc6..b7550da3 100644 --- a/src/lib/processors/validate-processor.test.ts +++ b/src/lib/processors/validate-processor.test.ts @@ -7,7 +7,34 @@ import { AdmissionRequest, Binding, Filters } from "../types"; import { Event, Operation } from "../enums"; import { PeprValidateRequest } from "../validate-request"; import { clone } from "ramda"; -import * as sut from "./validate-processor"; +import { processRequest, validateProcessor } from "./validate-processor"; +import { ModuleConfig } from "../types"; +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(), +})); const testFilters: Filters = { annotations: {}, @@ -71,7 +98,7 @@ describe("processRequest", () => { const callback = jest.fn().mockImplementation(() => cbResult) as Binding["validateCallback"]; binding = { ...clone(testBinding), validateCallback: callback }; - const result = await sut.processRequest(binding, actionMetadata, peprValidateRequest); + const result = await processRequest(binding, actionMetadata, peprValidateRequest); expect(result).toEqual({ uid: peprValidateRequest.Request.uid, @@ -89,7 +116,7 @@ describe("processRequest", () => { }) as Binding["validateCallback"]; binding = { ...clone(testBinding), validateCallback: callback }; - const result = await sut.processRequest(binding, actionMetadata, peprValidateRequest); + const result = await processRequest(binding, actionMetadata, peprValidateRequest); expect(result).toEqual({ uid: peprValidateRequest.Request.uid, @@ -101,3 +128,134 @@ describe("processRequest", () => { }); }); }); + +describe("validateProcessor", () => { + 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 = testAdmissionRequest; + const reqMetadata = {}; + + const spyStart = jest.spyOn(MeasureWebhookTimeout.prototype, "start"); + + await validateProcessor(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 = { ...testAdmissionRequest, kind: testGroupVersionKind }; + const reqMetadata = {}; + + const spyConvert = jest.spyOn(utils, "convertFromBase64Map"); + + await validateProcessor(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 = testAdmissionRequest; + const reqMetadata = {}; + + const spyStop = jest.spyOn(MeasureWebhookTimeout.prototype, "stop"); + + await validateProcessor(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 20cc0ff3058bbf82c6fd7f616e6a6394f87e2cb8 Mon Sep 17 00:00:00 2001 From: Sam Mayer Date: Wed, 5 Mar 2025 17:03:02 -0600 Subject: [PATCH 2/5] chore(tests): refactor test data generation for RBAC tests (#1898) ## Description This PR is a continued effort to resolve #1329. It uses a generator function to create test data. I had a sync discussion with @cmwylie19 to review how we confidently moved from the hardcoded data to a generator function. See PR state as of (4d900364e905763acf292289640dcbcfa6da0375) for the technical details. ## Related Issue Fixes #1329 Relates to #1878 ## Type of change - [ ] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [x] Other (security config, docs update, etc) ## Checklist before merging - [x] Unit, [Journey](https://github.com/defenseunicorns/pepr/tree/main/journey), [E2E Tests](https://github.com/defenseunicorns/pepr-excellent-examples), [docs](https://github.com/defenseunicorns/pepr/tree/main/docs), [adr](https://github.com/defenseunicorns/pepr/tree/main/adr) added or updated as needed - [x] [Contributor Guide Steps](https://docs.pepr.dev/main/contribute/#submitting-a-pull-request) followed --- src/lib/assets/defaultTestObjects.ts | 634 +++++---------------------- src/lib/assets/rbac.test.ts | 328 ++++++++++---- 2 files changed, 368 insertions(+), 594 deletions(-) diff --git a/src/lib/assets/defaultTestObjects.ts b/src/lib/assets/defaultTestObjects.ts index 28814a98..adec2f99 100644 --- a/src/lib/assets/defaultTestObjects.ts +++ b/src/lib/assets/defaultTestObjects.ts @@ -1,533 +1,137 @@ import { GenericClass } from "kubernetes-fluent-client"; import { Event } from "../enums"; -import { CapabilityExport } from "../types"; -import { describe, beforeEach, jest, it, expect } from "@jest/globals"; +import { Binding, CapabilityExport } from "../types"; +import { defaultFilters } from "../filter/adjudicators/defaultTestObjects"; import { V1PolicyRule as PolicyRule } from "@kubernetes/client-node"; -import fs from "fs"; -import { clusterRole } from "./rbac"; -import * as helpers from "../helpers"; + +export const createMockRbacRule = ( + apiGroups: string[] = ["pepr.dev"], + resources: string[] = ["peprstores"], + verbs: string[] = ["create", "get", "patch", "watch"], +): PolicyRule => ({ + apiGroups, + resources, + verbs, +}); + +export const createMockBinding = ( + kindDetails: { group?: string; version?: string; kind?: string; plural?: string } = {}, + options: { isWatch?: boolean; event?: Event; isFinalize?: boolean } = {}, +): Binding => { + const { group = "pepr.dev", version = "v1", kind = "peprstore", plural = "peprstores" } = kindDetails; + + const { isWatch = false, event = Event.CREATE, isFinalize } = options; + + return { + kind: { group, version, kind, plural }, + isWatch, + ...(isFinalize !== undefined && { isFinalize }), + event, + model: {} as GenericClass, + filters: { ...defaultFilters, regexName: "" }, + }; +}; + +export const createMockCapability = ( + rbacRules = [createMockRbacRule()], + bindings = [createMockBinding()], +): CapabilityExport => ({ + name: "", + hasSchedule: false, + description: "", + rbac: rbacRules, + bindings, +}); export const mockCapabilities: CapabilityExport[] = [ - { - rbac: [ - { - apiGroups: ["pepr.dev"], - resources: ["peprstores"], - verbs: ["create", "get", "patch", "watch"], - }, - ], - bindings: [ - { - kind: { group: "pepr.dev", version: "v1", kind: "peprstore", plural: "peprstores" }, - isWatch: false, - event: Event.CREATE, - model: {} as GenericClass, - filters: { - name: "", - regexName: "", - namespaces: [], - regexNamespaces: [], - labels: {}, - annotations: {}, - deletionTimestamp: false, - }, - }, - ], - hasSchedule: false, - name: "", - description: "", - }, - { - rbac: [ - { - apiGroups: ["apiextensions.k8s.io"], - resources: ["customresourcedefinitions"], - verbs: ["patch", "create"], - }, - ], - bindings: [ - { - kind: { + createMockCapability(), + createMockCapability( + [createMockRbacRule(["apiextensions.k8s.io"], ["customresourcedefinitions"], ["patch", "create"])], + [ + createMockBinding( + { group: "apiextensions.k8s.io", version: "v1", kind: "customresourcedefinition", plural: "customresourcedefinitions", }, - isWatch: false, - isFinalize: false, - event: Event.CREATE, - model: {} as GenericClass, - filters: { - name: "", - regexName: "", - namespaces: [], - regexNamespaces: [], - labels: {}, - annotations: {}, - deletionTimestamp: false, - }, - }, + { isWatch: false, event: Event.CREATE, isFinalize: false }, + ), ], - hasSchedule: false, - name: "", - description: "", - }, - { - rbac: [ - { - apiGroups: [""], - resources: ["namespaces"], - verbs: ["watch"], - }, - ], - bindings: [ - { - kind: { group: "", version: "v1", kind: "namespace", plural: "namespaces" }, - isWatch: true, - isFinalize: false, - event: Event.CREATE, - model: {} as GenericClass, - filters: { - name: "", - regexName: "", - namespaces: [], - regexNamespaces: [], - labels: {}, - annotations: {}, - deletionTimestamp: false, - }, - }, + ), + createMockCapability( + [createMockRbacRule([""], ["namespaces"], ["watch"])], + [ + createMockBinding( + { group: "", version: "v1", kind: "namespace", plural: "namespaces" }, + { isWatch: true, event: Event.CREATE, isFinalize: false }, + ), ], - hasSchedule: false, - name: "", - description: "", - }, - { - rbac: [ - { - apiGroups: [""], - resources: ["configmaps"], - verbs: ["watch"], - }, + ), + createMockCapability( + [createMockRbacRule([""], ["configmaps"], ["watch"])], + [ + createMockBinding( + { group: "", version: "v1", kind: "configmap", plural: "configmaps" }, + { isWatch: true, event: Event.CREATE, isFinalize: false }, + ), ], - bindings: [ - { - kind: { group: "", version: "v1", kind: "configmap", plural: "configmaps" }, - isWatch: true, - isFinalize: false, - event: Event.CREATE, - model: {} as GenericClass, - filters: { - name: "", - regexName: "", - namespaces: [], - regexNamespaces: [], - labels: {}, - annotations: {}, - deletionTimestamp: false, - }, - }, - ], - hasSchedule: false, - name: "", - description: "", - }, + ), ]; -describe("RBAC generation", () => { - beforeEach(() => { - jest.clearAllMocks(); - const mockPackageJsonRBAC = {}; - - jest.spyOn(fs, "readFileSync").mockImplementation((path: unknown) => { - if (typeof path === "string" && path.includes("package.json")) { - return JSON.stringify({ rbac: mockPackageJsonRBAC }); - } - return "{}"; - }); - }); - - it("should generate correct ClusterRole rules in scoped mode", () => { - const result = clusterRole("test-role", mockCapabilities, "scoped", []); - - expect(result.rules).toEqual([ - { - apiGroups: ["pepr.dev"], - resources: ["peprstores"], - verbs: ["create", "get", "patch", "watch"], - }, - { - apiGroups: ["apiextensions.k8s.io"], - resources: ["customresourcedefinitions"], - verbs: ["patch", "create"], - }, - { - apiGroups: [""], - resources: ["namespaces"], - verbs: ["watch"], - }, - { - apiGroups: [""], - resources: ["configmaps"], - verbs: ["watch"], - }, - ]); - }); - - it("should generate a ClusterRole with wildcard rules when not in scoped mode", () => { - const expectedWildcardRules = [ - { - apiGroups: ["*"], - resources: ["*"], - verbs: ["create", "delete", "get", "list", "patch", "update", "watch"], - }, - ]; - - const result = clusterRole("test-role", mockCapabilities, "admin", []); - - expect(result.rules).toEqual(expectedWildcardRules); - }); - - it("should return an empty rules array when capabilities are empty in scoped mode", () => { - const result = clusterRole("test-role", [], "scoped", []); - - expect(result.rules).toEqual([]); - }); - - it("should include finalize verbs if isFinalize is true in scoped mode", () => { - const capabilitiesWithFinalize: CapabilityExport[] = [ - { - rbac: [ - { - apiGroups: ["pepr.dev"], - resources: ["peprstores"], - verbs: ["patch"], - }, - ], - bindings: [ - { - kind: { group: "pepr.dev", version: "v1", kind: "peprstore", plural: "peprstores" }, - isWatch: false, - isFinalize: true, - event: Event.CREATE, - model: {} as GenericClass, - filters: { - name: "", - regexName: "", - namespaces: [], - regexNamespaces: [], - labels: {}, - annotations: {}, - deletionTimestamp: false, - }, - }, - ], - hasSchedule: false, - name: "", - description: "", - }, - ]; - - const result = clusterRole( - "test-role", - capabilitiesWithFinalize, - "scoped", - capabilitiesWithFinalize.flatMap(c => c.rbac).filter((rule): rule is PolicyRule => rule !== undefined), - ); - - expect(result.rules).toEqual([ - { - apiGroups: ["pepr.dev"], - resources: ["peprstores"], - verbs: ["patch"], - }, - { - apiGroups: ["apiextensions.k8s.io"], - resources: ["customresourcedefinitions"], - verbs: ["patch", "create"], - }, - ]); - }); - - it("should deduplicate verbs and resources in rules", () => { - const capabilitiesWithDuplicates: CapabilityExport[] = [ - { - rbac: [ - { - apiGroups: ["pepr.dev"], - resources: ["peprstores"], - verbs: ["create", "get"], - }, - ], - bindings: [ - { - kind: { group: "pepr.dev", version: "v1", kind: "peprlog", plural: "peprlogs" }, - isWatch: false, - event: Event.CREATE, - model: {} as GenericClass, - filters: { - name: "", - regexName: "", - namespaces: [], - regexNamespaces: [], - labels: {}, - annotations: {}, - deletionTimestamp: false, - }, - }, - ], - hasSchedule: false, - name: "", - description: "", - }, - { - rbac: [ - { - apiGroups: ["pepr.dev"], - resources: ["peprstores"], - verbs: ["get", "patch"], - }, - ], - bindings: [ - { - kind: { group: "pepr.dev", version: "v1", kind: "peprlog", plural: "peprlogs" }, - isWatch: false, - event: Event.CREATE, - model: {} as GenericClass, - filters: { - name: "", - regexName: "", - namespaces: [], - regexNamespaces: [], - labels: {}, - annotations: {}, - deletionTimestamp: false, - }, - }, - ], - hasSchedule: false, - name: "", - description: "", - }, - ]; - - const result = clusterRole( - "test-role", - capabilitiesWithDuplicates, - "scoped", - capabilitiesWithDuplicates.flatMap(c => c.rbac).filter((rule): rule is PolicyRule => rule !== undefined), - ); - - // Filter out only the rules for 'pepr.dev' and 'peprstores' - const filteredRules = result.rules?.filter( - rule => rule.apiGroups?.includes("pepr.dev") && rule.resources?.includes("peprstores"), - ); - expect(filteredRules).toEqual([ - { - apiGroups: ["pepr.dev"], - resources: ["peprstores"], - verbs: ["create", "get", "patch", "watch"], - }, - ]); - }); -}); -describe("clusterRole", () => { - // Mocking the readRBACFromPackageJson function to return null - jest.mock("./rbac", () => ({ - ...(jest.requireActual("./rbac") as object), - readRBACFromPackageJson: jest.fn(() => null), - })); - - // Mocking createRBACMap to isolate the behavior of clusterRole function - jest.mock("../helpers", () => ({ - ...(jest.requireActual("../helpers") as object), - createRBACMap: jest.fn(), - })); - - beforeEach(() => { - jest.clearAllMocks(); - jest.restoreAllMocks(); - }); - - it("should handle keys with less than 3 segments and set group to an empty string", () => { - jest.spyOn(helpers, "createRBACMap").mockReturnValue({ - nodes: { - plural: "nodes", - verbs: ["get"], - }, - }); - - const capabilitiesWithShortKey: CapabilityExport[] = [ - { - rbac: [ - { - apiGroups: [""], - resources: ["nodes"], - verbs: ["get"], - }, - ], - bindings: [ - { - kind: { group: "", version: "v1", kind: "node", plural: "nodes" }, - isWatch: false, - event: Event.CREATE, - model: {} as GenericClass, - filters: { - name: "", - regexName: "", - namespaces: [], - regexNamespaces: [], - labels: {}, - annotations: {}, - deletionTimestamp: false, - }, - }, - ], - hasSchedule: false, - name: "", - description: "", - }, - ]; - - const result = clusterRole( - "test-role", - capabilitiesWithShortKey, - "scoped", - capabilitiesWithShortKey.flatMap(c => c.rbac).filter((rule): rule is PolicyRule => rule !== undefined), - ); - - expect(result.rules).toEqual([ - { - apiGroups: [""], - resources: ["nodes"], - verbs: ["get"], - }, - ]); - }); - - it("should handle keys with 3 or more segments and set group correctly", () => { - jest.spyOn(helpers, "createRBACMap").mockReturnValue({ - "apps/v1/deployments": { - plural: "deployments", - verbs: ["create"], - }, - }); - - const capabilitiesWithLongKey: CapabilityExport[] = [ - { - rbac: [ - { - apiGroups: ["apps"], - resources: ["deployments"], - verbs: ["create"], - }, - ], - bindings: [ - { - kind: { group: "apps", version: "v1", kind: "deployment", plural: "deployments" }, - isWatch: false, - event: Event.CREATE, - model: {} as GenericClass, - filters: { - name: "", - regexName: "", - namespaces: [], - regexNamespaces: [], - labels: {}, - annotations: {}, - deletionTimestamp: false, - }, - }, - ], - hasSchedule: false, - name: "", - description: "", - }, - ]; - - const result = clusterRole( - "test-role", - capabilitiesWithLongKey, - "scoped", - capabilitiesWithLongKey.flatMap(c => c.rbac).filter((rule): rule is PolicyRule => rule !== undefined), - ); - - expect(result.rules).toEqual([ - { - apiGroups: ["apps"], - resources: ["deployments"], - verbs: ["create"], - }, - ]); - }); - - it("should handle non-array custom RBAC by defaulting to an empty array", () => { - // Mock readRBACFromPackageJson to return a non-array value - jest.spyOn(fs, "readFileSync").mockImplementation(() => { - return JSON.stringify({ - pepr: { - rbac: "not-an-array", // Simulate invalid RBAC structure - }, - }); - }); - - const result = clusterRole( - "test-role", - mockCapabilities, - "scoped", - mockCapabilities.flatMap(c => c.rbac).filter((rule): rule is PolicyRule => rule !== undefined), - ); - - // The result should only contain rules from the capabilities, not from the invalid custom RBAC - expect(result.rules).toEqual([ - { - apiGroups: ["pepr.dev"], - resources: ["peprstores"], - verbs: ["create", "get", "patch", "watch"], - }, - { - apiGroups: ["apiextensions.k8s.io"], - resources: ["customresourcedefinitions"], - verbs: ["patch", "create"], - }, - { - apiGroups: [""], - resources: ["namespaces"], - verbs: ["watch"], - }, - { - apiGroups: [""], - resources: ["configmaps"], - verbs: ["watch"], - }, - ]); - }); - - it("should default to an empty verbs array if rule.verbs is undefined", () => { - // Simulate a custom RBAC rule with empty verbs - const customRbacWithNoVerbs: PolicyRule[] = [ - { - apiGroups: ["pepr.dev"], - resources: ["customresources"], - verbs: [], // Set verbs to an empty array to satisfy the V1PolicyRule type - }, - ]; +export const capabilityWithFinalize: CapabilityExport[] = [ + createMockCapability( + [createMockRbacRule(["pepr.dev"], ["peprstores"], ["patch"])], + [ + createMockBinding( + { group: "pepr.dev", version: "v1", kind: "peprstore", plural: "peprstores" }, + { isWatch: false, event: Event.CREATE, isFinalize: true }, + ), + ], + ), +]; - jest.spyOn(fs, "readFileSync").mockImplementation(() => { - return JSON.stringify({ - pepr: { - rbac: customRbacWithNoVerbs, - }, - }); - }); +export const capabilityWithDuplicates: CapabilityExport[] = [ + createMockCapability( + [createMockRbacRule(["pepr.dev"], ["peprstores"], ["create", "get"])], + [ + createMockBinding( + { group: "pepr.dev", version: "v1", kind: "peprlog", plural: "peprlogs" }, + { isWatch: false, event: Event.CREATE }, + ), + ], + ), + createMockCapability( + [createMockRbacRule(["pepr.dev"], ["peprstores"], ["get", "patch"])], + [ + createMockBinding( + { group: "pepr.dev", version: "v1", kind: "peprlog", plural: "peprlogs" }, + { isWatch: false, event: Event.CREATE }, + ), + ], + ), +]; - const result = clusterRole("test-role", mockCapabilities, "scoped", customRbacWithNoVerbs); +export const capabilityWithShortKey: CapabilityExport[] = [ + createMockCapability( + [createMockRbacRule([""], ["nodes"], ["get"])], + [ + createMockBinding( + { group: "", version: "v1", kind: "node", plural: "nodes" }, + { isWatch: false, event: Event.CREATE }, + ), + ], + ), +]; - // Check that the verbs array is empty for the custom RBAC rule - expect(result.rules).toContainEqual({ - apiGroups: ["pepr.dev"], - resources: ["customresources"], - verbs: [], - }); - }); -}); +export const capabilityWithLongKey: CapabilityExport[] = [ + createMockCapability( + [createMockRbacRule(["apps"], ["deployments"], ["create"])], + [ + createMockBinding( + { group: "apps", version: "v1", kind: "deployment", plural: "deployments" }, + { isWatch: false, event: Event.CREATE }, + ), + ], + ), +]; diff --git a/src/lib/assets/rbac.test.ts b/src/lib/assets/rbac.test.ts index d465f4fc..546bc366 100644 --- a/src/lib/assets/rbac.test.ts +++ b/src/lib/assets/rbac.test.ts @@ -1,66 +1,23 @@ // SPDX-License-Identifier: Apache-2.0 // SPDX-FileCopyrightText: 2023-Present The Pepr Authors import { clusterRole, clusterRoleBinding, storeRole, serviceAccount, storeRoleBinding } from "./rbac"; -import { it, describe, expect, beforeEach, jest } from "@jest/globals"; +import { it, describe, expect, jest } from "@jest/globals"; import { V1PolicyRule as PolicyRule } from "@kubernetes/client-node"; import fs from "fs"; -import { mockCapabilities } from "./defaultTestObjects"; - -describe("RBAC generation with mocked package.json", () => { - beforeEach(() => { - jest.clearAllMocks(); - jest.spyOn(fs, "readFileSync").mockImplementation((path: unknown) => { - if (typeof path === "string" && path.includes("package.json")) { - return JSON.stringify({ - pepr: { - rbac: [ - { - apiGroups: ["pepr.dev"], - resources: ["pods"], - verbs: ["get", "list"], - }, - { - apiGroups: ["pepr.dev"], - resources: ["pods"], - verbs: ["list"], - }, - { - apiGroups: ["apps"], - resources: ["deployments"], - verbs: ["create", "delete"], - }, - ], - }, - }); - } - return "{}"; - }); - }); - - it("should generate a ClusterRole with wildcard rules when not in scoped mode", () => { - const expectedWildcardRules = [ - { - apiGroups: ["*"], - resources: ["*"], - verbs: ["create", "delete", "get", "list", "patch", "update", "watch"], - }, - ]; - - const result = clusterRole( - "test-role", - mockCapabilities, - "admin", - mockCapabilities.flatMap(c => c.rbac).filter((rule): rule is PolicyRule => rule !== undefined), - ); - - expect(result.rules).toEqual(expectedWildcardRules); - }); -}); +import { kind } from "kubernetes-fluent-client"; +import * as helpers from "../helpers"; +import { + capabilityWithDuplicates, + mockCapabilities, + capabilityWithFinalize, + capabilityWithLongKey, + capabilityWithShortKey, +} from "./defaultTestObjects"; -describe("clusterRoleBinding", () => { +describe("RBAC Resource Creation", () => { it("should create a ClusterRoleBinding with the specified name", () => { const roleName = "test-cluster-role"; - const expectedClusterRoleBinding = { + const expectedClusterRoleBinding: kind.ClusterRoleBinding = { apiVersion: "rbac.authorization.k8s.io/v1", kind: "ClusterRoleBinding", metadata: { name: roleName }, @@ -82,30 +39,10 @@ describe("clusterRoleBinding", () => { expect(result).toEqual(expectedClusterRoleBinding); }); -}); - -describe("serviceAccount", () => { - it("should create a ServiceAccount with the specified name", () => { - const accountName = "test-service-account"; - const expectedServiceAccount = { - apiVersion: "v1", - kind: "ServiceAccount", - metadata: { - name: accountName, - namespace: "pepr-system", - }, - }; - - const result = serviceAccount(accountName); - - expect(result).toEqual(expectedServiceAccount); - }); -}); -describe("storeRole", () => { it("should create a Role for managing peprstores with the specified name", () => { const roleName = "test-role"; - const expectedRole = { + const expectedRole: kind.Role = { apiVersion: "rbac.authorization.k8s.io/v1", kind: "Role", metadata: { @@ -126,12 +63,10 @@ describe("storeRole", () => { expect(result).toEqual(expectedRole); }); -}); -describe("storeRoleBinding", () => { it("should create a RoleBinding for the specified Role", () => { const roleName = "test-role"; - const expectedRoleBinding = { + const expectedRoleBinding: kind.RoleBinding = { apiVersion: "rbac.authorization.k8s.io/v1", kind: "RoleBinding", metadata: { @@ -156,4 +91,239 @@ describe("storeRoleBinding", () => { expect(result).toEqual(expectedRoleBinding); }); + + it("should create a ServiceAccount with the specified name", () => { + const accountName = "test-service-account"; + const expectedServiceAccount: kind.ServiceAccount = { + apiVersion: "v1", + kind: "ServiceAccount", + metadata: { + name: accountName, + namespace: "pepr-system", + }, + }; + + const result = serviceAccount(accountName); + + expect(result).toEqual(expectedServiceAccount); + }); +}); + +describe("RBAC Rule Processing", () => { + it("should deduplicate verbs and resources in rules", () => { + const result = clusterRole( + "test-role", + capabilityWithDuplicates, + "scoped", + capabilityWithDuplicates.flatMap(c => c.rbac).filter((rule): rule is PolicyRule => rule !== undefined), + ); + + // Filter out only the rules for 'pepr.dev' and 'peprstores' + const filteredRules = result.rules?.filter( + rule => rule.apiGroups?.includes("pepr.dev") && rule.resources?.includes("peprstores"), + ); + + expect(filteredRules).toEqual([ + { + apiGroups: ["pepr.dev"], + resources: ["peprstores"], + verbs: ["create", "get", "patch", "watch"], + }, + ]); + }); + it("should default to an empty verbs array if rule.verbs is undefined", () => { + // Simulate a custom RBAC rule with empty verbs + const customRbacWithNoVerbs: PolicyRule[] = [ + { + apiGroups: ["pepr.dev"], + resources: ["customresources"], + verbs: [], // Set verbs to an empty array to satisfy the V1PolicyRule type + }, + ]; + + jest.spyOn(fs, "readFileSync").mockImplementation(() => { + return JSON.stringify({ + pepr: { + rbac: customRbacWithNoVerbs, + }, + }); + }); + + const result = clusterRole("test-role", mockCapabilities, "scoped", customRbacWithNoVerbs); + + // Check that the verbs array is empty for the custom RBAC rule + expect(result.rules).toContainEqual({ + apiGroups: ["pepr.dev"], + resources: ["customresources"], + verbs: [], + }); + }); + it("should handle non-array custom RBAC by defaulting to an empty array", () => { + // Mock readRBACFromPackageJson to return a non-array value + jest.spyOn(fs, "readFileSync").mockImplementation(() => { + return JSON.stringify({ + pepr: { + rbac: "not-an-array", // Simulate invalid RBAC structure + }, + }); + }); + + const expected: PolicyRule[] = [ + { + apiGroups: ["pepr.dev"], + resources: ["peprstores"], + verbs: ["create", "get", "patch", "watch"], + }, + { + apiGroups: ["apiextensions.k8s.io"], + resources: ["customresourcedefinitions"], + verbs: ["patch", "create"], + }, + { + apiGroups: [""], + resources: ["namespaces"], + verbs: ["watch"], + }, + { + apiGroups: [""], + resources: ["configmaps"], + verbs: ["watch"], + }, + ]; + + const result = clusterRole( + "test-role", + mockCapabilities, + "scoped", + mockCapabilities.flatMap(c => c.rbac).filter((rule): rule is PolicyRule => rule !== undefined), + ); + + // The result should only contain rules from the capabilities, not from the invalid custom RBAC + expect(result.rules).toEqual(expected); + }); +}); + +describe("ClusterRole Generation", () => { + it("should generate a ClusterRole with wildcard rules when not in scoped mode", () => { + const expectedWildcardRules = [ + { + apiGroups: ["*"], + resources: ["*"], + verbs: ["create", "delete", "get", "list", "patch", "update", "watch"], + }, + ]; + + const result = clusterRole("test-role", mockCapabilities, "admin", []); + + expect(result.rules).toEqual(expectedWildcardRules); + }); + it("should generate correct ClusterRole rules in scoped mode", () => { + const expected: PolicyRule[] = [ + { + apiGroups: ["pepr.dev"], + resources: ["peprstores"], + verbs: ["create", "get", "patch", "watch"], + }, + { + apiGroups: ["apiextensions.k8s.io"], + resources: ["customresourcedefinitions"], + verbs: ["patch", "create"], + }, + { + apiGroups: [""], + resources: ["namespaces"], + verbs: ["watch"], + }, + { + apiGroups: [""], + resources: ["configmaps"], + verbs: ["watch"], + }, + ]; + const result = clusterRole("test-role", mockCapabilities, "scoped", []); + + expect(result.rules).toEqual(expected); + }); + + it("should include finalize verbs if isFinalize is true in scoped mode", () => { + const expected: PolicyRule[] = [ + { + apiGroups: ["pepr.dev"], + resources: ["peprstores"], + verbs: ["patch"], + }, + { + apiGroups: ["apiextensions.k8s.io"], + resources: ["customresourcedefinitions"], + verbs: ["patch", "create"], + }, + ]; + + const result = clusterRole( + "test-role", + capabilityWithFinalize, + "scoped", + capabilityWithFinalize.flatMap(c => c.rbac).filter((rule): rule is PolicyRule => rule !== undefined), + ); + + expect(result.rules).toEqual(expected); + }); + it("should return an empty rules array when capabilities are empty in scoped mode", () => { + const result = clusterRole("test-role", [], "scoped", []); + + expect(result.rules).toEqual([]); + }); +}); + +describe("RBAC Key Handling", () => { + it("should handle keys with 3 or more segments and set group correctly", () => { + jest.spyOn(helpers, "createRBACMap").mockReturnValue({ + "apps/v1/deployments": { + plural: "deployments", + verbs: ["create"], + }, + }); + + const expected: PolicyRule[] = [ + { + apiGroups: ["apps"], + resources: ["deployments"], + verbs: ["create"], + }, + ]; + + const result = clusterRole( + "test-role", + capabilityWithLongKey, + "scoped", + capabilityWithLongKey.flatMap(c => c.rbac).filter((rule): rule is PolicyRule => rule !== undefined), + ); + + expect(result.rules).toEqual(expected); + }); + + it("should handle keys with less than 3 segments and set group to an empty string", () => { + jest.spyOn(helpers, "createRBACMap").mockReturnValue({ + nodes: { + plural: "nodes", + verbs: ["get"], + }, + }); + + const expected: PolicyRule[] = [ + { + apiGroups: [""], + resources: ["nodes"], + verbs: ["get"], + }, + ]; + const result = clusterRole( + "test-role", + capabilityWithShortKey, + "scoped", + capabilityWithShortKey.flatMap(c => c.rbac).filter((rule): rule is PolicyRule => rule !== undefined), + ); + + expect(result.rules).toEqual(expected); + }); }); From f770185b89386bf2c108bcc1cadfdd28824879ac Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Wed, 5 Mar 2025 20:06:38 -0500 Subject: [PATCH 3/5] chore: bump trufflesecurity/trufflehog from 3.88.14 to 3.88.15 (#1899) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bumps [trufflesecurity/trufflehog](https://github.com/trufflesecurity/trufflehog) from 3.88.14 to 3.88.15.
Release notes

Sourced from trufflesecurity/trufflehog's releases.

v3.88.15

What's Changed

Full Changelog: https://github.com/trufflesecurity/trufflehog/compare/v3.88.14...v3.88.15

Commits

[![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=trufflesecurity/trufflehog&package-manager=github_actions&previous-version=3.88.14&new-version=3.88.15)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) ---
Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- .github/workflows/secret-scan.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/secret-scan.yml b/.github/workflows/secret-scan.yml index 69a1499e..705dabfd 100644 --- a/.github/workflows/secret-scan.yml +++ b/.github/workflows/secret-scan.yml @@ -23,6 +23,6 @@ jobs: with: fetch-depth: 0 - name: Default Secret Scanning - uses: trufflesecurity/trufflehog@7dc056a193116ba8d82154bf0549381c8fb8545c # main + uses: trufflesecurity/trufflehog@ca270a7e14a3542229c2cadf1d408ecac1455815 # main with: extra_args: --debug --no-verification # Warn on potential violations From aa0e4a15a8e11ced38d730325573be2a58980f7b Mon Sep 17 00:00:00 2001 From: Case Wylie Date: Thu, 6 Mar 2025 10:06:41 -0500 Subject: [PATCH 4/5] chore: account for early exit and stop timer (#1895) ## Description There are several places in the mutate process where the request will exit early before the final return of the response. This leads to metricCollector incrementing the count for mutation timeouts which is inaccurate. We need to stop the timer to ensure we have accurate metrics. ## Related Issue Fixes #1894 Relates to # ## 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] Unit, [Journey](https://github.com/defenseunicorns/pepr/tree/main/journey), [E2E Tests](https://github.com/defenseunicorns/pepr-excellent-examples), [docs](https://github.com/defenseunicorns/pepr/tree/main/docs), [adr](https://github.com/defenseunicorns/pepr/tree/main/adr) added or updated as needed - [x] [Contributor Guide Steps](https://docs.pepr.dev/main/contribute/#submitting-a-pull-request) followed Signed-off-by: Case Wylie --- src/lib/processors/mutate-processor.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/lib/processors/mutate-processor.ts b/src/lib/processors/mutate-processor.ts index 4d1788be..29a656f6 100644 --- a/src/lib/processors/mutate-processor.ts +++ b/src/lib/processors/mutate-processor.ts @@ -186,6 +186,7 @@ export async function mutateProcessor( for (const bindable of bindables) { ({ wrapped, response } = await processRequest(bindable, wrapped, response)); if (config.onError === OnError.REJECT && response?.warnings!.length > 0) { + webhookTimer.stop(); return response; } } @@ -196,11 +197,13 @@ 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; } // delete operations can't be mutate, just return before the transformation if (req.operation === "DELETE") { + webhookTimer.stop(); return response; } From 666bdb2b6d05836c9c294f3fb4b91b6a0d43eec9 Mon Sep 17 00:00:00 2001 From: Case Wylie Date: Thu, 6 Mar 2025 11:27:47 -0500 Subject: [PATCH 5/5] chore: webhook timer coverage in mutate-processor test (#1897) ## Description This is very hard to test because the Capability does not have actual bindings. Because of that the test stops [here](https://github.com/defenseunicorns/pepr/blob/4e8549a43bb911072bd0316f7dace7172e5eb1a0/src/lib/processors/mutate-processor.ts#L197) and returns early. The only meaningful tests I could do was to ensure the timer start and stop were called. There is already too much mocking in this file I cannot get any assertions to pass for base64 calls here. **UPDATE**: was able to validate the first decodeData was called by shuffling files around and mocking the call in `mutate-process.test.ts`. The rest of the test is untestable due to no bindings in the capabilities Needs https://github.com/defenseunicorns/pepr/pull/1895 to pass. ## Related Issue Fixes #1892 Relates to # ## Type of change - [ ] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [x] Other (security config, docs update, etc) ## Checklist before merging - [ ] Unit, [Journey](https://github.com/defenseunicorns/pepr/tree/main/journey), [E2E Tests](https://github.com/defenseunicorns/pepr-excellent-examples), [docs](https://github.com/defenseunicorns/pepr/tree/main/docs), [adr](https://github.com/defenseunicorns/pepr/tree/main/adr) added or updated as needed - [ ] [Contributor Guide Steps](https://docs.pepr.dev/main/contribute/#submitting-a-pull-request) followed --------- Signed-off-by: Case Wylie --- src/lib/assets/defaultTestObjects.ts | 19 +- src/lib/processors/decode-utils.test.ts | 101 ++++++++++ src/lib/processors/decode-utils.ts | 31 +++ src/lib/processors/mutate-processor.test.ts | 205 +++++++++++--------- src/lib/processors/mutate-processor.ts | 34 +--- 5 files changed, 262 insertions(+), 128 deletions(-) create mode 100644 src/lib/processors/decode-utils.test.ts create mode 100644 src/lib/processors/decode-utils.ts 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/processors/decode-utils.test.ts b/src/lib/processors/decode-utils.test.ts new file mode 100644 index 00000000..6be065c2 --- /dev/null +++ b/src/lib/processors/decode-utils.test.ts @@ -0,0 +1,101 @@ +import { beforeEach, describe, expect, it, jest } from "@jest/globals"; +import { convertFromBase64Map, convertToBase64Map } from "../utils"; +import { PeprMutateRequest } from "../mutate-request"; +import { decodeData, reencodeData } from "./decode-utils"; +import { createMockAdmissionRequest } from "../assets/defaultTestObjects"; + +jest.mock("../utils"); + +const defaultAdmissionRequest = createMockAdmissionRequest(); + +const defaultPeprMutateRequest = (admissionRequest = defaultAdmissionRequest) => + new PeprMutateRequest(admissionRequest); + +const mockConvertToBase64Map = jest.mocked(convertToBase64Map); +const mockConvertFromBase64Map = jest.mocked(convertFromBase64Map); + +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/processors/decode-utils.ts b/src/lib/processors/decode-utils.ts new file mode 100644 index 00000000..54a6e8be --- /dev/null +++ b/src/lib/processors/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 06dc5e12..4967fe04 100644 --- a/src/lib/processors/mutate-processor.test.ts +++ b/src/lib/processors/mutate-processor.test.ts @@ -5,19 +5,52 @@ 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 { base64Encode } from "../utils"; import { GenericClass, KubernetesObject } from "kubernetes-fluent-client"; import { MutateResponse } from "../k8s"; import { OnError } from "../../cli/init/enums"; -import { updateResponsePatchAndWarnings } from "./mutate-processor"; +import { + updateResponsePatchAndWarnings, + Bindable, + mutateProcessor, + updateStatus, + logMutateErrorMessage, + 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(), + 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); -const mockConvertToBase64Map = jest.mocked(convertToBase64Map); const defaultModuleConfig: ModuleConfig = { uuid: "test-uuid", @@ -51,6 +84,10 @@ const defaultPeprMutateRequest = (admissionRequest = defaultAdmissionRequest) => beforeEach(() => { jest.resetAllMocks(); + (decodeData as jest.Mock).mockReturnValue({ + wrapped: { mockWrapped: true }, + skipped: [], + }); }); describe("updateStatus", () => { @@ -60,7 +97,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 +114,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,96 +133,11 @@ 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); }); }); -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 } = sut.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 } = sut.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 = sut.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 = sut.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, @@ -206,7 +158,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 +178,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 +202,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 +227,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 +264,62 @@ describe("updateResponsePatchAndWarnings", () => { expect(localMutateResponse.warnings).not.toBeDefined(); }); }); + +describe("mutateProcessor", () => { + let config: ModuleConfig; + let capability: Capability; + beforeEach(() => { + jest.clearAllMocks(); + config = { + webhookTimeout: 11, + uuid: "some-uuid", + alwaysIgnore: {}, + }; + 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 = {}; + + const spyStart = jest.spyOn(MeasureWebhookTimeout.prototype, "start"); + + await mutateProcessor(config, [capability], req, reqMetadata); + + expect(spyStart).toHaveBeenCalledWith(config.webhookTimeout); + spyStart.mockRestore(); + }); + + it("should stop the timer after processing", async () => { + 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 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 29a656f6..a35b55dc 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,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,