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
This commit is contained in:
Dan Winship 2023-03-12 17:31:10 -04:00
parent 3734fe7ab1
commit c4575c3438
4 changed files with 51 additions and 117 deletions

View File

@ -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
}

View File

@ -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)

View File

@ -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"))...)

View File

@ -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 {