From 26801d6541af414f213b1bbacac08c9052ff9af3 Mon Sep 17 00:00:00 2001 From: Antonio Ojea Date: Tue, 6 Jun 2023 10:20:14 +0000 Subject: [PATCH] kube-proxy avoid race condition using LocalModeNodeCIDR Since kube-proxy in LocalModeNodeCIDR needs to obtain the PodCIDR assigned to the node it watches for the Node object. However, kube-proxy startup process requires to have these watches in different places, that opens the possibility of having a race condition if the same node is recreated and a different PodCIDR is assigned. Initializing the second watch with the value obtained in the first one allows us to detect this situation. Change-Id: I6adeedb6914ad2afd3e0694dcab619c2a66135f8 Signed-off-by: Antonio Ojea --- cmd/kube-proxy/app/server.go | 4 +- cmd/kube-proxy/app/server_others.go | 1 + cmd/kube-proxy/app/server_others_test.go | 54 ++++++++++++++++++++++-- pkg/proxy/node.go | 6 +++ pkg/proxy/node_test.go | 5 +++ 5 files changed, 65 insertions(+), 5 deletions(-) diff --git a/cmd/kube-proxy/app/server.go b/cmd/kube-proxy/app/server.go index 78a9639f2d5..8c71657d30c 100644 --- a/cmd/kube-proxy/app/server.go +++ b/cmd/kube-proxy/app/server.go @@ -522,6 +522,8 @@ type ProxyServer struct { Hostname string NodeIP net.IP + podCIDRs []string // only used for LocalModeNodeCIDR + Proxier proxy.Provider } @@ -754,7 +756,7 @@ func (s *ProxyServer) Run() error { nodeConfig := config.NewNodeConfig(currentNodeInformerFactory.Core().V1().Nodes(), s.Config.ConfigSyncPeriod.Duration) // https://issues.k8s.io/111321 if s.Config.DetectLocalMode == kubeproxyconfig.LocalModeNodeCIDR { - nodeConfig.RegisterEventHandler(&proxy.NodePodCIDRHandler{}) + nodeConfig.RegisterEventHandler(proxy.NewNodePodCIDRHandler(s.podCIDRs)) } nodeConfig.RegisterEventHandler(s.Proxier) diff --git a/cmd/kube-proxy/app/server_others.go b/cmd/kube-proxy/app/server_others.go index 96b52495155..ac403b2e157 100644 --- a/cmd/kube-proxy/app/server_others.go +++ b/cmd/kube-proxy/app/server_others.go @@ -88,6 +88,7 @@ func (s *ProxyServer) createProxier(config *proxyconfigapi.KubeProxyConfiguratio if err != nil { return nil, err } + s.podCIDRs = nodeInfo.Spec.PodCIDRs klog.InfoS("NodeInfo", "podCIDR", nodeInfo.Spec.PodCIDR, "podCIDRs", nodeInfo.Spec.PodCIDRs) } diff --git a/cmd/kube-proxy/app/server_others_test.go b/cmd/kube-proxy/app/server_others_test.go index e595ba282ef..c087621a498 100644 --- a/cmd/kube-proxy/app/server_others_test.go +++ b/cmd/kube-proxy/app/server_others_test.go @@ -34,16 +34,14 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/watch" - netutils "k8s.io/utils/net" - "k8s.io/utils/pointer" - clientsetfake "k8s.io/client-go/kubernetes/fake" clientgotesting "k8s.io/client-go/testing" - proxyconfigapi "k8s.io/kubernetes/pkg/proxy/apis/config" proxyutiliptables "k8s.io/kubernetes/pkg/proxy/util/iptables" utiliptables "k8s.io/kubernetes/pkg/util/iptables" utiliptablestest "k8s.io/kubernetes/pkg/util/iptables/testing" + netutils "k8s.io/utils/net" + "k8s.io/utils/pointer" ) func Test_platformApplyDefaults(t *testing.T) { @@ -781,3 +779,51 @@ func TestGetConntrackMax(t *testing.T) { } } } + +func TestProxyServer_createProxier(t *testing.T) { + tests := []struct { + name string + node *v1.Node + config *proxyconfigapi.KubeProxyConfiguration + wantPodCIDRs []string + }{ + { + name: "LocalModeNodeCIDR store the node PodCIDRs obtained", + node: makeNodeWithPodCIDRs("10.0.0.0/24"), + config: &proxyconfigapi.KubeProxyConfiguration{DetectLocalMode: proxyconfigapi.LocalModeNodeCIDR}, + wantPodCIDRs: []string{"10.0.0.0/24"}, + }, + { + name: "LocalModeNodeCIDR store the node PodCIDRs obtained dual stack", + node: makeNodeWithPodCIDRs("10.0.0.0/24", "2001:db2:1/64"), + config: &proxyconfigapi.KubeProxyConfiguration{DetectLocalMode: proxyconfigapi.LocalModeNodeCIDR}, + wantPodCIDRs: []string{"10.0.0.0/24", "2001:db2:1/64"}, + }, + { + name: "LocalModeClusterCIDR does not get the node PodCIDRs", + node: makeNodeWithPodCIDRs("10.0.0.0/24", "2001:db2:1/64"), + config: &proxyconfigapi.KubeProxyConfiguration{DetectLocalMode: proxyconfigapi.LocalModeClusterCIDR}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + client := clientsetfake.NewSimpleClientset(tt.node) + s := &ProxyServer{ + Config: tt.config, + Client: client, + Hostname: "nodename", + NodeIP: netutils.ParseIPSloppy("127.0.0.1"), + } + _, err := s.createProxier(tt.config) + // TODO: mock the exec.Interface to not fail probing iptables + if (err != nil) && !strings.Contains(err.Error(), "iptables is not supported for primary IP family") { + t.Errorf("ProxyServer.createProxier() error = %v", err) + return + } + if !reflect.DeepEqual(s.podCIDRs, tt.wantPodCIDRs) { + t.Errorf("Expected PodCIDRs %v got %v", tt.wantPodCIDRs, s.podCIDRs) + } + + }) + } +} diff --git a/pkg/proxy/node.go b/pkg/proxy/node.go index f2cbf6b1f2d..1845818945a 100644 --- a/pkg/proxy/node.go +++ b/pkg/proxy/node.go @@ -33,6 +33,12 @@ type NodePodCIDRHandler struct { podCIDRs []string } +func NewNodePodCIDRHandler(podCIDRs []string) *NodePodCIDRHandler { + return &NodePodCIDRHandler{ + podCIDRs: podCIDRs, + } +} + var _ config.NodeHandler = &NodePodCIDRHandler{} // OnNodeAdd is a handler for Node creates. diff --git a/pkg/proxy/node_test.go b/pkg/proxy/node_test.go index ab20130b033..2f6d7b54ad7 100644 --- a/pkg/proxy/node_test.go +++ b/pkg/proxy/node_test.go @@ -37,6 +37,11 @@ func TestNodePodCIDRHandlerAdd(t *testing.T) { name: "initialized correctly", newNodePodCIDRs: []string{"192.168.1.0/24", "fd00:1:2:3::/64"}, }, + { + name: "already initialized and same node", + oldNodePodCIDRs: []string{"10.0.0.0/24", "fd00:3:2:1::/64"}, + newNodePodCIDRs: []string{"10.0.0.0/24", "fd00:3:2:1::/64"}, + }, { name: "already initialized and different node", oldNodePodCIDRs: []string{"192.168.1.0/24", "fd00:1:2:3::/64"},