Fix kube-apiserver policy matching feature with tunneling enabled #18527

christarazi · 2022-01-18T20:32:39Z

bpf: Fix kube-apiserver policy drop in tunneling
test/K8sT: Add case for duplicate kube-apiserver policy
docs: Remove kube-apiserver policy feature limitation

See commit msgs.

Fixes: #18049
Updates: #17962
Updates: #17829

christarazi · 2022-01-18T20:35:23Z

bpf/bpf_overlay.c

+
+		/* Maybe overwrite the REMOTE_NODE_ID with
+		 * KUBE_APISERVER_NODE_ID to support upgrade. After v1.12,
+		 * this should be removed. */
+		if (*identity == REMOTE_NODE_ID) {
+			struct remote_endpoint_info *info;
+
+			/* Look up the ipcache for the src IP, it will give us
+			 * the real identity of that IP */
+			info = ipcache_lookup6(&IPCACHE_MAP,
+					       (union v6addr *) &ip6->saddr,
+					       V6_CACHE_KEY_LEN);
+			if (info)
+				*identity = info->sec_label;
+		}


It would be good to get a look from the @cilium/sig-encryption team on this change.

joestringer

Just one nit in the commit message.

bpf/bpf_overlay.c

When running in tunneling mode with a kube-apiserver in the cluster, the node with the kube-apiserver will transmit packets towards remote pods with identity 6 (remote-node). The recipient node will receive the packet and trust the identity as "remote-node" and not "kube-apiserver". If the receiving pod has a policy to only allow "from kube-apiserver" then this policy would not work, and thus drop the packet. To fix this, we switch the source identity in bpf_overlay from "remote-node" to the identity of the destination IP via the ipcache, which we expect to be the "kube-apiserver" identity. If the ipcache entry doesn't exist or the identity in the ipcache is "remote-node" already, then this is a no-op (besides the extra ipcache lookup). Going forward, there are two future changes that are needed in order: 1) A subsequent change to make any packet associated with the kube-apiserver IP have ID 7 (rather than ID 6; without needing the logic added by this commit). 2) Once v1.12.y is released, another subsequent change to remove this specific code added in this commit because we can expect all of this traffic to be transmitted with "kube-apiserver" identity (7), **after** (1) is implemented. For a hitless upgrade, we would only support a path from Cilium v1.11.x (this commit would need to be backported to v1.11.x) to v1.12.y. This is because only v1.11.x or later can successfully receive this traffic and handle policy correctly, as guaranteed by the above. Co-authored-by: Joe Stringer <joe@cilium.io> Signed-off-by: Chris Tarazi <chris@isovalent.com>

christarazi · 2022-01-18T21:56:36Z

/test

Job 'Cilium-PR-K8s-1.21-kernel-5.4' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sPolicyTestExtended Validate toEntities KubeAPIServer Still allows connection to KubeAPIServer with a duplicate policy

Failure Output

FAIL: policy /home/jenkins/workspace/Cilium-PR-K8s-1.21-kernel-5.4/src/github.com/cilium/cilium/test/k8sT/manifests/cnp-to-entities-kube-apiserver.yaml cannot be deleted in "default" namespace

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-1.21-kernel-5.4 so I can create a new GitHub issue to track it.

Job 'Cilium-PR-K8s-GKE' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sPolicyTestExtended Validate toEntities KubeAPIServer Still allows connection to KubeAPIServer with a duplicate policy

Failure Output

FAIL: policy /home/jenkins/workspace/Cilium-PR-K8s-GKE/src/github.com/cilium/cilium/test/k8sT/manifests/cnp-to-entities-kube-apiserver.yaml cannot be deleted in "default" namespace

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-GKE so I can create a new GitHub issue to track it.

joestringer

Oh, one small nit: Please update Documentation/policy/language.rst to remove the text about the known issue. (Won't need re-running the full CI, can add a small patch at the end before merging or follow up with the change in another PR after this)

Additional sanity check coverage that the kube-apiserver policy matching feature still works with a duplicate policy added and the previous removed. Updates: cilium#17829 Signed-off-by: Chris Tarazi <chris@isovalent.com>

Now that the limitation has been fixed, remove it from the docs. Signed-off-by: Chris Tarazi <chris@isovalent.com>

christarazi · 2022-01-19T22:39:59Z

I've added an extra commit to address Joe's comment above. The CI failures were legitimate (related to test setup). They should be fixed now.

christarazi · 2022-01-19T22:48:42Z

/test

Job 'Cilium-PR-K8s-1.22-kernel-4.19' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sServicesTest Checks graceful termination of service endpoints Checks client terminates gracefully on service endpoint deletion

Failure Output

FAIL: Timed out after 60.001s.

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-1.22-kernel-4.19 so I can create a new GitHub issue to track it.

Job 'Cilium-PR-K8s-1.23-kernel-net-next' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sPolicyTest Multi-node policy test validates fromEntities policies Validates fromEntities all policy

Failure Output

FAIL: Can not connect to service "10.0.0.31" from outside cluster (1/1)

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-1.23-kernel-net-next so I can create a new GitHub issue to track it.

christarazi · 2022-01-20T06:57:20Z

net-next hit CI: net-next K8sPolicyTest Multi-node policy test validates fromEntities policies Validates fromEntities all policy #18520
4.19 hit CI: K8sServicesTest Checks graceful termination of service endpoints Checks client terminates gracefully on service endpoint deletion #18318
runtime failed to provision

christarazi · 2022-01-24T18:33:37Z

@joamaki ping :)

christarazi requested a review from a team January 18, 2022 20:32

christarazi requested a review from a team as a code owner January 18, 2022 20:32

christarazi requested a review from a team January 18, 2022 20:32

christarazi added release-note/bug This PR fixes an issue in a previous release of Cilium. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. kind/bug This is a bug in the Cilium logic. needs-backport/1.11 labels Jan 18, 2022

maintainer-s-little-helper bot added dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. and removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Jan 18, 2022

christarazi requested review from joamaki, nbusseneau and twpayne January 18, 2022 20:32

maintainer-s-little-helper bot assigned joamaki Jan 18, 2022

maintainer-s-little-helper bot added this to Needs backport from master in 1.11.1 Jan 18, 2022

maintainer-s-little-helper bot assigned nbusseneau and twpayne Jan 18, 2022

christarazi requested a review from joestringer January 18, 2022 20:32

maintainer-s-little-helper bot assigned joestringer Jan 18, 2022

christarazi force-pushed the pr/christarazi/kube-apiserver-tunneling branch from a7cdab0 to dcda34e Compare January 18, 2022 20:33

christarazi commented Jan 18, 2022

View reviewed changes

This was referenced Jan 18, 2022

Add e2e coverage of apiserver policy functionality #17829

Open

kube-apiserver policy matching followups #17962

Open

joestringer approved these changes Jan 18, 2022

View reviewed changes

bpf/bpf_overlay.c Show resolved Hide resolved

maintainer-s-little-helper bot unassigned joestringer Jan 18, 2022

christarazi force-pushed the pr/christarazi/kube-apiserver-tunneling branch 2 times, most recently from f63ec15 to 1fb0de3 Compare January 18, 2022 21:04

christarazi force-pushed the pr/christarazi/kube-apiserver-tunneling branch from 1fb0de3 to a9316a6 Compare January 18, 2022 21:11

joestringer added this to Needs backport from master in 1.11.2 Jan 18, 2022

joestringer removed this from Needs backport from master in 1.11.1 Jan 18, 2022

twpayne approved these changes Jan 19, 2022

View reviewed changes

maintainer-s-little-helper bot unassigned twpayne Jan 19, 2022

nbusseneau approved these changes Jan 19, 2022

View reviewed changes

maintainer-s-little-helper bot unassigned nbusseneau Jan 19, 2022

joestringer reviewed Jan 19, 2022

View reviewed changes

christarazi added 2 commits January 19, 2022 14:38

test/K8sT: Add case for duplicate kube-apiserver policy

1ea1bf1

Additional sanity check coverage that the kube-apiserver policy matching feature still works with a duplicate policy added and the previous removed. Updates: cilium#17829 Signed-off-by: Chris Tarazi <chris@isovalent.com>

docs: Remove kube-apiserver policy feature limitation

d1d3203

Now that the limitation has been fixed, remove it from the docs. Signed-off-by: Chris Tarazi <chris@isovalent.com>

christarazi force-pushed the pr/christarazi/kube-apiserver-tunneling branch from a9316a6 to d1d3203 Compare January 19, 2022 22:38

christarazi requested a review from a team as a code owner January 19, 2022 22:38

joamaki approved these changes Jan 24, 2022

View reviewed changes

maintainer-s-little-helper bot unassigned joamaki Jan 24, 2022

christarazi added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jan 26, 2022

kkourt merged commit bd7ee73 into cilium:master Jan 26, 2022

christarazi deleted the pr/christarazi/kube-apiserver-tunneling branch January 26, 2022 21:17

glibsm mentioned this pull request Jan 31, 2022

v1.11 backports 2022-01-31 #18669

Merged

glibsm added backport-pending/1.11 and removed needs-backport/1.11 labels Jan 31, 2022

maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.10 in 1.11.2 Jan 31, 2022

joamaki added backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. and removed backport-pending/1.11 labels Feb 8, 2022

maintainer-s-little-helper bot moved this from Backport pending to v1.10 to Backport done to v1.11 in 1.11.2 Feb 8, 2022

joestringer mentioned this pull request Feb 23, 2022

Prepare for release v1.11.2 #18922

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix kube-apiserver policy matching feature with tunneling enabled #18527

Fix kube-apiserver policy matching feature with tunneling enabled #18527

christarazi commented Jan 18, 2022 •

edited

Loading

christarazi Jan 18, 2022

joestringer left a comment •

edited

Loading

christarazi commented Jan 18, 2022 •

edited by maintainer-s-little-helper bot

Loading

Test Name

Failure Output

Test Name

Failure Output

joestringer left a comment

christarazi commented Jan 19, 2022 •

edited

Loading

christarazi commented Jan 19, 2022 •

edited by maintainer-s-little-helper bot

Loading

Test Name

Failure Output

Test Name

Failure Output

christarazi commented Jan 20, 2022

christarazi commented Jan 24, 2022

Fix kube-apiserver policy matching feature with tunneling enabled #18527

Fix kube-apiserver policy matching feature with tunneling enabled #18527

Conversation

christarazi commented Jan 18, 2022 • edited Loading

christarazi Jan 18, 2022

Choose a reason for hiding this comment

joestringer left a comment • edited Loading

Choose a reason for hiding this comment

christarazi commented Jan 18, 2022 • edited by maintainer-s-little-helper bot Loading

Test Name

Failure Output

Test Name

Failure Output

joestringer left a comment

Choose a reason for hiding this comment

christarazi commented Jan 19, 2022 • edited Loading

christarazi commented Jan 19, 2022 • edited by maintainer-s-little-helper bot Loading

Test Name

Failure Output

Test Name

Failure Output

christarazi commented Jan 20, 2022

christarazi commented Jan 24, 2022

christarazi commented Jan 18, 2022 •

edited

Loading

joestringer left a comment •

edited

Loading

christarazi commented Jan 18, 2022 •

edited by maintainer-s-little-helper bot

Loading

christarazi commented Jan 19, 2022 •

edited

Loading

christarazi commented Jan 19, 2022 •

edited by maintainer-s-little-helper bot

Loading