8000 nodediscovery: ensure we cache the nodeResource by odinuge · Pull Request #20158 · cilium/cilium · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

nodediscovery: ensure we cache the nodeResource #20158

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
Merged
merged 1 commit into from
Jun 14, 2022
Merged

nodediscovery: ensure we cache the nodeResource #20158

merged 1 commit into from
Jun 14, 2022

Conversation

odinuge
Copy link
Member
@odinuge odinuge commented Jun 10, 2022 •
edited
Loading

Please ensure your pull request adheres to the following guidelines:

  • For first time contributors, read Submitting a pull request
  • All code is covered by unit and/or runtime tests where feasible.
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.
  • Thanks for contributing!

When retrying we have to explicitly use the previously fetched
nodeResource in case we want to skip fetching it. Otherwise we end up
with a null pointer exception.

Fixes: 91e68c2 ("nodediscovery: ensure instanceID is not empty")
Signed-off-by: Odin Ugedal ougedal@palantir.com

Found this when testing the ipsec key rotation work by @jibi

2022-06-10 11:45:25.633
Loading IPsec keyfile
subsys: "ipsec"
file-path: "/etc/ipsec/keys"

2022-06-10 11:45:25.634
Creating or updating CiliumNode resource
node: "ip-10-0-2-6.ec2.internal"
subsys: "nodediscovery"

2022-06-10 11:45:55.642
Unable to mutate nodeResource
subsys: "nodediscovery"
retryCount: "0"
error: "failed to fetch Kubernetes Node resource: Get "[https://10.100.0.1:443/api/v1/nodes/ip-10-0-2-6.ec2.internal](https://10.100.0.1/api/v1/nodes/ip-10-0-2-6.ec2.internal)": dial tcp 10.100.0.1:443: i/o timeout"

2022-06-10 11:45:55.644
/go/src/github.com/cilium/cilium/pkg/datapath/linux/ipsec/ipsec_linux.go:732 +0x105
created by github.com/cilium/cilium/pkg/datapath/linux/ipsec.StartKeyfileWatcher
panic: runtime error: invalid memory address or nil pointer dereference
/go/src/github.com/cilium/cilium/pkg/datapath/linux/ipsec/ipsec_linux.go:713 +0x3eb
github.com/cilium/cilium/pkg/datapath/linux/ipsec.keyfileWatcher(0x2ba1dc0, 0xc000366c80, 0xc001ce2ff0, 0xc0001ca930, 0xf, 0xc000ef2b00, 0x2bc05c0, 0xc000d601e0)
github.com/cilium/cilium/pkg/nodediscovery.(*NodeDiscovery).UpdateCiliumNodeResource(0xc000ef2b00)
/go/src/github.com/cilium/cilium/pkg/nodediscovery/nodediscovery.go:389 +0x3d
github.com/cilium/cilium/pkg/nodediscovery.(*NodeDiscovery).mutateNodeResource(0xc000ef2b00, 0x0, 0xc00541cff0, 0x1)
/go/src/github.com/cilium/cilium/pkg/nodediscovery/nodediscovery.go:300 +0x7d
goroutine
8000
 961 [running]:
github.com/cilium/cilium/pkg/nodediscovery.(*NodeDiscovery).UpdateLocalNode(0xc000ef2b00)
/go/src/github.com/cilium/cilium/pkg/nodediscovery/nodediscovery.go:291 +0x5a
github.com/cilium/cilium/pkg/nodediscovery.(*NodeDiscovery).updateLocalNode(0xc000ef2b00)
/go/src/github.com/cilium/cilium/pkg/nodediscovery/nodediscovery.go:348 +0x230

Fixes: #issue-number

nodediscovery: ensure we cache the nodeResource correctly to avoid null pointer dereferencing

@odinuge odinuge requested a review from a team as a code owner June 10, 2022 13:26
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jun 10, 2022
@jibi jibi added area/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. release-note/bug This PR fixes an issue in a previous release of Cilium. needs-backport/1.9 labels Jun 10, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Jun 10, 2022
aanm
aanm approved these changes Jun 10, 2022
View reviewed changes
Copy link
Member
@aanm aanm left a comment

Choose a reason for hiding this comment

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

Nice catch

@aanm
Copy link
Member
aanm commented Jun 10, 2022

/test

@jibi
Copy link
Member
jibi commented Jun 13, 2022

@odinuge could you please rebase on top of master to pull afe5414 into your branch? 🙏
This should fix the Travis CI test that is currently failing

@odinuge-work
nodediscovery: ensure we cache the nodeResource
8900be5 
When retrying we have to explicitly use the previously fetched
nodeResource in case we want to skip fetching it. Otherwise we end up
with a null pointer exception.

Fixes: 91e68c2 ("nodediscovery: ensure instanceID is not empty")
Signed-off-by: Odin Ugedal <ougedal@palantir.com>
@odinuge
Copy link
Member Author
odinuge commented Jun 13, 2022

@odinuge could you please rebase on top of master to pull afe5414 into your branch? 🙏
This should fix the Travis CI test that is currently failing

Ack. Rebased on top of master now. Thanks!

@jibi
Copy link
Member
jibi commented Jun 13, 2022

/test

ldelossa
ldelossa approved these changes Jun 13, 2022
View reviewed changes
Copy link
Contributor
@ldelossa ldelossa left a comment

Choose a reason for hiding this comment

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

LGTM

@jibi
Copy link
Member
jibi commented Jun 13, 2022

/test-runtime

@jibi jibi added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 13, 2022
@aanm aanm merged commit a91e00e into cilium:master Jun 14, 2022
@kkourt kkourt mentioned this pull request Jun 21, 2022
Merged
@aanm aanm mentioned this pull request Jun 22, 2022
Merged
@pchaigno pchaigno mentioned this pull request Jun 28, 2022
Merged
@qmonnet qmonnet mentioned this pull request Jun 30, 2022
Merged
4 tasks
@qmonnet qmonnet added backport-done/1.10 backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. and removed backport-pending/1.10 labels Jul 5, 2022
This was referenced Jul 15, 2022
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ldelossa ldelossa ldelossa approved these changes

@aanm aanm aanm approved these changes

Assignees
No one assigned
Labels
area/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@odinuge @aanm @jibi @ldelossa @kkourt @pchaigno @qmonnet @odinuge-work
0