weave-npc doesn't handle named ports

[mikebryant]

What you expected to happen?

For weave-npc not to crash

What happened?

I used a NetworkPolicy with a named port, as per https://kubernetes.io/docs/api-reference/extensions/v1beta1/definitions/#_v1beta1_networkpolicyport
This can either be a numerical or named port on a pod.

This causes weave-npc to crash

How to reproduce it?

Use a NetworkPolicy with a named port

Versions:

$ weave version
1.9.5
$ kubectl version
Client Version: version.Info{Major:"1", Minor:"6", GitVersion:"v1.6.5", GitCommit:"490c6f13df1cb6612e0993c4c14f2ff90f8cdbf3", GitTreeState:"clean", BuildDate:"2017-06-14T20:15:53Z", GoVersion:"go1.7.6", Compiler:"gc", Platform:"linux/amd64"}
Server Version: version.Info{Major:"1", Minor:"6", GitVersion:"v1.6.1+coreos.0", GitCommit:"9212f77ed8c169a0afa02e58dce87913c6387b3e", GitTreeState:"clean", BuildDate:"2017-04-04T00:32:53Z", GoVersion:"go1.7.5", Compiler:"gc", Platform:"linux/amd64"}

Logs:

INFO: 2017/06/23 14:13:29.046710 EVENT AddNetworkPolicy {"metadata":{"name":"postgres","namespace":"screenful","selfLink":"/apis/extensions/v1beta1/namespaces/screenful/networkpolicies/postgres","uid":"33035a7e-581d-11e7-9212-fa163e65bc02","resourceVersion":"5949240","generation":1,"creationTimestamp":"2017-06-23T14:06:49Z","annotations":{"kubectl.kubernetes.io/last-applied-configuration":"{\"apiVersion\":\"extensions/v1beta1\",\"kind\":\"NetworkPolicy\",\"metadata\":{\"annotations\":{},\"name\":\"postgres\",\"namespace\":\"screenful\"},\"spec\":{\"ingress\":[{\"from\":[{\"podSelector\":{\"matchLabels\":{\"app\":\"screenful\"}}}],\"ports\":[{\"port\":\"postgres\",\"protocol\":\"TCP\"}]}],\"podSelector\":{\"matchLabels\":{\"app\":\"postgres\"}}}}\n"}},"spec":{"podSelector":{"matchLabels":{"app":"postgres"}},"ingress":[{"ports":[{"protocol":"TCP","port":"postgres"}],"from":[{"podSelector":{"matchLabels":{"app":"screenful"}}}]}]}}
INFO: 2017/06/23 14:13:29.047504 creating ipset: &npc.selectorSpec{key:"", selector:labels.internalSelector{}, ipsetType:"hash:ip", ipsetName:"weave-yw.D0czlULhY^Na(}/meeeGHH"}
INFO: 2017/06/23 14:13:29.052048 creating ipset: &npc.selectorSpec{key:"app=postgres", selector:labels.internalSelector{labels.Requirement{key:"app", operator:"=", strValues:[]string{"postgres"}}}, ipsetType:"hash:ip", ipsetName:"weave-5*HDryZlO>b4*738kizjErDH)"}
INFO: 2017/06/23 14:13:29.056654 creating ipset: &npc.selectorSpec{key:"app=screenful", selector:labels.internalSelector{labels.Requirement{key:"app", operator:"=", strValues:[]string{"screenful"}}}, ipsetType:"hash:ip", ipsetName:"weave-QYIlr%V?kjp#HJ7cHYQjMOivC"}
INFO: 2017/06/23 14:13:29.067510 adding rule: [-p TCP -m set --match-set weave-QYIlr%V?kjp#HJ7cHYQjMOivC src -m set --match-set weave-5*HDryZlO>b4*738kizjErDH) dst --dport postgres -j ACCEPT]
FATA: 2017/06/23 14:13:29.071935 add network policy: exit status 2: iptables v1.6.0: invalid port/service `postgres' specified
Try `iptables -h' or 'iptables --help' for more information.

[bboreham]

Yes, sorry, this should be documented.

Named ports make the implementation hugely more complicated, as they can map to different numerical values on different pods.

[mikebryant]

Mhm. Even if we can't easily implement it right now, I think the bug of it crashing and stopping all of your policies working for a whole cluster needs to be fixed, and just log an error message.

[bboreham]

Yes. Some way to surface the fact this isn't working without having to hunt through log files would be even better.

[mikebryant]

Events API?
You could add an event referencing the NetworkPolicy object saying it wasn't able to be applied. (And keep incrementing the counter/last seen time, every time one of the weave pods tries to apply it)

[bboreham]

Ref #2612

[deitch]

Aha! I just stumbled across this problem. Yeah, the pods crash when someone used named ports instead of numbered.

I think it is ok to say, "use numbered ports only for NetworkPolicy (although it should be well documented), but the pods crashing probably isn't a good reaction.

The NetworkPolicy spec included:

    ports:
    - protocol: TCP
      port: rest

And the weave-npc logs:

INFO: 2018/05/15 14:31:25.280938 adding rule: [-p TCP -m set --match-set weave-2at9bG:l!BgZVhn*u5MlUOCHX dst --dport rest -j ACCEPT -m comment --comment anywhere -> pods: namespace: default, selector: secure=something]
FATA: 2018/05/15 14:31:25.284177 add network policy: exit status 2: iptables v1.6.1: invalid port/service `rest' specified
Try `iptables -h' or 'iptables --help' for more information.

[mikebryant]

I'm adding the security label here, having a policy stop npc in the entire cluster and then leave things fail-open is bad.

(We have namespaces which are expected to be in default deny where this wasn't applied due to the npc crashing)

[bboreham]

It's supposed to fail closed, i.e. if there isn't a rule allowing the connection then it will be blocked. Do you know the sequence of events under which it fails open?

[brb]

Probably -A WEAVE-NPC -m set ! --match-set weave-local-pods dst -j ACCEPT installed by crashing weave-npc allows the traffic. The weave-local-pods ipset is maintained by weave-npc, so in your case it might stay empty.

[rowan-simms]

Yes, sorry, this should be documented.

Named ports make the implementation hugely more complicated, as they can map to different numerical values on different pods.

The documentation for the port field of NetworkPolicyPort (https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.11/#networkpolicyport-v1beta1-extensions) says

This can either be a numerical or named port on a pod.

And even if it were only accepting numerical ports, ideally it should be rejected at the point of running kubectl apply so proper validation can be done, and not silently pass at the apply stage and then crash the whole networkpolicy controller.

[bboreham]

ideally it should be rejected at the point of running kubectl apply

Sadly that is extremely asynchronous from the Weave Net NPC code.