test/K8sServices: Fix externalTrafficPolicy=Local with kube-proxy (on GKE) by gandro · Pull Request #12709 · cilium/cilium

gandro · 2020-07-29T15:51:18Z

This commit fixes the test harness for externalTrafficPolicy=Local in
the case where we are accessing a node IP without a local backend from a
node with host reachable services enabled.

This is a known incompatibility between our kube-proxy replacement and
upstream kube-proxy. The existing test harness assumed that we only need
to handle this case if we are running without kube-proxy. This
assumption however is wrong, as we are running these tests in hybrid
mode, where we are running with both kube-proxy and Cilium's kube-proxy
replacement.

This has not been hit in our existing test suites up until recently,
because we did not have a test setup with both kube-proxy and the
kube-proxy replacement enabled at the same time. As GKE has been
upgraded to Linux 4.19, it is now using the described hybrid setup which has
caused the tests to break.

The test matrix as of writing now looks as follows:

Test Suite	kube-proxy	kube-proxy replacement
K8s-1.18-kernel-4.9	Yes	No (Kernel 4.9.x)
K8s-1.17-Kernel-4.19	No	Yes (Kernel 4.19.57)
K8s-1.12-Kernel-netnext	No	Yes (Kernel 5.8.0-rc1+)
Cilium-PR-Ginkgo-Tests-k8s	Yes	No (Kernel 4.9.x)
Cilium-PR-K8s-GKE	Yes	Yes (Kernel 4.19.112+)

Fixes: 67f85e3 ("tests: enable additional externalTrafficPolicy=Local tests")

gandro · 2020-07-29T15:52:46Z

test-me-please

coveralls · 2020-07-29T16:30:49Z

Coverage decreased (-0.009%) to 36.634% when pulling bee0976 on pr/gandro/ci-fix-externaltrafficpolicy-local-tests-on-gke into ea53587 on master.

nebril · 2020-07-29T17:38:20Z

gke seems to be failing due to unrelated cause, will restart

nebril · 2020-07-29T17:38:28Z

test-gke

nebril · 2020-07-30T09:00:31Z

test-gke

gandro · 2020-07-30T09:57:10Z

test-gke

brb · 2020-07-30T09:58:04Z

-			// kube-proxy free case since we'll hit the wildcard rule in bpf_sock
+			// In-cluster connectivity from k8s2 to k8s1 IP will still work with
+			// HostRechableServices (regardless of if we are running with or
+			// without kube-proxy) since we'll hit the wildcard rule in bpf_sock


Could you extend the comment saying why it won't work on kube-proxy?

Added the following comment:

// This is a known incompatibility with kube-proxy: // kube-proxy 1.15+ will only load-balance requests from k8s1 to k8s1, // but not from k8s2 to k8s1. In the k8s2 to k8s1 case, kube-proxy // would send traffic to k8s1, where it would be subsequently // dropped, because k8s1 has no service backend. // However, if HostReachableServices is enabled, then Cilium does // the service translation already on the client node, bypassing // kube-proxy completely.

brb

Thanks!

gandro · 2020-07-30T11:11:12Z

test-me-please

nebril · 2020-07-30T13:40:01Z

test-gke

nebril · 2020-07-30T16:20:12Z

test-me-please

`--validate` helm flag introduced in 82cc7c3 caused ci to fail in gke where we enable node init daemonset. It needs to be cleared before cilium installation. Signed-off-by: Maciej Kwiek <maciej@isovalent.com>

Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>

gandro · 2020-07-30T19:33:34Z

Rebased on #12725 - as GKE can not pass otherwise.

Naturally blocked on #12725 getting merged first.

gandro · 2020-07-30T19:34:59Z

test-me-please

nebril · 2020-07-30T20:44:59Z

#12725 is merged, please rebase

ninja edit - didn't refresh the pr page

nebril · 2020-07-30T22:00:07Z

test-gke

gandro · 2020-07-31T07:09:27Z

Looks like GetCiliumPodOnNode returned an empty string on GKE?

Edit: That function probably doesn't work on GKE anways. Switched to GetCiliumPodOnNodeWithLabel

This commit fixes the test harness for `externalTrafficPolicy=Local` in the case where we are accessing a node IP without a local backend from a node with host reachable services enabled. This is a known incompatibility between our kube-proxy replacement and upstream kube-proxy. The existing test harness assumed that we only need to handle this case if we are running without kube-proxy. This assumption however is wrong, as we are running these tests in hybrid mode, where we are running with both kube-proxy and Cilium's kube-proxy replacement. This has not been hit in our existing test suites up until recently, because we did not have a test setup with both kube-proxy and the kube-proxy replacement enabled at the same time. As GKE has been upgraded to Linux 4.19, it is now using the described setup which has caused the tests to break. The test matrix as of writing now looks as follows: | Test Suite | kube-proxy | kube-proxy replacement | | -------------------------- | ---------- | ----------------------- | | K8s-1.18-kernel-4.9 | Yes | No (Kernel 4.9.x) | | K8s-1.17-Kernel-4.19 | No | Yes (Kernel 4.19.57) | | K8s-1.12-Kernel-netnext | No | Yes (Kernel 5.8.0-rc1+) | | Cilium-PR-Ginkgo-Tests-k8s | Yes | No (Kernel 4.9.x) | | Cilium-PR-K8s-GKE | Yes | Yes (Kernel 4.19.112+) | Fixes: 67f85e3 ("tests: enable additional externalTrafficPolicy=Local tests") Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>

gandro · 2020-07-31T07:26:41Z

test-me-please

gandro · 2020-07-31T09:12:16Z

GKE hit Suite-k8s-1.15.K8sChaosTest Connectivity demo application Endpoint can still connect while Cilium is not running which seems unrelated.

gandro · 2020-07-31T09:12:24Z

test-gke

gandro · 2020-07-31T10:59:49Z

Again hit Suite-k8s-1.15.K8sChaosTest Connectivity demo application Endpoint can still connect while Cilium is not running (on GKE)

gandro · 2020-07-31T11:03:22Z

I'm merging this. It seems like it did fix the externalTrafficPolicy=Local issue on GKE (it passed two times) as well as all other CI. The GKE failure in the Chaos test is unrelated.

gandro added release-note/ci This PR makes changes to the CI. needs-backport/1.8 labels Jul 29, 2020

gandro requested review from brb and nebril July 29, 2020 15:51

gandro requested review from a team as code owners July 29, 2020 15:51

nebril approved these changes Jul 29, 2020

View reviewed changes

brb requested changes Jul 30, 2020

View reviewed changes

gandro force-pushed the pr/gandro/ci-fix-externaltrafficpolicy-local-tests-on-gke branch from f9dc804 to 6c7fed3 Compare July 30, 2020 11:06

brb approved these changes Jul 30, 2020

View reviewed changes

maintainer-s-little-helper Bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 30, 2020

gandro changed the title ~~test/K8sServices: Fix externalTrafficPolicy=Local with kube-proxy~~ test/K8sServices: Fix externalTrafficPolicy=Local with kube-proxy (on GKE) Jul 30, 2020

christarazi approved these changes Jul 30, 2020

View reviewed changes

ci: Delete cilium-node-init ds before cilium install

5752e65

`--validate` helm flag introduced in 82cc7c3 caused ci to fail in gke where we enable node init daemonset. It needs to be cleared before cilium installation. Signed-off-by: Maciej Kwiek <maciej@isovalent.com>

gandro removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 30, 2020

docs: Update versions used in GKE pipeline

01a2ab2

Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>

gandro force-pushed the pr/gandro/ci-fix-externaltrafficpolicy-local-tests-on-gke branch from 6c7fed3 to 9575367 Compare July 30, 2020 19:32

gandro changed the base branch from master to pr/delete-node-init-ci July 30, 2020 19:32

nebril mentioned this pull request Jul 30, 2020

ci: Delete cilium-node-init ds before cilium install #12725

Merged

Base automatically changed from pr/delete-node-init-ci to master July 30, 2020 20:44

maintainer-s-little-helper Bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 30, 2020

gandro removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 31, 2020

gandro force-pushed the pr/gandro/ci-fix-externaltrafficpolicy-local-tests-on-gke branch from 9575367 to bee0976 Compare July 31, 2020 07:25

maintainer-s-little-helper Bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 31, 2020

gandro removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 31, 2020

maintainer-s-little-helper Bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 31, 2020

gandro removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 31, 2020

gandro merged commit ac9732a into master Jul 31, 2020

gandro deleted the pr/gandro/ci-fix-externaltrafficpolicy-local-tests-on-gke branch July 31, 2020 11:03

maintainer-s-little-helper Bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 31, 2020

qmonnet mentioned this pull request Aug 3, 2020

v1.8 backports 2020-08-03 #12745

Merged

qmonnet added backport-pending/1.8 and removed needs-backport/1.8 labels Aug 3, 2020

tklauser added backport-done/1.8 and removed backport-pending/1.8 labels Aug 3, 2020

Conversation

gandro commented Jul 29, 2020

Uh oh!

gandro commented Jul 29, 2020

Uh oh!

coveralls commented Jul 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nebril commented Jul 29, 2020

Uh oh!

nebril commented Jul 29, 2020

Uh oh!

nebril commented Jul 30, 2020

Uh oh!

gandro commented Jul 30, 2020

Uh oh!

brb Jul 30, 2020

Choose a reason for hiding this comment

Uh oh!

gandro Jul 30, 2020

Choose a reason for hiding this comment

Uh oh!

brb left a comment

Choose a reason for hiding this comment

Uh oh!

gandro commented Jul 30, 2020

Uh oh!

nebril commented Jul 30, 2020

Uh oh!

nebril commented Jul 30, 2020

Uh oh!

gandro commented Jul 30, 2020

Uh oh!

gandro commented Jul 30, 2020

Uh oh!

nebril commented Jul 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nebril commented Jul 30, 2020

Uh oh!

gandro commented Jul 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gandro commented Jul 31, 2020

Uh oh!

gandro commented Jul 31, 2020

Uh oh!

gandro commented Jul 31, 2020

Uh oh!

gandro commented Jul 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gandro commented Jul 31, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

coveralls commented Jul 29, 2020 •

edited

Loading

nebril commented Jul 30, 2020 •

edited

Loading

gandro commented Jul 31, 2020 •

edited

Loading

gandro commented Jul 31, 2020 •

edited

Loading