Weave crashed when named port was specified in network policy

[vonsch]

What you expected to happen?

Weave didn't crash when named port was specified. It should rather throw some error but not crash networking on whole k8s cluster.

What happened?

One colleague specified named port in helm chart and attempted to deliver the chart to our kubernetes cluster. Instead of throwing some error, whole cluster was knocked down because weave died.

How to reproduce it?

Try following helm chart:

---
apiVersion: networking.k8s.io/v1
kind: NetworkPolicy
...
spec:
  ...
  egress:
    - protocol: TCP
      port: https

Anything else we need to know?

nothing

Versions:

weave: 2.6.0

# crictl version
Version:  0.1.0
RuntimeName:  containerd
RuntimeVersion:  1.2.12
RuntimeApiVersion:  v1alpha2

# cat /etc/redhat-release 
CentOS Linux release 7.7.1908 (Core)

Logs:

INFO: 2020/03/13 15:52:39.470977 EVENT AddNetworkPolicy {"metadata":{"creationTimestamp":"2020-03-13T14:33:10Z","generation":1,"labels":{"app.kubernetes.io/instance":"connectors-mariadb-galera","app.kubernetes.io/managed-by":"Tiller","app.kubernetes.io/name":"connectors-mariadb-galera","helm.sh/chart":"connectors-mariadb-galera-0.0.19_ee942b1"},"name":"connectors-mariadb-galera-backup","namespace":"connectors","resourceVersion":"142168210","selfLink":"/apis/networking.k8s.io/v1/namespaces/connectors/networkpolicies/connectors-mariadb-galera-backup","uid":"4c3b08b3-6769-4a42-a2ac-e29ea45eab5e"},"spec":{"egress":[{"ports":[{"port":53,"protocol":"UDP"},{"port":53,"protocol":"TCP"},{"port":"https","protocol":"TCP"}]}],"podSelector":{"matchLabels":{"app":"connectors-mariadb-galera-backup","release":"connectors-mariadb-galera"}},"policyTypes":["Egress"]}}
FATA: 2020/03/13 15:52:39.471084 add network policy: named ports in network policies is not supported yet. Rejecting network policy: connectors-mariadb-galera-backup from further processing. named port https in network policy

[vonsch]

Please note this was discussed also in #3032

[johny-mnemonic]

Presumably it should have been fixed by #3375, but either that fix is not in 2.6.0, or it doesn't work.

[murali-reddy]

As mentioned in #3032 comment #3375 only added error message indicating named port is not supported. In general any errors seen while processing network policies is considered as fatal error, intent is that to avoid a case error is logged in to weave-npc logs, but user may not notice it and assume network policies are imposed.

Fatal error results in weave-npc crash, which is not best way to go about it as it. We will try to find a better way the case where invalid or unsupported netwrok policies are applied.

[johny-mnemonic]

Oh my :-(
Thanks for explanation @murali-reddy.
There are multiple correct ways of handling unsupported policy statements, but fatal error and crash is definitely not one of them...
What about just refusing to apply unsupported policy statements? Or even better, when encountering unsupported policy statement, simply revert to last applied policy and return big error.
Why on earth #3375 just added log message is beyond me :-O

[tranx]

I do agree with @johny-mnemonic here, if it is not applied, we need to find a different solution than fatal error. An event sent to the service may be the best thing to do?

In kubernetes, we use helm charts provided by 3rd party and it is hard to control how charts are written. Having a design where the install of a 3rd party chart crashing an entire cluster is not good option aw we expect good separation between apps.