-
Notifications
You must be signed in to change notification settings - Fork 3.4k
bpf: remap MARK_MAGIC_SNAT_DONE marker to avoid conflicts #11008
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.
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
Merged
Conversation
This file contains hidden or 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
test-me-please |
test-focus K8sService.* |
Commit f25d8b9 ("bpf: Preserve source identity for hairpin via stack") recently broke BPF NodePort since the use of MARK_MAGIC_SNAT_DONE is now in conflict/overlapping with MARK_MAGIC_IDENTITY. In tc egress hooks of bpf_{netdev,overlay} we have a check whether the SNAT is not needed via if ((ctx->mark & MARK_MAGIC_SNAT_DONE) == MARK_MAGIC_SNAT_DONE). MARK_MAGIC_SNAT_DONE is 0x0500 whereas MARK_MAGIC_IDENTITY is 0x0F00. So far it was never a problem since MARK_MAGIC_IDENTITY was not used in a path where MARK_MAGIC_SNAT_DONE was tested until this changed via f25d8b9 where now SNAT was skipped for Pod traffic. As a result, we've seen various flakes in SNAT or Hybrid NodePort setups mostly on the tftp/UDP tests. Reverting f25d8b9 confirmed to fix these flakes. As a workaround/fix, remap MARK_MAGIC_SNAT_DONE to 0x1500, so that setting the MARK_MAGIC_IDENTITY will get a mismatch on above mentioned test and hence we'll end up doing the SNAT. This reaches into MARK_MAGIC_KEY_ID bit space, but the agent today cannot be configured to run with both BPF NodePort and IPSec at the same time. Fixes: #10942 Fixes: f25d8b9 ("bpf: Preserve source identity for hairpin via stack") Reported-by: Martynas Pumputis <m@lambda.lt> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
f83714d
to
f89a91a
Compare
test-me-please |
test-focus K8sService.* |
test-gke |
brb
approved these changes
Apr 16, 2020
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! We should revisit the mark once we start supporting NodePort BPF + transparent encryption.
tgraf
approved these changes
Apr 16, 2020
(Needs to be backported where also #10926 gets backported.) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
area/datapath
Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
kind/bug
This is a bug in the Cilium logic.
release-note/misc
This PR makes changes that have no direct user impact.
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.
See commit msg.
Fixes: #10942
Fixes: f25d8b9 ("bpf: Preserve source identity for hairpin via stack")
Reported-by: Martynas Pumputis m@lambda.lt
Signed-off-by: Daniel Borkmann daniel@iogearbox.net