From 3734fe7ab1dc1b3d43abf0f22dd31f66becd568f Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Thu, 13 Apr 2023 19:50:26 -0400 Subject: [PATCH 1/4] Remove some useless error checks Don't return errors in the event of programmer errors, and don't double-check things that were already validated. --- cmd/kube-proxy/app/server_others.go | 8 -------- cmd/kube-proxy/app/server_windows.go | 4 ---- 2 files changed, 12 deletions(-) diff --git a/cmd/kube-proxy/app/server_others.go b/cmd/kube-proxy/app/server_others.go index c4121feb989..9f8d4bed03f 100644 --- a/cmd/kube-proxy/app/server_others.go +++ b/cmd/kube-proxy/app/server_others.go @@ -79,10 +79,6 @@ func newProxyServer( config *proxyconfigapi.KubeProxyConfiguration, master string) (*ProxyServer, error) { - if config == nil { - return nil, errors.New("config is required") - } - if c, err := configz.New(proxyconfigapi.GroupName); err == nil { c.Set(config) } else { @@ -188,10 +184,6 @@ func newProxyServer( if proxyMode == proxyconfigapi.ProxyModeIPTables { klog.InfoS("Using iptables Proxier") - if config.IPTables.MasqueradeBit == nil { - // MasqueradeBit must be specified or defaulted. - return nil, fmt.Errorf("unable to read IPTables MasqueradeBit from config") - } if dualStack { klog.InfoS("kube-proxy running in dual-stack mode", "ipFamily", iptInterface.Protocol()) diff --git a/cmd/kube-proxy/app/server_windows.go b/cmd/kube-proxy/app/server_windows.go index a6ba3eff5ce..02eccb8ef31 100644 --- a/cmd/kube-proxy/app/server_windows.go +++ b/cmd/kube-proxy/app/server_windows.go @@ -51,10 +51,6 @@ func NewProxyServer(o *Options) (*ProxyServer, error) { } func newProxyServer(config *proxyconfigapi.KubeProxyConfiguration, master string) (*ProxyServer, error) { - if config == nil { - return nil, errors.New("config is required") - } - if c, err := configz.New(proxyconfigapi.GroupName); err == nil { c.Set(config) } else { From c4575c34382997ca377ab9565a1a03bd3363fc63 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Sun, 12 Mar 2023 17:31:10 -0400 Subject: [PATCH 2/4] 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 { From 10a869fc752ca7bdf48625300056ce9179a0da65 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Sun, 12 Mar 2023 17:37:50 -0400 Subject: [PATCH 3/4] Remove duplicated config fields from ProxyServer Rather than duplicating some of the KubeProxyConfiguration into ProxyServer, just store the KubeProxyConfiguration itself so later code can reference it directly. For the fields that get platform-specific defaults (Mode, DetectLocalMode), fill the defaults directly into the KubeProxyConfiguration rather than keeping the original there and the defaulted version in the ProxyServer. --- cmd/kube-proxy/app/server.go | 57 +++++++++---------- cmd/kube-proxy/app/server_others.go | 72 +++++++++--------------- cmd/kube-proxy/app/server_others_test.go | 68 ++++++++++++++++------ cmd/kube-proxy/app/server_windows.go | 26 ++++----- pkg/proxy/kubemark/hollow_proxy.go | 21 ++++--- 5 files changed, 131 insertions(+), 113 deletions(-) diff --git a/cmd/kube-proxy/app/server.go b/cmd/kube-proxy/app/server.go index 3503f585fe0..2aa6d5a8244 100644 --- a/cmd/kube-proxy/app/server.go +++ b/cmd/kube-proxy/app/server.go @@ -236,6 +236,8 @@ func (o *Options) Complete() error { } } + o.platformApplyDefaults(o.config) + if err := o.processHostnameOverrideFlag(); err != nil { return err } @@ -522,21 +524,16 @@ with the apiserver API to configure the proxy.`, // ProxyServer represents all the parameters required to start the Kubernetes proxy server. All // fields are required. type ProxyServer struct { - Client clientset.Interface - Proxier proxy.Provider - Broadcaster events.EventBroadcaster - Recorder events.EventRecorder - ConntrackConfiguration kubeproxyconfig.KubeProxyConntrackConfiguration - Conntracker Conntracker // if nil, ignored - ProxyMode kubeproxyconfig.ProxyMode - NodeRef *v1.ObjectReference - MetricsBindAddress string - BindAddressHardFail bool - EnableProfiling bool - OOMScoreAdj *int32 - ConfigSyncPeriod time.Duration - HealthzServer healthcheck.ProxierHealthUpdater - localDetectorMode kubeproxyconfig.LocalMode + Config *kubeproxyconfig.KubeProxyConfiguration + + Client clientset.Interface + Broadcaster events.EventBroadcaster + Recorder events.EventRecorder + Conntracker Conntracker // if nil, ignored + NodeRef *v1.ObjectReference + HealthzServer healthcheck.ProxierHealthUpdater + + Proxier proxy.Provider } // createClient creates a kube client from the given config and masterOverride. @@ -645,9 +642,9 @@ func (s *ProxyServer) Run() error { // TODO(vmarmol): Use container config for this. var oomAdjuster *oom.OOMAdjuster - if s.OOMScoreAdj != nil { + if s.Config.OOMScoreAdj != nil { oomAdjuster = oom.NewOOMAdjuster() - if err := oomAdjuster.ApplyOOMScoreAdj(0, int(*s.OOMScoreAdj)); err != nil { + if err := oomAdjuster.ApplyOOMScoreAdj(0, int(*s.Config.OOMScoreAdj)); err != nil { klog.V(2).InfoS("Failed to apply OOMScore", "err", err) } } @@ -660,7 +657,7 @@ func (s *ProxyServer) Run() error { // TODO(thockin): make it possible for healthz and metrics to be on the same port. var errCh chan error - if s.BindAddressHardFail { + if s.Config.BindAddressHardFail { errCh = make(chan error) } @@ -668,12 +665,12 @@ func (s *ProxyServer) Run() error { serveHealthz(s.HealthzServer, errCh) // Start up a metrics server if requested - serveMetrics(s.MetricsBindAddress, s.ProxyMode, s.EnableProfiling, errCh) + serveMetrics(s.Config.MetricsBindAddress, s.Config.Mode, s.Config.EnableProfiling, errCh) // Tune conntrack, if requested // Conntracker is always nil for windows if s.Conntracker != nil { - max, err := getConntrackMax(s.ConntrackConfiguration) + max, err := getConntrackMax(s.Config.Conntrack) if err != nil { return err } @@ -696,15 +693,15 @@ func (s *ProxyServer) Run() error { } } - if s.ConntrackConfiguration.TCPEstablishedTimeout != nil && s.ConntrackConfiguration.TCPEstablishedTimeout.Duration > 0 { - timeout := int(s.ConntrackConfiguration.TCPEstablishedTimeout.Duration / time.Second) + if s.Config.Conntrack.TCPEstablishedTimeout != nil && s.Config.Conntrack.TCPEstablishedTimeout.Duration > 0 { + timeout := int(s.Config.Conntrack.TCPEstablishedTimeout.Duration / time.Second) if err := s.Conntracker.SetTCPEstablishedTimeout(timeout); err != nil { return err } } - if s.ConntrackConfiguration.TCPCloseWaitTimeout != nil && s.ConntrackConfiguration.TCPCloseWaitTimeout.Duration > 0 { - timeout := int(s.ConntrackConfiguration.TCPCloseWaitTimeout.Duration / time.Second) + if s.Config.Conntrack.TCPCloseWaitTimeout != nil && s.Config.Conntrack.TCPCloseWaitTimeout.Duration > 0 { + timeout := int(s.Config.Conntrack.TCPCloseWaitTimeout.Duration / time.Second) if err := s.Conntracker.SetTCPCloseWaitTimeout(timeout); err != nil { return err } @@ -725,7 +722,7 @@ func (s *ProxyServer) Run() error { labelSelector = labelSelector.Add(*noProxyName, *noHeadlessEndpoints) // Make informers that filter out objects that want a non-default service proxy. - informerFactory := informers.NewSharedInformerFactoryWithOptions(s.Client, s.ConfigSyncPeriod, + informerFactory := informers.NewSharedInformerFactoryWithOptions(s.Client, s.Config.ConfigSyncPeriod.Duration, informers.WithTweakListOptions(func(options *metav1.ListOptions) { options.LabelSelector = labelSelector.String() })) @@ -734,11 +731,11 @@ func (s *ProxyServer) Run() error { // Note: RegisterHandler() calls need to happen before creation of Sources because sources // only notify on changes, and the initial update (on process start) may be lost if no handlers // are registered yet. - serviceConfig := config.NewServiceConfig(informerFactory.Core().V1().Services(), s.ConfigSyncPeriod) + serviceConfig := config.NewServiceConfig(informerFactory.Core().V1().Services(), s.Config.ConfigSyncPeriod.Duration) serviceConfig.RegisterEventHandler(s.Proxier) go serviceConfig.Run(wait.NeverStop) - endpointSliceConfig := config.NewEndpointSliceConfig(informerFactory.Discovery().V1().EndpointSlices(), s.ConfigSyncPeriod) + endpointSliceConfig := config.NewEndpointSliceConfig(informerFactory.Discovery().V1().EndpointSlices(), s.Config.ConfigSyncPeriod.Duration) endpointSliceConfig.RegisterEventHandler(s.Proxier) go endpointSliceConfig.Run(wait.NeverStop) @@ -747,13 +744,13 @@ func (s *ProxyServer) Run() error { informerFactory.Start(wait.NeverStop) // Make an informer that selects for our nodename. - currentNodeInformerFactory := informers.NewSharedInformerFactoryWithOptions(s.Client, s.ConfigSyncPeriod, + currentNodeInformerFactory := informers.NewSharedInformerFactoryWithOptions(s.Client, s.Config.ConfigSyncPeriod.Duration, informers.WithTweakListOptions(func(options *metav1.ListOptions) { options.FieldSelector = fields.OneTermEqualSelector("metadata.name", s.NodeRef.Name).String() })) - nodeConfig := config.NewNodeConfig(currentNodeInformerFactory.Core().V1().Nodes(), s.ConfigSyncPeriod) + nodeConfig := config.NewNodeConfig(currentNodeInformerFactory.Core().V1().Nodes(), s.Config.ConfigSyncPeriod.Duration) // https://issues.k8s.io/111321 - if s.localDetectorMode == kubeproxyconfig.LocalModeNodeCIDR { + if s.Config.DetectLocalMode == kubeproxyconfig.LocalModeNodeCIDR { nodeConfig.RegisterEventHandler(&proxy.NodePodCIDRHandler{}) } nodeConfig.RegisterEventHandler(s.Proxier) diff --git a/cmd/kube-proxy/app/server_others.go b/cmd/kube-proxy/app/server_others.go index b731a413f9d..f78ab70792b 100644 --- a/cmd/kube-proxy/app/server_others.go +++ b/cmd/kube-proxy/app/server_others.go @@ -70,6 +70,19 @@ import ( // node after it is registered. var timeoutForNodePodCIDR = 5 * time.Minute +func (o *Options) platformApplyDefaults(config *proxyconfigapi.KubeProxyConfiguration) { + if config.Mode == "" { + klog.InfoS("Using iptables proxy") + config.Mode = proxyconfigapi.ProxyModeIPTables + } + + if config.DetectLocalMode == "" { + klog.V(4).InfoS("Defaulting detect-local-mode", "localModeClusterCIDR", string(proxyconfigapi.LocalModeClusterCIDR)) + config.DetectLocalMode = proxyconfigapi.LocalModeClusterCIDR + } + klog.V(2).InfoS("DetectLocalMode", "localMode", string(config.DetectLocalMode)) +} + // NewProxyServer returns a new ProxyServer. func NewProxyServer(o *Options) (*ProxyServer, error) { return newProxyServer(o.config, o.master) @@ -123,11 +136,8 @@ func newProxyServer( var proxier proxy.Provider - proxyMode := getProxyMode(config.Mode) - detectLocalMode := getDetectLocalMode(config) - var nodeInfo *v1.Node - if detectLocalMode == proxyconfigapi.LocalModeNodeCIDR { + if config.DetectLocalMode == proxyconfigapi.LocalModeNodeCIDR { klog.InfoS("Watching for node, awaiting podCIDR allocation", "hostname", hostname) nodeInfo, err = waitForPodCIDR(client, hostname) if err != nil { @@ -136,8 +146,6 @@ func newProxyServer( klog.InfoS("NodeInfo", "podCIDR", nodeInfo.Spec.PodCIDR, "podCIDRs", nodeInfo.Spec.PodCIDRs) } - klog.V(2).InfoS("DetectLocalMode", "localMode", string(detectLocalMode)) - primaryFamily := v1.IPv4Protocol primaryProtocol := utiliptables.ProtocolIPv4 if netutils.IsIPv6(nodeIP) { @@ -178,7 +186,7 @@ func newProxyServer( } } - if proxyMode == proxyconfigapi.ProxyModeIPTables { + if config.Mode == proxyconfigapi.ProxyModeIPTables { klog.InfoS("Using iptables Proxier") if dualStack { @@ -186,7 +194,7 @@ func newProxyServer( klog.InfoS("Creating dualStackProxier for iptables") // Always ordered to match []ipt var localDetectors [2]proxyutiliptables.LocalTrafficDetector - localDetectors, err = getDualStackLocalDetectorTuple(detectLocalMode, config, ipt, nodeInfo) + localDetectors, err = getDualStackLocalDetectorTuple(config.DetectLocalMode, config, ipt, nodeInfo) if err != nil { return nil, fmt.Errorf("unable to create proxier: %v", err) } @@ -211,7 +219,7 @@ func newProxyServer( } else { // Create a single-stack proxier if and only if the node does not support dual-stack (i.e, no iptables support). var localDetector proxyutiliptables.LocalTrafficDetector - localDetector, err = getLocalDetector(detectLocalMode, config, iptInterface, nodeInfo) + localDetector, err = getLocalDetector(config.DetectLocalMode, config, iptInterface, nodeInfo) if err != nil { return nil, fmt.Errorf("unable to create proxier: %v", err) } @@ -240,7 +248,7 @@ func newProxyServer( return nil, fmt.Errorf("unable to create proxier: %v", err) } proxymetrics.RegisterMetrics() - } else if proxyMode == proxyconfigapi.ProxyModeIPVS { + } else if config.Mode == proxyconfigapi.ProxyModeIPVS { kernelHandler := ipvs.NewLinuxKernelHandler() ipsetInterface = utilipset.New(execer) ipvsInterface = utilipvs.New() @@ -256,7 +264,7 @@ func newProxyServer( // Always ordered to match []ipt var localDetectors [2]proxyutiliptables.LocalTrafficDetector - localDetectors, err = getDualStackLocalDetectorTuple(detectLocalMode, config, ipt, nodeInfo) + localDetectors, err = getDualStackLocalDetectorTuple(config.DetectLocalMode, config, ipt, nodeInfo) if err != nil { return nil, fmt.Errorf("unable to create proxier: %v", err) } @@ -287,7 +295,7 @@ func newProxyServer( ) } else { var localDetector proxyutiliptables.LocalTrafficDetector - localDetector, err = getLocalDetector(detectLocalMode, config, iptInterface, nodeInfo) + localDetector, err = getLocalDetector(config.DetectLocalMode, config, iptInterface, nodeInfo) if err != nil { return nil, fmt.Errorf("unable to create proxier: %v", err) } @@ -325,21 +333,14 @@ func newProxyServer( } return &ProxyServer{ - Client: client, - Proxier: proxier, - Broadcaster: eventBroadcaster, - Recorder: recorder, - ConntrackConfiguration: config.Conntrack, - Conntracker: &realConntracker{}, - ProxyMode: proxyMode, - NodeRef: nodeRef, - MetricsBindAddress: config.MetricsBindAddress, - BindAddressHardFail: config.BindAddressHardFail, - EnableProfiling: config.EnableProfiling, - OOMScoreAdj: config.OOMScoreAdj, - ConfigSyncPeriod: config.ConfigSyncPeriod.Duration, - HealthzServer: healthzServer, - localDetectorMode: detectLocalMode, + Config: config, + Client: client, + Proxier: proxier, + Broadcaster: eventBroadcaster, + Recorder: recorder, + Conntracker: &realConntracker{}, + NodeRef: nodeRef, + HealthzServer: healthzServer, }, nil } @@ -396,14 +397,6 @@ func detectNumCPU() int { return numCPU } -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 - } - 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: @@ -521,15 +514,6 @@ func cidrTuple(cidrList string) [2]string { return cidrs } -func getProxyMode(proxyMode proxyconfigapi.ProxyMode) proxyconfigapi.ProxyMode { - if proxyMode == "" { - klog.InfoS("Using iptables proxy") - return proxyconfigapi.ProxyModeIPTables - } else { - return proxyMode - } -} - // cleanupAndExit remove iptables rules and ipset/ipvs rules func cleanupAndExit() error { execer := exec.New() diff --git a/cmd/kube-proxy/app/server_others_test.go b/cmd/kube-proxy/app/server_others_test.go index 0640b20d28e..e6836138c2f 100644 --- a/cmd/kube-proxy/app/server_others_test.go +++ b/cmd/kube-proxy/app/server_others_test.go @@ -44,34 +44,66 @@ import ( utiliptablestest "k8s.io/kubernetes/pkg/util/iptables/testing" ) -func Test_getDetectLocalMode(t *testing.T) { - cases := []struct { - detectLocal string - expected proxyconfigapi.LocalMode +func Test_platformApplyDefaults(t *testing.T) { + testCases := []struct { + name string + mode proxyconfigapi.ProxyMode + expectedMode proxyconfigapi.ProxyMode + detectLocal proxyconfigapi.LocalMode + expectedDetectLocal proxyconfigapi.LocalMode }{ { - detectLocal: "", - expected: proxyconfigapi.LocalModeClusterCIDR, + name: "defaults", + mode: "", + expectedMode: proxyconfigapi.ProxyModeIPTables, + detectLocal: "", + expectedDetectLocal: proxyconfigapi.LocalModeClusterCIDR, }, { - detectLocal: string(proxyconfigapi.LocalModeClusterCIDR), - expected: proxyconfigapi.LocalModeClusterCIDR, + name: "explicit", + mode: proxyconfigapi.ProxyModeIPTables, + expectedMode: proxyconfigapi.ProxyModeIPTables, + detectLocal: proxyconfigapi.LocalModeClusterCIDR, + expectedDetectLocal: proxyconfigapi.LocalModeClusterCIDR, }, { - detectLocal: string(proxyconfigapi.LocalModeInterfaceNamePrefix), - expected: proxyconfigapi.LocalModeInterfaceNamePrefix, + name: "override mode", + mode: "ipvs", + expectedMode: proxyconfigapi.ProxyModeIPVS, + detectLocal: "", + expectedDetectLocal: proxyconfigapi.LocalModeClusterCIDR, }, { - detectLocal: string(proxyconfigapi.LocalModeBridgeInterface), - expected: proxyconfigapi.LocalModeBridgeInterface, + name: "override detect-local", + mode: "", + expectedMode: proxyconfigapi.ProxyModeIPTables, + detectLocal: "NodeCIDR", + expectedDetectLocal: proxyconfigapi.LocalModeNodeCIDR, + }, + { + name: "override both", + mode: "ipvs", + expectedMode: proxyconfigapi.ProxyModeIPVS, + detectLocal: "NodeCIDR", + expectedDetectLocal: proxyconfigapi.LocalModeNodeCIDR, }, } - for i, c := range cases { - proxyConfig := &proxyconfigapi.KubeProxyConfiguration{DetectLocalMode: proxyconfigapi.LocalMode(c.detectLocal)} - r := getDetectLocalMode(proxyConfig) - if r != c.expected { - t.Errorf("Case[%d] Expected %q got %q", i, c.expected, r) - } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + options := NewOptions() + config := &proxyconfigapi.KubeProxyConfiguration{ + Mode: tc.mode, + DetectLocalMode: tc.detectLocal, + } + + options.platformApplyDefaults(config) + if config.Mode != tc.expectedMode { + t.Fatalf("expected mode: %s, but got: %s", tc.expectedMode, config.Mode) + } + if config.DetectLocalMode != tc.expectedDetectLocal { + t.Fatalf("expected detect-local: %s, but got: %s", tc.expectedDetectLocal, config.DetectLocalMode) + } + }) } } diff --git a/cmd/kube-proxy/app/server_windows.go b/cmd/kube-proxy/app/server_windows.go index 02eccb8ef31..43ae62600e9 100644 --- a/cmd/kube-proxy/app/server_windows.go +++ b/cmd/kube-proxy/app/server_windows.go @@ -45,6 +45,12 @@ import ( "k8s.io/kubernetes/pkg/proxy/winkernel" ) +func (o *Options) platformApplyDefaults(config *proxyconfigapi.KubeProxyConfiguration) { + if config.Mode == "" { + config.Mode = proxyconfigapi.ProxyModeKernelspace + } +} + // NewProxyServer returns a new ProxyServer. func NewProxyServer(o *Options) (*ProxyServer, error) { return newProxyServer(o.config, o.master) @@ -99,7 +105,6 @@ func newProxyServer(config *proxyconfigapi.KubeProxyConfiguration, master string } var proxier proxy.Provider - proxyMode := proxyconfigapi.ProxyModeKernelspace dualStackMode := getDualStackMode(config.Winkernel.NetworkName, winkernel.DualStackCompatTester{}) if dualStackMode { klog.InfoS("Creating dualStackProxier for Windows kernel.") @@ -134,18 +139,13 @@ func newProxyServer(config *proxyconfigapi.KubeProxyConfiguration, master string winkernel.RegisterMetrics() return &ProxyServer{ - Client: client, - Proxier: proxier, - Broadcaster: eventBroadcaster, - Recorder: recorder, - ProxyMode: proxyMode, - NodeRef: nodeRef, - MetricsBindAddress: config.MetricsBindAddress, - BindAddressHardFail: config.BindAddressHardFail, - EnableProfiling: config.EnableProfiling, - OOMScoreAdj: config.OOMScoreAdj, - ConfigSyncPeriod: config.ConfigSyncPeriod.Duration, - HealthzServer: healthzServer, + Config: config, + Client: client, + Proxier: proxier, + Broadcaster: eventBroadcaster, + Recorder: recorder, + NodeRef: nodeRef, + HealthzServer: healthzServer, }, nil } diff --git a/pkg/proxy/kubemark/hollow_proxy.go b/pkg/proxy/kubemark/hollow_proxy.go index e87d165b84b..1462c30a84f 100644 --- a/pkg/proxy/kubemark/hollow_proxy.go +++ b/pkg/proxy/kubemark/hollow_proxy.go @@ -22,6 +22,7 @@ import ( v1 "k8s.io/api/core/v1" discoveryv1 "k8s.io/api/discovery/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" clientset "k8s.io/client-go/kubernetes" v1core "k8s.io/client-go/kubernetes/typed/core/v1" @@ -29,6 +30,7 @@ import ( utilsysctl "k8s.io/component-helpers/node/util/sysctl" proxyapp "k8s.io/kubernetes/cmd/kube-proxy/app" "k8s.io/kubernetes/pkg/proxy" + proxyconfigapi "k8s.io/kubernetes/pkg/proxy/apis/config" proxyconfig "k8s.io/kubernetes/pkg/proxy/config" "k8s.io/kubernetes/pkg/proxy/iptables" proxyutiliptables "k8s.io/kubernetes/pkg/proxy/util/iptables" @@ -124,14 +126,17 @@ func NewHollowProxyOrDie( } return &HollowProxy{ ProxyServer: &proxyapp.ProxyServer{ - Client: client, - Proxier: proxier, - Broadcaster: broadcaster, - Recorder: recorder, - ProxyMode: "fake", - NodeRef: nodeRef, - OOMScoreAdj: utilpointer.Int32Ptr(0), - ConfigSyncPeriod: 30 * time.Second, + Config: &proxyconfigapi.KubeProxyConfiguration{ + Mode: proxyconfigapi.ProxyMode("fake"), + ConfigSyncPeriod: metav1.Duration{Duration: 30 * time.Second}, + OOMScoreAdj: utilpointer.Int32Ptr(0), + }, + + Client: client, + Proxier: proxier, + Broadcaster: broadcaster, + Recorder: recorder, + NodeRef: nodeRef, }, }, nil } From 0c9f55588c54f468f1d1bb4e6793b440dcaf3f0b Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Thu, 4 May 2023 08:55:25 -0400 Subject: [PATCH 4/4] Simplify creation of default KubeProxyConfiguration --- cmd/kube-proxy/app/server.go | 41 ++++++++++++------------------------ 1 file changed, 13 insertions(+), 28 deletions(-) diff --git a/cmd/kube-proxy/app/server.go b/cmd/kube-proxy/app/server.go index 2aa6d5a8244..b9048aa3aed 100644 --- a/cmd/kube-proxy/app/server.go +++ b/cmd/kube-proxy/app/server.go @@ -205,10 +205,22 @@ func (o *Options) AddFlags(fs *pflag.FlagSet) { fs.StringVar(&o.config.DetectLocal.InterfaceNamePrefix, "pod-interface-name-prefix", o.config.DetectLocal.InterfaceNamePrefix, "An interface prefix in the cluster. Kube-proxy considers traffic as local if originating from interfaces that match the given prefix. This argument should be set if DetectLocalMode is set to InterfaceNamePrefix.") } +// newKubeProxyConfiguration returns a KubeProxyConfiguration with default values +func newKubeProxyConfiguration() *kubeproxyconfig.KubeProxyConfiguration { + versionedConfig := &v1alpha1.KubeProxyConfiguration{} + proxyconfigscheme.Scheme.Default(versionedConfig) + internalConfig, err := proxyconfigscheme.Scheme.ConvertToVersion(versionedConfig, kubeproxyconfig.SchemeGroupVersion) + if err != nil { + panic(fmt.Sprintf("Unable to create default config: %v", err)) + } + + return internalConfig.(*kubeproxyconfig.KubeProxyConfiguration) +} + // NewOptions returns initialized Options func NewOptions() *Options { return &Options{ - config: new(kubeproxyconfig.KubeProxyConfiguration), + config: newKubeProxyConfiguration(), healthzPort: ports.ProxyHealthzPort, metricsPort: ports.ProxyStatusPort, errCh: make(chan error), @@ -438,25 +450,6 @@ func (o *Options) loadConfig(data []byte) (*kubeproxyconfig.KubeProxyConfigurati return proxyConfig, nil } -// ApplyDefaults applies the default values to Options. -func (o *Options) ApplyDefaults(in *kubeproxyconfig.KubeProxyConfiguration) (*kubeproxyconfig.KubeProxyConfiguration, error) { - external, err := proxyconfigscheme.Scheme.ConvertToVersion(in, v1alpha1.SchemeGroupVersion) - if err != nil { - return nil, err - } - - proxyconfigscheme.Scheme.Default(external) - - internal, err := proxyconfigscheme.Scheme.ConvertToVersion(external, kubeproxyconfig.SchemeGroupVersion) - if err != nil { - return nil, err - } - - out := internal.(*kubeproxyconfig.KubeProxyConfiguration) - - return out, nil -} - // NewProxyCommand creates a *cobra.Command object with default parameters func NewProxyCommand() *cobra.Command { opts := NewOptions() @@ -504,14 +497,6 @@ with the apiserver API to configure the proxy.`, }, } - var err error - opts.config, err = opts.ApplyDefaults(opts.config) - if err != nil { - klog.ErrorS(err, "Unable to create flag defaults") - // ACTION REQUIRED: Exit code changed from 255 to 1 - os.Exit(1) - } - fs := cmd.Flags() opts.AddFlags(fs) fs.AddGoFlagSet(goflag.CommandLine) // for --boot-id-file and --machine-id-file