From c4575c34382997ca377ab9565a1a03bd3363fc63 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Sun, 12 Mar 2023 17:31:10 -0400 Subject: [PATCH] Fix up detect-local-mode validation Validate the --detect-local-mode value in the API object validation rather than doing it separately later. Also, remove runtime checks and unit tests for cases that would be blocked by validation --- cmd/kube-proxy/app/server_others.go | 32 ++----- cmd/kube-proxy/app/server_others_test.go | 96 +------------------ .../apis/config/validation/validation.go | 18 ++++ .../apis/config/validation/validation_test.go | 22 +++++ 4 files changed, 51 insertions(+), 117 deletions(-) diff --git a/cmd/kube-proxy/app/server_others.go b/cmd/kube-proxy/app/server_others.go index 9f8d4bed03f..b731a413f9d 100644 --- a/cmd/kube-proxy/app/server_others.go +++ b/cmd/kube-proxy/app/server_others.go @@ -122,13 +122,9 @@ func newProxyServer( } var proxier proxy.Provider - var detectLocalMode proxyconfigapi.LocalMode proxyMode := getProxyMode(config.Mode) - detectLocalMode, err = getDetectLocalMode(config) - if err != nil { - return nil, fmt.Errorf("cannot determine detect-local-mode: %v", err) - } + detectLocalMode := getDetectLocalMode(config) var nodeInfo *v1.Node if detectLocalMode == proxyconfigapi.LocalModeNodeCIDR { @@ -400,23 +396,19 @@ func detectNumCPU() int { return numCPU } -func getDetectLocalMode(config *proxyconfigapi.KubeProxyConfiguration) (proxyconfigapi.LocalMode, error) { - mode := config.DetectLocalMode - switch mode { - case proxyconfigapi.LocalModeClusterCIDR, proxyconfigapi.LocalModeNodeCIDR, proxyconfigapi.LocalModeBridgeInterface, proxyconfigapi.LocalModeInterfaceNamePrefix: - return mode, nil - default: - if strings.TrimSpace(mode.String()) != "" { - return mode, fmt.Errorf("unknown detect-local-mode: %v", mode) - } +func getDetectLocalMode(config *proxyconfigapi.KubeProxyConfiguration) proxyconfigapi.LocalMode { + if config.DetectLocalMode == "" { klog.V(4).InfoS("Defaulting detect-local-mode", "localModeClusterCIDR", string(proxyconfigapi.LocalModeClusterCIDR)) - return proxyconfigapi.LocalModeClusterCIDR, nil + return proxyconfigapi.LocalModeClusterCIDR } + return config.DetectLocalMode } func getLocalDetector(mode proxyconfigapi.LocalMode, config *proxyconfigapi.KubeProxyConfiguration, ipt utiliptables.Interface, nodeInfo *v1.Node) (proxyutiliptables.LocalTrafficDetector, error) { switch mode { case proxyconfigapi.LocalModeClusterCIDR: + // LocalModeClusterCIDR is the default if --detect-local-mode wasn't passed, + // but --cluster-cidr is optional. if len(strings.TrimSpace(config.ClusterCIDR)) == 0 { klog.InfoS("Detect-local-mode set to ClusterCIDR, but no cluster CIDR defined") break @@ -429,14 +421,8 @@ func getLocalDetector(mode proxyconfigapi.LocalMode, config *proxyconfigapi.Kube } return proxyutiliptables.NewDetectLocalByCIDR(nodeInfo.Spec.PodCIDR, ipt) case proxyconfigapi.LocalModeBridgeInterface: - if len(strings.TrimSpace(config.DetectLocal.BridgeInterface)) == 0 { - return nil, fmt.Errorf("Detect-local-mode set to BridgeInterface, but no bridge-interface-name %s is defined", config.DetectLocal.BridgeInterface) - } return proxyutiliptables.NewDetectLocalByBridgeInterface(config.DetectLocal.BridgeInterface) case proxyconfigapi.LocalModeInterfaceNamePrefix: - if len(strings.TrimSpace(config.DetectLocal.InterfaceNamePrefix)) == 0 { - return nil, fmt.Errorf("Detect-local-mode set to InterfaceNamePrefix, but no interface-prefix %s is defined", config.DetectLocal.InterfaceNamePrefix) - } return proxyutiliptables.NewDetectLocalByInterfaceNamePrefix(config.DetectLocal.InterfaceNamePrefix) } klog.InfoS("Defaulting to no-op detect-local", "detectLocalMode", string(mode)) @@ -448,6 +434,8 @@ func getDualStackLocalDetectorTuple(mode proxyconfigapi.LocalMode, config *proxy localDetectors := [2]proxyutiliptables.LocalTrafficDetector{proxyutiliptables.NewNoOpLocalDetector(), proxyutiliptables.NewNoOpLocalDetector()} switch mode { case proxyconfigapi.LocalModeClusterCIDR: + // LocalModeClusterCIDR is the default if --detect-local-mode wasn't passed, + // but --cluster-cidr is optional. if len(strings.TrimSpace(config.ClusterCIDR)) == 0 { klog.InfoS("Detect-local-mode set to ClusterCIDR, but no cluster CIDR defined") break @@ -471,7 +459,7 @@ func getDualStackLocalDetectorTuple(mode proxyconfigapi.LocalMode, config *proxy } return localDetectors, err case proxyconfigapi.LocalModeNodeCIDR: - if nodeInfo == nil || len(strings.TrimSpace(nodeInfo.Spec.PodCIDR)) == 0 { + if len(strings.TrimSpace(nodeInfo.Spec.PodCIDR)) == 0 { klog.InfoS("No node info available to configure detect-local-mode NodeCIDR") break } diff --git a/cmd/kube-proxy/app/server_others_test.go b/cmd/kube-proxy/app/server_others_test.go index a1f3f691180..0640b20d28e 100644 --- a/cmd/kube-proxy/app/server_others_test.go +++ b/cmd/kube-proxy/app/server_others_test.go @@ -48,47 +48,27 @@ func Test_getDetectLocalMode(t *testing.T) { cases := []struct { detectLocal string expected proxyconfigapi.LocalMode - errExpected bool }{ { detectLocal: "", expected: proxyconfigapi.LocalModeClusterCIDR, - errExpected: false, }, { detectLocal: string(proxyconfigapi.LocalModeClusterCIDR), expected: proxyconfigapi.LocalModeClusterCIDR, - errExpected: false, }, { detectLocal: string(proxyconfigapi.LocalModeInterfaceNamePrefix), expected: proxyconfigapi.LocalModeInterfaceNamePrefix, - errExpected: false, }, { detectLocal: string(proxyconfigapi.LocalModeBridgeInterface), expected: proxyconfigapi.LocalModeBridgeInterface, - errExpected: false, - }, - { - detectLocal: "abcd", - expected: proxyconfigapi.LocalMode("abcd"), - errExpected: true, }, } for i, c := range cases { proxyConfig := &proxyconfigapi.KubeProxyConfiguration{DetectLocalMode: proxyconfigapi.LocalMode(c.detectLocal)} - r, err := getDetectLocalMode(proxyConfig) - if c.errExpected { - if err == nil { - t.Errorf("Expected error, but did not fail for mode %v", c.detectLocal) - } - continue - } - if err != nil { - t.Errorf("Got error parsing mode: %v", err) - continue - } + r := getDetectLocalMode(proxyConfig) if r != c.expected { t.Errorf("Case[%d] Expected %q got %q", i, c.expected, r) } @@ -224,20 +204,6 @@ func Test_getLocalDetector(t *testing.T) { expected: resolveLocalDetector(t)(proxyutiliptables.NewDetectLocalByCIDR("2002::1234:abcd:ffff:c0a8:101/64", utiliptablestest.NewIPv6Fake())), errExpected: false, }, - { - mode: proxyconfigapi.LocalModeClusterCIDR, - config: &proxyconfigapi.KubeProxyConfiguration{ClusterCIDR: "10.0.0.0"}, - ipt: utiliptablestest.NewFake(), - expected: nil, - errExpected: true, - }, - { - mode: proxyconfigapi.LocalModeClusterCIDR, - config: &proxyconfigapi.KubeProxyConfiguration{ClusterCIDR: "2002::1234:abcd:ffff:c0a8:101"}, - ipt: utiliptablestest.NewIPv6Fake(), - expected: nil, - errExpected: true, - }, { mode: proxyconfigapi.LocalModeClusterCIDR, config: &proxyconfigapi.KubeProxyConfiguration{ClusterCIDR: "10.0.0.0/14"}, @@ -276,22 +242,6 @@ func Test_getLocalDetector(t *testing.T) { nodeInfo: makeNodeWithPodCIDRs("2002::1234:abcd:ffff:c0a8:101/96"), errExpected: false, }, - { - mode: proxyconfigapi.LocalModeNodeCIDR, - config: &proxyconfigapi.KubeProxyConfiguration{ClusterCIDR: "10.0.0.0"}, - ipt: utiliptablestest.NewFake(), - expected: nil, - nodeInfo: makeNodeWithPodCIDRs("10.0.0.0"), - errExpected: true, - }, - { - mode: proxyconfigapi.LocalModeNodeCIDR, - config: &proxyconfigapi.KubeProxyConfiguration{ClusterCIDR: "2002::1234:abcd:ffff:c0a8:101"}, - ipt: utiliptablestest.NewIPv6Fake(), - expected: nil, - nodeInfo: makeNodeWithPodCIDRs("2002::1234:abcd:ffff:c0a8:101"), - errExpected: true, - }, { mode: proxyconfigapi.LocalModeNodeCIDR, config: &proxyconfigapi.KubeProxyConfiguration{ClusterCIDR: "10.0.0.0/14"}, @@ -333,13 +283,6 @@ func Test_getLocalDetector(t *testing.T) { expected: resolveLocalDetector(t)(proxyutiliptables.NewDetectLocalByBridgeInterface("eth")), errExpected: false, }, - { - mode: proxyconfigapi.LocalModeBridgeInterface, - config: &proxyconfigapi.KubeProxyConfiguration{ - DetectLocal: proxyconfigapi.DetectLocalConfiguration{BridgeInterface: ""}, - }, - errExpected: true, - }, { mode: proxyconfigapi.LocalModeBridgeInterface, config: &proxyconfigapi.KubeProxyConfiguration{ @@ -357,13 +300,6 @@ func Test_getLocalDetector(t *testing.T) { expected: resolveLocalDetector(t)(proxyutiliptables.NewDetectLocalByInterfaceNamePrefix("eth")), errExpected: false, }, - { - mode: proxyconfigapi.LocalModeInterfaceNamePrefix, - config: &proxyconfigapi.KubeProxyConfiguration{ - DetectLocal: proxyconfigapi.DetectLocalConfiguration{InterfaceNamePrefix: ""}, - }, - errExpected: true, - }, { mode: proxyconfigapi.LocalModeInterfaceNamePrefix, config: &proxyconfigapi.KubeProxyConfiguration{ @@ -493,22 +429,6 @@ func Test_getDualStackLocalDetectorTuple(t *testing.T) { nodeInfo: makeNodeWithPodCIDRs(), errExpected: false, }, - { - mode: proxyconfigapi.LocalModeNodeCIDR, - config: &proxyconfigapi.KubeProxyConfiguration{ClusterCIDR: ""}, - ipt: [2]utiliptables.Interface{utiliptablestest.NewFake(), utiliptablestest.NewIPv6Fake()}, - expected: [2]proxyutiliptables.LocalTrafficDetector{proxyutiliptables.NewNoOpLocalDetector(), proxyutiliptables.NewNoOpLocalDetector()}, - nodeInfo: nil, - errExpected: false, - }, - // unknown mode, nodeInfo would be nil for these cases - { - mode: proxyconfigapi.LocalMode("abcd"), - config: &proxyconfigapi.KubeProxyConfiguration{ClusterCIDR: ""}, - ipt: [2]utiliptables.Interface{utiliptablestest.NewFake(), utiliptablestest.NewIPv6Fake()}, - expected: [2]proxyutiliptables.LocalTrafficDetector{proxyutiliptables.NewNoOpLocalDetector(), proxyutiliptables.NewNoOpLocalDetector()}, - errExpected: false, - }, // LocalModeBridgeInterface, nodeInfo and ipt are not needed for these cases { mode: proxyconfigapi.LocalModeBridgeInterface, @@ -520,13 +440,6 @@ func Test_getDualStackLocalDetectorTuple(t *testing.T) { proxyutiliptables.NewDetectLocalByBridgeInterface("eth")), errExpected: false, }, - { - mode: proxyconfigapi.LocalModeBridgeInterface, - config: &proxyconfigapi.KubeProxyConfiguration{ - DetectLocal: proxyconfigapi.DetectLocalConfiguration{BridgeInterface: ""}, - }, - errExpected: true, - }, // LocalModeInterfaceNamePrefix, nodeInfo and ipt are not needed for these cases { mode: proxyconfigapi.LocalModeInterfaceNamePrefix, @@ -538,13 +451,6 @@ func Test_getDualStackLocalDetectorTuple(t *testing.T) { proxyutiliptables.NewDetectLocalByInterfaceNamePrefix("veth")), errExpected: false, }, - { - mode: proxyconfigapi.LocalModeInterfaceNamePrefix, - config: &proxyconfigapi.KubeProxyConfiguration{ - DetectLocal: proxyconfigapi.DetectLocalConfiguration{InterfaceNamePrefix: ""}, - }, - errExpected: true, - }, } for i, c := range cases { r, err := getDualStackLocalDetectorTuple(c.mode, c.config, c.ipt, c.nodeInfo) diff --git a/pkg/proxy/apis/config/validation/validation.go b/pkg/proxy/apis/config/validation/validation.go index 607a5928aaa..e487e826037 100644 --- a/pkg/proxy/apis/config/validation/validation.go +++ b/pkg/proxy/apis/config/validation/validation.go @@ -95,6 +95,8 @@ func Validate(config *kubeproxyconfig.KubeProxyConfiguration) field.ErrorList { allErrs = append(allErrs, validateKubeProxyNodePortAddress(config.NodePortAddresses, newPath.Child("NodePortAddresses"))...) allErrs = append(allErrs, validateShowHiddenMetricsVersion(config.ShowHiddenMetricsForVersion, newPath.Child("ShowHiddenMetricsForVersion"))...) + + allErrs = append(allErrs, validateDetectLocalMode(config.DetectLocalMode, newPath.Child("DetectLocalMode"))...) if config.DetectLocalMode == kubeproxyconfig.LocalModeBridgeInterface { allErrs = append(allErrs, validateInterface(config.DetectLocal.BridgeInterface, newPath.Child("InterfaceName"))...) } @@ -205,6 +207,22 @@ func validateProxyModeWindows(mode kubeproxyconfig.ProxyMode, fldPath *field.Pat return field.ErrorList{field.Invalid(fldPath.Child("ProxyMode"), string(mode), errMsg)} } +func validateDetectLocalMode(mode kubeproxyconfig.LocalMode, fldPath *field.Path) field.ErrorList { + validModes := []string{ + string(kubeproxyconfig.LocalModeClusterCIDR), + string(kubeproxyconfig.LocalModeNodeCIDR), + string(kubeproxyconfig.LocalModeBridgeInterface), + string(kubeproxyconfig.LocalModeInterfaceNamePrefix), + "", + } + + if sets.New(validModes...).Has(string(mode)) { + return nil + } + + return field.ErrorList{field.NotSupported(fldPath, string(mode), validModes)} +} + func validateClientConnectionConfiguration(config componentbaseconfig.ClientConnectionConfiguration, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} allErrs = append(allErrs, apivalidation.ValidateNonnegativeField(int64(config.Burst), fldPath.Child("Burst"))...) diff --git a/pkg/proxy/apis/config/validation/validation_test.go b/pkg/proxy/apis/config/validation/validation_test.go index 8810bc4f1ce..ea040622378 100644 --- a/pkg/proxy/apis/config/validation/validation_test.go +++ b/pkg/proxy/apis/config/validation/validation_test.go @@ -414,6 +414,28 @@ func TestValidateKubeProxyConfiguration(t *testing.T) { }, expectedErrs: field.ErrorList{field.Invalid(newPath.Child("InterfaceName"), "", "must not be empty")}, }, + "invalid DetectLocalMode": { + config: kubeproxyconfig.KubeProxyConfiguration{ + BindAddress: "10.10.12.11", + HealthzBindAddress: "0.0.0.0:12345", + MetricsBindAddress: "127.0.0.1:10249", + ClusterCIDR: "192.168.59.0/24", + ConfigSyncPeriod: metav1.Duration{Duration: 1 * time.Second}, + IPTables: kubeproxyconfig.KubeProxyIPTablesConfiguration{ + MasqueradeAll: true, + SyncPeriod: metav1.Duration{Duration: 5 * time.Second}, + MinSyncPeriod: metav1.Duration{Duration: 2 * time.Second}, + }, + Conntrack: kubeproxyconfig.KubeProxyConntrackConfiguration{ + MaxPerCore: pointer.Int32(1), + Min: pointer.Int32(1), + TCPEstablishedTimeout: &metav1.Duration{Duration: 5 * time.Second}, + TCPCloseWaitTimeout: &metav1.Duration{Duration: 5 * time.Second}, + }, + DetectLocalMode: "Guess", + }, + expectedErrs: field.ErrorList{field.NotSupported(newPath.Child("DetectLocalMode"), "Guess", []string{"ClusterCIDR", "NodeCIDR", "BridgeInterface", "InterfaceNamePrefix", ""})}, + }, } for name, testCase := range testCases {