From 705ba7b4bc4e46325ddaa200cf826a16ac7e6517 Mon Sep 17 00:00:00 2001 From: "Christopher M. Luciano" Date: Thu, 24 Sep 2020 12:28:13 -0400 Subject: [PATCH] proxy: validate each CIDR config seperately and check for errors This commit revises validateProxyNodePortAddress and validateExcludeCIDRS to report on the exact CIDR that is invalid within the array of strings. Previously we would just return the whole block of addresses and now we identify the exact address within the block to eliminate confusion. I also removed the break from validateProxyNodeAddress so that we can report on all addresses that may not be valid. The tests for each function have also been revised to check the errors explicitly upon validating. This also will properly catch occasions where we should be returning multiple errors if more than one CIDR is invalid. Signed-off-by: Christopher M. Luciano --- .../apis/config/validation/validation.go | 5 +- .../apis/config/validation/validation_test.go | 124 +++++++++--------- 2 files changed, 64 insertions(+), 65 deletions(-) diff --git a/pkg/proxy/apis/config/validation/validation.go b/pkg/proxy/apis/config/validation/validation.go index 019ba44bc5b..5e360283ba5 100644 --- a/pkg/proxy/apis/config/validation/validation.go +++ b/pkg/proxy/apis/config/validation/validation.go @@ -276,8 +276,7 @@ func validateKubeProxyNodePortAddress(nodePortAddresses []string, fldPath *field for i := range nodePortAddresses { if _, _, err := net.ParseCIDR(nodePortAddresses[i]); err != nil { - allErrs = append(allErrs, field.Invalid(fldPath, nodePortAddresses, "must be a valid IP block")) - break + allErrs = append(allErrs, field.Invalid(fldPath.Index(i), nodePortAddresses[i], "must be a valid CIDR")) } } @@ -307,7 +306,7 @@ func validateIPVSExcludeCIDRs(excludeCIDRs []string, fldPath *field.Path) field. for i := range excludeCIDRs { if _, _, err := net.ParseCIDR(excludeCIDRs[i]); err != nil { - allErrs = append(allErrs, field.Invalid(fldPath, excludeCIDRs, "must be a valid IP block")) + allErrs = append(allErrs, field.Invalid(fldPath.Index(i), excludeCIDRs[i], "must be a valid CIDR")) } } return allErrs diff --git a/pkg/proxy/apis/config/validation/validation_test.go b/pkg/proxy/apis/config/validation/validation_test.go index 35bf9d77e0b..97f03593108 100644 --- a/pkg/proxy/apis/config/validation/validation_test.go +++ b/pkg/proxy/apis/config/validation/validation_test.go @@ -924,52 +924,51 @@ func TestValidateKubeProxyNodePortAddress(t *testing.T) { } } - errorCases := []struct { - addresses []string - msg string + testCases := map[string]struct { + addresses []string + expectedErrs field.ErrorList }{ - { - addresses: []string{"foo"}, - msg: "must be a valid IP block", + "invalid foo address": { + addresses: []string{"foo"}, + expectedErrs: field.ErrorList{field.Invalid(newPath.Child("NodePortAddresses[0]"), "foo", "must be a valid CIDR")}, }, - { - addresses: []string{"1.2.3"}, - msg: "must be a valid IP block", + "invalid octet address": { + addresses: []string{"10.0.0.0/0", "1.2.3"}, + expectedErrs: field.ErrorList{field.Invalid(newPath.Child("NodePortAddresses[1]"), "1.2.3", "must be a valid CIDR")}, }, - { - addresses: []string{""}, - msg: "must be a valid IP block", + "address cannot be 0": { + addresses: []string{"127.0.0.1/32", "0", "1.2.3.0/24"}, + expectedErrs: field.ErrorList{field.Invalid(newPath.Child("NodePortAddresses[1]"), "0", "must be a valid CIDR")}, }, - { - addresses: []string{"10.20.30.40"}, - msg: "must be a valid IP block", + "address missing subnet range": { + addresses: []string{"127.0.0.1/32", "10.20.30.40", "1.2.3.0/24"}, + expectedErrs: field.ErrorList{field.Invalid(newPath.Child("NodePortAddresses[1]"), "10.20.30.40", "must be a valid CIDR")}, }, - { - addresses: []string{"::1"}, - msg: "must be a valid IP block", + "missing ipv6 subnet ranges": { + addresses: []string{"::0", "::1", "2001:db8::/32"}, + expectedErrs: field.ErrorList{field.Invalid(newPath.Child("NodePortAddresses[0]"), "::0", "must be a valid CIDR"), + field.Invalid(newPath.Child("NodePortAddresses[1]"), "::1", "must be a valid CIDR")}, }, - { - addresses: []string{"2001:db8:1"}, - msg: "must be a valid IP block", - }, - { - addresses: []string{"2001:db8:xyz/64"}, - msg: "must be a valid IP block", + "invalid ipv6 ip format": { + addresses: []string{"::1/128", "2001:db8::/32", "2001:db8:xyz/64"}, + expectedErrs: field.ErrorList{field.Invalid(newPath.Child("NodePortAddresses[2]"), "2001:db8:xyz/64", "must be a valid CIDR")}, }, } - for _, errorCase := range errorCases { - if errs := validateKubeProxyNodePortAddress(errorCase.addresses, newPath.Child("NodePortAddresses")); len(errs) == 0 { - t.Errorf("expected failure for %s", errorCase.msg) - } else if !strings.Contains(errs[0].Error(), errorCase.msg) { - t.Errorf("unexpected error: %v, expected: %s", errs[0], errorCase.msg) + for _, testCase := range testCases { + errs := validateKubeProxyNodePortAddress(testCase.addresses, newPath.Child("NodePortAddresses")) + if len(testCase.expectedErrs) != len(errs) { + t.Errorf("Expected %d errors, got %d errors: %v", len(testCase.expectedErrs), len(errs), errs) + } + for i, err := range errs { + if err.Error() != testCase.expectedErrs[i].Error() { + t.Errorf("Expected error: %s, got %s", testCase.expectedErrs[i], err.Error()) + } } } } func TestValidateKubeProxyExcludeCIDRs(t *testing.T) { - // TODO(rramkumar): This test is a copy of TestValidateKubeProxyNodePortAddress. - // Maybe some code can be shared? newPath := field.NewPath("KubeProxyConfiguration") successCases := []struct { @@ -996,45 +995,46 @@ func TestValidateKubeProxyExcludeCIDRs(t *testing.T) { } } - errorCases := []struct { - addresses []string - msg string + testCases := map[string]struct { + addresses []string + expectedErrs field.ErrorList }{ - { - addresses: []string{"foo"}, - msg: "must be a valid IP block", + "invalid foo address": { + addresses: []string{"foo"}, + expectedErrs: field.ErrorList{field.Invalid(newPath.Child("ExcludeCIDRS[0]"), "foo", "must be a valid CIDR")}, }, - { - addresses: []string{"1.2.3"}, - msg: "must be a valid IP block", + "invalid octet address": { + addresses: []string{"10.0.0.0/0", "1.2.3"}, + expectedErrs: field.ErrorList{field.Invalid(newPath.Child("ExcludeCIDRS[1]"), "1.2.3", "must be a valid CIDR")}, }, - { - addresses: []string{""}, - msg: "must be a valid IP block", + "address cannot be 0": { + addresses: []string{"127.0.0.1/32", "0", "1.2.3.0/24"}, + expectedErrs: field.ErrorList{field.Invalid(newPath.Child("ExcludeCIDRS[1]"), "0", "must be a valid CIDR")}, }, - { - addresses: []string{"10.20.30.40"}, - msg: "must be a valid IP block", + "address missing subnet range": { + addresses: []string{"127.0.0.1/32", "10.20.30.40", "1.2.3.0/24"}, + expectedErrs: field.ErrorList{field.Invalid(newPath.Child("ExcludeCIDRS[1]"), "10.20.30.40", "must be a valid CIDR")}, }, - { - addresses: []string{"::1"}, - msg: "must be a valid IP block", + "missing ipv6 subnet ranges": { + addresses: []string{"::0", "::1", "2001:db8::/32"}, + expectedErrs: field.ErrorList{field.Invalid(newPath.Child("ExcludeCIDRS[0]"), "::0", "must be a valid CIDR"), + field.Invalid(newPath.Child("ExcludeCIDRS[1]"), "::1", "must be a valid CIDR")}, }, - { - addresses: []string{"2001:db8:1"}, - msg: "must be a valid IP block", - }, - { - addresses: []string{"2001:db8:xyz/64"}, - msg: "must be a valid IP block", + "invalid ipv6 ip format": { + addresses: []string{"::1/128", "2001:db8::/32", "2001:db8:xyz/64"}, + expectedErrs: field.ErrorList{field.Invalid(newPath.Child("ExcludeCIDRS[2]"), "2001:db8:xyz/64", "must be a valid CIDR")}, }, } - for _, errorCase := range errorCases { - if errs := validateIPVSExcludeCIDRs(errorCase.addresses, newPath.Child("ExcludeCIDRs")); len(errs) == 0 { - t.Errorf("expected failure for %s", errorCase.msg) - } else if !strings.Contains(errs[0].Error(), errorCase.msg) { - t.Errorf("unexpected error: %v, expected: %s", errs[0], errorCase.msg) + for _, testCase := range testCases { + errs := validateIPVSExcludeCIDRs(testCase.addresses, newPath.Child("ExcludeCIDRS")) + if len(testCase.expectedErrs) != len(errs) { + t.Errorf("Expected %d errors, got %d errors: %v", len(testCase.expectedErrs), len(errs), errs) + } + for i, err := range errs { + if err.Error() != testCase.expectedErrs[i].Error() { + t.Errorf("Expected error: %s, got %s", testCase.expectedErrs[i], err.Error()) + } } } }