[v1.15] route: Specify "proto kernel" for ip routes and rules #31777
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
maintainer-s-little-helper
bot
added
backport/1.15
|
/ci-ipsec-upgrade |
jschwinger233
force-pushed
the
gray/1.15/route-fix
branch
2 times, most recently
from
8c1da7e
to
144e080
Compare
jschwinger233
changed the title
|
/test-backport-1.15 |
pkg/proxy/routes.go
Outdated
Comment on lines
211
to
237
| // removeStaleProxyRulesIPv6 removes stale proxy rules. This is a v1.15 only function. | ||
| func removeStaleProxyRulesIPv6() error { | ||
| return removeProtoUnspecRules(netlink.FAMILY_V6) | ||
| } | ||
|
|
||
| // removeProtoUnspecRules removes all routing rules with protocol RTPROT_UNSPEC. This is a v1.15 only function. | ||
| func removeProtoUnspecRules(family int) error { | ||
| rules, err := route.ListRules(family, nil) | ||
| if err != nil { | ||
| return fmt.Errorf("listing routing rules: %w", err) | ||
| } | ||
| for _, r := range rules { | ||
| if r.Protocol == unix.RTPROT_UNSPEC { | ||
| if err := netlink.RuleDel(&r); err != nil { | ||
| if !errors.Is(err, syscall.ENOENT) && !errors.Is(err, syscall.EAFNOSUPPORT) { | ||
| return fmt.Errorf("removing proto unspec routing rule: %w", err) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Would it help for review to split the custom cleanup logic into a separate patch (and make the first patch a clean cherry-pick from upstream) ? Or is it crucial for bisectability that all these changes go in as one patch?
There was a problem hiding this comment.
Seems no harm to split into two patches. 1.15-old -> 1.15-tip upgrade is the only one that could be affected in terms of git bisect, but I don't see too many risks.
jschwinger233
force-pushed
the
gray/1.15/route-fix
branch
3 times, most recently
from
0d13ff3
to
9ca1a9a
Compare
|
/test-backport-1.15 |
rgo3
reviewed
Apr 8, 2024
jschwinger233
force-pushed
the
gray/1.15/route-fix
branch
from
9ca1a9a
to
c6cfac2
Compare
[ upstream commit: 318a648 ] v1.14 installs routes and rules with specific "proto kernel", v1.15 missed them. Without "proto kernel", it causes troubles when downgrade from v1.15 to v1.14, as v1.14 is deleting routes with "proto kernel" but there are no matching ones. Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com> Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
This commit adds "removeStaleProxyRulesIPvX()" which removes any ip rules with "proto unspec" to ensure upgrade/downgrade goes smoothly. Scenario 1: upgrade from v1.15-old to v1.15-tip v1.15-old cilium installs ip rules with "proto unspec", then v1.15-tip will install "duplicate" ip rules with "proto kernel". This is the moment when "removeStaleProxyRulesIPvX()" plays a role, it cleans those "proto unspec" stale rules without breaking connectivity. Scenario 2: downgrade from v1.15-tip to v1.15-old v1.15-tip has rules with "proto kernel". When v1.15-old tries to "ReplaceRule()" with "proto unspec", thanks to [this](https://github.com/cilium/cilium/blob/v1.15.3/pkg/datapath/linux/route/route_linux.go#L402), "ReplaceRule()" won't replace the rules because they already exist (with a different proto). This ensures connectivity can survive the downgrade too. Scenario 3: upgrade from v1.15-tip to v1.16 Since v1.15-tip installs correct rules with "proto kernel", v1.16 will do nothing after confirming existance by "lookupRule()". It should be painless as well. This is a v1.15-only commit because: 1. v1.14 is still using bpf/init.sh which sets rules with "proto kernel" properly; 2. v1.16 has been fixed to set "proto kernel"; 3. v1.15-tip -> v1.16 upgrade has been discussed above without any issue; Also please note that we don't have to clean up leftover ip routes with "proto unspec", because we replace them via "route.Upsert()" which replaces the old ones unconditionally, leaving no stale routes. Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>
jschwinger233
force-pushed
the
gray/1.15/route-fix
branch
from
c6cfac2
to
9796c77
Compare
|
/test-backport-1.15 |
rgo3
approved these changes
Apr 9, 2024
lmb
approved these changes
Apr 9, 2024
maintainer-s-little-helper
bot
added
the
ready-to-merge
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a customized backport to take care of upgrade/downgrade. Please see commit message for details.