v1.10 backports 2022-04-26

[jrajahalme]

• daemon: Dump ipcache without probing for support #19501 -- daemon: Dump ipcache without probing for support (@jrajahalme)
• identity: Initialize local identity allocator early #19556 -- identity: Initialize local identity allocator early (@jrajahalme)

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

$ for pr in 19501 19556; do contrib/backporting/set-labels.py $pr done 1.10; done

[jrajahalme]

/test-backport-1.10

[aanm]

/ci-gke-1.10

[aanm]

/ci-gke-1.10

[jrajahalme]

/test-backport-1.10

Job 'Cilium-PR-Runtime-net-next' failed:

Click to show.

Test Name

RuntimeFQDNPolicies DNS proxy policy works if Cilium stops

Failure Output

FAIL: CIDR identities were not restored correctly: Unified diff:

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

Job 'Cilium-PR-K8s-1.19-kernel-5.4' failed:

Click to show.

Test Name

K8sDatapathConfig AutoDirectNodeRoutes Check connectivity with sockops and direct routing

Failure Output

FAIL: Error creating resource /home/jenkins/workspace/Cilium-PR-K8s-1.19-kernel-5.4/src/github.com/cilium/cilium/test/k8sT/manifests/l3-policy-demo.yaml: Cannot retrieve cilium pod cilium-ksvc6 policy revision: cannot get revision from json: could not parse JSON from command "kubectl exec -n kube-system cilium-ksvc6 -- cilium policy get -o json"

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

Job 'Cilium-PR-K8s-1.20-kernel-4.19' failed:

Click to show.

Test Name

K8sServicesTest Checks service across nodes Tests NodePort BPF Tests with direct routing Tests NodePort with externalTrafficPolicy=Local

Failure Output

FAIL: Request from k8s1 to service http://[fd04::12]:32310 failed

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

[jrajahalme]

Cannot parse JSON error on test-1.19-5.4, restarting

[jrajahalme]

/test-1.19-5.4

[jrajahalme]

/test-1.20-4.19

[jrajahalme]

Runtime test fail is due to this backport not working correctly, I'll have to investigate it, flipping this to draft in the meanwhile.

[jrajahalme]

Had to apply this for the v1.10 backport:

diff --git a/daemon/cmd/policy.go b/daemon/cmd/policy.go
index b9f1cbe2d1..cff454b087 100644
--- a/daemon/cmd/policy.go
+++ b/daemon/cmd/policy.go
@@ -81,6 +81,11 @@ func (d *Daemon) policyUpdateTrigger(reasons []string) {
 // in agent configuration, changes in endpoint labels, and change of security
 // identities.
 func (d *Daemon) TriggerPolicyUpdates(force bool, reason string) {
+	// CIDR identity restoration may trigger early when endpoints can not yet be regenerated
+	if d.policyTrigger == nil {
+		return
+	}
+
 	if force {
 		log.Debugf("Artificially increasing policy revision to enforce policy recalculation")
 		d.policy.BumpRevision()

This was not necessary on v1.11 due to the policyTrigger mechanism being replaced with policy.Updater.

[jrajahalme]

/test-backport-1.10

Job 'Cilium-PR-K8s-1.19-kernel-4.9' failed:

Click to show.

Test Name

K8sServicesTest Checks service across nodes Tests NodePort (kube-proxy) 

Failure Output

FAIL: Request from k8s1 to service tftp://[fd04::12]:31408/hello failed

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

[jrajahalme]

/test-1.19-4.9

[jrajahalme]

/test-1.16-netnext

[jrajahalme]

restarted test-1.19-4.9 and test-1.16-netnext due to unrelated but familiar looking flakes.

aws-cni test is currently broken.

[joestringer]

Had to apply this for the v1.10 backport:

diff --git a/daemon/cmd/policy.go b/daemon/cmd/policy.go
index b9f1cbe2d1..cff454b087 100644
--- a/daemon/cmd/policy.go
+++ b/daemon/cmd/policy.go
@@ -81,6 +81,11 @@ func (d *Daemon) policyUpdateTrigger(reasons []string) {
// in agent configuration, changes in endpoint labels, and change of security
// identities.
func (d *Daemon) TriggerPolicyUpdates(force bool, reason string) {
+	// CIDR identity restoration may trigger early when endpoints can not yet be regenerated
+	if d.policyTrigger == nil {
+		return
+	}
+
	if force {
		log.Debugf("Artificially increasing policy revision to enforce policy recalculation")
		d.policy.BumpRevision()

This was not necessary on v1.11 due to the policyTrigger mechanism being replaced with policy.Updater.

Why is the policy engine not initialized when something calls into TriggerPolicyUpdates()? Presumably something that got rearranged is now attempting to trigger policy updates before that code is ready to handle them? If we can guarantee that this logic will be called again soon, then I think we're "OK" here. If it doesn't trigger again then this is potentially avoiding handling the policy update event altogether. Then we'd need another event to come along later to make sure that the trigger eventually happens. On average I guess that's the case, but I just don't know if in some case it could get "stuck" and policy won't get recalculated due to this logic.

If the above concerns you, we'd just want to ensure that after d.policyTrigger is initialized, we then trigger policy updates. This could be a static call at the d.policyTrigger initialization point, or it could be triggered directly after k8s is fully synced, or we could introduce a sync.Once into this TriggerPolicyUpdates case iff d.policyTrigger == nil, then run a goroutine to wait until d.policyTrigger is not nil, and then call the trigger. Alternatively, if you're fairly confident that we'll get & handle another policy update trigger soon anyway then maybe we just don't worry about it.

I agree that replacing the mechanism with policy.Updater in 4881234 changed the calculation here, because the trigger is now initialized earlier in bootstrap, which is probably what ensures that this nil dereference doesn't occur with this patch on newer releases. And I don't think it makes sense to backport that commit/PR, as it's pretty invasive and risky to backport. On the other hand... if we are now triggering policy updates much earlier on newer versions (successfully), is it at all possible that this early trigger of policy updates could cause the policy to be evaluated before we've fully synced the k8s policy resources? That seems like it could introduce a whole different cause for policy drops during restart.

[joestringer]

@jrajahalme pointed out to me out-of-band that these triggers will not find any endpoints until all k8s resources are synced, so my point above is moot. LGTM.

[jrajahalme]

@jrajahalme pointed out to me out-of-band that these triggers will not find any endpoints until all k8s resources are synced, so my point above is moot. LGTM.

Labeled as ready-to-merge based on Joe's approval.

[aditighag]

@jrajahalme The PR needs to be rebased; looks like some other v1.10 backport PR was merged in between.

[joestringer]

There aren't any conflicts, so we can technically merge this, but it does come with some risk if we think that these changes may cause breakage in combination with the other patches on the v1.10 branch (since we haven't actually tested all of this PR's changes together with all of the recent changes in the upstream branch):

$ git lo HEAD..upstream/v1.10
55e076cebabd (upstream/v1.10) docs: fix version warning URL to point to docs.cilium.io
9e95af3436cf fqdn: Limit the number of zombies per host
955ba8232ff5 fqdn: Refactor zombie sort function
1ee37c1b736c make: check that Go major/minor version matches required version
ee0049f691f5 build(deps): bump docker/setup-buildx-action from 1.6.0 to 1.7.0
fec2ba7f9180 (v1.10) build(deps): bump actions/checkout from 3.0.1 to 3.0.2
28713df2579a docs: Update section title to improve readability
67f0d7723fda pkg/redirectpolicy: Improve error logs
7e425d49b63b docs: improve description for session affinity with KPR
d7ea906209bc pkg/bpf: add map name in error message for OpenParallel
56a1c4566ae6 jenkinsfiles: Increase VM boot timeout
43824d853975 install: Update image digests for v1.10.10
8a77a4a22ab3 (tag: v1.10.10, tag: 1.10.10) Prepare for release v1.10.10
3f3fec152876 build(deps): bump actions/checkout from 3.0.0 to 3.0.1
47e81a6828a6 envoy: Limit accesslog socket permissions
266183d379b1 Add a 'Limitations' section to 'External Workloads'.
9228eed8c470 ci: run documentation workflow on workflow changes
698afebc2d36 ci: pin down image for documentation workflow
adff281aa9fc Use RLock while reading data in DNS proxy
4f5148cf7ee3 wireguard: Reject duplicate public keys
c5fa809f93bb fix(helm): do not override CHECKPOINT_PATH
a77f324fabb8 fix(helm): use a directory to store bootstrap time

There's some DNS/FQDN code changes going on here, but actually most of the changes are in helm, ci, docs, dependencies which should have no impact. I'm OK with merging this as-is. To be clear, the simple answer to this kind of question is always, force a rebase & re-run the tests, but this takes additional effort & time, and I don't think that it is likely to reveal much important information in this case. As the merger, I'll take responsibility to follow up if it happens to break something.