Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: validate unique names for monitors #666

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from
Draft

fix: validate unique names for monitors #666

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b7fe919
added validator for checking that each array item in 'monitors' has a…
andygodish Aug 13, 2024
ee3b54f
added partial monitorList array input to makeMockReq and fixed subequ…
andygodish Aug 13, 2024
6a30f21
added unit tests for non-unique monitor descriptions
andygodish Aug 13, 2024
7b57f2e
Merge branch 'main' into monitor-validate
andygodish Aug 15, 2024
cd1f9c3
updated validator to reflect the same logic as the name generator fun…
andygodish Aug 21, 2024
9523745
Merge branch 'main' into monitor-validate
andygodish Aug 21, 2024
2fd8ede
caught up branch to main, resolved merge conflicts and added in monit…
andygodish Aug 21, 2024
8f5fcbc
uds run lint-check after CI fail
andygodish Aug 21, 2024
c9db9e0
Merge branch 'main' into monitor-validate
mjnagel Aug 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 86 additions & 7 deletions src/pepr/operator/crd/validators/package-validator.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,24 @@
import { afterEach, describe, expect, it, jest } from "@jest/globals";
import { PeprValidateRequest } from "pepr";
import { Allow, Direction, Expose, Gateway, Protocol, RemoteGenerated, Sso, UDSPackage } from "..";
import {
Allow,
Direction,
Expose,
Gateway,
Monitor,
Protocol,
RemoteGenerated,
Sso,
UDSPackage,
} from "..";
import { validator } from "./package-validator";

const makeMockReq = (
pkg: Partial<UDSPackage>,
exposeList: Partial<Expose>[],
allowList: Partial<Allow>[],
ssoClients: Partial<Sso>[],
monitorList: Partial<Monitor>[],
) => {
const defaultPkg: UDSPackage = {
metadata: {
Expand All @@ -20,6 +31,7 @@ const makeMockReq = (
allow: [],
},
sso: [],
monitor: [],
},
};

Expand All @@ -46,6 +58,16 @@ const makeMockReq = (
defaultPkg.spec!.sso?.push({ ...defaultClient, ...client });
}

for (const monitor of monitorList) {
const defaultMonitor: Monitor = {
description: "Default Monitor",
portName: "http-metrics",
selector: {},
targetPort: 8080,
};
defaultPkg.spec!.monitor?.push({ ...defaultMonitor, ...monitor });
}

return {
Raw: { ...defaultPkg, ...pkg },
Approve: jest.fn(),
Expand All @@ -59,13 +81,13 @@ describe("Test validation of Exemption CRs", () => {
});

it("allows packages that have no issues", async () => {
const mockReq = makeMockReq({}, [{}], [{}], [{}]);
const mockReq = makeMockReq({}, [{}], [{}], [{}], [{}]);
await validator(mockReq);
expect(mockReq.Approve).toHaveBeenCalledTimes(1);
});

it("denies system namespaces", async () => {
const mockReq = makeMockReq({ metadata: { namespace: "kube-system" } }, [], [], []);
const mockReq = makeMockReq({ metadata: { namespace: "kube-system" } }, [], [], [], []);
await validator(mockReq);
expect(mockReq.Deny).toHaveBeenCalledTimes(1);
});
Expand All @@ -83,6 +105,7 @@ describe("Test validation of Exemption CRs", () => {
],
[],
[],
[],
);
await validator(mockReq);
expect(mockReq.Deny).toHaveBeenCalledTimes(1);
Expand All @@ -101,6 +124,7 @@ describe("Test validation of Exemption CRs", () => {
],
[],
[],
[],
);
await validator(mockReq);
expect(mockReq.Deny).toHaveBeenCalledTimes(1);
Expand All @@ -119,6 +143,7 @@ describe("Test validation of Exemption CRs", () => {
],
[],
[],
[],
);
await validator(mockReq);
expect(mockReq.Deny).toHaveBeenCalledTimes(1);
Expand All @@ -137,6 +162,7 @@ describe("Test validation of Exemption CRs", () => {
],
[],
[],
[],
);
await validator(mockReq);
expect(mockReq.Deny).toHaveBeenCalledTimes(1);
Expand All @@ -155,13 +181,14 @@ describe("Test validation of Exemption CRs", () => {
],
[],
[],
[],
);
await validator(mockReq);
expect(mockReq.Deny).toHaveBeenCalledTimes(1);
});

it("denies virtual services that are the same name", async () => {
const mockReq = makeMockReq({}, [{}, {}], [], []);
const mockReq = makeMockReq({}, [{}, {}], [], [], []);
await validator(mockReq);
expect(mockReq.Deny).toHaveBeenCalledTimes(1);
});
Expand All @@ -177,6 +204,7 @@ describe("Test validation of Exemption CRs", () => {
},
],
[],
[],
);
await validator(mockReq);
expect(mockReq.Deny).toHaveBeenCalledTimes(1);
Expand All @@ -193,19 +221,20 @@ describe("Test validation of Exemption CRs", () => {
},
],
[],
[],
);
await validator(mockReq);
expect(mockReq.Deny).toHaveBeenCalledTimes(1);
});

it("denies network policies that are the same name", async () => {
const mockReq = makeMockReq({}, [], [{}, {}], []);
const mockReq = makeMockReq({}, [], [{}, {}], [], []);
await validator(mockReq);
expect(mockReq.Deny).toHaveBeenCalledTimes(1);
});

it("denies clients with clientIDs that are not unique", async () => {
const mockReq = makeMockReq({}, [], [], [{}, {}]);
const mockReq = makeMockReq({}, [], [], [{}, {}], []);
await validator(mockReq);
expect(mockReq.Deny).toHaveBeenCalledTimes(1);
});
Expand All @@ -220,6 +249,7 @@ describe("Test validation of Exemption CRs", () => {
secretName: "HELLO_KITTEH",
},
],
[],
);
await validator(mockReq);
expect(mockReq.Deny).toHaveBeenCalledTimes(1);
Expand All @@ -235,6 +265,7 @@ describe("Test validation of Exemption CRs", () => {
redirectUris: undefined,
},
],
[],
);
await validator(mockReq);
expect(mockReq.Deny).toHaveBeenCalledTimes(1);
Expand All @@ -251,6 +282,7 @@ describe("Test validation of Exemption CRs", () => {
redirectUris: undefined,
},
],
[],
);
await validator(mockReq);
expect(mockReq.Approve).toHaveBeenCalledTimes(1);
Expand All @@ -268,6 +300,7 @@ describe("Test validation of Exemption CRs", () => {
standardFlowEnabled: true,
},
],
[],
);
await validator(mockReq);
expect(mockReq.Deny).toHaveBeenCalledTimes(1);
Expand All @@ -286,6 +319,7 @@ describe("Test validation of Exemption CRs", () => {
secret: "app-client-secret",
},
],
[],
);
await validator(mockReq);
expect(mockReq.Deny).toHaveBeenCalledTimes(1);
Expand All @@ -304,6 +338,7 @@ describe("Test validation of Exemption CRs", () => {
secretName: "app-k8s-secret",
},
],
[],
);
await validator(mockReq);
expect(mockReq.Deny).toHaveBeenCalledTimes(1);
Expand All @@ -322,6 +357,7 @@ describe("Test validation of Exemption CRs", () => {
secretTemplate: {},
},
],
[],
);
await validator(mockReq);
expect(mockReq.Deny).toHaveBeenCalledTimes(1);
Expand All @@ -340,6 +376,7 @@ describe("Test validation of Exemption CRs", () => {
enableAuthserviceSelector: {},
},
],
[],
);
await validator(mockReq);
expect(mockReq.Deny).toHaveBeenCalledTimes(1);
Expand All @@ -358,6 +395,7 @@ describe("Test validation of Exemption CRs", () => {
protocol: Protocol.Saml,
},
],
[],
);
await validator(mockReq);
expect(mockReq.Deny).toHaveBeenCalledTimes(1);
Expand All @@ -374,6 +412,7 @@ describe("Test validation of Exemption CRs", () => {
standardFlowEnabled: false,
},
],
[],
);
await validator(mockReq);
expect(mockReq.Deny).toHaveBeenCalledTimes(1);
Expand All @@ -391,6 +430,7 @@ describe("Test validation of Exemption CRs", () => {
standardFlowEnabled: false,
},
],
[],
);
await validator(mockReq);
expect(mockReq.Approve).toHaveBeenCalledTimes(1);
Expand All @@ -409,6 +449,7 @@ describe("Test validation of Exemption CRs", () => {
},
},
],
[],
);
await validator(mockReq);
expect(mockReq.Deny).toHaveBeenCalledTimes(1);
Expand All @@ -425,6 +466,7 @@ describe("Test validation of Exemption CRs", () => {
enableAuthserviceSelector: undefined, // explicitly undefined
},
],
[],
);
await validator(mockReq);
expect(mockReq.Approve).toHaveBeenCalledTimes(1);
Expand All @@ -448,6 +490,7 @@ describe("Test Allowed SSO Client Attributes", () => {
},
},
],
[],
);
await validator(mockReq);
expect(mockReq.Deny).toHaveBeenCalledTimes(1);
Expand All @@ -474,6 +517,7 @@ describe("Test Allowed SSO Client Attributes", () => {
},
},
],
[],
);
await validator(mockReq);
expect(mockReq.Approve).toHaveBeenCalledTimes(1);
Expand All @@ -492,6 +536,7 @@ describe("Test Allowed SSO Client Attributes", () => {
},
},
],
[],
);
await validator(mockReq);
expect(mockReq.Deny).toHaveBeenCalledTimes(1);
Expand All @@ -510,14 +555,48 @@ describe("Test Allowed SSO Client Attributes", () => {
attributes: {},
},
],
[],
);
await validator(mockReq);
expect(mockReq.Approve).toHaveBeenCalledTimes(1);
});

it("allows clients with no attributes defined", async () => {
const mockReq = makeMockReq({}, [], [], [{}]);
const mockReq = makeMockReq({}, [], [], [{}], []);
await validator(mockReq);
expect(mockReq.Approve).toHaveBeenCalledTimes(1);
});
});

describe("Test proper generation of a unique name for service monitors", () => {
afterEach(() => {
jest.resetAllMocks();
});

it("given an undefined description, a unique serviceMonitor name should be generated using the selector and portName fields", async () => {
const mockReq = makeMockReq(
{},
[],
[],
[],
[
{ description: undefined, portName: "http-foo", selector: { key: "foo" } },
{ description: undefined, portName: "http-bar", selector: { key: "bar" } },
],
);
await validator(mockReq);
expect(mockReq.Deny).toHaveBeenCalledTimes(0);
});

it("denies monitors that do not have unique descriptions", async () => {
const mockReq = makeMockReq(
{},
[],
[],
[],
[{ description: "Metrics" }, { description: "Metrics" }],
);
await validator(mockReq);
expect(mockReq.Deny).toHaveBeenCalledTimes(1);
});
});
19 changes: 19 additions & 0 deletions src/pepr/operator/crd/validators/package-validator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,5 +141,24 @@ export async function validator(req: PeprValidateRequest<UDSPackage>) {
}
}

const monitors = pkg.spec?.monitor ?? [];

// Ensure serviceMonitors use a unique description or selector/portName used for generating the resource name
const monitorNames = new Set<string>();

for (const monitor of monitors) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't think about this before but I think there's another edge case where podmonitors and servicemonitors should be allowed to have overlapping names, but this would deny them. We may need separate sets per type and check the kind in this loop.

const monitorName = sanitizeResourceName(
monitor.description || `${Object.values(monitor.selector)}-${monitor.portName}`,
);
Comment on lines +150 to +152
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Realized in retrospect we could just import generateMonitorName and use that here to ensure we keep this logic in sync with our name generation rather than duplicating.


if (monitorNames.has(monitorName)) {
return req.Deny(
`A serviceMonitor resource generated from the Package, ${pkg.metadata?.name} contains a duplicate name, please provide a unique description for each item in the monitor array`,
);
Comment on lines +155 to +157
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: might be good to align this with the wording we use for the VirtualService check, and make sure we are capturing kind accurately here (pod vs svc monitor).

}

monitorNames.add(monitorName);
}

return req.Approve();
}