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

linux: check policy routing of running kernel #10068

Merged
merged 1 commit into from
Feb 21, 2020
Merged

linux: check policy routing of running kernel #10068

merged 1 commit into from
Feb 21, 2020

Conversation

iecedge
Copy link

@iecedge iecedge commented Feb 5, 2020 •
edited by tgraf

Check the payload of the NLMSG_DONE type netlink
message for possible error, then determine whether
CONFIG_IP_MULTIPLE_TABLES is enabled.

Signed-off-by: Jianlin Lv Jianlin.Lv@arm.com

Fixes: #9834

This change is Reviewable

@iecedge iecedge requested a review from a team February 5, 2020 15:38
@iecedge iecedge requested a review from a team as a code owner February 5, 2020 15:38
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 Feb 5, 2020
@iecedge iecedge mentioned this pull request Feb 5, 2020
Closed
@coveralls
Copy link

coveralls commented Feb 5, 2020 •
edited

Coverage Status

Coverage increased (+0.01%) to 44.561% when pulling 2caaa97 on iecedge:check_policy_ru into 8fc4bde on cilium:master.

joestringer
joestringer approved these changes Feb 5, 2020
View reviewed changes
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

This looks nice and tidy!

Did you already submit the vendor change to the upstream library? If not, please do. We can rebase the netlink dependency on top when the upstream change is merged.

@joestringer joestringer added the dont-merge/waiting-for-upstream Only merge once upstream library or kernel features have landed label Feb 5, 2020
@maintainer-s-little-helper
Copy link

Release note label not set, please set the appropriate release note.

@joestringer joestringer added the release-note/misc This PR makes changes that have no direct user impact. label Feb 5, 2020
@joestringer
Copy link
Member

test-me-please

@Jianlin-lv
Copy link
Contributor

This looks nice and tidy!

Did you already submit the vendor change to the upstream library? If not, please do. We can rebase the netlink dependency on top when the upstream change is merged.

The change is already upstream, vishvananda/netlink#520

@joestringer
Copy link
Member

The change is already upstream, vishvananda/netlink#520

Thanks. When the above is merged, please update this PR to the new version of the library using these instructions:

http://docs.cilium.io/en/latest/contributing/development/dev_setup/#add-update-a-golang-dependency

@aanm aanm added the dont-merge/wait-until-release Freeze window for current release is blocking non-bugfix PRs label Feb 7, 2020
@Jianlin-lv
Copy link
Contributor

The change is already upstream, vishvananda/netlink#520

Thanks. When the above is merged, please update this PR to the new version of the library using these instructions:

http://docs.cilium.io/en/latest/contributing/development/dev_setup/#add-update-a-golang-dependency

@joestringer, vishvananda/netlink#520 have been merged,
When locally update github.com/vishvananda/netlink to latest and recompiling,
I encountered one issue that
"# Github.com/vishvananda/netlink/nl
../../vendor/github.com/vishvananda/netlink/nl/addr_linux.go:58:2: undefined: unix.IfaCacheinfo
"
This issue needs to be resolved by upgrading golang.org/x/sys/unix， Refer：vishvananda/netlink#514

I see "pinned to release-branch.go1.13" in the go.mod about sys/unix
Could I upgrade golang.org/x/sys/unix to latest?

@joestringer
Copy link
Member

I think that makes sense, unfortunate it's relying on such a new implementation. /cc @aanm who will review such changes.

@aanm
Copy link
Member

aanm commented Feb 10, 2020

@Jianlin-lv it is be fine to pin the sys/unix to latest because Go 1.14 will be released during the Cilium 1.8 dev cycle.

@iecedge iecedge requested a review from a team as a code owner February 11, 2020 15:32
@aanm aanm added this to the 1.8 milestone Feb 18, 2020
@aanm aanm added pending-review and removed dont-merge/wait-until-release Freeze window for current release is blocking non-bugfix PRs dont-merge/waiting-for-upstream Only merge once upstream library or kernel features have landed labels Feb 18, 2020
aanm
aanm requested changes Feb 18, 2020
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.

Please run ./contrib/go-mod/update-vendor.sh for each commit as there are files being added in the vendor directory that are not required.

@aanm
Copy link
Member

aanm commented Feb 18, 2020

@iecedge ^^^

@Jianlin-lv
linux: Check policy routing of running kernel
2caaa97 
Check the payload of the NLMSG_DONE type netlink
message for possible error, then determine whether
CONFIG_IP_MULTIPLE_TABLES is enabled.

Signed-off-by: Jianlin Lv <Jianlin.Lv@arm.com>
@iecedge
Copy link
Author

iecedge commented Feb 19, 2020

Please run ./contrib/go-mod/update-vendor.sh for each commit as there are files being added in the vendor directory that are not required.

vendor have been update by #10138
This PR only keep code of checking policy routing

@aanm
Copy link
Member

aanm commented Feb 19, 2020

test-me-please

@aanm aanm merged commit 974f6c5 into cilium:master Feb 21, 2020
1.8.0 automation moved this from In progress to Merged Feb 21, 2020
@iecedge iecedge deleted the check_policy_ru branch February 26, 2020 01:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joestringer joestringer joestringer approved these changes

@aanm aanm aanm approved these changes

Assignees
No one assigned
Labels
release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.8.0
  
Merged
Milestone
1.8
Development

Successfully merging this pull request may close these issues.

Lost default route when deployment cilium on MACCHIATObin
5 participants
@iecedge @coveralls @joestringer @Jianlin-lv @aanm