Skip to content

Commit

Permalink
Re-allow 0 for kube-proxy conntrack settings
Browse files Browse the repository at this point in the history
When kube-proxy was refactored to use a configuration file, the ability
to use 0 for conntrack min, max, max per core, and tcp timeouts was
inadvertently broken; if you specified 0, it would instead apply the
default value from defaults.go.

This change restores the ability to use 0 to mean 0.

Signed-off-by: Andy Goldstein <andy.goldstein@gmail.com>
  • Loading branch information
ncdc committed Nov 7, 2017
1 parent ff82be0 commit ea78586
Show file tree
Hide file tree
Showing 10 changed files with 250 additions and 154 deletions.
53 changes: 29 additions & 24 deletions cmd/kube-proxy/app/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -146,12 +146,15 @@ func AddFlags(options *Options, fs *pflag.FlagSet) {
fs.Float32Var(&options.config.ClientConnection.QPS, "kube-api-qps", options.config.ClientConnection.QPS, "QPS to use while talking with kubernetes apiserver")
fs.IntVar(&options.config.ClientConnection.Burst, "kube-api-burst", options.config.ClientConnection.Burst, "Burst to use while talking with kubernetes apiserver")
fs.DurationVar(&options.config.UDPIdleTimeout.Duration, "udp-timeout", options.config.UDPIdleTimeout.Duration, "How long an idle UDP connection will be kept open (e.g. '250ms', '2s'). Must be greater than 0. Only applicable for proxy-mode=userspace")
fs.Int32Var(&options.config.Conntrack.Max, "conntrack-max", options.config.Conntrack.Max,
if options.config.Conntrack.Max == nil {
options.config.Conntrack.Max = utilpointer.Int32Ptr(0)
}
fs.Int32Var(options.config.Conntrack.Max, "conntrack-max", *options.config.Conntrack.Max,
"Maximum number of NAT connections to track (0 to leave as-is). This overrides conntrack-max-per-core and conntrack-min.")
fs.MarkDeprecated("conntrack-max", "This feature will be removed in a later release.")
fs.Int32Var(&options.config.Conntrack.MaxPerCore, "conntrack-max-per-core", options.config.Conntrack.MaxPerCore,
fs.Int32Var(options.config.Conntrack.MaxPerCore, "conntrack-max-per-core", *options.config.Conntrack.MaxPerCore,
"Maximum number of NAT connections to track per CPU core (0 to leave the limit as-is and ignore conntrack-min).")
fs.Int32Var(&options.config.Conntrack.Min, "conntrack-min", options.config.Conntrack.Min,
fs.Int32Var(options.config.Conntrack.Min, "conntrack-min", *options.config.Conntrack.Min,
"Minimum number of conntrack entries to allocate, regardless of conntrack-max-per-core (set conntrack-max-per-core=0 to leave the limit as-is).")
fs.DurationVar(&options.config.Conntrack.TCPEstablishedTimeout.Duration, "conntrack-tcp-timeout-established", options.config.Conntrack.TCPEstablishedTimeout.Duration, "Idle timeout for established TCP connections (0 to leave as-is)")
fs.DurationVar(
Expand Down Expand Up @@ -179,6 +182,17 @@ func (o *Options) Complete() error {
o.applyDeprecatedHealthzPortToConfig()
}

// Load the config file here in Complete, so that Validate validates the fully-resolved config.
if len(o.ConfigFile) > 0 {
if c, err := o.loadConfigFromFile(o.ConfigFile); err != nil {
return err
} else {
o.config = c

This comment has been minimized.

Copy link
@seh

seh Aug 8, 2018

Contributor

This overwrites any configuration set earlier by the flags that are bound to o.config in (*Options).AddFlags above.

Motivating discussion from kubernetes/website#9663.

This comment has been minimized.

Copy link
@seh

seh Aug 8, 2018

Contributor

Granted, though, this patch just moved this statement from the (*Options).Run function.

This comment has been minimized.

Copy link
@seh

seh Aug 8, 2018

Contributor
// Make sure we apply the feature gate settings in the config file.
utilfeature.DefaultFeatureGate.Set(o.config.FeatureGates)
}
}

return nil
}

Expand All @@ -196,23 +210,11 @@ func (o *Options) Validate(args []string) error {
}

func (o *Options) Run() error {
config := o.config

if len(o.WriteConfigTo) > 0 {
return o.writeConfigFile()
}

if len(o.ConfigFile) > 0 {
if c, err := o.loadConfigFromFile(o.ConfigFile); err != nil {
return err
} else {
config = c
// Make sure we apply the feature gate settings in the config file.
utilfeature.DefaultFeatureGate.Set(config.FeatureGates)
}
}

proxyServer, err := NewProxyServer(config, o.CleanupAndExit, o.scheme, o.master)
proxyServer, err := NewProxyServer(o.config, o.CleanupAndExit, o.scheme, o.master)
if err != nil {
return err
}
Expand Down Expand Up @@ -502,14 +504,14 @@ func (s *ProxyServer) Run() error {
}
}

if s.ConntrackConfiguration.TCPEstablishedTimeout.Duration > 0 {
if s.ConntrackConfiguration.TCPEstablishedTimeout != nil && s.ConntrackConfiguration.TCPEstablishedTimeout.Duration > 0 {
timeout := int(s.ConntrackConfiguration.TCPEstablishedTimeout.Duration / time.Second)
if err := s.Conntracker.SetTCPEstablishedTimeout(timeout); err != nil {
return err
}
}

if s.ConntrackConfiguration.TCPCloseWaitTimeout.Duration > 0 {
if s.ConntrackConfiguration.TCPCloseWaitTimeout != nil && s.ConntrackConfiguration.TCPCloseWaitTimeout.Duration > 0 {
timeout := int(s.ConntrackConfiguration.TCPCloseWaitTimeout.Duration / time.Second)
if err := s.Conntracker.SetTCPCloseWaitTimeout(timeout); err != nil {
return err
Expand Down Expand Up @@ -548,16 +550,19 @@ func (s *ProxyServer) birthCry() {
}

func getConntrackMax(config kubeproxyconfig.KubeProxyConntrackConfiguration) (int, error) {
if config.Max > 0 {
if config.MaxPerCore > 0 {
if config.Max != nil && *config.Max > 0 {
if config.MaxPerCore != nil && *config.MaxPerCore > 0 {
return -1, fmt.Errorf("invalid config: Conntrack Max and Conntrack MaxPerCore are mutually exclusive")
}
glog.V(3).Infof("getConntrackMax: using absolute conntrack-max (deprecated)")
return int(config.Max), nil
return int(*config.Max), nil
}
if config.MaxPerCore > 0 {
floor := int(config.Min)
scaled := int(config.MaxPerCore) * goruntime.NumCPU()
if config.MaxPerCore != nil && *config.MaxPerCore > 0 {
floor := 0
if config.Min != nil {
floor = int(*config.Min)
}
scaled := int(*config.MaxPerCore) * goruntime.NumCPU()
if scaled > floor {
glog.V(3).Infof("getConntrackMax: using scaled conntrack-max-per-core")
return scaled, nil
Expand Down
16 changes: 8 additions & 8 deletions cmd/kube-proxy/app/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -203,9 +203,9 @@ func TestGetConntrackMax(t *testing.T) {

for i, tc := range testCases {
cfg := kubeproxyconfig.KubeProxyConntrackConfiguration{
Min: tc.min,
Max: tc.max,
MaxPerCore: tc.maxPerCore,
Min: utilpointer.Int32Ptr(tc.min),
Max: utilpointer.Int32Ptr(tc.max),
MaxPerCore: utilpointer.Int32Ptr(tc.maxPerCore),
}
x, e := getConntrackMax(cfg)
if e != nil {
Expand Down Expand Up @@ -344,11 +344,11 @@ udpTimeoutMilliseconds: 123ms
ClusterCIDR: tc.clusterCIDR,
ConfigSyncPeriod: metav1.Duration{Duration: 15 * time.Second},
Conntrack: kubeproxyconfig.KubeProxyConntrackConfiguration{
Max: 4,
MaxPerCore: 2,
Min: 1,
TCPCloseWaitTimeout: metav1.Duration{Duration: 10 * time.Second},
TCPEstablishedTimeout: metav1.Duration{Duration: 20 * time.Second},
Max: utilpointer.Int32Ptr(4),
MaxPerCore: utilpointer.Int32Ptr(2),
Min: utilpointer.Int32Ptr(1),
TCPCloseWaitTimeout: &metav1.Duration{Duration: 10 * time.Second},
TCPEstablishedTimeout: &metav1.Duration{Duration: 20 * time.Second},
},
FeatureGates: "all",
HealthzBindAddress: tc.healthzBindAddress,
Expand Down
18 changes: 9 additions & 9 deletions pkg/proxy/apis/kubeproxyconfig/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,21 +73,21 @@ type KubeProxyIPVSConfiguration struct {
// the Kubernetes proxy server.
type KubeProxyConntrackConfiguration struct {
// max is the maximum number of NAT connections to track (0 to
// leave as-is). This takes precedence over conntrackMaxPerCore and conntrackMin.
Max int32
// leave as-is). This takes precedence over maxPerCore and min.
Max *int32
// maxPerCore is the maximum number of NAT connections to track
// per CPU core (0 to leave the limit as-is and ignore conntrackMin).
MaxPerCore int32
// per CPU core (0 to leave the limit as-is and ignore min).
MaxPerCore *int32
// min is the minimum value of connect-tracking records to allocate,
// regardless of conntrackMaxPerCore (set conntrackMaxPerCore=0 to leave the limit as-is).
Min int32
// regardless of maxPerCore (set maxPerCore=0 to leave the limit as-is).
Min *int32
// tcpEstablishedTimeout is how long an idle TCP connection will be kept open
// (e.g. '2s'). Must be greater than 0.
TCPEstablishedTimeout metav1.Duration
// (e.g. '2s'). Must be greater than 0 to set.
TCPEstablishedTimeout *metav1.Duration
// tcpCloseWaitTimeout is how long an idle conntrack entry
// in CLOSE_WAIT state will remain in the conntrack
// table. (e.g. '60s'). Must be greater than 0 to set.
TCPCloseWaitTimeout metav1.Duration
TCPCloseWaitTimeout *metav1.Duration
}

// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
Expand Down
19 changes: 10 additions & 9 deletions pkg/proxy/apis/kubeproxyconfig/v1alpha1/defaults.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
kruntime "k8s.io/apimachinery/pkg/runtime"
"k8s.io/kubernetes/pkg/kubelet/qos"
"k8s.io/kubernetes/pkg/master/ports"
"k8s.io/kubernetes/pkg/util/pointer"
)

func addDefaultingFuncs(scheme *kruntime.Scheme) error {
Expand Down Expand Up @@ -63,23 +64,23 @@ func SetDefaults_KubeProxyConfiguration(obj *KubeProxyConfiguration) {
obj.UDPIdleTimeout = metav1.Duration{Duration: 250 * time.Millisecond}
}
// If ConntrackMax is set, respect it.
if obj.Conntrack.Max == 0 {
if obj.Conntrack.Max == nil {
// If ConntrackMax is *not* set, use per-core scaling.
if obj.Conntrack.MaxPerCore == 0 {
obj.Conntrack.MaxPerCore = 32 * 1024
if obj.Conntrack.MaxPerCore == nil {
obj.Conntrack.MaxPerCore = pointer.Int32Ptr(32 * 1024)
}
if obj.Conntrack.Min == 0 {
obj.Conntrack.Min = 128 * 1024
if obj.Conntrack.Min == nil {
obj.Conntrack.Min = pointer.Int32Ptr(128 * 1024)
}
}
if obj.IPTables.MasqueradeBit == nil {
temp := int32(14)
obj.IPTables.MasqueradeBit = &temp
}
if obj.Conntrack.TCPEstablishedTimeout == zero {
obj.Conntrack.TCPEstablishedTimeout = metav1.Duration{Duration: 24 * time.Hour} // 1 day (1/5 default)
if obj.Conntrack.TCPEstablishedTimeout == nil {
obj.Conntrack.TCPEstablishedTimeout = &metav1.Duration{Duration: 24 * time.Hour} // 1 day (1/5 default)
}
if obj.Conntrack.TCPCloseWaitTimeout == zero {
if obj.Conntrack.TCPCloseWaitTimeout == nil {
// See https://github.com/kubernetes/kubernetes/issues/32551.
//
// CLOSE_WAIT conntrack state occurs when the Linux kernel
Expand All @@ -100,7 +101,7 @@ func SetDefaults_KubeProxyConfiguration(obj *KubeProxyConfiguration) {
//
// We set CLOSE_WAIT to one hour by default to better match
// typical server timeouts.
obj.Conntrack.TCPCloseWaitTimeout = metav1.Duration{Duration: 1 * time.Hour}
obj.Conntrack.TCPCloseWaitTimeout = &metav1.Duration{Duration: 1 * time.Hour}
}
if obj.ConfigSyncPeriod.Duration == 0 {
obj.ConfigSyncPeriod.Duration = 15 * time.Minute
Expand Down
18 changes: 9 additions & 9 deletions pkg/proxy/apis/kubeproxyconfig/v1alpha1/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,21 +69,21 @@ type KubeProxyIPVSConfiguration struct {
// the Kubernetes proxy server.
type KubeProxyConntrackConfiguration struct {
// max is the maximum number of NAT connections to track (0 to
// leave as-is). This takes precedence over conntrackMaxPerCore and conntrackMin.
Max int32 `json:"max"`
// leave as-is). This takes precedence over maxPerCore and min.
Max *int32 `json:"max"`
// maxPerCore is the maximum number of NAT connections to track
// per CPU core (0 to leave the limit as-is and ignore conntrackMin).
MaxPerCore int32 `json:"maxPerCore"`
// per CPU core (0 to leave the limit as-is and ignore min).
MaxPerCore *int32 `json:"maxPerCore"`
// min is the minimum value of connect-tracking records to allocate,
// regardless of conntrackMaxPerCore (set conntrackMaxPerCore=0 to leave the limit as-is).
Min int32 `json:"min"`
// regardless of conntrackMaxPerCore (set maxPerCore=0 to leave the limit as-is).
Min *int32 `json:"min"`
// tcpEstablishedTimeout is how long an idle TCP connection will be kept open
// (e.g. '2s'). Must be greater than 0.
TCPEstablishedTimeout metav1.Duration `json:"tcpEstablishedTimeout"`
// (e.g. '2s'). Must be greater than 0 to set.
TCPEstablishedTimeout *metav1.Duration `json:"tcpEstablishedTimeout"`
// tcpCloseWaitTimeout is how long an idle conntrack entry
// in CLOSE_WAIT state will remain in the conntrack
// table. (e.g. '60s'). Must be greater than 0 to set.
TCPCloseWaitTimeout metav1.Duration `json:"tcpCloseWaitTimeout"`
TCPCloseWaitTimeout *metav1.Duration `json:"tcpCloseWaitTimeout"`
}

// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
Expand Down
21 changes: 11 additions & 10 deletions pkg/proxy/apis/kubeproxyconfig/v1alpha1/zz_generated.conversion.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

50 changes: 47 additions & 3 deletions pkg/proxy/apis/kubeproxyconfig/v1alpha1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 7 additions & 7 deletions pkg/proxy/apis/kubeproxyconfig/validation/validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -93,24 +93,24 @@ func validateKubeProxyIPTablesConfiguration(config kubeproxyconfig.KubeProxyIPTa
func validateKubeProxyConntrackConfiguration(config kubeproxyconfig.KubeProxyConntrackConfiguration, fldPath *field.Path) field.ErrorList {
allErrs := field.ErrorList{}

if config.Max < 0 {
if config.Max != nil && *config.Max < 0 {
allErrs = append(allErrs, field.Invalid(fldPath.Child("Max"), config.Max, "must be greater than or equal to 0"))
}

if config.MaxPerCore < 0 {
if config.MaxPerCore != nil && *config.MaxPerCore < 0 {
allErrs = append(allErrs, field.Invalid(fldPath.Child("MaxPerCore"), config.MaxPerCore, "must be greater than or equal to 0"))
}

if config.Min < 0 {
if config.Min != nil && *config.Min < 0 {
allErrs = append(allErrs, field.Invalid(fldPath.Child("Min"), config.Min, "must be greater than or equal to 0"))
}

if config.TCPEstablishedTimeout.Duration <= 0 {
allErrs = append(allErrs, field.Invalid(fldPath.Child("TCPEstablishedTimeout"), config.TCPEstablishedTimeout, "must be greater than 0"))
if config.TCPEstablishedTimeout.Duration < 0 {
allErrs = append(allErrs, field.Invalid(fldPath.Child("TCPEstablishedTimeout"), config.TCPEstablishedTimeout, "must be greater than or equal to 0"))
}

if config.TCPCloseWaitTimeout.Duration <= 0 {
allErrs = append(allErrs, field.Invalid(fldPath.Child("TCPCloseWaitTimeout"), config.TCPCloseWaitTimeout, "must be greater than 0"))
if config.TCPCloseWaitTimeout.Duration < 0 {
allErrs = append(allErrs, field.Invalid(fldPath.Child("TCPCloseWaitTimeout"), config.TCPCloseWaitTimeout, "must be greater than or equal to 0"))
}

return allErrs
Expand Down
Loading

0 comments on commit ea78586

Please sign in to comment.