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.

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

Make sure that policies with no valid peers are enforced #65

Merged
merged 2 commits into from
Sep 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
13 changes: 2 additions & 11 deletions .github/workflows/kind-e2e.yml
Original file line number Diff line number Diff line change
Expand Up @@ -30,17 +30,8 @@ jobs:
export TERM=dumb
# enable ip6_tables
sudo modprobe ip6_tables
bats ./tests/simple-v4-ingress.bats
bats ./tests/simple-v4-egress.bats
bats ./tests/simple-v6-ingress.bats
bats ./tests/ipblock.bats
# stacked case
bats ./tests/stacked.bats
bats ./tests/ipblock-stacked.bats
# multiple items in egress/ingress
bats ./tests/ipblock-list.bats
bats ./tests/simple-v4-ingress-list.bats
bats ./tests/simple-v4-egress-list.bats

bats ./tests/*.bats

- name: Export kind logs
if: ${{ failure() }}
Expand Down
63 changes: 63 additions & 0 deletions e2e/tests/ingress-ns-selector-no-pods.bats
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
#!/usr/bin/env bats


# Note:
# These test cases verify that an ingress rule on the server that matches no pods
# does not allow any pods to reach the server.

setup() {
cd $BATS_TEST_DIRNAME
load "common"
server_net1=$(get_net1_ip "test-ingress-ns-selector-no-pods" "pod-server")
client_a_net1=$(get_net1_ip "test-ingress-ns-selector-no-pods" "pod-client-a")
client_b_net1=$(get_net1_ip "test-ingress-ns-selector-no-pods-blue" "pod-client-b")
}

@test "setup ingress-ns-selector-no-pods test environments" {
# create test manifests
kubectl create -f ingress-ns-selector-no-pods.yml

# verify all pods are available
run kubectl -n test-ingress-ns-selector-no-pods wait --for=condition=ready -l app=test-ingress-ns-selector-no-pods pod --timeout=${kubewait_timeout}
[ "$status" -eq "0" ]

sleep 5
}

@test "test-ingress-ns-selector-no-pods check client-a -> server" {
# nc should NOT succeed from client-a to server by policy
run kubectl -n test-ingress-ns-selector-no-pods exec pod-client-a -- sh -c "echo x | nc -w 1 ${server_net1} 5555"
[ "$status" -eq "1" ]
}

@test "test-ingress-ns-selector-no-pods check client-b -> server" {
# nc should NOT succeed from client-b to server by policy
run kubectl -n test-ingress-ns-selector-no-pods-blue exec pod-client-b -- sh -c "echo x | nc -w 1 ${server_net1} 5555"
[ "$status" -eq "1" ]
}

@test "test-ingress-ns-selector-no-pods check server -> client-a" {
# nc should succeed from server to client-a by no policy definition for direction (egress for pod-server)
run kubectl -n test-ingress-ns-selector-no-pods exec pod-server -- sh -c "echo x | nc -w 1 ${client_a_net1} 5555"
[ "$status" -eq "0" ]
}

@test "test-ingress-ns-selector-no-pods check server -> client-b" {
# nc should succeed from server to client-b by no policy definition for direction (egress for pod-server)
run kubectl -n test-ingress-ns-selector-no-pods exec pod-server -- sh -c "echo x | nc -w 1 ${client_b_net1} 5555"
[ "$status" -eq "0" ]
}

@test "cleanup environments" {
# remove test manifests
kubectl delete -f ingress-ns-selector-no-pods.yml
run kubectl -n test-ingress-ns-selector-no-pods wait --for=delete -l app=test-ingress-ns-selector-no-pods pod --timeout=${kubewait_timeout}
[ "$status" -eq "0" ]

sleep 3
# check that no iptables files in pod-iptables
pod_name=$(kubectl -n kube-system get pod -o wide | grep 'kind-worker' | grep multi-net | cut -f 1 -d ' ')
run kubectl -n kube-system exec ${pod_name} -- \
sh -c "find /var/lib/multi-networkpolicy/iptables/ -name '*.iptables' | wc -l"
[ "$output" = "0" ]
}
109 changes: 109 additions & 0 deletions e2e/tests/ingress-ns-selector-no-pods.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
---
apiVersion: "k8s.cni.cncf.io/v1"
kind: NetworkAttachmentDefinition
metadata:
namespace: default
name: macvlan1-simple
spec:
config: '{
"cniVersion": "0.3.1",
"name": "macvlan1-simple",
"plugins": [
{
"type": "macvlan",
"mode": "bridge",
"ipam":{
"type":"host-local",
"subnet":"2.2.6.0/24",
"rangeStart":"2.2.6.8",
"rangeEnd":"2.2.6.67"
}
}]
}'
---
# namespace for MultiNetworkPolicy
apiVersion: v1
kind: Namespace
metadata:
name: test-ingress-ns-selector-no-pods
---
apiVersion: v1
kind: Namespace
metadata:
name: test-ingress-ns-selector-no-pods-blue
---
# Pods
apiVersion: v1
kind: Pod
metadata:
name: pod-server
namespace: test-ingress-ns-selector-no-pods
annotations:
k8s.v1.cni.cncf.io/networks: default/macvlan1-simple
labels:
app: test-ingress-ns-selector-no-pods
name: pod-server
spec:
containers:
- name: macvlan-worker1
image: ghcr.io/k8snetworkplumbingwg/multi-networkpolicy-iptables:e2e-test
command: ["nc", "-klp", "5555"]
securityContext:
privileged: true
---
apiVersion: v1
kind: Pod
metadata:
name: pod-client-a
namespace: test-ingress-ns-selector-no-pods
annotations:
k8s.v1.cni.cncf.io/networks: default/macvlan1-simple
labels:
app: test-ingress-ns-selector-no-pods
name: pod-client-a
spec:
containers:
- name: macvlan-worker1
image: ghcr.io/k8snetworkplumbingwg/multi-networkpolicy-iptables:e2e-test
command: ["nc", "-klp", "5555"]
securityContext:
privileged: true
---
apiVersion: v1
kind: Pod
metadata:
name: pod-client-b
namespace: test-ingress-ns-selector-no-pods-blue
annotations:
k8s.v1.cni.cncf.io/networks: default/macvlan1-simple
labels:
app: test-ingress-ns-selector-no-pods
name: pod-client-b
spec:
containers:
- name: macvlan-worker1
image: ghcr.io/k8snetworkplumbingwg/multi-networkpolicy-iptables:e2e-test
command: ["nc", "-klp", "5555"]
securityContext:
privileged: true
---
# MultiNetworkPolicies
# this policy accepts ingress trafic from pod-client-a to pod-server
apiVersion: k8s.cni.cncf.io/v1beta1
kind: MultiNetworkPolicy
metadata:
name: policy-test-ingress-ns-selector-no-pods
namespace: test-ingress-ns-selector-no-pods
annotations:
k8s.v1.cni.cncf.io/policy-for: default/macvlan1-simple
spec:
podSelector:
matchLabels:
name: pod-server
ingress:
- from:
- namespaceSelector:
matchLabels:
foo: bar # No namespace with this label exists
policyTypes:
- Ingress
12 changes: 2 additions & 10 deletions pkg/server/policyrules.go
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,6 @@ func (ipt *iptableBuffer) renderIngressFrom(s *Server, podInfo *controllers.PodI
writeLine(ipt.policyIndex, "-A", fmt.Sprintf("MULTI-%d-INGRESS", pIndex), "-j", chainName)

s.podMap.Update(s.podChanges)
validPeers := 0

Choose a reason for hiding this comment

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

it seems to me that you don't need to count. This could be a boolean and switches to true when there is at least one valid peer

Copy link
Member Author

Choose a reason for hiding this comment

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

I completely removed that variable, as in my idea there is no need to check if there is at least one valid peer. Does it sound good?

Choose a reason for hiding this comment

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

right, I did not see it correctly

for _, peer := range from {
if peer.PodSelector != nil || peer.NamespaceSelector != nil {
podSelectorMap, err := metav1.LabelSelectorAsMap(peer.PodSelector)
Expand Down Expand Up @@ -334,7 +333,6 @@ func (ipt *iptableBuffer) renderIngressFrom(s *Server, podInfo *controllers.PodI
writeLine(ipt.ingressFrom, "-A", chainName,
"-i", podIntf.InterfaceName, "-s", ip,
"-j", "MARK", "--set-xmark", "0x20000/0x20000")
validPeers++
}
}
// ingress should accept reverse path
Expand All @@ -357,7 +355,6 @@ func (ipt *iptableBuffer) renderIngressFrom(s *Server, podInfo *controllers.PodI
if ipt.isIPFamilyCompatible(except) {
writeLine(ipt.ingressFrom, "-A", chainName,
"-i", podIntf.InterfaceName, "-s", except, "-j", "DROP")
validPeers++
}
}
}
Expand All @@ -369,7 +366,6 @@ func (ipt *iptableBuffer) renderIngressFrom(s *Server, podInfo *controllers.PodI
writeLine(ipt.ingressFrom, "-A", chainName,
"-i", podIntf.InterfaceName, "-s", peer.IPBlock.CIDR,
"-j", "MARK", "--set-xmark", "0x20000/0x20000")
validPeers++
}
}
for _, podIntf := range podInfo.Interfaces {
Expand All @@ -390,7 +386,7 @@ func (ipt *iptableBuffer) renderIngressFrom(s *Server, podInfo *controllers.PodI
}

// Add skip rule if no froms
if len(from) == 0 || validPeers == 0 {
if len(from) == 0 {
writeLine(ipt.ingressFrom, "-A", chainName,
"-m", "comment", "--comment", "\"no ingress from, skipped\"",
"-j", "MARK", "--set-xmark", "0x20000/0x20000")
Expand Down Expand Up @@ -509,7 +505,6 @@ func (ipt *iptableBuffer) renderEgressTo(s *Server, podInfo *controllers.PodInfo
writeLine(ipt.policyIndex, "-A", fmt.Sprintf("MULTI-%d-EGRESS", pIndex), "-j", chainName)

s.podMap.Update(s.podChanges)
validPeers := 0
for _, peer := range to {
if peer.PodSelector != nil || peer.NamespaceSelector != nil {
podSelectorMap, err := metav1.LabelSelectorAsMap(peer.PodSelector)
Expand Down Expand Up @@ -564,7 +559,6 @@ func (ipt *iptableBuffer) renderEgressTo(s *Server, podInfo *controllers.PodInfo
writeLine(ipt.egressTo, "-A", chainName,
"-o", podIntf.InterfaceName, "-d", ip,
"-j", "MARK", "--set-xmark", "0x20000/0x20000")
validPeers++
}
}
// egress should accept reverse path
Expand All @@ -587,7 +581,6 @@ func (ipt *iptableBuffer) renderEgressTo(s *Server, podInfo *controllers.PodInfo
if ipt.isIPFamilyCompatible(except) {
writeLine(ipt.egressTo, "-A", chainName,
"-o", multi.InterfaceName, "-d", except, "-j", "DROP")
validPeers++
}
}
}
Expand All @@ -599,7 +592,6 @@ func (ipt *iptableBuffer) renderEgressTo(s *Server, podInfo *controllers.PodInfo
writeLine(ipt.egressTo, "-A", chainName,
"-o", podIntf.InterfaceName, "-d", peer.IPBlock.CIDR,
"-j", "MARK", "--set-xmark", "0x20000/0x20000")
validPeers++
}
}
// egress should accept reverse path
Expand All @@ -621,7 +613,7 @@ func (ipt *iptableBuffer) renderEgressTo(s *Server, podInfo *controllers.PodInfo
}

// Add skip rules if no to
if len(to) == 0 || validPeers == 0 {
if len(to) == 0 {
writeLine(ipt.egressTo, "-A", chainName,
"-m", "comment", "--comment", "\"no egress to, skipped\"",
"-j", "MARK", "--set-xmark", "0x20000/0x20000")
Expand Down
Loading
Loading