From 620e6b2c98a1a810995f4431578b6c2a71479db9 Mon Sep 17 00:00:00 2001 From: Micah Nagel Date: Tue, 10 Dec 2024 12:41:20 -0700 Subject: [PATCH] fix: kubeapi netpol initialization / support for ingress policies (#1097) ## Description Fixes some issues with the netpol update logic to ensure we are accounting for ingress policies, as well as ensuring this only runs on watcher pods. Also adds jest test coverage of this function. ## Related Issue Fixes https://github.com/defenseunicorns/uds-core/issues/1101 ## Steps to Validate The primary fix here has to do with Pepr crashing on startup when an `Ingress` kubeapi policy is present. The below section steps through testing this.
Validation Steps ```console # Deploy base layer (using unicorn flavor to avoid dockerhub rate limiting) uds run test-single-layer --set LAYER=base --set FLAVOR=unicorn # Create a namespace for our test package kubectl create ns test # Create a package CR with egress and ingress kubeapi policies cat < ## 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] Test, docs, adr added or updated as needed - [x] [Contributor Guide](https://github.com/defenseunicorns/uds-template-capability/blob/main/CONTRIBUTING.md) followed --------- Co-authored-by: Chance <139784371+UnicornChance@users.noreply.github.com> --- .../network/generators/kubeAPI.spec.ts | 324 +++++++++++++++++- .../controllers/network/generators/kubeAPI.ts | 61 +++- src/pepr/operator/index.ts | 4 +- tasks.yaml | 2 +- 4 files changed, 372 insertions(+), 19 deletions(-) diff --git a/src/pepr/operator/controllers/network/generators/kubeAPI.spec.ts b/src/pepr/operator/controllers/network/generators/kubeAPI.spec.ts index 90f7bad9a..0de63061d 100644 --- a/src/pepr/operator/controllers/network/generators/kubeAPI.spec.ts +++ b/src/pepr/operator/controllers/network/generators/kubeAPI.spec.ts @@ -5,7 +5,7 @@ import { beforeEach, describe, expect, it, jest } from "@jest/globals"; import { K8s, kind } from "pepr"; -import { updateAPIServerCIDR } from "./kubeAPI"; +import { updateAPIServerCIDR, updateKubeAPINetworkPolicies } from "./kubeAPI"; type KubernetesList = { items: T[]; @@ -19,10 +19,10 @@ jest.mock("pepr", () => { }; }); -describe("updateAPIServerCIDR", () => { - const mockApply = jest.fn(); - const mockGet = jest.fn<() => Promise>>(); +const mockApply = jest.fn(); +const mockGet = jest.fn<() => Promise>>(); +describe("updateAPIServerCIDR", () => { beforeEach(() => { jest.clearAllMocks(); (K8s as jest.Mock).mockImplementation(() => ({ @@ -259,3 +259,319 @@ describe("updateAPIServerCIDR", () => { expect(mockApply).not.toHaveBeenCalled(); }); }); + +describe("updateKubeAPINetworkPolicies", () => { + beforeEach(() => { + jest.clearAllMocks(); + (K8s as jest.Mock).mockImplementation(() => ({ + WithLabel: jest.fn(() => ({ + Get: mockGet, + })), + Apply: mockApply, + })); + }); + + it("does not update an egress NetworkPolicy if the peers are already correct", async () => { + const newPeers = [{ ipBlock: { cidr: "10.0.0.1/32" } }]; + mockGet.mockResolvedValue({ + items: [ + { + metadata: { + name: "mock-netpol", + namespace: "default", + }, + spec: { + egress: [ + { + to: newPeers, + }, + ], + }, + }, + ], + } as KubernetesList); + + await updateKubeAPINetworkPolicies(newPeers); + + expect(mockGet).toHaveBeenCalled(); + expect(mockApply).not.toHaveBeenCalled(); // No update needed + }); + + it("does not update an ingress NetworkPolicy if the peers are already correct", async () => { + const newPeers = [{ ipBlock: { cidr: "10.0.0.1/32" } }]; + mockGet.mockResolvedValue({ + items: [ + { + metadata: { + name: "mock-netpol", + namespace: "default", + }, + spec: { + ingress: [ + { + from: newPeers, + }, + ], + }, + }, + ], + } as KubernetesList); + + await updateKubeAPINetworkPolicies(newPeers); + + expect(mockGet).toHaveBeenCalled(); + expect(mockApply).not.toHaveBeenCalled(); // No update needed + }); + + it("updates an egress NetworkPolicy with different peers", async () => { + const newPeers = [{ ipBlock: { cidr: "10.0.0.1/32" } }]; + const oldPeers = [{ ipBlock: { cidr: "192.168.1.0/32" } }]; + mockGet.mockResolvedValue({ + items: [ + { + metadata: { + name: "mock-netpol", + namespace: "default", + }, + spec: { + egress: [ + { + to: oldPeers, + }, + ], + }, + }, + ], + } as KubernetesList); + + await updateKubeAPINetworkPolicies(newPeers); + + expect(mockGet).toHaveBeenCalled(); + expect(mockApply).toHaveBeenCalledWith( + expect.objectContaining({ + metadata: { + name: "mock-netpol", + namespace: "default", + }, + spec: { + egress: [ + { + to: newPeers, + }, + ], + }, + }), + { force: true }, + ); + }); + + it("updates an ingress NetworkPolicy with different peers", async () => { + const newPeers = [{ ipBlock: { cidr: "10.0.0.1/32" } }]; + const oldPeers = [{ ipBlock: { cidr: "192.168.1.0/32" } }]; + mockGet.mockResolvedValue({ + items: [ + { + metadata: { + name: "mock-netpol", + namespace: "default", + }, + spec: { + ingress: [ + { + from: oldPeers, + }, + ], + }, + }, + ], + } as KubernetesList); + + await updateKubeAPINetworkPolicies(newPeers); + + expect(mockGet).toHaveBeenCalled(); + expect(mockApply).toHaveBeenCalledWith( + expect.objectContaining({ + metadata: { + name: "mock-netpol", + namespace: "default", + }, + spec: { + ingress: [ + { + from: newPeers, + }, + ], + }, + }), + { force: true }, + ); + }); + + it("updates an egress NetworkPolicy with no peers", async () => { + const newPeers = [{ ipBlock: { cidr: "10.0.0.1/32" } }]; + mockGet.mockResolvedValue({ + items: [ + { + metadata: { + name: "mock-netpol", + namespace: "default", + }, + spec: { + egress: [ + { + to: undefined, + }, + ], + }, + }, + ], + } as KubernetesList); + + await updateKubeAPINetworkPolicies(newPeers); + + expect(mockGet).toHaveBeenCalled(); + expect(mockApply).toHaveBeenCalledWith( + expect.objectContaining({ + metadata: { + name: "mock-netpol", + namespace: "default", + }, + spec: { + egress: [ + { + to: newPeers, + }, + ], + }, + }), + { force: true }, + ); + }); + + it("updates an ingress NetworkPolicy with no peers", async () => { + const newPeers = [{ ipBlock: { cidr: "10.0.0.1/32" } }]; + mockGet.mockResolvedValue({ + items: [ + { + metadata: { + name: "mock-netpol", + namespace: "default", + }, + spec: { + ingress: [ + { + from: undefined, + }, + ], + }, + }, + ], + } as KubernetesList); + + await updateKubeAPINetworkPolicies(newPeers); + + expect(mockGet).toHaveBeenCalled(); + expect(mockApply).toHaveBeenCalledWith( + expect.objectContaining({ + metadata: { + name: "mock-netpol", + namespace: "default", + }, + spec: { + ingress: [ + { + from: newPeers, + }, + ], + }, + }), + { force: true }, + ); + }); + + it("initializes missing egress rules", async () => { + const newPeers = [{ ipBlock: { cidr: "10.0.0.1/32" } }]; + mockGet.mockResolvedValue({ + items: [ + { + metadata: { + name: "mock-netpol", + namespace: "default", + }, + spec: { + egress: [{}], + }, // No egress at all + }, + ], + } as KubernetesList); + + await updateKubeAPINetworkPolicies(newPeers); + + expect(mockGet).toHaveBeenCalled(); + expect(mockApply).toHaveBeenCalledWith( + expect.objectContaining({ + metadata: { + name: "mock-netpol", + namespace: "default", + }, + spec: { + egress: [ + { + to: newPeers, + }, + ], + }, + }), + { force: true }, + ); + }); + + it("initializes missing ingress rules", async () => { + const newPeers = [{ ipBlock: { cidr: "10.0.0.1/32" } }]; + mockGet.mockResolvedValue({ + items: [ + { + metadata: { + name: "mock-netpol", + namespace: "default", + }, + spec: { + ingress: [{}], + }, // No egress at all + }, + ], + } as KubernetesList); + + await updateKubeAPINetworkPolicies(newPeers); + + expect(mockGet).toHaveBeenCalled(); + expect(mockApply).toHaveBeenCalledWith( + expect.objectContaining({ + metadata: { + name: "mock-netpol", + namespace: "default", + }, + spec: { + ingress: [ + { + from: newPeers, + }, + ], + }, + }), + { force: true }, + ); + }); + + it("handles no matching NetworkPolicies", async () => { + const newPeers = [{ ipBlock: { cidr: "10.0.0.1/32" } }]; + mockGet.mockResolvedValue({ + items: [], // No NetworkPolicies found + } as KubernetesList); + + await updateKubeAPINetworkPolicies(newPeers); + + expect(mockGet).toHaveBeenCalled(); + expect(mockApply).not.toHaveBeenCalled(); // No policies to update + }); +}); diff --git a/src/pepr/operator/controllers/network/generators/kubeAPI.ts b/src/pepr/operator/controllers/network/generators/kubeAPI.ts index b148a9a6a..6e90e0d92 100644 --- a/src/pepr/operator/controllers/network/generators/kubeAPI.ts +++ b/src/pepr/operator/controllers/network/generators/kubeAPI.ts @@ -26,17 +26,23 @@ let apiServerPeers: V1NetworkPolicyPeer[]; * Otherwise, it fetches the EndpointSlice and updates the CIDR dynamically. */ export async function initAPIServerCIDR() { - const svc = await retryWithDelay(fetchKubernetesService, log); + try { + const svc = await retryWithDelay(fetchKubernetesService, log); - // If static CIDR is defined, pass it directly - if (UDSConfig.kubeApiCidr) { - log.info( - `Static CIDR (${UDSConfig.kubeApiCidr}) is defined for KubeAPI, skipping EndpointSlice lookup.`, - ); - await updateAPIServerCIDR(svc, UDSConfig.kubeApiCidr); // Pass static CIDR - } else { - const slice = await retryWithDelay(fetchKubernetesEndpointSlice, log); - await updateAPIServerCIDR(svc, slice); + // If static CIDR is defined, pass it directly + if (UDSConfig.kubeApiCidr) { + log.info( + `Static CIDR (${UDSConfig.kubeApiCidr}) is defined for KubeAPI, skipping EndpointSlice lookup.`, + ); + await updateAPIServerCIDR(svc, UDSConfig.kubeApiCidr); // Pass static CIDR + } else { + const slice = await retryWithDelay(fetchKubernetesEndpointSlice, log); + await updateAPIServerCIDR(svc, slice); + } + } catch (error) { + log.error("Failed to initialize API Server CIDR for KubeAPI generated network policies", { + err: JSON.stringify(error), + }); } } @@ -156,10 +162,39 @@ export async function updateKubeAPINetworkPolicies(newPeers: V1NetworkPolicyPeer .Get(); for (const netPol of netPols.items) { - const oldPeers = netPol.spec?.egress?.[0].to; + // Safety check for network policy spec existence + if (!netPol.spec) { + log.warn( + `KubeAPI NetworkPolicy ${netPol.metadata!.namespace}/${netPol.metadata!.name} is missing spec.`, + ); + continue; + } + + let updateRequired = false; + // Handle egress policies + if (netPol.spec.egress) { + if (!netPol.spec.egress[0]) { + netPol.spec.egress[0] = { to: [] }; + } + const oldPeers = netPol.spec.egress[0].to; + if (!R.equals(oldPeers, newPeers)) { + updateRequired = true; + netPol.spec.egress[0].to = newPeers; + } + // Handle ingress policies + } else if (netPol.spec.ingress) { + if (!netPol.spec.ingress[0]) { + netPol.spec.ingress[0] = { from: [] }; + } + const oldPeers = netPol.spec.ingress[0].from; + if (!R.equals(oldPeers, newPeers)) { + updateRequired = true; + netPol.spec.ingress[0].from = newPeers; + } + } - if (!R.equals(oldPeers, newPeers)) { - netPol.spec!.egress![0].to = newPeers; + // If the policy required a change, apply the new policy + if (updateRequired) { if (netPol.metadata) { // Remove managed fields to prevent errors on server side apply netPol.metadata.managedFields = undefined; diff --git a/src/pepr/operator/index.ts b/src/pepr/operator/index.ts index e4ca3d9ff..1e8882cb1 100644 --- a/src/pepr/operator/index.ts +++ b/src/pepr/operator/index.ts @@ -31,7 +31,9 @@ const log = setupLogger(Component.OPERATOR); // Pre-populate the API server CIDR since we are not persisting the EndpointSlice // Note ignore any errors since the watch will still be running hereafter -void initAPIServerCIDR(); +if (process.env.PEPR_WATCH_MODE === "true" || process.env.PEPR_MODE === "dev") { + void initAPIServerCIDR(); +} // Watch for changes to the API server EndpointSlice and update the API server CIDR // Skip if a CIDR is defined in the UDS Config diff --git a/tasks.yaml b/tasks.yaml index 5e9b388eb..ce2a6986c 100644 --- a/tasks.yaml +++ b/tasks.yaml @@ -40,7 +40,7 @@ tasks: echo "Next steps:" echo " - To test & develop the Pepr module, run 'npx pepr dev' from a Javascript debug terminal" echo " - Otherwise run 'npx pepr deploy' to deploy the Pepr module to the cluster" - echo " - Additional source packages can be deployed with 'zarf dev deploy src/'" + echo " - Additional source packages can be deployed with 'zarf dev deploy src/ --flavor upstream'" - name: slim-dev actions: