From 67aa06ca7eb24ec9b1f829c889b12fdf89a88c48 Mon Sep 17 00:00:00 2001 From: Sam Mayer Date: Mon, 4 Nov 2024 12:13:04 -0600 Subject: [PATCH 1/5] Initial refactor to separate processing function --- src/lib/validate-processor.ts | 120 ++++++++++++++++++++-------------- 1 file changed, 70 insertions(+), 50 deletions(-) diff --git a/src/lib/validate-processor.ts b/src/lib/validate-processor.ts index 43571a330..d1f2bcbc7 100644 --- a/src/lib/validate-processor.ts +++ b/src/lib/validate-processor.ts @@ -1,12 +1,12 @@ // SPDX-License-Identifier: Apache-2.0 // SPDX-FileCopyrightText: 2023-Present The Pepr Authors -import { kind } from "kubernetes-fluent-client"; +import { kind, KubernetesObject } from "kubernetes-fluent-client"; import { Capability } from "./capability"; import { shouldSkipRequest } from "./filter/shouldSkipRequest"; import { ValidateResponse } from "./k8s"; -import { AdmissionRequest } from "./types"; +import { AdmissionRequest, Binding } from "./types"; import Log from "./logger"; import { convertFromBase64Map } from "./utils"; import { PeprValidateRequest } from "./validate-request"; @@ -19,7 +19,7 @@ export async function validateProcessor( reqMetadata: Record, ): Promise { const wrapped = new PeprValidateRequest(req); - const response: ValidateResponse[] = []; + let response: ValidateResponse[] = []; // If the resource is a secret, decode the data const isSecret = req.kind.version === "v1" && req.kind.kind === "Secret"; @@ -27,59 +27,79 @@ export async function validateProcessor( convertFromBase64Map(wrapped.Raw as unknown as kind.Secret); } + const localWrapped = wrapped; + Log.info(reqMetadata, `Processing validation request`); for (const { name, bindings, namespaces } of capabilities) { const actionMetadata = { ...reqMetadata, name }; - for (const action of bindings) { - // Skip this action if it's not a validation action - if (!action.validateCallback) { - continue; - } - - const localResponse: ValidateResponse = { - uid: req.uid, - allowed: true, // Assume it's allowed until a validation check fails - }; - - // Continue to the next action without doing anything if this one should be skipped - const shouldSkip = shouldSkipRequest(action, req, namespaces, config?.alwaysIgnore?.namespaces); - if (shouldSkip !== "") { - Log.debug(shouldSkip); - continue; - } - - const label = action.validateCallback.name; - Log.info(actionMetadata, `Processing validation action (${label})`); - - try { - // Run the validation callback, if it fails set allowed to false - const resp = await action.validateCallback(wrapped); - localResponse.allowed = resp.allowed; - - // If the validation callback returned a status code or message, set it in the Response - if (resp.statusCode || resp.statusMessage) { - localResponse.status = { - code: resp.statusCode || 400, - message: resp.statusMessage || `Validation failed for ${name}`, - }; - } - - Log.info(actionMetadata, `Validation action complete (${label}): ${resp.allowed ? "allowed" : "denied"}`); - } catch (e) { - // If any validation throws an error, note the failure in the Response - Log.error(actionMetadata, `Action failed: ${JSON.stringify(e)}`); - localResponse.allowed = false; - localResponse.status = { - code: 500, - message: `Action failed with error: ${JSON.stringify(e)}`, - }; - return [localResponse]; - } - response.push(localResponse); - } + response = await Promise.all( + bindings + .map(binding => processBinding(response, binding, req, namespaces, config, localWrapped, actionMetadata)) + .filter((promise): promise is Promise => promise !== undefined), + ); } return response; } + +// eslint-disable-next-line max-statements +const processBinding = async ( + response: ValidateResponse[], + binding: Binding, + req: AdmissionRequest, + namespaces: string[], + config: ModuleConfig, + wrapped: PeprValidateRequest, + actionMetadata: { name: string }, +): Promise => { + if (binding.validateCallback === undefined) { + // Skip this action if it's not a validation action + return undefined; + } + + if (!binding.validateCallback) { + return undefined; + } + + const localResponse: ValidateResponse = { + uid: req.uid, + allowed: true, // Assume it's allowed until a validation check fails + }; + + // Continue to the next action without doing anything if this one should be skipped + const shouldSkip = shouldSkipRequest(binding, req, namespaces, config?.alwaysIgnore?.namespaces); + if (shouldSkip !== "") { + Log.debug(shouldSkip); + return undefined; + } + + const label = binding.validateCallback.name; + Log.info(actionMetadata, `Processing validation action (${label})`); + + try { + // Run the validation callback, if it fails set allowed to false + const resp = await binding.validateCallback(wrapped); + localResponse.allowed = resp.allowed; + + // If the validation callback returned a status code or message, set it in the Response + if (resp.statusCode || resp.statusMessage) { + localResponse.status = { + code: resp.statusCode || 400, + message: resp.statusMessage || `Validation failed for ${name}`, + }; + } + + Log.info(actionMetadata, `Validation action complete (${label}): ${resp.allowed ? "allowed" : "denied"}`); + } catch (e) { + // If any validation throws an error, note the failure in the Response + Log.error(actionMetadata, `Action failed: ${JSON.stringify(e)}`); + localResponse.allowed = false; + localResponse.status = { + code: 500, + message: `Action failed with error: ${JSON.stringify(e)}`, + }; + } + return localResponse; +}; From d9692ac071a7d467ad60401995e63eb4f64992dc Mon Sep 17 00:00:00 2001 From: Sam Mayer Date: Mon, 4 Nov 2024 13:17:10 -0600 Subject: [PATCH 2/5] Ignore test with pattern filter instead of using uncommon file-extension --- package.json | 2 +- src/sdk/{cosign.e2e.tezt.ts => cosign.e2e.test.ts} | 0 2 files changed, 1 insertion(+), 1 deletion(-) rename src/sdk/{cosign.e2e.tezt.ts => cosign.e2e.test.ts} (100%) diff --git a/package.json b/package.json index 1d6797d28..ccf483458 100644 --- a/package.json +++ b/package.json @@ -26,7 +26,7 @@ "build": "tsc && node build.mjs && npm pack", "build:image": "npm run build && docker buildx build --output type=docker --tag pepr:dev .", "test": "npm run test:unit && npm run test:journey", - "test:unit": "npm run gen-data-json && jest src --coverage --detectOpenHandles --coverageDirectory=./coverage", + "test:unit": "npm run gen-data-json && jest src --coverage --detectOpenHandles --coverageDirectory=./coverage --testPathIgnorePatterns='cosign.e2e.test.ts'", "test:journey": "npm run test:journey:k3d && npm run build && npm run test:journey:image && npm run test:journey:run", "test:journey:prep": "if [ ! -d ./pepr-upgrade-test ]; then git clone https://github.com/defenseunicorns/pepr-upgrade-test.git ; fi", "test:journey-wasm": "npm run test:journey:k3d && npm run build && npm run test:journey:image && npm run test:journey:run-wasm", diff --git a/src/sdk/cosign.e2e.tezt.ts b/src/sdk/cosign.e2e.test.ts similarity index 100% rename from src/sdk/cosign.e2e.tezt.ts rename to src/sdk/cosign.e2e.test.ts From 3d867b7400c5f66c05c2ca470cbf6f072fc714ec Mon Sep 17 00:00:00 2001 From: Sam Mayer Date: Mon, 4 Nov 2024 13:10:16 -0600 Subject: [PATCH 3/5] Revert "Initial refactor to separate processing function" This reverts commit 67aa06ca7eb24ec9b1f829c889b12fdf89a88c48. --- src/lib/validate-processor.ts | 120 ++++++++++++++-------------------- 1 file changed, 50 insertions(+), 70 deletions(-) diff --git a/src/lib/validate-processor.ts b/src/lib/validate-processor.ts index d1f2bcbc7..43571a330 100644 --- a/src/lib/validate-processor.ts +++ b/src/lib/validate-processor.ts @@ -1,12 +1,12 @@ // SPDX-License-Identifier: Apache-2.0 // SPDX-FileCopyrightText: 2023-Present The Pepr Authors -import { kind, KubernetesObject } from "kubernetes-fluent-client"; +import { kind } from "kubernetes-fluent-client"; import { Capability } from "./capability"; import { shouldSkipRequest } from "./filter/shouldSkipRequest"; import { ValidateResponse } from "./k8s"; -import { AdmissionRequest, Binding } from "./types"; +import { AdmissionRequest } from "./types"; import Log from "./logger"; import { convertFromBase64Map } from "./utils"; import { PeprValidateRequest } from "./validate-request"; @@ -19,7 +19,7 @@ export async function validateProcessor( reqMetadata: Record, ): Promise { const wrapped = new PeprValidateRequest(req); - let response: ValidateResponse[] = []; + const response: ValidateResponse[] = []; // If the resource is a secret, decode the data const isSecret = req.kind.version === "v1" && req.kind.kind === "Secret"; @@ -27,79 +27,59 @@ export async function validateProcessor( convertFromBase64Map(wrapped.Raw as unknown as kind.Secret); } - const localWrapped = wrapped; - Log.info(reqMetadata, `Processing validation request`); for (const { name, bindings, namespaces } of capabilities) { const actionMetadata = { ...reqMetadata, name }; - response = await Promise.all( - bindings - .map(binding => processBinding(response, binding, req, namespaces, config, localWrapped, actionMetadata)) - .filter((promise): promise is Promise => promise !== undefined), - ); - } - - return response; -} - -// eslint-disable-next-line max-statements -const processBinding = async ( - response: ValidateResponse[], - binding: Binding, - req: AdmissionRequest, - namespaces: string[], - config: ModuleConfig, - wrapped: PeprValidateRequest, - actionMetadata: { name: string }, -): Promise => { - if (binding.validateCallback === undefined) { - // Skip this action if it's not a validation action - return undefined; - } - - if (!binding.validateCallback) { - return undefined; - } + for (const action of bindings) { + // Skip this action if it's not a validation action + if (!action.validateCallback) { + continue; + } - const localResponse: ValidateResponse = { - uid: req.uid, - allowed: true, // Assume it's allowed until a validation check fails - }; - - // Continue to the next action without doing anything if this one should be skipped - const shouldSkip = shouldSkipRequest(binding, req, namespaces, config?.alwaysIgnore?.namespaces); - if (shouldSkip !== "") { - Log.debug(shouldSkip); - return undefined; - } - - const label = binding.validateCallback.name; - Log.info(actionMetadata, `Processing validation action (${label})`); - - try { - // Run the validation callback, if it fails set allowed to false - const resp = await binding.validateCallback(wrapped); - localResponse.allowed = resp.allowed; - - // If the validation callback returned a status code or message, set it in the Response - if (resp.statusCode || resp.statusMessage) { - localResponse.status = { - code: resp.statusCode || 400, - message: resp.statusMessage || `Validation failed for ${name}`, + const localResponse: ValidateResponse = { + uid: req.uid, + allowed: true, // Assume it's allowed until a validation check fails }; - } - Log.info(actionMetadata, `Validation action complete (${label}): ${resp.allowed ? "allowed" : "denied"}`); - } catch (e) { - // If any validation throws an error, note the failure in the Response - Log.error(actionMetadata, `Action failed: ${JSON.stringify(e)}`); - localResponse.allowed = false; - localResponse.status = { - code: 500, - message: `Action failed with error: ${JSON.stringify(e)}`, - }; + // Continue to the next action without doing anything if this one should be skipped + const shouldSkip = shouldSkipRequest(action, req, namespaces, config?.alwaysIgnore?.namespaces); + if (shouldSkip !== "") { + Log.debug(shouldSkip); + continue; + } + + const label = action.validateCallback.name; + Log.info(actionMetadata, `Processing validation action (${label})`); + + try { + // Run the validation callback, if it fails set allowed to false + const resp = await action.validateCallback(wrapped); + localResponse.allowed = resp.allowed; + + // If the validation callback returned a status code or message, set it in the Response + if (resp.statusCode || resp.statusMessage) { + localResponse.status = { + code: resp.statusCode || 400, + message: resp.statusMessage || `Validation failed for ${name}`, + }; + } + + Log.info(actionMetadata, `Validation action complete (${label}): ${resp.allowed ? "allowed" : "denied"}`); + } catch (e) { + // If any validation throws an error, note the failure in the Response + Log.error(actionMetadata, `Action failed: ${JSON.stringify(e)}`); + localResponse.allowed = false; + localResponse.status = { + code: 500, + message: `Action failed with error: ${JSON.stringify(e)}`, + }; + return [localResponse]; + } + response.push(localResponse); + } } - return localResponse; -}; + + return response; +} From 72f43115026a0e88c3e27b4201cd574600715113 Mon Sep 17 00:00:00 2001 From: Sam Mayer Date: Mon, 4 Nov 2024 13:42:37 -0600 Subject: [PATCH 4/5] Create initial test skeleton --- src/lib/validate-processor.test.ts | 54 ++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) create mode 100644 src/lib/validate-processor.test.ts diff --git a/src/lib/validate-processor.test.ts b/src/lib/validate-processor.test.ts new file mode 100644 index 000000000..4be184f52 --- /dev/null +++ b/src/lib/validate-processor.test.ts @@ -0,0 +1,54 @@ +import { describe, expect, it } from "@jest/globals"; +import { validateProcessor } from "./validate-processor"; +import { Capability } from "./capability"; +import { KubernetesObject } from "kubernetes-fluent-client"; +import { AdmissionRequest } from "./types"; +import { Operation } from "./enums"; + +describe("validate-processor tests", () => { + const defaultModuleConfig = { uuid: "some-uuid", alwaysIgnore: { namespaces: [] } }; + const defaultCapabilities: Capability[] = []; + const defaultRequestMetadata = {}; + const defaultRequest: AdmissionRequest = { + operation: Operation.CREATE, + uid: "test-uid", + kind: { + group: "", + version: "v1", + kind: "Pod", + }, + resource: { + group: "", + version: "v1", + resource: "pods", + }, + name: "test-pod", + userInfo: { + username: "test-user", + groups: ["test-group"], + }, + object: { + apiVersion: "v1", + kind: "Pod", + metadata: { + name: "test-pod", + labels: { + "test-label": "true", + }, + annotations: { + "test-annotation": "true", + }, + }, + }, + }; + + it("should return an empty validate response", async () => { + const result = await validateProcessor( + defaultModuleConfig, + defaultCapabilities, + defaultRequest, + defaultRequestMetadata, + ); + expect(result).toStrictEqual([]); + }); +}); From 62bb1fee073cbfcd43157fa2cf5b13e6f9d8ca1e Mon Sep 17 00:00:00 2001 From: Sam Mayer Date: Mon, 4 Nov 2024 13:51:55 -0600 Subject: [PATCH 5/5] Add test cases to start increasing code coverage --- src/lib/validate-processor.test.ts | 34 ++++++++++++++++++++++++------ src/lib/validate-processor.ts | 10 ++++----- 2 files changed, 32 insertions(+), 12 deletions(-) diff --git a/src/lib/validate-processor.test.ts b/src/lib/validate-processor.test.ts index 4be184f52..814541861 100644 --- a/src/lib/validate-processor.test.ts +++ b/src/lib/validate-processor.test.ts @@ -2,21 +2,28 @@ import { describe, expect, it } from "@jest/globals"; import { validateProcessor } from "./validate-processor"; import { Capability } from "./capability"; import { KubernetesObject } from "kubernetes-fluent-client"; -import { AdmissionRequest } from "./types"; +import { AdmissionRequest, CapabilityCfg } from "./types"; import { Operation } from "./enums"; describe("validate-processor tests", () => { + const defaultCapabilityConfig: CapabilityCfg = { + name: "test-capability", + description: "Test capability description", + namespaces: ["default"], + }; + const defaultModuleConfig = { uuid: "some-uuid", alwaysIgnore: { namespaces: [] } }; - const defaultCapabilities: Capability[] = []; + const defaultCapabilities: Capability[] = [new Capability(defaultCapabilityConfig)]; const defaultRequestMetadata = {}; + const defaultKind = { + group: "", + version: "v1", + kind: "Pod", + }; const defaultRequest: AdmissionRequest = { operation: Operation.CREATE, uid: "test-uid", - kind: { - group: "", - version: "v1", - kind: "Pod", - }, + kind: defaultKind, resource: { group: "", version: "v1", @@ -51,4 +58,17 @@ describe("validate-processor tests", () => { ); expect(result).toStrictEqual([]); }); + + it("TODO: should do something when secret", async () => { + const request = { ...defaultRequest, kind: { group: "", kind: "Secret", version: "v1" } }; + const result = await validateProcessor(defaultModuleConfig, defaultCapabilities, request, defaultRequestMetadata); + expect(result).toStrictEqual([]); + }); + + it("TODO should do something with bindings", async () => { + const capabilities: Capability[] = [new Capability({ ...defaultCapabilityConfig })]; + const request = { ...defaultRequest, kind: { group: "", kind: "Secret", version: "v1" } }; + const result = await validateProcessor(defaultModuleConfig, capabilities, request, defaultRequestMetadata); + expect(result).toStrictEqual([]); + }); }); diff --git a/src/lib/validate-processor.ts b/src/lib/validate-processor.ts index 43571a330..3e94d7bfa 100644 --- a/src/lib/validate-processor.ts +++ b/src/lib/validate-processor.ts @@ -32,9 +32,9 @@ export async function validateProcessor( for (const { name, bindings, namespaces } of capabilities) { const actionMetadata = { ...reqMetadata, name }; - for (const action of bindings) { + for (const binding of bindings) { // Skip this action if it's not a validation action - if (!action.validateCallback) { + if (!binding.validateCallback) { continue; } @@ -44,18 +44,18 @@ export async function validateProcessor( }; // Continue to the next action without doing anything if this one should be skipped - const shouldSkip = shouldSkipRequest(action, req, namespaces, config?.alwaysIgnore?.namespaces); + const shouldSkip = shouldSkipRequest(binding, req, namespaces, config?.alwaysIgnore?.namespaces); if (shouldSkip !== "") { Log.debug(shouldSkip); continue; } - const label = action.validateCallback.name; + const label = binding.validateCallback.name; Log.info(actionMetadata, `Processing validation action (${label})`); try { // Run the validation callback, if it fails set allowed to false - const resp = await action.validateCallback(wrapped); + const resp = await binding.validateCallback(wrapped); localResponse.allowed = resp.allowed; // If the validation callback returned a status code or message, set it in the Response