Merge pull request kubernetes#90391 from johscheuer/improve-error-message-svc-cidr

Improve the error message for the service cidr check

Author: k8s-ci-robot

cmd/kube-apiserver/app/options/validation.go

@@ -35,6 +35,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 {
@@ -48,9 +50,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 err := validateMaxCIDRRange(options.PrimaryServiceClusterIPRange, maxCIDRBits, "--service-cluster-ip-range"); err != nil {
+		errs = append(errs, err)
 	}
 
 	// Secondary IP validation
@@ -74,18 +75,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 > 20 {
-			errs = append(errs, errors.New("specified --secondary-service-cluster-ip-range is too large"))
+		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
 

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",
@@ -130,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())
+			}
+		})
+	}
+}