From a5ae3cc35944b4b42182e3a44a48a9f0483710ed Mon Sep 17 00:00:00 2001 From: Case Wylie Date: Fri, 30 Aug 2024 13:13:44 -0400 Subject: [PATCH] feat: withdeletiontimestamp filter (#1026) ## Description Adds a new filter `.WithDeletionTimestamp` to aid Operator developers as they use `ownerRefs`. Decide if `.HasDeletionTimestamp` needs to be done. Needs: - [x] e2e test (https://github.com/defenseunicorns/pepr-excellent-examples/pull/61) - [x] docs ## Related Issue Fixes #1018 Fixes: #1027 Relates to # ## Type of change - [ ] Bug fix (non-breaking change which fixes an issue) - [x] New feature (non-breaking change which adds functionality) - [ ] Other (security config, docs update, etc) ## Checklist before merging - [ ] Test, docs, 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 Co-authored-by: Barrett <81570928+btlghrants@users.noreply.github.com> --- codecov.yaml | 4 +- docs/030_user-guide/130_filters.md | 33 ++++++++++++ src/lib/capability.ts | 10 +++- src/lib/filter.test.ts | 86 +++++++++++++++++++++++++++++- src/lib/filter.ts | 9 ++++ src/lib/helpers.test.ts | 34 ++++++++++++ src/lib/helpers.ts | 5 ++ src/lib/types.ts | 3 ++ 8 files changed, 180 insertions(+), 4 deletions(-) create mode 100644 docs/030_user-guide/130_filters.md diff --git a/codecov.yaml b/codecov.yaml index cf89f7839..066b11aec 100644 --- a/codecov.yaml +++ b/codecov.yaml @@ -12,8 +12,8 @@ coverage: project: default: target: auto - threshold: 10% + threshold: 30% patch: # Settings for PR patches default: target: auto - threshold: 10% # Adjust as needed + threshold: 30% # Adjust as needed diff --git a/docs/030_user-guide/130_filters.md b/docs/030_user-guide/130_filters.md new file mode 100644 index 000000000..3ea3d8fc2 --- /dev/null +++ b/docs/030_user-guide/130_filters.md @@ -0,0 +1,33 @@ +# Pepr Filters + +Filters are functions that take a `AdmissionReview` or Watch event and return a boolean. They are used to filter out resources that do not meet certain criteria. Filters are used in the package to filter out resources that are not relevant to the user-defined admission or watch process. + +```ts +When(a.ConfigMap) + // This limits the action to only act on new resources. + .IsCreated() + // Namespace filter + .InNamespace("webapp") + // Name filter + .WithName("example-1") + // Label filter + .WithLabel("app", "webapp") + .WithLabel("env", "prod") + .Mutate(request => { + request + .SetLabel("pepr", "was-here") + .SetAnnotation("pepr.dev", "annotations-work-too"); + }); +``` + + +## `Filters` + +- `.WithName("name")`: Filters resources by name. +- `.InNamespace("namespace")`: Filters resources by namespace. +- `.WithLabel("key", "value")`: Filters resources by label. (Can be multiple) +- `.WithDeletionTimestamp()`: Filters resources that have a deletion timestamp. + +Notes: +- `WithDeletionTimestamp()` is does not work on Delete through the `Mutate` or `Validate` methods because the Kubernetes Admission Process does not fire the DELETE event with a deletion timestamp on the resource. +- `WithDeletionTimestamp()` _will_ match on an Update event during Admission (`Mutate` or `Validate`) when pending-deletion permitted changes (like removing a finalizer) occur. diff --git a/src/lib/capability.ts b/src/lib/capability.ts index 719aa8015..1106839ec 100644 --- a/src/lib/capability.ts +++ b/src/lib/capability.ts @@ -198,12 +198,13 @@ export class Capability implements CapabilityExport { namespaces: [], labels: {}, annotations: {}, + deletionTimestamp: false, }, }; const bindings = this.#bindings; const prefix = `${this.#name}: ${model.name}`; - const commonChain = { WithLabel, WithAnnotation, Mutate, Validate, Watch, Reconcile }; + const commonChain = { WithLabel, WithAnnotation, WithDeletionTimestamp, Mutate, Validate, Watch, Reconcile }; const isNotEmpty = (value: object) => Object.keys(value).length > 0; const log = (message: string, cbString: string) => { const filteredObj = pickBy(isNotEmpty, binding.filters); @@ -277,6 +278,12 @@ export class Capability implements CapabilityExport { return { ...commonChain, WithName }; } + function WithDeletionTimestamp(): BindingFilter { + Log.debug("Add deletionTimestamp filter"); + binding.filters.deletionTimestamp = true; + return commonChain; + } + function WithName(name: string): BindingFilter { Log.debug(`Add name filter ${name}`, prefix); binding.filters.name = name; @@ -301,6 +308,7 @@ export class Capability implements CapabilityExport { ...commonChain, InNamespace, WithName, + WithDeletionTimestamp, }; } diff --git a/src/lib/filter.test.ts b/src/lib/filter.test.ts index 565c7a31d..dac4f0a09 100644 --- a/src/lib/filter.test.ts +++ b/src/lib/filter.test.ts @@ -29,6 +29,7 @@ describe("Fuzzing shouldSkipRequest", () => { namespaces: fc.array(fc.string()), labels: fc.dictionary(fc.string(), fc.string()), annotations: fc.dictionary(fc.string(), fc.string()), + deletionTimestamp: fc.boolean(), }), }), fc.record({ @@ -41,6 +42,11 @@ describe("Fuzzing shouldSkipRequest", () => { version: fc.string(), kind: fc.string(), }), + object: fc.record({ + metadata: fc.record({ + deletionTimestamp: fc.option(fc.date()), + }), + }), }), fc.array(fc.string()), (binding, req, capabilityNamespaces) => { @@ -69,6 +75,7 @@ describe("Property-Based Testing shouldSkipRequest", () => { namespaces: fc.array(fc.string()), labels: fc.dictionary(fc.string(), fc.string()), annotations: fc.dictionary(fc.string(), fc.string()), + deletionTimestamp: fc.boolean(), }), }), fc.record({ @@ -81,6 +88,11 @@ describe("Property-Based Testing shouldSkipRequest", () => { version: fc.string(), kind: fc.string(), }), + object: fc.record({ + metadata: fc.record({ + deletionTimestamp: fc.option(fc.date()), + }), + }), }), fc.array(fc.string()), (binding, req, capabilityNamespaces) => { @@ -103,6 +115,7 @@ test("should reject when name does not match", () => { namespaces: [], labels: {}, annotations: {}, + deletionTimestamp: false, }, callback, }; @@ -121,6 +134,7 @@ test("should reject when kind does not match", () => { namespaces: [], labels: {}, annotations: {}, + deletionTimestamp: false, }, callback, }; @@ -139,6 +153,7 @@ test("should reject when group does not match", () => { namespaces: [], labels: {}, annotations: {}, + deletionTimestamp: false, }, callback, }; @@ -161,6 +176,7 @@ test("should reject when version does not match", () => { namespaces: [], labels: {}, annotations: {}, + deletionTimestamp: false, }, callback, }; @@ -179,6 +195,7 @@ test("should allow when group, version, and kind match", () => { namespaces: [], labels: {}, annotations: {}, + deletionTimestamp: false, }, callback, }; @@ -201,6 +218,7 @@ test("should allow when kind match and others are empty", () => { namespaces: [], labels: {}, annotations: {}, + deletionTimestamp: false, }, callback, }; @@ -219,6 +237,7 @@ test("should reject when teh capability namespace does not match", () => { namespaces: [], labels: {}, annotations: {}, + deletionTimestamp: false, }, callback, }; @@ -237,6 +256,7 @@ test("should reject when namespace does not match", () => { namespaces: ["bleh"], labels: {}, annotations: {}, + deletionTimestamp: false, }, callback, }; @@ -255,6 +275,7 @@ test("should allow when namespace is match", () => { namespaces: ["default", "unicorn", "things"], labels: {}, annotations: {}, + deletionTimestamp: false, }, callback, }; @@ -275,6 +296,7 @@ test("should reject when label does not match", () => { foo: "bar", }, annotations: {}, + deletionTimestamp: false, }, callback, }; @@ -290,7 +312,7 @@ test("should allow when label is match", () => { kind: podKind, filters: { name: "", - + deletionTimestamp: false, namespaces: [], labels: { foo: "bar", @@ -324,6 +346,7 @@ test("should reject when annotation does not match", () => { annotations: { foo: "bar", }, + deletionTimestamp: false, }, callback, }; @@ -345,6 +368,7 @@ test("should allow when annotation is match", () => { foo: "bar", test: "test1", }, + deletionTimestamp: false, }, callback, }; @@ -368,6 +392,7 @@ test("should use `oldObject` when the operation is `DELETE`", () => { filters: { name: "", namespaces: [], + deletionTimestamp: false, labels: { "app.kubernetes.io/name": "cool-name-podinfo", }, @@ -382,3 +407,62 @@ test("should use `oldObject` when the operation is `DELETE`", () => { expect(shouldSkipRequest(binding, pod, [])).toBe(false); }); + +test("should skip processing when deletionTimestamp is not present on pod", () => { + const binding = { + model: kind.Pod, + event: Event.Any, + kind: podKind, + filters: { + name: "", + namespaces: [], + labels: {}, + annotations: { + foo: "bar", + test: "test1", + }, + deletionTimestamp: true, + }, + callback, + }; + + const pod = CreatePod(); + pod.object.metadata = pod.object.metadata || {}; + pod.object.metadata.annotations = { + foo: "bar", + test: "test1", + test2: "test2", + }; + + expect(shouldSkipRequest(binding, pod, [])).toBe(true); +}); + +test("should processing when deletionTimestamp is not present on pod", () => { + const binding = { + model: kind.Pod, + event: Event.Any, + kind: podKind, + filters: { + name: "", + namespaces: [], + labels: {}, + annotations: { + foo: "bar", + test: "test1", + }, + deletionTimestamp: true, + }, + callback, + }; + + const pod = CreatePod(); + pod.object.metadata = pod.object.metadata || {}; + pod.object.metadata!.deletionTimestamp = new Date("2021-09-01T00:00:00Z"); + pod.object.metadata.annotations = { + foo: "bar", + test: "test1", + test2: "test2", + }; + + expect(shouldSkipRequest(binding, pod, [])).toBe(false); +}); diff --git a/src/lib/filter.ts b/src/lib/filter.ts index 698d1f4af..86988e3f5 100644 --- a/src/lib/filter.ts +++ b/src/lib/filter.ts @@ -22,6 +22,15 @@ export function shouldSkipRequest(binding: Binding, req: AdmissionRequest, capab const { metadata } = srcObject || {}; const combinedNamespaces = [...namespaces, ...capabilityNamespaces]; + // Delete bindings do not work through admission with DeletionTimestamp + if (binding.event.includes(Event.Delete) && binding.filters?.deletionTimestamp) { + return true; + } + + // Test for deletionTimestamp + if (binding.filters?.deletionTimestamp && !req.object.metadata?.deletionTimestamp) { + return true; + } // Test for matching operation if (!binding.event.includes(operation) && !binding.event.includes(Event.Any)) { return true; diff --git a/src/lib/helpers.test.ts b/src/lib/helpers.test.ts index 78399ec37..e0949e74b 100644 --- a/src/lib/helpers.test.ts +++ b/src/lib/helpers.test.ts @@ -1056,6 +1056,40 @@ describe("filterMatcher", () => { expect(result).toEqual("Ignoring Watch Callback: Cannot use a namespace filter in a namespace object."); }); + test("return deletionTimestamp error when there is no deletionTimestamp in the object", () => { + const binding = { + filters: { deletionTimestamp: true }, + }; + const obj = { + metadata: {}, + }; + const capabilityNamespaces: string[] = []; + const result = filterNoMatchReason( + binding as unknown as Partial, + obj as unknown as Partial, + capabilityNamespaces, + ); + expect(result).toEqual("Ignoring Watch Callback: Object does not have a deletion timestamp."); + }); + + test("return no deletionTimestamp error when there is a deletionTimestamp in the object", () => { + const binding = { + filters: { deletionTimestamp: true }, + }; + const obj = { + metadata: { + deletionTimestamp: "2021-01-01T00:00:00Z", + }, + }; + const capabilityNamespaces: string[] = []; + const result = filterNoMatchReason( + binding as unknown as Partial, + obj as unknown as Partial, + capabilityNamespaces, + ); + expect(result).not.toEqual("Ignoring Watch Callback: Object does not have a deletion timestamp."); + }); + test("returns label overlap error when there is no overlap between binding and object labels", () => { const binding = { filters: { labels: { key: "value" } }, diff --git a/src/lib/helpers.ts b/src/lib/helpers.ts index 82dc573b5..34760d20a 100644 --- a/src/lib/helpers.ts +++ b/src/lib/helpers.ts @@ -73,6 +73,11 @@ export function filterNoMatchReason( obj: Partial, capabilityNamespaces: string[], ): string { + // binding deletionTimestamp filter and object deletionTimestamp dont match + if (binding.filters?.deletionTimestamp && !obj.metadata?.deletionTimestamp) { + return `Ignoring Watch Callback: Object does not have a deletion timestamp.`; + } + // binding kind is namespace with a InNamespace filter if (binding.kind && binding.kind.kind === "Namespace" && binding.filters && binding.filters.namespaces.length !== 0) { return `Ignoring Watch Callback: Cannot use a namespace filter in a namespace object.`; diff --git a/src/lib/types.ts b/src/lib/types.ts index aa1fbdc84..9214108df 100644 --- a/src/lib/types.ts +++ b/src/lib/types.ts @@ -94,6 +94,7 @@ export type Binding = { namespaces: string[]; labels: Record; annotations: Record; + deletionTimestamp: boolean; }; readonly mutateCallback?: MutateAction>; readonly validateCallback?: ValidateAction>; @@ -137,6 +138,8 @@ export type BindingFilter = CommonActionChain & { * @param value */ WithAnnotation: (key: string, value?: string) => BindingFilter; + /** Only apply the action if the resource has a deletionTimestamp. */ + WithDeletionTimestamp: () => BindingFilter; }; export type BindingWithName = BindingFilter & {