nat: Create SNAT maps only if BPF NodePort is enabled #15175
Merged
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
pchaigno
added
kind/bug
pchaigno
force-pushed
the
fix-snat-map-controller
branch
from
1b91da6
to
09e72f9
Compare
aanm
reviewed
Mar 9, 2021
pchaigno
force-pushed
the
fix-snat-map-controller
branch
from
09e72f9
to
4915d75
Compare
joestringer
approved these changes
Mar 9, 2021
jibi
approved these changes
Mar 9, 2021
christarazi
approved these changes
Mar 9, 2021
Using the NatMap interface was causing issues when trying to check if the natMap attribute is nil. We don't need the NatMap interface and can simply use *nat.Map directly. Suggested-by: Tobias Klauser <tklauser@distanz.ch> Signed-off-by: Paul Chaignon <paul@cilium.io>
Initialize the kube-proxy replacement options earlier in the agent startup, to ensure all options have reach their final state (based on kernel probes and option conflict) when we initialize the BPF map information (e.g., create or not NAT maps based on whether BPF NodePort is enabled). Signed-off-by: Paul Chaignon <paul@cilium.io>
The IPv4 and IPv6 SNAT maps are only used if BPF NodePort is enabled. Commit cac5218 ("datapath: remove SNAT maps entries when kube-proxy is enabled") removed the maps on startup if BPF NodePort is disabled. We were however still creating them regardless of the BPF NodePort status. The creation started a controller which then fails once the actual map is removed. This commit fixes the issue by not creating the userspace object, including the controller, for the SNAT maps when BPF NodePort is disabled. Fixes: cac5218 ("datapath: remove SNAT maps entries when kube-proxy is enabled") Fixes: cilium#15141 Signed-off-by: Paul Chaignon <paul@cilium.io>
pchaigno
force-pushed
the
fix-snat-map-controller
branch
from
4915d75
to
d639905
Compare
|
test-me-please |
maintainer-s-little-helper
bot
moved this from Needs backport from master
to Backport pending to v1.9
in 1.9.5
maintainer-s-little-helper
bot
moved this from Needs backport from master
to Backport pending to v1.9
in 1.9.5
kkourt
added a commit
that referenced
this pull request
Apr 8, 2021
PR #14721 introduced changes that removed the cilium_snat_v{4,6}_external maps if NodePort is not enabled. Issue #15141 was attributed to the above PR, where controllers were failing with: > error=Unable to get object /sys/fs/bpf/tc/globals/cilium_snat_v4_external: no such file or directory name=bpf-map-sync-cilium_snat_v4_external In an attempt to fix #15141, #15175 added a nodeport argument to InitMapInfo() so that the cilium_snat_v{4,6}_external maps are not created if NodePort is not enabled. PR #15141 did not fix the issue: #15337 (comment) This PR takes another shot at fixing #15141 by removing a call of InitMapInfo from init(), where the (new) nodeport argument is always true. Not that in init(), option.Config.EnableNodePort is not properly set yet, so we cannot pass the config option because it would always be false. For this change to properly work, this patch also adds explicit InitMapInfo() calls since removing init() means that this function is called in contexts such as tests and the cli. Fixes: cac5218 Fixes: d639905 Fixes: #15141 Fixes: #15337 Signed-off-by: Kornilios Kourtis <kornilios@isovalent.com>
pchaigno
pushed a commit
that referenced
this pull request
Apr 9, 2021
PR #14721 introduced changes that removed the cilium_snat_v{4,6}_external maps if NodePort is not enabled. Issue #15141 was attributed to the above PR, where controllers were failing with: > error=Unable to get object /sys/fs/bpf/tc/globals/cilium_snat_v4_external: no such file or directory name=bpf-map-sync-cilium_snat_v4_external In an attempt to fix #15141, #15175 added a nodeport argument to InitMapInfo() so that the cilium_snat_v{4,6}_external maps are not created if NodePort is not enabled. PR #15141 did not fix the issue: #15337 (comment) This PR takes another shot at fixing #15141 by removing a call of InitMapInfo from init(), where the (new) nodeport argument is always true. Not that in init(), option.Config.EnableNodePort is not properly set yet, so we cannot pass the config option because it would always be false. For this change to properly work, this patch also adds explicit InitMapInfo() calls since removing init() means that this function is called in contexts such as tests and the cli. Fixes: cac5218 Fixes: d639905 Fixes: #15141 Fixes: #15337 Signed-off-by: Kornilios Kourtis <kornilios@isovalent.com>
jibi
pushed a commit
that referenced
this pull request
Apr 13, 2021
[ upstream commit e83dd53 ] PR #14721 introduced changes that removed the cilium_snat_v{4,6}_external maps if NodePort is not enabled. Issue #15141 was attributed to the above PR, where controllers were failing with: > error=Unable to get object /sys/fs/bpf/tc/globals/cilium_snat_v4_external: no such file or directory name=bpf-map-sync-cilium_snat_v4_external In an attempt to fix #15141, #15175 added a nodeport argument to InitMapInfo() so that the cilium_snat_v{4,6}_external maps are not created if NodePort is not enabled. PR #15141 did not fix the issue: #15337 (comment) This PR takes another shot at fixing #15141 by removing a call of InitMapInfo from init(), where the (new) nodeport argument is always true. Not that in init(), option.Config.EnableNodePort is not properly set yet, so we cannot pass the config option because it would always be false. For this change to properly work, this patch also adds explicit InitMapInfo() calls since removing init() means that this function is called in contexts such as tests and the cli. Fixes: cac5218 Fixes: d639905 Fixes: #15141 Fixes: #15337 Signed-off-by: Kornilios Kourtis <kornilios@isovalent.com> Signed-off-by: Gilberto Bertin <gilberto@isovalent.com>
qmonnet
pushed a commit
that referenced
this pull request
Apr 14, 2021
[ upstream commit e83dd53 ] PR #14721 introduced changes that removed the cilium_snat_v{4,6}_external maps if NodePort is not enabled. Issue #15141 was attributed to the above PR, where controllers were failing with: > error=Unable to get object /sys/fs/bpf/tc/globals/cilium_snat_v4_external: no such file or directory name=bpf-map-sync-cilium_snat_v4_external In an attempt to fix #15141, #15175 added a nodeport argument to InitMapInfo() so that the cilium_snat_v{4,6}_external maps are not created if NodePort is not enabled. PR #15141 did not fix the issue: #15337 (comment) This PR takes another shot at fixing #15141 by removing a call of InitMapInfo from init(), where the (new) nodeport argument is always true. Not that in init(), option.Config.EnableNodePort is not properly set yet, so we cannot pass the config option because it would always be false. For this change to properly work, this patch also adds explicit InitMapInfo() calls since removing init() means that this function is called in contexts such as tests and the cli. Fixes: cac5218 Fixes: d639905 Fixes: #15141 Fixes: #15337 Signed-off-by: Kornilios Kourtis <kornilios@isovalent.com> Signed-off-by: Gilberto Bertin <gilberto@isovalent.com>
christarazi
pushed a commit
to christarazi/cilium
that referenced
this pull request
Apr 28, 2021
[ upstream commit e83dd53 ] PR cilium#14721 introduced changes that removed the cilium_snat_v{4,6}_external maps if NodePort is not enabled. Issue cilium#15141 was attributed to the above PR, where controllers were failing with: > error=Unable to get object /sys/fs/bpf/tc/globals/cilium_snat_v4_external: no such file or directory name=bpf-map-sync-cilium_snat_v4_external In an attempt to fix cilium#15141, cilium#15175 added a nodeport argument to InitMapInfo() so that the cilium_snat_v{4,6}_external maps are not created if NodePort is not enabled. PR cilium#15141 did not fix the issue: cilium#15337 (comment) This PR takes another shot at fixing cilium#15141 by removing a call of InitMapInfo from init(), where the (new) nodeport argument is always true. Not that in init(), option.Config.EnableNodePort is not properly set yet, so we cannot pass the config option because it would always be false. For this change to properly work, this patch also adds explicit InitMapInfo() calls since removing init() means that this function is called in contexts such as tests and the cli. Fixes: cac5218 Fixes: d639905 Fixes: cilium#15141 Fixes: cilium#15337 Signed-off-by: Kornilios Kourtis <kornilios@isovalent.com> Signed-off-by: Chris Tarazi <chris@isovalent.com>
ti-mo
pushed a commit
that referenced
this pull request
May 4, 2021
[ upstream commit e83dd53 ] PR #14721 introduced changes that removed the cilium_snat_v{4,6}_external maps if NodePort is not enabled. Issue #15141 was attributed to the above PR, where controllers were failing with: > error=Unable to get object /sys/fs/bpf/tc/globals/cilium_snat_v4_external: no such file or directory name=bpf-map-sync-cilium_snat_v4_external In an attempt to fix #15141, #15175 added a nodeport argument to InitMapInfo() so that the cilium_snat_v{4,6}_external maps are not created if NodePort is not enabled. PR #15141 did not fix the issue: #15337 (comment) This PR takes another shot at fixing #15141 by removing a call of InitMapInfo from init(), where the (new) nodeport argument is always true. Not that in init(), option.Config.EnableNodePort is not properly set yet, so we cannot pass the config option because it would always be false. For this change to properly work, this patch also adds explicit InitMapInfo() calls since removing init() means that this function is called in contexts such as tests and the cli. Fixes: cac5218 Fixes: d639905 Fixes: #15141 Fixes: #15337 Signed-off-by: Kornilios Kourtis <kornilios@isovalent.com> Signed-off-by: Chris Tarazi <chris@isovalent.com>
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.
The IPv4 and IPv6 SNAT maps are only used if BPF NodePort is enabled. #14721 removed the maps on startup if BPF NodePort is disabled.
We were however still creating them regardless of the BPF NodePort status. The creation started a controller which then fails once the actual map is removed. This commit fixes the issue by not creating the userspace object, including the controller, for the SNAT maps when BPF NodePort is disabled.
See commits for details.
Fixes: #14721
Fixes: #15141