Add policy support for named ports #11092

jrajahalme · 2020-04-22T05:05:48Z

Add support for container port names in policy ports. With this change, in addition to using a L4 port number (like "80") in the network policy (K8s Network Policy or Cilium Network Policy), the port number can be expressed as a name specified in any one of the POD specs in the cluster. If the port name is not specified in any container spec, the traffic specified in the network policy rule can't be allowed, but will be allowed as soon as a container with a matching port name is added.

Port names are stored from a POD spec on the node in which the POD is deployed, from where then name/port mappings are distributed to other nodes via CEP CRDs.

Support for named k8s container ports is added to both K8s Network Policies and Cilium Network Policies.

jrajahalme · 2020-04-22T05:06:11Z

test-me-please

coveralls · 2020-04-22T05:39:36Z

Coverage decreased (-0.05%) to 44.462% when pulling b149e9b170f7801c0dec325a3fae7789fb0f1dc4 on pr/jrajahalme/named-ports into 0b203d4 on master.

jrajahalme · 2020-04-22T19:40:43Z

test-me-please

aanm

I didn't finish my review but I'm leaving some comments for now

aanm · 2020-04-22T21:21:23Z

pkg/k8s/apis/cilium.io/v2/client/register.go

+				// uint16 or port name string regex
 				Pattern: `^(6553[0-5]|655[0-2][0-9]|65[0-4][0-9]{2}|6[0-4][0-9]{3}|` +
-					`[1-5][0-9]{4}|[0-9]{1,4})$`,
+					`[1-5][0-9]{4}|[0-9]{1,4}|` +
+					`([a-zA-Z0-9]-?)*[a-zA-Z](-?[a-zA-Z0-9])*)$`,


This requires a bump in the version set in CustomResourceDefinitionSchemaVersion

Both places? (pkg/k8s/apis/cilium.io/v2/client/register.go and pkg/k8s/apis/cilium.io/v2/register.go)?

Actually, it looks like one in pkg/k8s/apis/cilium.io/v2/register.go is not used for anything anymore.

aanm · 2020-04-22T21:25:17Z

pkg/endpoint/endpoint.go

+			continue // skip unnamed ports
+		}
+		if !api.IsNamedPort(cp.Name) {
+			log.WithField(logfields.PortName, cp.Name).Warning("ContainerPort: Invalid port name, not using as a named port")


How can the user visualize this error better? Thinking on this better, this should never happened has the validation is done in k8s side.

Right, this is likely not needed, but still wanted to have logging if this happens.

aanm · 2020-04-22T21:25:37Z

pkg/endpoint/endpoint.go

+			log.WithField(logfields.PortName, cp.Name).Warning("ContainerPort: Invalid port name, not using as a named port")
+			continue
+		}
+		name := strings.ToLower(cp.Name)


I assume named ports are case insensitive?

Yes. They are defined as IANA_SVC_NAME, which is defined as case insensitive in
https://tools.ietf.org/rfc/rfc6335.txt

pkg/policy/l4.go

aanm · 2020-04-22T21:47:25Z

pkg/u8proto/u8proto.go

@@ -41,6 +41,7 @@ var protoNames = map[U8proto]string{
 }

 var ProtoIDs = map[string]U8proto{
+	"":       0,


Why this change?

Not sure any more... To map a missing proto name to ANY, but then in K8s ContainerPort's Protocol defaults to "TCP". I'll check if this is still needed & verify the default behavior.

It seems my assumption was that ContainerPort could have ANY protocol, but that is not the case, even though this PR would allow it. Our policy rules allow ANY protocol, which we internally treat as both UDP and TCP and generate two rules (one for each). One of them gets pruned away when the port name is resolved.

It might be better if I make this implementation less permissive and only allow one protocol, missing defaulting to TCP?

Changed empty proto string to default to TCP, but kept the internal processing that allows "ANY".

aanm · 2020-04-22T21:49:31Z

pkg/ipcache/ipcache.go

+	// NamedPorts is the set of named ports for the pod
+	NamedPorts policy.NamedPortsMap


I would be extremely careful with this. Adding this field removes the ability to compare 2 variables created from this structure type with the == operator.

It has an Equal method already, I did expand it. Could also consider making NamedPorts a separate member/argument, although port names are semantically "k8s metadata".

pkg/ipcache/ipcache.go

jrajahalme · 2020-04-22T22:00:15Z

test-me-please

api/v1/openapi.yaml

pkg/endpoint/bpf.go

tgraf · 2020-04-22T22:03:01Z

pkg/endpoint/endpoint.go

+			log.WithField(logfields.Port, cp.ContainerPort).Warning("ContainerPort: Port number out of 16-bit range")
+			continue
+		}
+		k8sPorts[name] = policy.NamedPort{


Should we warn on duplicates?

This is for the Endpoint, so these come from the POD spec, and I'd expect K8s to not allow duplicate names in the POD spec?

Not sure. I would warn so we at least know.

pkg/policy/l4.go

pkg/ipcache/ipcache.go

jrajahalme · 2020-04-23T01:56:54Z

test-me-please

jrajahalme · 2020-04-23T04:23:45Z

test-me-please

jrajahalme · 2020-04-23T04:25:09Z

Fixed unit test breakage, Ginkgo passed except for one unrelated test that depended on external connectivity to 1.1.1.1, most likely a flake.

jrajahalme · 2020-04-27T21:44:51Z

test-gke

jrajahalme · 2020-04-28T17:23:04Z

@aanm Rebase is needed so I'm setting up for one more round for clean-ups. Now would be a good time for comments, if you have any now that I have addressed your requested changes :-)

daemon/cmd/ipcache.go

cilium/cmd/policy_trace.go

test/kubernetes-test.sh

test/k8sT/Policies.go

pkg/policy/l4.go

pkg/endpoint/restore.go

pkg/envoy/server.go

pkg/k8s/types/types.go

pkg/policy/api/l4.go

pkg/policy/l4.go

jrajahalme · 2020-05-01T09:49:28Z

Rebased, testing again.

jrajahalme · 2020-05-01T09:49:47Z

never-tell-me-the-odds

jrajahalme · 2020-05-01T13:51:58Z

VirtualBox fail with kernel-4.19 CI, retesting

jrajahalme · 2020-05-01T13:52:04Z

test-with-kernel

Signed-off-by: André Martins <andre@cilium.io>

Init Containers should also be part of our Pod spec since the network namespace is shared for those containers as well otherwise we won't be able to enforce hostport or any other features in init containers Signed-off-by: André Martins <andre@cilium.io>

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

Store the set of named ContainerPorts in Endpoint. The ports are immutable after Endpoint has been created. Signed-off-by: Andre Martins <andre@cilium.io> Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

When using named ports that map to an already existing port number in the policy we need to fold the rules together as otherwise Envoy will NACK them due to the duplicate port number. We probably should also detect mismatching L7 parsers here? Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

Add named ports to endpoint status. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

jrajahalme · 2020-05-01T22:09:47Z

never-tell-me-the-odds

jrajahalme requested review from a team as code owners April 22, 2020 05:05

jrajahalme requested review from a team April 22, 2020 05:05

jrajahalme requested a review from a team as a code owner April 22, 2020 05:05

maintainer-s-little-helper bot added this to In progress in 1.8.0 Apr 22, 2020

aanm added the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Apr 22, 2020

jrajahalme force-pushed the pr/jrajahalme/named-ports branch from 2864669 to 0f45c77 Compare April 22, 2020 19:40

aanm requested changes Apr 22, 2020

View reviewed changes

jrajahalme force-pushed the pr/jrajahalme/named-ports branch from 0f45c77 to fb11564 Compare April 22, 2020 22:00

tgraf requested changes Apr 22, 2020

View reviewed changes

jrajahalme force-pushed the pr/jrajahalme/named-ports branch from fb11564 to d6857cf Compare April 23, 2020 01:55

jrajahalme force-pushed the pr/jrajahalme/named-ports branch from d6857cf to 62e9fde Compare April 23, 2020 04:23

jrajahalme requested review from tgraf and aanm April 23, 2020 06:27

tgraf approved these changes Apr 28, 2020

View reviewed changes

aanm requested changes Apr 29, 2020

View reviewed changes

jrajahalme force-pushed the pr/jrajahalme/named-ports branch from a7a26ad to b149e9b Compare May 1, 2020 09:49

jrajahalme requested a review from aanm May 1, 2020 09:51

aanm approved these changes May 1, 2020

View reviewed changes

aanm and others added 10 commits May 1, 2020 12:49

pkg/k8s: rename HostPorts to ContainerPorts

9722e32

Signed-off-by: André Martins <andre@cilium.io>

pkg/k8s: copy init containers

c42f549

Init Containers should also be part of our Pod spec since the network namespace is shared for those containers as well otherwise we won't be able to enforce hostport or any other features in init containers Signed-off-by: André Martins <andre@cilium.io>

policy: Pass L4Filter to AccumulateMapChanges()

6447d3f

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

policy: Pass named k8s ContainerPorts to Endpoint

9c2a85c

Store the set of named ContainerPorts in Endpoint. The ports are immutable after Endpoint has been created. Signed-off-by: Andre Martins <andre@cilium.io> Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

api: Add named ports

adc630f

Add named ports to endpoint status. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

k8s: Add named ports to CEP

00a4f9a

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

policy: Support named ports in policy trace

71bf2d8

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

ipcache: Store named ports in kv-store

d5f578a

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

CI: Use named port "http-80" in k8s policy tests.

ddd88e4

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

jrajahalme force-pushed the pr/jrajahalme/named-ports branch from b149e9b to ddd88e4 Compare May 1, 2020 22:09

aanm merged commit 981d53f into master May 2, 2020

1.8.0 automation moved this from In progress to Merged May 2, 2020

aanm deleted the pr/jrajahalme/named-ports branch May 2, 2020 12:42

borkmann mentioned this pull request May 4, 2020

hostport support for init containers #10714

Closed

brb mentioned this pull request May 7, 2020

Support for Kubernetes Named Ports #2942

Closed

2 tasks

jrajahalme mentioned this pull request May 13, 2020

iana: Hook up unit test #11516

Merged

jrajahalme mentioned this pull request Nov 7, 2023

Resolve named ports for DNS policies #29023

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add policy support for named ports #11092

Add policy support for named ports #11092

jrajahalme commented Apr 22, 2020

jrajahalme commented Apr 22, 2020

coveralls commented Apr 22, 2020 •

edited

jrajahalme commented Apr 22, 2020

aanm left a comment

aanm Apr 22, 2020

jrajahalme Apr 22, 2020

jrajahalme Apr 22, 2020

jrajahalme Apr 22, 2020

aanm Apr 22, 2020

jrajahalme Apr 22, 2020

aanm Apr 22, 2020

jrajahalme Apr 22, 2020

aanm Apr 22, 2020

jrajahalme Apr 22, 2020

jrajahalme Apr 22, 2020

jrajahalme Apr 22, 2020

aanm Apr 22, 2020

jrajahalme Apr 22, 2020

jrajahalme commented Apr 22, 2020

tgraf Apr 22, 2020

jrajahalme Apr 22, 2020

tgraf Apr 24, 2020

jrajahalme commented Apr 23, 2020

jrajahalme commented Apr 23, 2020

jrajahalme commented Apr 23, 2020

jrajahalme commented Apr 27, 2020

jrajahalme commented Apr 28, 2020

jrajahalme commented May 1, 2020

jrajahalme commented May 1, 2020

jrajahalme commented May 1, 2020

jrajahalme commented May 1, 2020

jrajahalme commented May 1, 2020

		// NamedPorts is the set of named ports for the pod
		NamedPorts policy.NamedPortsMap

Add policy support for named ports #11092

Add policy support for named ports #11092

Conversation

jrajahalme commented Apr 22, 2020

jrajahalme commented Apr 22, 2020

coveralls commented Apr 22, 2020 • edited

jrajahalme commented Apr 22, 2020

aanm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrajahalme commented Apr 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrajahalme commented Apr 23, 2020

jrajahalme commented Apr 23, 2020

jrajahalme commented Apr 23, 2020

jrajahalme commented Apr 27, 2020

jrajahalme commented Apr 28, 2020

jrajahalme commented May 1, 2020

jrajahalme commented May 1, 2020

jrajahalme commented May 1, 2020

jrajahalme commented May 1, 2020

jrajahalme commented May 1, 2020

coveralls commented Apr 22, 2020 •

edited