Compare annotations before discarding CiliumNode updates. #25465

LynneD · 2023-05-15T21:21:27Z

Currently we discard CiliumNode updates based on DeepEqual and labels. However DeepEqual is set to ignore Annotations, and the wg-pub-key annotation is used to exchange rotated Wireguard keys.

maintainer-s-little-helper · 2023-05-15T21:21:30Z

Commit 0fc487f01be34f75bf5ae6ad24e2f4b8d6753821 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

LynneD · 2023-05-15T21:24:41Z

Hi @aojea, this is the fix for Wiregurad tunnel drop issue.

The wireguard tunnel is broken when any node restart happens. After restart, the restarted node's public key got refreshed but not propagated to other nodes.

The issue is caused by cilium dropping CiliumNode update events when the spec, status and labels between the old and new nodes are the same. Wireguard public key updates are transmitted through annotations.

We want to confirm if this is a known issue since it can be reproduced by restarting any node easily.

Thanks!

aojea · 2023-05-15T21:35:08Z

pkg/k8s/watchers/cilium_node.go

@@ -69,7 +69,8 @@ func (k *K8sWatcher) ciliumNodeInit(ciliumNPClient client.Clientset, asyncContro
 							valid = true
 							isLocal := k8s.IsLocalCiliumNode(ciliumNode)
 							if oldCN.DeepEqual(ciliumNode) &&
-								comparator.MapStringEquals(oldCN.ObjectMeta.Labels, ciliumNode.ObjectMeta.Labels) {
+								comparator.MapStringEquals(oldCN.ObjectMeta.Labels, ciliumNode.ObjectMeta.Labels) &&
+								comparator.MapStringEquals(oldCN.ObjectMeta.Annotations, ciliumNode.ObjectMeta.Annotations) {


This causes issue in case the wireguard key changes but the rest of the node object not

cilium/pkg/nodediscovery/nodediscovery.go

Lines 460 to 465 in e52fe1d

if pk := n.localNode.WireguardPubKey; pk != "" {

if nodeResource.ObjectMeta.Annotations == nil {

nodeResource.ObjectMeta.Annotations = make(map[string]string)

}

nodeResource.ObjectMeta.Annotations[annotation.WireguardPubKey] = pk

}

since, we don't compare Annotations the event is dropped and not processed

aojea · 2023-05-15T21:35:25Z

/assign @aanm @squeed

tommyp1ckles · 2023-05-15T23:25:25Z

Thanks for the PR 😄

I wonder if instead of checking all annotation changes, it might make more sense to target the specific wireguard key annotation and just see if that's changed.

It seems like the intention is to avoid unnecessary node updates.

pkg/k8s/watchers/cilium_node.go

aojea · 2023-05-15T23:59:59Z

I wonder if instead of checking all annotation changes, it might make more sense to target the specific wireguard key annotation and just see if that's changed.

in the long term my experience is that new annotations got added but you never remember to update this comparison, it is also coherent with existing comparison with labels

LynneD · 2023-05-16T01:16:19Z

I wonder if instead of checking all annotation changes, it might make more sense to target the specific wireguard key annotation and just see if that's changed.

in the long term my experience is that new annotations got added but you never remember to update this comparison, it is also coherent with existing comparison with labels

Hi @tommyp1ckles, I agree with @aojea's point here. Is there a specific concern about checking all annotation changes?

christarazi · 2023-05-16T07:30:22Z

/test

LynneD · 2023-05-17T15:28:28Z

Good finds! Could you add #25465 (comment) to the commit message?

Added the comment. Is the the PR ready to merge? We are waiting for the CL to be merged to cherry-pick it to our internal branch.

christarazi · 2023-05-17T18:57:00Z

Good finds! Could you add #25465 (comment) to the commit message?

Added the comment. Is the the PR ready to merge? We are waiting for the CL to be merged to cherry-pick it to our internal branch.

A PR is ready to merge when all codeowners have approved the PR and the CI is all green. Right now, Martynas is required to approve (I've re-requested him) and I will trigger the CI.

christarazi · 2023-05-17T18:57:31Z

/test

Job 'Cilium-PR-K8s-1.26-kernel-net-next' failed:

Click to show.

Test Name

K8sDatapathServicesTest Checks N/S loadbalancing With host policy Tests NodePort

Failure Output

FAIL: Can not connect to service "tftp://192.168.56.12:31104/hello" from outside cluster (1/10)

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.26-kernel-net-next/2380/

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

Then please upload the Jenkins artifacts to that issue.

LynneD · 2023-05-17T19:00:06Z

Good finds! Could you add #25465 (comment) to the commit message?

Added the comment. Is the the PR ready to merge? We are waiting for the CL to be merged to cherry-pick it to our internal branch.

A PR is ready to merge when all codeowners have approved the PR and the CI is all green. Right now, Martynas is required to approve (I've re-requested him) and I will trigger the CI.

Thanks @christarazi

tommyp1ckles

I agree with @aojea's point here. Is there a specific concern about checking all annotation changes?

Nothing specific - mostly just wondering if it would lead to unnecessary updates. This seems reasonable to me.

LynneD · 2023-05-18T05:46:32Z

Good finds! Could you add #25465 (comment) to the commit message?

Hi @brb, I've updated the commit message, can you please take another look?

brb

Thanks!

LynneD · 2023-05-18T06:21:08Z

/mlh new-flake Cilium-PR-K8s-1.26-kernel-net-next

LynneD · 2023-05-18T06:25:11Z

Thanks @christarazi @brb @tommyp1ckles @aojea, I've got enough approvals, and the "k8s-1.26-kernel-net-next" test seems not related to this PR. Can you please take another look and see how I can unblock the merging?

Currently we discard CiliumNode updates based on DeepEqual and labels. However DeepEqual is set to ignore Annotations, and the wg-pub-key annotation is used to exchange rotated Wireguard keys. The wireguard tunnel is broken when any node restart happens. After restart, the restarted node's public key got refreshed but not propagated to other nodes. The issue is caused by cilium dropping CiliumNode update events when the spec, status and labels between the old and new nodes are the same. Wireguard public key updates are transmitted through annotations. Signed-off-by: Lin Dong <lindongchn@gmail.com>

christarazi · 2023-05-18T18:54:18Z

/test

christarazi · 2023-05-18T18:55:00Z

I've rebased to re-run the tests. Please don't push unless the CI points out a failure and you need to push to address it.

LynneD · 2023-05-18T18:56:39Z

I've rebased to re-run the tests. Please don't push unless the CI points out a failure and you need to push to address it.

Got it. Thanks @christarazi!

christarazi · 2023-05-18T21:28:08Z

Thanks for the PR and your patience!

LynneD · 2023-05-18T21:30:30Z

Thanks so much @christarazi!

LynneD requested a review from a team as a code owner May 15, 2023 21:21

LynneD requested a review from tommyp1ckles May 15, 2023 21:21

maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels May 15, 2023

github-actions bot added the kind/community-contribution This was a contribution made by a community member. label May 15, 2023

aojea reviewed May 15, 2023

View reviewed changes

LynneD force-pushed the pr/wireguard-publickey-propagation branch from 0fc487f to d7db202 Compare May 15, 2023 22:56

maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label May 15, 2023

LynneD force-pushed the pr/wireguard-publickey-propagation branch from d7db202 to b495ea8 Compare May 15, 2023 23:10

tommyp1ckles reviewed May 15, 2023

View reviewed changes

pkg/k8s/watchers/cilium_node.go Show resolved Hide resolved

LynneD force-pushed the pr/wireguard-publickey-propagation branch from b495ea8 to 52942f6 Compare May 16, 2023 01:04

aanm requested a review from gandro May 16, 2023 07:31

maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label May 16, 2023

aanm added dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. affects/v1.11 This issue affects v1.11 branch affects/v1.12 This issue affects v1.12 branch and removed needs-backport/1.12 labels May 16, 2023

LynneD force-pushed the pr/wireguard-publickey-propagation branch from 81ff7f1 to 1d614f3 Compare May 17, 2023 15:27

christarazi requested a review from brb May 17, 2023 18:56

tommyp1ckles approved these changes May 18, 2023

View reviewed changes

brb approved these changes May 18, 2023

View reviewed changes

LynneD force-pushed the pr/wireguard-publickey-propagation branch from 1d614f3 to d38b6b6 Compare May 18, 2023 06:35

christarazi force-pushed the pr/wireguard-publickey-propagation branch from d38b6b6 to 975a0d4 Compare May 18, 2023 18:54

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 May 18, 2023

christarazi merged commit 76eca78 into cilium:main May 18, 2023

tklauser mentioned this pull request May 22, 2023

v1.13 Backports 2023-05-22 #25588

Merged

4 tasks

tklauser added backport-pending/1.13 backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed needs-backport/1.13 labels May 22, 2023

qmonnet mentioned this pull request Jun 9, 2023

Prepare for release v1.13.4 #26071

Closed

This was referenced Jun 9, 2023

Prepare for release v1.13.4 #26091

Closed

Prepare for release v1.13.4 #26202

Merged

pippolo84 mentioned this pull request Nov 21, 2023

Use Resource[T] to implement CiliumNode watcher #29222

Merged

	if pk := n.localNode.WireguardPubKey; pk != "" {
	if nodeResource.ObjectMeta.Annotations == nil {
	nodeResource.ObjectMeta.Annotations = make(map[string]string)
	}
	nodeResource.ObjectMeta.Annotations[annotation.WireguardPubKey] = pk
	}

Compare annotations before discarding CiliumNode updates. #25465

Compare annotations before discarding CiliumNode updates. #25465

Uh oh!

Conversation

LynneD commented May 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maintainer-s-little-helper bot commented May 15, 2023

Uh oh!

LynneD commented May 15, 2023

Uh oh!

aojea May 15, 2023

Choose a reason for hiding this comment

Uh oh!

aojea commented May 15, 2023

Uh oh!

tommyp1ckles commented May 15, 2023

Uh oh!

Uh oh!

aojea commented May 15, 2023

Uh oh!

LynneD commented May 16, 2023

Uh oh!

christarazi commented May 16, 2023

Uh oh!

LynneD commented May 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

christarazi commented May 17, 2023

Uh oh!

christarazi commented May 17, 2023 • edited by maintainer-s-little-helper bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Name

Failure Output

Uh oh!

LynneD commented May 17, 2023

Uh oh!

tommyp1ckles left a comment

Choose a reason for hiding this comment

Uh oh!

LynneD commented May 18, 2023

Uh oh!

brb left a comment

Choose a reason for hiding this comment

Uh oh!

LynneD commented May 18, 2023

Uh oh!

LynneD commented May 18, 2023

Uh oh!

christarazi commented May 18, 2023

Uh oh!

christarazi commented May 18, 2023

Uh oh!

LynneD commented May 18, 2023

Uh oh!

christarazi commented May 18, 2023

Uh oh!

LynneD commented May 18, 2023

Uh oh!

Uh oh!

LynneD commented May 15, 2023 •

edited

Loading

LynneD commented May 17, 2023 •

edited

Loading

christarazi commented May 17, 2023 •

edited by maintainer-s-little-helper bot

Loading