Skip to content

Commit

Permalink
fix: pepr operator derived netpol name collisions (#480)
Browse files Browse the repository at this point in the history
## Description
Fixes network policy naming bug that caused name collisions for operator
derived network policies. This causes network policies being created for
VirtualServices using the same selector and gateway but different ports
to be dropped. Multiple legitimate policies would override each other
and only the last would survive.

There are still gaps in the fundamentals of naming and validation that I
came across on different iterations on solutions for this. I will be
creating a follow on issue for a deeper dive into that though. This is a
small improvement that fixes a specific issue.
...

## Related Issue

Fixes #466 

## 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
Steps](https://github.com/defenseunicorns/uds-template-capability/blob/main/CONTRIBUTING.md)(https://github.com/defenseunicorns/uds-template-capability/blob/main/CONTRIBUTING.md#submitting-a-pull-request)
followed
  • Loading branch information
anthonywendt authored Jun 13, 2024
1 parent f967f9a commit de60e25
Show file tree
Hide file tree
Showing 6 changed files with 86 additions and 53 deletions.
8 changes: 5 additions & 3 deletions src/pepr/operator/controllers/network/generate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { intraNamespace } from "./generators/intraNamespace";
import { kubeAPI } from "./generators/kubeAPI";

export function generate(namespace: string, policy: Allow): kind.NetworkPolicy {
// Create a unique name for the NetworkPolicy based on the package name, index, direction, pod labels, and port
// Generate a unique name for the NetworkPolicy
const name = generateName(policy);

// Create the NetworkPolicy
Expand Down Expand Up @@ -112,9 +112,11 @@ export function generate(namespace: string, policy: Allow): kind.NetworkPolicy {
}

/**
* Generates a unique name for the NetworkPolicy based on the description, direction, and combination of remote properties
* Generates a unique name for the NetworkPolicy based on the policy.
* Will use the description if it exists, otherwise it will use the
* direction and combination of remote properties.
*
* @param policy the name of the policy
* @param policy The policy to generate a name for
*/
export function generateName(policy: Allow) {
const name =
Expand Down
13 changes: 9 additions & 4 deletions src/pepr/operator/controllers/network/policies.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ export async function networkPolicies(pkg: UDSPackage, namespace: string) {

Log.debug(pkg.metadata, `Generating NetworkPolicies for generation ${generation}`);

// Create default policies
const policies = [
// All traffic must be explicitly allowed
defaultDenyAll(namespace),
Expand All @@ -41,6 +42,9 @@ export async function networkPolicies(pkg: UDSPackage, namespace: string) {
for (const expose of exposeList.filter(exp => !exp.advancedHTTP?.directResponse)) {
const { gateway = Gateway.Tenant, port, selector = {}, targetPort } = expose;

// Use the same port as the VirtualService if targetPort is not set
const policyPort = targetPort ?? port;

// Create the NetworkPolicy for the VirtualService
const policy: Allow = {
direction: Direction.Ingress,
Expand All @@ -49,9 +53,9 @@ export async function networkPolicies(pkg: UDSPackage, namespace: string) {
remoteSelector: {
app: `${gateway}-ingressgateway`,
},
// Use the same port as the VirtualService if targetPort is not set
port: targetPort ?? port,
description: `${Object.values(selector)} Istio ${gateway} gateway`,
port: policyPort,
// Use the port, selector, and gateway to generate a description for VirtualService derived policies
description: `${policyPort}-${Object.values(selector)} Istio ${gateway} gateway`,
};

// Generate the policy
Expand All @@ -74,7 +78,8 @@ export async function networkPolicies(pkg: UDSPackage, namespace: string) {
app: "prometheus",
},
port: targetPort,
description: `${Object.values(selector)} Metrics`,
// Use the targetPort and selector to generate a description for the ServiceMonitor derived policies
description: `${targetPort}-${Object.values(selector)} Metrics`,
};
// Generate the policy
const generatedPolicy = generate(namespace, policy);
Expand Down
8 changes: 4 additions & 4 deletions src/test/app-admin.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -75,10 +75,10 @@ spec:
name: httpbin
resources:
limits:
cpu: 100m
memory: 128Mi
cpu: 50m
memory: 64Mi
requests:
cpu: 100m
memory: 128Mi
cpu: 50m
memory: 64Mi
ports:
- containerPort: 80
91 changes: 55 additions & 36 deletions src/test/app-tenant.yaml
Original file line number Diff line number Diff line change
@@ -1,74 +1,93 @@
apiVersion: v1
kind: Namespace
metadata:
name: test-app
name: test-tenant-app
---
apiVersion: uds.dev/v1alpha1
kind: Package
metadata:
name: httpbin
namespace: test-app
name: test-tenant-app
namespace: test-tenant-app
spec:
network:
expose:
- service: httpbin
- service: test-tenant-app
selector:
app: httpbin
app: test-tenant-app
gateway: tenant
host: demo
port: 8000
targetPort: 80
---
apiVersion: v1
kind: ServiceAccount
metadata:
name: httpbin
namespace: test-app
host: demo-8080
port: 8080
- service: test-tenant-app
selector:
app: test-tenant-app
gateway: tenant
host: demo-8081
port: 8081
---
apiVersion: v1
kind: Service
metadata:
name: httpbin
namespace: test-app
name: test-tenant-app
namespace: test-tenant-app
labels:
app: httpbin
service: httpbin
app: test-tenant-app
service: test-tenant-app
spec:
ports:
- name: http
port: 8000
targetPort: 80
- name: port8080
port: 8080
targetPort: 8080
- name: port8081
port: 8081
targetPort: 8081
selector:
app: httpbin
app: test-tenant-app
---
apiVersion: apps/v1
kind: Deployment
metadata:
name: httpbin
namespace: test-app
name: http-echo-multi-port
namespace: test-tenant-app
spec:
replicas: 1
selector:
matchLabels:
app: httpbin
version: v1
app: test-tenant-app
template:
metadata:
labels:
app: httpbin
version: v1
app: test-tenant-app
spec:
serviceAccountName: httpbin
containers:
- image: docker.io/kong/httpbin
- name: http-echo-port-8080
image: hashicorp/http-echo
imagePullPolicy: IfNotPresent
args:
- "-text=Hello from port 8080"
- "-status-code=200"
- "-listen=:8080"
resources:
limits:
cpu: 50m
memory: 64Mi
requests:
cpu: 50m
memory: 64Mi
ports:
- containerPort: 8080
- name: http-echo-port-8081
image: hashicorp/http-echo
imagePullPolicy: IfNotPresent
name: httpbin
args:
- "-text=Hello from port 8081"
- "-status-code=200"
- "-listen=:8081"
resources:
limits:
cpu: 100m
memory: 128Mi
cpu: 50m
memory: 64Mi
requests:
cpu: 100m
memory: 128Mi
cpu: 50m
memory: 64Mi
ports:
- containerPort: 80
- containerPort: 8081
17 changes: 12 additions & 5 deletions src/test/tasks.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ tasks:
wait:
cluster:
kind: Deployment
name: httpbin
namespace: test-app
name: http-echo-multi-port
namespace: test-tenant-app

- description: Verify the admin app is accessible
wait:
Expand All @@ -43,12 +43,19 @@ tasks:
address: demo.admin.uds.dev/status/410
code: 410

- description: Verify the tenant app is accessible
- description: Verify the tenant app 8080 is accessible
wait:
network:
protocol: https
address: demo.uds.dev/status/202
code: 202
address: demo-8080.uds.dev
code: 200

- description: Verify the tenant app 8081 is accessible
wait:
network:
protocol: https
address: demo-8081.uds.dev
code: 200

- description: Verify podinfo is healthy
wait:
Expand Down
2 changes: 1 addition & 1 deletion src/test/zarf.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@ components:
- name: app-tenant
files:
- "app-tenant.yaml"

images:
- docker.io/kong/httpbin:latest
- hashicorp/http-echo:latest

- name: podinfo
required: true
Expand Down

0 comments on commit de60e25

Please sign in to comment.