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

fix: validate unique names for monitors #666

Merged
merged 14 commits into from
Jan 6, 2025
Merged

fix: validate unique names for monitors #666

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 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
2d9757b
Merge branch 'main' into monitor-validate
mjnagel Jan 6, 2025
60e419b
chore: update based on PR comments
mjnagel Jan 6, 2025
97da4dd
fix: missing args in validator
mjnagel Jan 6, 2025
42e885a
Merge branch 'main' into monitor-validate
mjnagel Jan 6, 2025
7bf7db9
Merge branch 'main' into monitor-validate
mjnagel Jan 6, 2025
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
96 changes: 89 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
Expand Up @@ -5,14 +5,25 @@

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 @@ -25,6 +36,7 @@ const makeMockReq = (
allow: [],
},
sso: [],
monitor: [],
},
};

Expand All @@ -51,6 +63,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 @@ -64,13 +86,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 @@ -88,6 +110,7 @@ describe("Test validation of Exemption CRs", () => {
],
[],
[],
[],
);
await validator(mockReq);
expect(mockReq.Deny).toHaveBeenCalledTimes(1);
Expand All @@ -106,6 +129,7 @@ describe("Test validation of Exemption CRs", () => {
],
[],
[],
[],
);
await validator(mockReq);
expect(mockReq.Deny).toHaveBeenCalledTimes(1);
Expand All @@ -124,6 +148,7 @@ describe("Test validation of Exemption CRs", () => {
],
[],
[],
[],
);
await validator(mockReq);
expect(mockReq.Deny).toHaveBeenCalledTimes(1);
Expand All @@ -142,6 +167,7 @@ describe("Test validation of Exemption CRs", () => {
],
[],
[],
[],
);
await validator(mockReq);
expect(mockReq.Deny).toHaveBeenCalledTimes(1);
Expand All @@ -160,13 +186,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 @@ -182,6 +209,7 @@ describe("Test validation of Exemption CRs", () => {
},
],
[],
[],
);
await validator(mockReq);
expect(mockReq.Deny).toHaveBeenCalledTimes(1);
Expand All @@ -198,19 +226,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 @@ -225,6 +254,7 @@ describe("Test validation of Exemption CRs", () => {
secretName: "HELLO_KITTEH",
},
],
[],
);
await validator(mockReq);
expect(mockReq.Deny).toHaveBeenCalledTimes(1);
Expand All @@ -240,6 +270,7 @@ describe("Test validation of Exemption CRs", () => {
redirectUris: undefined,
},
],
[],
);
await validator(mockReq);
expect(mockReq.Deny).toHaveBeenCalledTimes(1);
Expand All @@ -256,6 +287,7 @@ describe("Test validation of Exemption CRs", () => {
redirectUris: undefined,
},
],
[],
);
await validator(mockReq);
expect(mockReq.Approve).toHaveBeenCalledTimes(1);
Expand All @@ -273,6 +305,7 @@ describe("Test validation of Exemption CRs", () => {
standardFlowEnabled: true,
},
],
[],
);
await validator(mockReq);
expect(mockReq.Deny).toHaveBeenCalledTimes(1);
Expand All @@ -289,6 +322,7 @@ describe("Test validation of Exemption CRs", () => {
serviceAccountsEnabled: true,
},
],
[],
);
await validator(mockReq);
expect(mockReq.Deny).toHaveBeenCalledTimes(1);
Expand All @@ -305,6 +339,7 @@ describe("Test validation of Exemption CRs", () => {
serviceAccountsEnabled: true,
},
],
[],
);
await validator(mockReq);
expect(mockReq.Deny).toHaveBeenCalledTimes(1);
Expand All @@ -323,6 +358,7 @@ describe("Test validation of Exemption CRs", () => {
secret: "app-client-secret",
},
],
[],
);
await validator(mockReq);
expect(mockReq.Deny).toHaveBeenCalledTimes(1);
Expand All @@ -341,6 +377,7 @@ describe("Test validation of Exemption CRs", () => {
secretName: "app-k8s-secret",
},
],
[],
);
await validator(mockReq);
expect(mockReq.Deny).toHaveBeenCalledTimes(1);
Expand All @@ -359,6 +396,7 @@ describe("Test validation of Exemption CRs", () => {
secretTemplate: {},
},
],
[],
);
await validator(mockReq);
expect(mockReq.Deny).toHaveBeenCalledTimes(1);
Expand All @@ -377,6 +415,7 @@ describe("Test validation of Exemption CRs", () => {
enableAuthserviceSelector: {},
},
],
[],
);
await validator(mockReq);
expect(mockReq.Deny).toHaveBeenCalledTimes(1);
Expand All @@ -395,6 +434,7 @@ describe("Test validation of Exemption CRs", () => {
protocol: Protocol.Saml,
},
],
[],
);
await validator(mockReq);
expect(mockReq.Deny).toHaveBeenCalledTimes(1);
Expand All @@ -411,6 +451,7 @@ describe("Test validation of Exemption CRs", () => {
standardFlowEnabled: false,
},
],
[],
);
await validator(mockReq);
expect(mockReq.Deny).toHaveBeenCalledTimes(1);
Expand All @@ -428,6 +469,7 @@ describe("Test validation of Exemption CRs", () => {
standardFlowEnabled: false,
},
],
[],
);
await validator(mockReq);
expect(mockReq.Approve).toHaveBeenCalledTimes(1);
Expand All @@ -444,6 +486,7 @@ describe("Test validation of Exemption CRs", () => {
standardFlowEnabled: false,
},
],
[],
);
await validator(mockReq);
expect(mockReq.Approve).toHaveBeenCalledTimes(1);
Expand All @@ -462,6 +505,7 @@ describe("Test validation of Exemption CRs", () => {
},
},
],
[],
);
await validator(mockReq);
expect(mockReq.Deny).toHaveBeenCalledTimes(1);
Expand All @@ -478,6 +522,7 @@ describe("Test validation of Exemption CRs", () => {
enableAuthserviceSelector: undefined, // explicitly undefined
},
],
[],
);
await validator(mockReq);
expect(mockReq.Approve).toHaveBeenCalledTimes(1);
Expand All @@ -501,6 +546,7 @@ describe("Test Allowed SSO Client Attributes", () => {
},
},
],
[],
);
await validator(mockReq);
expect(mockReq.Deny).toHaveBeenCalledTimes(1);
Expand Down Expand Up @@ -535,6 +581,7 @@ describe("Test Allowed SSO Client Attributes", () => {
},
},
],
[],
);
await validator(mockReq);
expect(mockReq.Approve).toHaveBeenCalledTimes(1);
Expand All @@ -553,6 +600,7 @@ describe("Test Allowed SSO Client Attributes", () => {
},
},
],
[],
);
await validator(mockReq);
expect(mockReq.Deny).toHaveBeenCalledTimes(1);
Expand All @@ -571,14 +619,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);
});
});
31 changes: 31 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 @@ -7,8 +7,10 @@ import { PeprValidateRequest } from "pepr";

import { Gateway, Protocol, UDSPackage } from "..";
import { generateVSName } from "../../controllers/istio/virtual-service";
import { generateMonitorName } from "../../controllers/monitoring/common";
import { generateName } from "../../controllers/network/generate";
import { sanitizeResourceName } from "../../controllers/utils";
import { Kind } from "../../crd/generated/package-v1alpha1";
import { migrate } from "../migrate";

const invalidNamespaces = ["kube-system", "kube-public", "_unknown_", "pepr-system"];
Expand Down Expand Up @@ -186,5 +188,34 @@ export async function validator(req: PeprValidateRequest<UDSPackage>) {
}
}

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

// Ensure service and pod monitors use a unique description or selector/portName used for generating the resource name
const podMonitorNames = new Set<string>();
const svcMonitorNames = new Set<string>();

for (const monitor of monitors) {
const monitorName = generateMonitorName(pkgName, monitor);
if (monitor.kind === Kind.PodMonitor) {
if (podMonitorNames.has(monitorName)) {
return req.Deny(
`The combination of characteristics of this monitor entry would create a duplicate PodMonitor. ` +
`Verify you do not have duplicate values, or add a unique "description" field for this monitor. ` +
`The duplicate rule would be named "${monitorName}".`,
);
}
podMonitorNames.add(monitorName);
} else {
if (svcMonitorNames.has(monitorName)) {
return req.Deny(
`The combination of characteristics of this monitor entry would create a duplicate ServiceMonitor. ` +
`Verify you do not have duplicate values, or add a unique "description" field for this monitor. ` +
`The duplicate rule would be named "${monitorName}".`,
);
}
svcMonitorNames.add(monitorName);
}
}

return req.Approve();
}
Loading