False positive on Network Policies .. Sometimes

[bpfoster]

Describe the bug
Running popeye sometimes reports

  · foo/foo-75f84cd95b-9s62d...................................................😱
    😱 [POP-1204] Pod Ingress is not secured by a network policy.
    😱 [POP-1204] Pod Egress is not secured by a network policy.

Despite there being a Network Policy for it.

Expected behavior
No warning on missing Network Policy

Versions (please complete the following information):

• OS: RHEL 8
• Popeye: v0.21.3
• K8s: v1.26.11

Additional context
I have created NetworkPolicies using the

apiVersion: networking.k8s.io/v1
kind: NetworkPolicy

resource type.

I notice the cluster also has Calico installed providing their NetworkPolicy CRD:

$ kubectl get crd | grep networkpolicies
globalnetworkpolicies.crd.projectcalico.org
networkpolicies.crd.projectcalico.org

Installed policies:

$ kubectl get networkpolicies.crd.projectcalico.org 
No resources found in foo namespace.

$ kubectl get networkpolicies.networking.k8s.io 
NAME                                     POD-SELECTOR
foo                 app.kubernetes.io/instance=foo,app.kubernetes.io/name=foo

When I debug popeye, I see that the GVR being queried in the DB for network policies is sometimes the calico one, and sometimes the k8s.io one.

popeye/internal/lint/pod.go

Line 96 in d09ec25

txn, it := s.db.MustITForNS(internal.Glossary[internal.NP], pod.Namespace)

When it's the Calico one, the NetworkPolicy is not found.

My guess is there's some non-determinism in the order in which the CRDs are returned/loaded, and from what I can see, the popeye DB only supports 1 per type and is overwriting whichever comes first with the second one.

popeye/internal/alias.go

Lines 92 to 116 in d09ec25

	func (a *Aliases) Realize() {
	for gvr, res := range a.metas {
	a.aliases[res.Name] = gvr
	if res.SingularName != "" {
	a.aliases[res.SingularName] = gvr
	}
	for _, n := range res.ShortNames {
	a.aliases[n] = gvr
	}
	if kk, ok := customShortNames[R(res.Name)]; ok {
	for _, k := range kk {
	a.aliases[k] = gvr
	}
	}
	if lgvr, ok := Glossary[R(res.SingularName)]; ok {
	if greaterV(gvr.V(), lgvr.V()) {
	Glossary[R(res.SingularName)] = gvr
	}
	} else if lgvr, ok := Glossary[R(res.Name)]; ok {
	if greaterV(gvr.V(), lgvr.V()) {
	Glossary[R(res.Name)] = gvr
	}
	}
	}
	}

Thinking about it... should popeye know the GVR for each resource type it cares about, and ignore others? In this case it knows it wants networkpolicies.networking.k8s.io and would discard the caclico type. If there was a calico NP (or some other random one), popeye would panic when it tries to assert to netv1.NetworkPolicy.

[bpfoster]

This is maybe out of scope for this issue, and admittedly I'm still working on understanding Network Policies, but I'm struggling to understand the logic in Popeye on checking ingress/egress policies.

If we get to this branch of code:

popeye/internal/lint/pod.go

Lines 119 to 126 in d09ec25

	if labelsMatch(&np.Spec.PodSelector, pod.Labels) {
	if s.checkIngresses(ctx, pod, np.Spec.Ingress) {
	matches[0]++
	}
	if s.checkEgresses(ctx, pod, np.Spec.Egress) {
	matches[1]++
	}
	}

I'm not sure the logic in checkIngresses / checkEgresses is wholly correct.

popeye/internal/lint/pod.go

Lines 177 to 199 in d09ec25

	func (s *Pod) checkIngresses(ctx context.Context, pod *v1.Pod, rr []netv1.NetworkPolicyIngressRule) bool {
	var match int
	if rr == nil {
	return false
	}
	for _, r := range rr {
	if r.From == nil {
	return true
	}
	for _, from := range r.From {
	ok, err := s.isPodTargeted(pod, from.NamespaceSelector, from.PodSelector, from.IPBlock)
	if err != nil {
	s.AddErr(ctx, err)
	return true
	}
	if ok {
	match++
	}
	}
	}
	return match > 0
	}

If we have a NetworkPolicy which targets the pod in question, and it has ingress rules, checkIngresses appears to want the ingress's from to point to the same pod.

Similarly for checkEgresses, it's wanting the egress's to to point to the same pod.

In other words, this seems to only pass when the NetworkPolicy says the pod can talk to itself.

[derailed]

@bpfoster For the initial issue, this is indeed a problem. Will fix on the next drop.
For the later, this code checks if a given pod is protected by a networkpolicy.

[derailed]

Fixes v0.21.5

[siegy22]

I just had a similar thing come up when denying egress:

  · about-me/about-me-5d8f856785-pb7vx.............................................................😱
    😱 [POP-1204] Pod Egress is not secured by a network policy.

This is my NetworkPolicy

apiVersion: networking.k8s.io/v1
kind: NetworkPolicy
metadata:
  name: deny-egress
spec:
  podSelector:
    matchLabels:
      app: about-me
  policyTypes:
    - Ingress
    - Egress
  ingress:
    - {}
  egress: []

I did verify that the policy works by looking at the drops in Cilium.

Am I missing something?

[bpfoster]

I hadn't revisited this. I too am still getting (at least consistently now!) false positives on networkpolicy linting (on latest popeye v0.21.5).

In my case, I get Pod Ingress is not secured by a network policy despite NetworkPolicies being present. checkIngresses() is still coming back with no matches so I still wonder about how that is being evaluated.
My Network Policies look like:

apiVersion: networking.k8s.io/v1
kind: NetworkPolicy
metadata:
  labels:
    app.kubernetes.io/instance: my-instance
    app.kubernetes.io/managed-by: Helm
    app.kubernetes.io/name: my-service
    app.kubernetes.io/version: 0.0.1
    helm.sh/chart: my-service-0.1.0
  name: my-networkpolicy
  namespace: my-namespace
spec:
  egress:
  - ports:
    - port: 53
      protocol: UDP
    - port: 53
      protocol: TCP
  - to:
    - namespaceSelector:
        matchLabels:
          kubernetes.io/metadata.name: my-namespace
  - ports:
    - port: 80
      protocol: TCP
  ingress:
  - from:
    - namespaceSelector:
        matchLabels:
          kubernetes.io/metadata.name: my-namespace
    ports:
    - port: http
      protocol: TCP
  podSelector:
    matchLabels:
      app.kubernetes.io/instance: my-instance
      app.kubernetes.io/name: my-service
  policyTypes:
  - Ingress
  - Egress

For @siegy22 's example, playing around with some unit tests, changing egress to look like ingress removes the finding:

ingress:
  - {}
egress:
  - {}

But I frankly don't know if the result is the same?

[siegy22]

@bpfoster Your example denies ingress and egress. My goal is to have any ingress and no egress.

Which is defined by:

ingress:
  - {}
egress: []

But popeye says there's no egress defined (which in theory is correct) but the effect is not the same as not defining it.

[bpfoster]

@derailed please see PR #367 - I think this will address the false positives @siegy22 and I are seeing, if I have not misunderstood something here.