pkg/k8s: do not read k8s node annotations if they are not written #22127

aanm · 2022-11-12T01:15:24Z

When there is an annotation in the k8s node object, the annotation io.cilium.network.ipv4-cilium-host is used as the CiliumInternal IP address of the CiliumNode object in [1]. Whenever Cilium is updating any state into the CiliumNode it retrieves all IP address from k8s node, including the ones from annotations, and appends the local node's IP addresses, including the newly correct internal / router IP address, in [2]. Since this is a list, the annotation's IP address is always used first and all other Cilium agents will wrongly use it for any operation.

[1]

cilium/pkg/nodediscovery/nodediscovery.go

Lines 453 to 459 in 927bd8c

    
           // Get the addresses from k8s node and add them as part of Cilium Node. 
        
           // Cilium Node should contain all addresses from k8s. 
        
           nodeInterface := k8s.ConvertToNode(k8sNode) 
        
           typesNode := nodeInterface.(*k8sTypes.Node) 
        
           k8sNodeParsed := k8s.ParseNode(typesNode, source.Unspec) 
        
           k8sNodeAddresses = k8sNodeParsed.IPAddresses

[2]

cilium/pkg/nodediscovery/nodediscovery.go

Lines 474 to 489 in 927bd8c

    
           	ciliumNodeAddress := address.IP.String() 
        
           	var found bool 
        
           	for _, nodeResourceAddress := range nodeResource.Spec.Addresses { 
        
           		if nodeResourceAddress.IP == ciliumNodeAddress { 
        
           			found = true 
        
           			break 
        
           		} 
        
           	} 
        
           	if !found { 
        
           		nodeResource.Spec.Addresses = append(nodeResource.Spec.Addresses, ciliumv2.NodeAddress{ 
        
           			Type: address.Type, 
        
           			IP:   ciliumNodeAddress, 
        
           		}) 
        
           	} 
        
           }

Fixes: 73d6cae ("install: default AnnotateK8sNode to false")
Signed-off-by: André Martins andre@cilium.io

Fix bug that could lead to inconsistent pod IP information between agents, sometimes leading to a failure to decrypt IPsec traffic.

Fixes #21735

aanm · 2022-11-12T01:16:56Z

/test

pkg/k8s/node.go

aanm · 2022-11-14T08:34:57Z

/test

bimmlerd

LGTM

pchaigno

Discussed offline: this pull request doesn't fix the bug for when annotate-k8s-node=true.

pkg/k8s/node.go

aanm · 2022-11-14T23:21:10Z

@pchaigno although it doesn't fix for the annotate-k8s-node=true, it does fix for the annotate-k8s-node=false. Regarding the case annotate-k8s-node=true it should be done in a different PR.

When there is an annotation in the k8s node object, the annotation `io.cilium.network.ipv4-cilium-host` is used as the CiliumInternal IP address of the CiliumNode object in [1]. Whenever Cilium is updating any state into the CiliumNode it retrieves all IP address from k8s node, including the ones from annotations, and appends the local node's IP addresses, including the newly correct internal / router IP address, in [2]. Since this is a list, the annotation's IP address is always used first and all other Cilium agents will wrongly use it for any operation. [1] https://github.com/cilium/cilium/blob/927bd8c26904ff92e42c61cec6d00ea8ac062c05/pkg/nodediscovery/nodediscovery.go#L453-L459 [2] https://github.com/cilium/cilium/blob/927bd8c26904ff92e42c61cec6d00ea8ac062c05/pkg/nodediscovery/nodediscovery.go#L474-L489 Fixes: 73d6cae ("install: default AnnotateK8sNode to false") Signed-off-by: André Martins <andre@cilium.io>

When using CiliumNode, the agent's source of truth should be the agent itself and not k8s node annotations. Thus we will not use the annotations for the CiliumInternalIP address when generating a CiliumNode from the k8s Node resource. Signed-off-by: André Martins <andre@cilium.io>

aanm · 2022-11-18T15:59:43Z

/test

pchaigno

My two comments above are still relevant I think. Other than that, looks good to me.

We try to restore the router IP both from the filesystem (first) and from Kubernetes objects (as a fallback). If the two IP addresses don't match, we emit a warning. There is no good reason for this to happen in CI so we should fail the test if that warning ever shows up. Doing so would have prevented the flake fixed by the previous commit. Signed-off-by: Paul Chaignon <paul@cilium.io>

aanm · 2022-11-22T12:48:03Z

/test

pchaigno

I've rerun my local reproduction 8 times with only the second commit and it didn't fail. Given my reproduction works with a rate 1/2, note that there's a probably of 1/256 that it doesn't fail even if it's not fixed.

The changes look good to me.

aanm added area/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. dont-merge/needs-release-note release-note/bug This PR fixes an issue in a previous release of Cilium. labels Nov 12, 2022

aanm requested review from pchaigno and christarazi November 12, 2022 01:15

aanm requested a review from a team as a code owner November 12, 2022 01:15

aanm force-pushed the pr/fix-bug branch from 163fb30 to 858c2d5 Compare November 12, 2022 01:15

Liz4187 approved these changes Nov 12, 2022

View reviewed changes

christarazi approved these changes Nov 12, 2022

View reviewed changes

bimmlerd reviewed Nov 14, 2022

View reviewed changes

pkg/k8s/node.go Outdated Show resolved Hide resolved

aanm force-pushed the pr/fix-bug branch from 858c2d5 to 344cb54 Compare November 14, 2022 08:34

aanm added needs-backport/1.10 labels Nov 14, 2022

aanm force-pushed the pr/fix-bug branch from 344cb54 to 1737749 Compare November 14, 2022 08:35

aanm requested a review from bimmlerd November 14, 2022 08:38

bimmlerd approved these changes Nov 14, 2022

View reviewed changes

pchaigno requested changes Nov 14, 2022

View reviewed changes

pkg/k8s/node.go Show resolved Hide resolved

pkg/k8s/node.go Show resolved Hide resolved

aanm added 2 commits November 18, 2022 11:01

aanm force-pushed the pr/fix-bug branch from 1737749 to 858407c Compare November 18, 2022 10:11

aanm requested a review from a team as a code owner November 18, 2022 10:11

aanm requested a review from pchaigno November 18, 2022 10:11

pchaigno requested changes Nov 21, 2022

View reviewed changes

aanm requested a review from a team as a code owner November 22, 2022 12:24

aanm requested a review from pchaigno November 22, 2022 12:25

pchaigno added kind/bug This is a bug in the Cilium logic. and removed dont-merge/needs-release-note labels Nov 22, 2022

pchaigno approved these changes Nov 22, 2022

View reviewed changes

aanm merged commit ee4ea1a into cilium:master Nov 22, 2022

aanm deleted the pr/fix-bug branch November 22, 2022 21:22

tklauser mentioned this pull request Nov 23, 2022

v1.12 backports 2022-11-23 #22328

Merged

pchaigno mentioned this pull request Nov 24, 2022

v1.12 backports 2022-11-22 #22308

Merged

jrajahalme added backport-pending/1.12 backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. and removed needs-backport/1.12 labels Nov 24, 2022

jrajahalme mentioned this pull request Nov 24, 2022

v1.11 backports 2022-11-22 #22309

Merged

jrajahalme added backport-pending/1.11 and removed needs-backport/1.11 labels Nov 24, 2022

jrajahalme mentioned this pull request Nov 24, 2022

v1.10 backports 2022-11-22 #22310

Merged

jrajahalme added backport-pending/1.10 backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. and removed needs-backport/1.10 labels Nov 24, 2022

This was referenced Dec 15, 2022

Prepare for release v1.10.18 #22765

Merged

Prepare for release v1.11.12 #22766

Merged

Prepare for release v1.12.5 #22767

Merged

giorio94 mentioned this pull request Apr 3, 2023

Kubernetes Host Scope IPAM via v1.Node annotations broke in 1.13 #24420

Open

2 tasks

joestringer mentioned this pull request Apr 17, 2023

CI: pkg/kvstore/allocator unit test failed: Timeout while waiting for initial allocator state #24904

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

pkg/k8s: do not read k8s node annotations if they are not written #22127

pkg/k8s: do not read k8s node annotations if they are not written #22127

Uh oh!

aanm commented Nov 12, 2022 •

edited by pchaigno

Loading

Uh oh!

aanm commented Nov 12, 2022

Uh oh!

Uh oh!

aanm commented Nov 14, 2022

Uh oh!

bimmlerd left a comment

Uh oh!

pchaigno left a comment

Uh oh!

Uh oh!

Uh oh!

aanm commented Nov 14, 2022

Uh oh!

aanm commented Nov 18, 2022

Uh oh!

pchaigno left a comment

Uh oh!

aanm commented Nov 22, 2022

Uh oh!

pchaigno left a comment •

edited

Loading

Uh oh!

Uh oh!

	// Get the addresses from k8s node and add them as part of Cilium Node.
	// Cilium Node should contain all addresses from k8s.
	nodeInterface := k8s.ConvertToNode(k8sNode)
	typesNode := nodeInterface.(*k8sTypes.Node)
	k8sNodeParsed := k8s.ParseNode(typesNode, source.Unspec)
	k8sNodeAddresses = k8sNodeParsed.IPAddresses

	ciliumNodeAddress := address.IP.String()
	var found bool
	for _, nodeResourceAddress := range nodeResource.Spec.Addresses {
	if nodeResourceAddress.IP == ciliumNodeAddress {
	found = true
	break
	}
	}
	if !found {
	nodeResource.Spec.Addresses = append(nodeResource.Spec.Addresses, ciliumv2.NodeAddress{
	Type: address.Type,
	IP: ciliumNodeAddress,
	})
	}
	}

pkg/k8s: do not read k8s node annotations if they are not written #22127

pkg/k8s: do not read k8s node annotations if they are not written #22127

Uh oh!

Conversation

aanm commented Nov 12, 2022 • edited by pchaigno Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aanm commented Nov 12, 2022

Uh oh!

Uh oh!

aanm commented Nov 14, 2022

Uh oh!

bimmlerd left a comment

Choose a reason for hiding this comment

Uh oh!

pchaigno left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aanm commented Nov 14, 2022

Uh oh!

aanm commented Nov 18, 2022

Uh oh!

pchaigno left a comment

Choose a reason for hiding this comment

Uh oh!

aanm commented Nov 22, 2022

Uh oh!

pchaigno left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aanm commented Nov 12, 2022 •

edited by pchaigno

Loading

pchaigno left a comment •

edited

Loading