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

kpr: ensure DirectRoutingDevice is in devices #14054

Merged
merged 1 commit into from
Dec 1, 2020
Merged

kpr: ensure DirectRoutingDevice is in devices #14054

merged 1 commit into from
Dec 1, 2020

Conversation

kkourt
Copy link
Contributor

@kkourt kkourt commented Nov 17, 2020 •
edited

As reported by #14052, there can be a panic if the congfigured
DirectRoutingDevice is not included in devices. (InitNodePortAddrs() is
called with option.Config.Devices.)

Fix this by checking that the specified is included and fail otherwise
since this is probably a user error.

docs state that:
If the direct routing device does not exist within devices, Cilium
will add the device to the latter list.

so this patch adds the device to the list.

Fixes #14052

Signed-off-by: Kornilios Kourtis kornilios@isovalent.com

@kkourt kkourt added the release-note/misc This PR makes changes that have no direct user impact. label Nov 17, 2020
@kkourt kkourt requested a review from a team November 17, 2020 09:37
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Nov 17, 2020
@kkourt kkourt requested a review from jibi November 17, 2020 09:37
@kkourt kkourt requested a review from brb November 17, 2020 09:37
@kkourt kkourt marked this pull request as draft November 17, 2020 09:40
@kkourt
Copy link
Contributor Author

kkourt commented Nov 17, 2020

Set to draft because it's untested.

brb
brb requested changes Nov 17, 2020
View reviewed changes
Copy link
Member

@brb brb left a comment

Choose a reason for hiding this comment

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

Thanks for debugging and fixing.

According to the docs:

If the direct routing device does not exist within devices, Cilium will add the device to the latter list.

So I guess instead of panicking we should just add the direct routing device to the devices list.

@brb brb added area/daemon Impacts operation of the Cilium daemon. kind/bug This is a bug in the Cilium logic. needs-backport/1.8 release-note/bug This PR fixes an issue in a previous release of Cilium. and removed release-note/misc This PR makes changes that have no direct user impact. labels Nov 17, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.8.6 Nov 17, 2020
@joestringer joestringer added this to Needs backport from master in 1.9.1 Nov 17, 2020
@kkourt kkourt force-pushed the pr/kkourt/fix-14052 branch 2 times, most recently from 8f512e2 to ca0b36d Compare November 20, 2020 15:34
@kkourt kkourt marked this pull request as ready for review November 20, 2020 16:04
@kkourt kkourt requested a review from brb November 20, 2020 16:04
@kkourt
Copy link
Contributor Author

kkourt commented Nov 20, 2020

So I guess instead of panicking we should just add the direct routing device to the devices list.

modified the patch to do so.

borkmann
borkmann reviewed Nov 20, 2020
View reviewed changes
daemon/cmd/kube_proxy_replacement.go Outdated Show resolved Hide resolved
daemon/cmd/kube_proxy_replacement.go Outdated Show resolved Hide resolved
daemon/cmd/kube_proxy_replacement.go Outdated Show resolved Hide resolved
christarazi
christarazi approved these changes Nov 20, 2020
View reviewed changes
Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

LGTM. Non-blocking comment.

daemon/cmd/kube_proxy_replacement.go Outdated Show resolved Hide resolved
@kkourt kkourt force-pushed the pr/kkourt/fix-14052 branch 2 times, most recently from ff3d86a to 11d410c Compare November 20, 2020 20:43
@maintainer-s-little-helper
Copy link

Commit 31c19b105e96cfffe0e129896461bb21eac0534c 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

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Nov 30, 2020
@kkourt @brb
kpr: ensure DirectRoutingDevice is in devices
17e6123 
As reported by #14052, there can be a panic if the configured
DirectRoutingDevice is not included in devices. (InitNodePortAddrs() is
called with option.Config.Devices.)

docs state that:
 If the direct routing device does not exist within devices, Cilium
 will add the device to the latter list.

so this patch adds the device to the list.

Fixes #14052

Signed-off-by: Kornilios Kourtis <kornilios@isovalent.com>
Co-authored-by: Martynas Pumputis <m@lambda.lt>
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Nov 30, 2020
@kkourt
Copy link
Contributor Author

kkourt commented Nov 30, 2020

test-me-please

brb
brb approved these changes Nov 30, 2020
View reviewed changes
Copy link
Member

@brb brb left a comment

Choose a reason for hiding this comment

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

Thanks!

@kkourt
Copy link
Contributor Author

kkourt commented Nov 30, 2020

retest-runtime

@kkourt
Copy link
Contributor Author

kkourt commented Nov 30, 2020

(runtime test failed because it could not provision a VM)

@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 Nov 30, 2020
@joestringer joestringer merged commit 63f19d2 into master Dec 1, 2020
@joestringer joestringer deleted the pr/kkourt/fix-14052 branch December 1, 2020 00:33
@borkmann borkmann mentioned this pull request Dec 2, 2020
Merged
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.9 in 1.9.1 Dec 2, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.9 in 1.9.1 Dec 2, 2020
@nathanjsweet nathanjsweet mentioned this pull request Dec 2, 2020
Merged
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.8 in 1.8.6 Dec 2, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.9 to Backport done to v1.9 in 1.9.1 Dec 3, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.9 to Backport done to v1.9 in 1.9.1 Dec 3, 2020
@aanm aanm mentioned this pull request Dec 4, 2020
Merged
@aanm aanm mentioned this pull request Dec 4, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@borkmann borkmann borkmann left review comments

@jibi jibi jibi approved these changes

@brb brb brb approved these changes

@christarazi christarazi christarazi approved these changes

Assignees

@jibi jibi

@brb brb

Labels
area/daemon Impacts operation of the Cilium daemon. kind/bug This is a bug in the Cilium logic. 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
No open projects
1.10.0
Done
1.8.6
Backport pending to v1.8
1.9.1
Backport done to v1.9
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

failed to start cilium agent with devices and directRoutingDevice set
9 participants
@kkourt @jibi @brb @borkmann @christarazi @nathanjsweet @joestringer @pchaigno @aanm