Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v1.10 backports 2022-09-28 #21469

Merged
merged 9 commits into from
Oct 4, 2022
Merged

Conversation

gandro
Copy link
Member

@gandro gandro commented Sep 28, 2022

Once this PR is merged, you can update the PR labels via:

$ for pr in 20979 21239 21365 21348 21254 21402 21429 21370 21449; do contrib/backporting/set-labels.py $pr done 1.10; done

dctrwatson and others added 9 commits September 28, 2022 11:14
[ upstream commit 751e2df ]

Signed-off-by: John Watson <johnw@planetscale.com>
Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
[ upstream commit ac377a4 ]

Let's use Grafana instead to monitor CI stability.

Ref: cilium#21238

Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
[ upstream commit 619366f ]

The DNS proxy is only allocated if L7 proxy is enabled. The cleanup code did
not check if it allocated causing a nil deref on shutdown.

controlplane test was modified to only set the mock DNS proxy when L7 proxy
is enabled to reflect behavior of bootstrapFDQN and catch similar issue
in the future.

Fixes: cilium#21264
Fixes: 266f705 ("dnsproxy: add cleanup")

Signed-off-by: Jussi Maki <jussi@isovalent.com>
Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
[ upstream commit 0cb4b97 ]

There is more and more usage of envoy proxy in cilium, so it's better to
have utility to dump its config for troubleshooting later.

```
root@minikube:/home/cilium# tar -xf /tmp/cilium-bugtool-20220918-132428.955+0000-UTC-2901326153.tar
root@minikube:/home/cilium# ls -lrt cilium-bugtool-20220918-132428.955+0000-UTC-2901326153/cmd/envoy-config.json
-rw-r--r-- 1 root root 28645 Sep 18 13:24 cilium-bugtool-20220918-132428.955+0000-UTC-2901326153/cmd/envoy-config.json
```

Signed-off-by: Tam Mach <tam.mach@cilium.io>
Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
[ upstream commit 563787e ]

Signed-off-by: Tom Hadlaw <tom.hadlaw@isovalent.com>
Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
[ upstream commit 3e0d679 ]

It's `log` in the [config.go](https://github.com/cilium/cilium/blob/master/pkg/option/config.go#L988).

Also fix the delimeter (the actual cli acceps the space but not a comma).

Signed-off-by: Vladimir Pouzanov <farcaller@gmail.com>
Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
[ upstream commit 41d1499 ]

Explicitly log if no policy maps are found
to improve debuggability.

Signed-off-by: Aditi Ghag <aditi@cilium.io>
Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
[ upstream commit 592ff13 ]

**TL;DR.** We only need one of an XFRM IN policy or an XFRM IN state to
match each packet. This commit removes one superfluous XFRM IN policy
and enables some additional simplification as a result.

What XFRM IN policy we install currently depends of whether we are
running in tunneling mode and with or without endpoint routes:

In tunneling mode:
    XFRM IN policy matching on mark 0x200/0xf00 (for proxy)
    XFRM IN policy matching on mark 0xd00/0xf00 (for decrypt.)

In native routing mode with endpoint routes:
    XFRM IN policy matching on mark 0x200/0xf00 (for proxy)

In all cases, we also have:
    XFRM IN state matching on mark 0xd00/0xf00 (for decrypt.)

The two policies in tunneling mode were introduced by a9f18f3
("datapath/linux/ipsec: Insert additional In rule when tunneling"). The
additional case for endpoint routes was introduced by 3ffe49e ("ipsec:
Fix L7 with endpoint routes"). Now, I got to wonder how 3ffe49e even
worked as it was missing an XFRM IN policy for 0xd00 which a9f18f3
suggested was necessary.

After some local testing, it turns out that the two XFRM IN policies for
tunneling mode are not required. All we need is to have either (1) an
XFRM IN policy or (2) an XFRM IN state matching the packets. The XFRM
state is needed if we want to decrypt packets; the XFRM policy is needed
to not drop packets that don't match an XFRM state.

Given we always have an XFRM IN state for packets coming with the
decryption mark, we don't need an XFRM IN policy for that. We only need
an XFRM IN policy for packets coming with the proxy mark because we
don't have a state for those, rightly so as we don't want to decrypt
them.

This commit therefore removes the XFRM IN policy for decryption. It also
removes any dependency on particular options: we will always install the
XFRM IN policy for the proxy. It doesn't hurt to have that policy even
if not required (e.g., in native routing mode without endpoint routes).

**How was this tested?**

This change was tested with our Jenkins IPsec tests (including the
quarantined one for VXLAN), as well as with GKE and EKS clusters of 3
nodes. In all cases, the connectivity tests were executed and L7
policies were thus covered.

Signed-off-by: Paul Chaignon <paul@cilium.io>
Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
[ upstream commit 2e36f12 ]

In case the call to endpoint.NewEndpointFromChangeModel in
(*Daemon).createEndpoint fails (e.g. due to invalid data in the
request), the returned *endpoint.Endpoint is nil while err is non-nil.
However, invalidDataError is called with ep=nil, leading to a nil
pointer dereference in ep.SetState.

Fixes: 0d6b7ad ("endpoint: Add Invalid state")

Signed-off-by: Tobias Klauser <tobias@cilium.io>
Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
@gandro gandro requested review from a team as code owners September 28, 2022 09:26
@gandro gandro added backport/1.10 kind/backports This PR provides functionality previously merged into master. labels Sep 28, 2022
Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still looks good on my commit, thanks 💯

Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My PR looks good. Thanks!

Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, my change looks good.

Copy link
Contributor

@michi-covalent michi-covalent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️ a8ecc2e

Copy link
Member

@aditighag aditighag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Backport of my changes look good. Thanks!
(unrelated: Liked how some of the commits with conflicts are highlighted with ⚠️).

Copy link
Contributor

@joamaki joamaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, my commit is good

Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving on behalf of @cilium/github-sec

@gandro
Copy link
Member Author

gandro commented Sep 29, 2022

/test-backport-1.10

Job 'Cilium-PR-K8s-GKE' failed:

Click to show.

Test Name

K8sDatapathConfig DirectRouting Check connectivity with direct routing and endpointRoutes

Failure Output

FAIL: Kubernetes DNS did not become ready in time

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-GKE so I can create one.

@gandro
Copy link
Member Author

gandro commented Oct 3, 2022

Marking this ready-to-merge. The failing GKE workflows are infrastructure related and both pipelines have been disabled on master for that reason.

@gandro gandro added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 3, 2022
@ti-mo ti-mo merged commit 7e8da0a into cilium:v1.10 Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/backports This PR provides functionality previously merged into master. ready-to-merge This PR has passed all tests and received consensus from code owners to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.