From 889648d6e5892ee1d51cc02a2a986ca0e3701c64 Mon Sep 17 00:00:00 2001 From: "Johannes M. Scheuermann" Date: Thu, 23 Apr 2020 07:55:57 +0200 Subject: [PATCH 1/2] Improve the error message for the service cidr check --- cmd/kube-apiserver/app/options/validation.go | 10 ++++++---- cmd/kube-apiserver/app/options/validation_test.go | 12 ++++++++++++ 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/cmd/kube-apiserver/app/options/validation.go b/cmd/kube-apiserver/app/options/validation.go index f28b98c228f..68b0e62d701 100644 --- a/cmd/kube-apiserver/app/options/validation.go +++ b/cmd/kube-apiserver/app/options/validation.go @@ -36,6 +36,8 @@ import ( // validateClusterIPFlags is expected to be called after Complete() func validateClusterIPFlags(options *ServerRunOptions) []error { var errs []error + // maxCIDRbits is used to define the maximum CIDR size for the cluster ip(s) + const maxCIDRbits = 20 // validate that primary has been processed by user provided values or it has been defaulted if options.PrimaryServiceClusterIPRange.IP == nil { @@ -50,8 +52,8 @@ func validateClusterIPFlags(options *ServerRunOptions) []error { // Complete() expected to have set Primary* and Secondary* // primary CIDR validation var ones, bits = options.PrimaryServiceClusterIPRange.Mask.Size() - if bits-ones > 20 { - errs = append(errs, errors.New("specified --service-cluster-ip-range is too large")) + if bits-ones > maxCIDRbits { + errs = append(errs, fmt.Errorf("specified --service-cluster-ip-range is too large. Network CIDR should not be bigger than /%d", bits-maxCIDRbits)) } // Secondary IP validation @@ -79,8 +81,8 @@ func validateClusterIPFlags(options *ServerRunOptions) []error { // bigger cidr (specially those offered by IPv6) will add no value // significantly increase snapshotting time. var ones, bits = options.SecondaryServiceClusterIPRange.Mask.Size() - if bits-ones > 20 { - errs = append(errs, errors.New("specified --secondary-service-cluster-ip-range is too large")) + if bits-ones > maxCIDRbits { + errs = append(errs, fmt.Errorf("specified --service-cluster-ip-range is too large. Network CIDR should not be bigger than /%d", bits-maxCIDRbits)) } } diff --git a/cmd/kube-apiserver/app/options/validation_test.go b/cmd/kube-apiserver/app/options/validation_test.go index 535149fdd8b..153dbbd25f6 100644 --- a/cmd/kube-apiserver/app/options/validation_test.go +++ b/cmd/kube-apiserver/app/options/validation_test.go @@ -95,6 +95,18 @@ func TestClusterSerivceIPRange(t *testing.T) { options: makeOptionsWithCIDRs("10.0.0.0/16", "3000::/108"), enableDualStack: false, }, + { + name: "service cidr to big", + expectErrors: true, + options: makeOptionsWithCIDRs("10.0.0.0/8", ""), + enableDualStack: true, + }, + { + name: "dual-stack secondary cidr to big", + expectErrors: true, + options: makeOptionsWithCIDRs("10.0.0.0/16", "3000::/64"), + enableDualStack: true, + }, /* success cases */ { name: "valid primary", From 4c5b46d2aea38c25fd7aa83b1a34b1e9a4cd1850 Mon Sep 17 00:00:00 2001 From: "Johannes M. Scheuermann" Date: Thu, 7 May 2020 15:37:20 +0200 Subject: [PATCH 2/2] Move validation in own function with tests --- cmd/kube-apiserver/app/options/validation.go | 29 +++++--- .../app/options/validation_test.go | 67 +++++++++++++++++++ 2 files changed, 85 insertions(+), 11 deletions(-) diff --git a/cmd/kube-apiserver/app/options/validation.go b/cmd/kube-apiserver/app/options/validation.go index 68b0e62d701..f010fdf6680 100644 --- a/cmd/kube-apiserver/app/options/validation.go +++ b/cmd/kube-apiserver/app/options/validation.go @@ -36,8 +36,8 @@ import ( // validateClusterIPFlags is expected to be called after Complete() func validateClusterIPFlags(options *ServerRunOptions) []error { var errs []error - // maxCIDRbits is used to define the maximum CIDR size for the cluster ip(s) - const maxCIDRbits = 20 + // maxCIDRBits is used to define the maximum CIDR size for the cluster ip(s) + const maxCIDRBits = 20 // validate that primary has been processed by user provided values or it has been defaulted if options.PrimaryServiceClusterIPRange.IP == nil { @@ -51,9 +51,8 @@ func validateClusterIPFlags(options *ServerRunOptions) []error { // Complete() expected to have set Primary* and Secondary* // primary CIDR validation - var ones, bits = options.PrimaryServiceClusterIPRange.Mask.Size() - if bits-ones > maxCIDRbits { - errs = append(errs, fmt.Errorf("specified --service-cluster-ip-range is too large. Network CIDR should not be bigger than /%d", bits-maxCIDRbits)) + if err := validateMaxCIDRRange(options.PrimaryServiceClusterIPRange, maxCIDRBits, "--service-cluster-ip-range"); err != nil { + errs = append(errs, err) } // Secondary IP validation @@ -77,18 +76,26 @@ func validateClusterIPFlags(options *ServerRunOptions) []error { errs = append(errs, errors.New("--service-cluster-ip-range and --secondary-service-cluster-ip-range must be of different IP family")) } - // Should be smallish sized cidr, this thing is kept in etcd - // bigger cidr (specially those offered by IPv6) will add no value - // significantly increase snapshotting time. - var ones, bits = options.SecondaryServiceClusterIPRange.Mask.Size() - if bits-ones > maxCIDRbits { - errs = append(errs, fmt.Errorf("specified --service-cluster-ip-range is too large. Network CIDR should not be bigger than /%d", bits-maxCIDRbits)) + if err := validateMaxCIDRRange(options.SecondaryServiceClusterIPRange, maxCIDRBits, "--secondary-service-cluster-ip-range"); err != nil { + errs = append(errs, err) } } return errs } +func validateMaxCIDRRange(cidr net.IPNet, maxCIDRBits int, cidrFlag string) error { + // Should be smallish sized cidr, this thing is kept in etcd + // bigger cidr (specially those offered by IPv6) will add no value + // significantly increase snapshotting time. + var ones, bits = cidr.Mask.Size() + if bits-ones > maxCIDRBits { + return fmt.Errorf("specified %s is too large; for %d-bit addresses, the mask must be >= %d", cidrFlag, bits, bits-maxCIDRBits) + } + + return nil +} + func validateServiceNodePort(options *ServerRunOptions) []error { var errs []error diff --git a/cmd/kube-apiserver/app/options/validation_test.go b/cmd/kube-apiserver/app/options/validation_test.go index 153dbbd25f6..73e66b58a1f 100644 --- a/cmd/kube-apiserver/app/options/validation_test.go +++ b/cmd/kube-apiserver/app/options/validation_test.go @@ -142,3 +142,70 @@ func TestClusterSerivceIPRange(t *testing.T) { }) } } + +func getIPnetFromCIDR(cidr string) *net.IPNet { + _, ipnet, _ := net.ParseCIDR(cidr) + return ipnet +} + +func TestValidateMaxCIDRRange(t *testing.T) { + testCases := []struct { + // tc.cidr, tc.maxCIDRBits, tc.cidrFlag) tc.expectedErrorMessage + name string + cidr net.IPNet + maxCIDRBits int + cidrFlag string + expectedErrorMessage string + expectErrors bool + }{ + { + name: "valid ipv4 cidr", + cidr: *getIPnetFromCIDR("10.92.0.0/12"), + maxCIDRBits: 20, + cidrFlag: "--service-cluster-ip-range", + expectedErrorMessage: "", + expectErrors: false, + }, + { + name: "valid ipv6 cidr", + cidr: *getIPnetFromCIDR("3000::/108"), + maxCIDRBits: 20, + cidrFlag: "--service-cluster-ip-range", + expectedErrorMessage: "", + expectErrors: false, + }, + { + name: "ipv4 cidr to big", + cidr: *getIPnetFromCIDR("10.92.0.0/8"), + maxCIDRBits: 20, + cidrFlag: "--service-cluster-ip-range", + expectedErrorMessage: "specified --service-cluster-ip-range is too large; for 32-bit addresses, the mask must be >= 12", + expectErrors: true, + }, + { + name: "ipv6 cidr to big", + cidr: *getIPnetFromCIDR("3000::/64"), + maxCIDRBits: 20, + cidrFlag: "--service-cluster-ip-range", + expectedErrorMessage: "specified --service-cluster-ip-range is too large; for 128-bit addresses, the mask must be >= 108", + expectErrors: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + err := validateMaxCIDRRange(tc.cidr, tc.maxCIDRBits, tc.cidrFlag) + if err != nil && !tc.expectErrors { + t.Errorf("expected no errors, error found %+v", err) + } + + if err == nil && tc.expectErrors { + t.Errorf("expected errors, no errors found") + } + + if err != nil && tc.expectErrors && err.Error() != tc.expectedErrorMessage { + t.Errorf("Expected error message: \"%s\"\nGot: \"%s\"", tc.expectedErrorMessage, err.Error()) + } + }) + } +}