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.

Sign up for GitHub

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

Jump to bottom

v1.14 Backports 2023-10-10 #28494

Merged
merged 6 commits into from
Oct 12, 2023
Merged

v1.14 Backports 2023-10-10 #28494

merged 6 commits into from
Oct 12, 2023

Conversation

joamaki
Copy link
Contributor

@joamaki joamaki commented Oct 10, 2023 •
edited

@joamaki joamaki added kind/backports This PR provides functionality previously merged into master. backport/1.14 This PR represents a backport for Cilium 1.14.x of a PR that was merged to main. labels Oct 10, 2023
@joamaki
Copy link
Contributor Author

joamaki commented Oct 11, 2023

/test-backport-1.14

michaelasp and others added 6 commits October 11, 2023 11:27
@michaelasp @julianwiedmann @joamaki
datapath: fix NodePort to remote hostns backend with tunnel config
4af89b3 
[ upstream commit f2dcc86 ]

[ Backporter's notes: Needed to adjust test/controlplane to avoid
  device detection ("AreDevicesRequired" is much looser now) ]

When forwarding external service requests to a remote backend in
hostNetwork, the NodePort code in tail_nodeport_nat_egress_ipv4() doesn't
redirect into the tunnel (as it's effectively a host-to-host connection).

Therefore it uses IPV4_DIRECT_ROUTING as src address. So if we're not
populating IPV4_DIRECT_ROUTING, such forwarded requests end up being SNATed
with 0.0.0.0.

Fixes: #22557
Fixes: 5283ec9 ("datapath: Introduce DirectRoutingDeviceRequired helper")
Co-authored-by: Julian Wiedmann <jwi@isovalent.com>
Signed-off-by: Michael Aspinwall <maspinwall@google.com>
Signed-off-by: Jussi Maki <jussi@isovalent.com>
@joamaki
resource: Add test for repeated deletions
4030df6 
[ upstream commit 75927f7 ]

Resource[T] does not correctly handle the events:
  Upsert -> Delete with Done(not-nil) -> Upsert (recreate)
    -> Delete (retry)

The retried delete event carries the old initial version of the
object causing the recreated object to be incorrectly deleted.

Signed-off-by: Jussi Maki <jussi@isovalent.com>
Signed-off-by: Jussi Maki <jussi@isovalent.com>
@joamaki
resource: Fix double upserts on subscribe and retrying of delete events
174279a 
[ upstream commit a7f1eef ]

[ Backporter's notes: adapted the patch to work without
  7959c43 and
  9cdcddc. The adaptions
  should not pose difficulties in backporting them. ]

Fix double upserts that were caused by store being manipulated without
synchronization with the subscriber queues by processing the deltas
under the resource read-lock and doing the initial key listing for
new subscriber with a write-lock. This way we cannot accidentally
see a key in the store and process it just before the key is queued.

As shown by test case in previous commit, the delete events are retried
with an old incorrect version of the object causing a recreated object
to be deleted.

Fix the deletion retrying by always queueing upserts and deletes by key
and keeping the last known state of objects emitted to the subscriber.
Only emit a delete event if the subscriber has seen its creation and
only use a version of the object that the subscriber has observed.

Fixes: 4101e2c ("k8s: Add resource package")
Signed-off-by: Jussi Maki <jussi@isovalent.com>
@joamaki
resource: Improve documentation on Events()
9e9ebcf 
[ upstream commit 2d00e56 ]

The semantics around retrying and the Sync event are subtle. Spell out the properties
in a comment for Events().

Signed-off-by: Jussi Maki <jussi@isovalent.com>
Signed-off-by: Jussi Maki <jussi@isovalent.com>
@julianwiedmann @joamaki
bpf: overlay: fix missing DBG_DECAP for Inter-Cluster-SNAT
d5e7132 
[ upstream commit 9dcc5d1 ]

For Inter-Cluster-SNAT traffic we tail-call in handle_ipv4() before
even reaching the DBG_DECAP message.

Pull up the cilium_dbg() call a bit so that it also applies to
inter-cluster-SNAT traffic. While at it also clarify which parameters are
relevant for the path that handles decrypted packets.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
Signed-off-by: Jussi Maki <jussi@isovalent.com>
@aanm @joamaki
pkg/k8s: use a deep copy of CNP in UpdateStatus to avoid race condition
9db9a3c 
[ upstream commit 3de85a1 ]

We modified the UpdateStatus function to ensure that the CNP object is
deep-copied before passing it as an argument. This change was necessary
because the UpdateStatus function was modifying the CNP object, specifically
clearing the LastAppliedConfiguration key from the annotations map. By
deep-copying the CNP object, we ensure that the original object remains
unmodified which fixes the following race condition:

```
Write at 0x00c002a98510 by goroutine 119834:
  runtime.mapassign_faststr()
      /usr/local/go/src/runtime/map_faststr.go:203 +0x0
  github.com/cilium/cilium/pkg/k8s.(*CNPStatusUpdateContext).updateViaAPIServer.func1()
      ./pkg/k8s/cnp.go:215 +0x53
  runtime.deferreturn()
      /usr/local/go/src/runtime/panic.go:477 +0x30
  github.com/cilium/cilium/pkg/k8s.(*CNPStatusUpdateContext).updateStatus()
      ./pkg/k8s/cnp.go:78 +0x2c7
  github.com/cilium/cilium/pkg/k8s.(*CNPStatusUpdateContext).UpdateStatus()
      ./pkg/k8s/cnp.go:146 +0x786
  github.com/cilium/cilium/pkg/k8s/watchers.(*K8sWatcher).addCiliumNetworkPolicyV2.func1()
      ./pkg/k8s/watchers/cilium_network_policy.go:352 +0x86
  github.com/cilium/cilium/pkg/controller.(*controller).runController()
      ./pkg/controller/controller.go:251 +0x171
  github.com/cilium/cilium/pkg/controller.(*Manager).createControllerLocked.func1()
      ./pkg/controller/manager.go:111 +0xa4

Previous read at 0x00c002a98510 by goroutine 1205:
  runtime.mapiterinit()
      /usr/local/go/src/runtime/map.go:816 +0x0
  github.com/cilium/cilium/pkg/comparator.MapStringEqualsIgnoreKeys()
      ./pkg/comparator/comparator.go:82 +0xb1
  github.com/cilium/cilium/pkg/k8s/apis/cilium.io/v2.objectMetaDeepEqual()
      ./pkg/k8s/apis/cilium.io/v2/cnp_types.go:65 +0xb0
  github.com/cilium/cilium/pkg/k8s/apis/cilium.io/v2.(*CiliumNetworkPolicy).DeepEqual()
      ./pkg/k8s/apis/cilium.io/v2/cnp_types.go:54 +0x177
  github.com/cilium/cilium/pkg/k8s/types.(*SlimCNP).DeepEqual()
      ./pkg/k8s/types/zz_generated.deepequal.go:82 +0xbd
  github.com/cilium/cilium/pkg/k8s/watchers.(*K8sWatcher).onUpsert()
      ./pkg/k8s/watchers/cilium_network_policy.go:238 +0x170
  github.com/cilium/cilium/pkg/k8s/watchers.(*K8sWatcher).ciliumNetworkPoliciesInit.func1()
      ./pkg/k8s/watchers/cilium_network_policy.go:175 +0xc64

Goroutine 119834 (running) created at:
  github.com/cilium/cilium/pkg/controller.(*Manager).createControllerLocked()
      ./pkg/controller/manager.go:111 +0x757
  github.com/cilium/cilium/pkg/controller.(*Manager).updateController()
      ./pkg/controller/manager.go:84 +0x44f
  github.com/cilium/cilium/pkg/controller.(*Manager).UpdateController()
      ./pkg/controller/manager.go:52 +0xe6f
  github.com/cilium/cilium/pkg/k8s/watchers.(*K8sWatcher).addCiliumNetworkPolicyV2()
      ./pkg/k8s/watchers/cilium_network_policy.go:348 +0xc75
  github.com/cilium/cilium/pkg/k8s/watchers.(*K8sWatcher).onUpsert()
      ./pkg/k8s/watchers/cilium_network_policy.go:271 +0x744
  github.com/cilium/cilium/pkg/k8s/watchers.(*K8sWatcher).ciliumNetworkPoliciesInit.func1()
      ./pkg/k8s/watchers/cilium_network_policy.go:175 +0xc64

Goroutine 1205 (running) created at:
  github.com/cilium/cilium/pkg/k8s/watchers.(*K8sWatcher).ciliumNetworkPoliciesInit()
      ./pkg/k8s/watchers/cilium_network_policy.go:91 +0x27c
  github.com/cilium/cilium/pkg/k8s/watchers.(*K8sWatcher).enableK8sWatchers.func1()
      ./pkg/k8s/watchers/watcher.go:578 +0x59
  sync.(*Once).doSlow()
      /usr/local/go/src/sync/once.go:74 +0xf0
  sync.(*Once).Do()
      /usr/local/go/src/sync/once.go:65 +0x44
  github.com/cilium/cilium/pkg/k8s/watchers.(*K8sWatcher).enableK8sWatchers()
      ./pkg/k8s/watchers/watcher.go:578 +0xa24
  github.com/cilium/cilium/pkg/k8s/watchers.(*K8sWatcher).InitK8sSubsystem()
      ./pkg/k8s/watchers/watcher.go:508 +0x104
  github.com/cilium/cilium/daemon/cmd.newDaemon()
      ./daemon/cmd/daemon.go:1001 +0x9070
  github.com/cilium/cilium/daemon/cmd.newDaemonPromise.func1()
      ./daemon/cmd/daemon_main.go:1687 +0xa4
  github.com/cilium/cilium/pkg/hive.Hook.Start()
      ./pkg/hive/lifecycle.go:34 +0x70
  github.com/cilium/cilium/pkg/hive.(*Hook).Start()
      <autogenerated>:1 +0x1f
  github.com/cilium/cilium/pkg/hive.(*DefaultLifecycle).Start()
      ./pkg/hive/lifecycle.go:103 +0x3f1
  github.com/cilium/cilium/pkg/hive.(*Hive).Start()
      ./pkg/hive/hive.go:291 +0x152
  github.com/cilium/cilium/pkg/hive.(*Hive).Run()
      ./pkg/hive/hive.go:191 +0xc4
  github.com/cilium/cilium/daemon/cmd.NewAgentCmd.func1()
      ./daemon/cmd/root.go:39 +0x264
  github.com/spf13/cobra.(*Command).execute()
      ./vendor/github.com/spf13/cobra/command.go:944 +0xcb8
  github.com/spf13/cobra.(*Command).ExecuteC()
      ./vendor/github.com/spf13/cobra/command.go:1068 +0x5c4
  github.com/spf13/cobra.(*Command).Execute()
      ./vendor/github.com/spf13/cobra/command.go:992 +0x2e
  github.com/cilium/cilium/daemon/cmd.Execute()
      ./daemon/cmd/root.go:79 +0x2f
  main.main()
      ./daemon/main.go:14 +0xa9
```

Signed-off-by: André Martins <andre@cilium.io>
Signed-off-by: Jussi Maki <jussi@isovalent.com>
@joamaki joamaki force-pushed the pr/v1.14-backport-2023-10-10 branch from 3c154dc to 9db9a3c Compare October 11, 2023 08:53
@joamaki
Copy link
Contributor Author

joamaki commented Oct 11, 2023

/test-backport-1.14

@maintainer-s-little-helper 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 Oct 11, 2023
@squeed squeed removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 11, 2023
@joamaki joamaki marked this pull request as ready for review October 12, 2023 08:10
@joamaki joamaki requested a review from a team as a code owner October 12, 2023 08:10
@joamaki joamaki added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 12, 2023
@jrajahalme jrajahalme merged commit b70df1e into v1.14 Oct 12, 2023
203 checks passed
@jrajahalme jrajahalme deleted the pr/v1.14-backport-2023-10-10 branch October 12, 2023 10:34
@jrajahalme jrajahalme mentioned this pull request Oct 18, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@squeed squeed squeed approved these changes

@aanm aanm aanm approved these changes

@julianwiedmann julianwiedmann julianwiedmann approved these changes

Assignees
No one assigned
Labels
backport/1.14 This PR represents a backport for Cilium 1.14.x of a PR that was merged to main. 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@joamaki @squeed @aanm @julianwiedmann @jrajahalme @michaelasp