From a2b22ec2b6277c1a40b6f6e63ba30f15dfb750ad Mon Sep 17 00:00:00 2001 From: dolibali <139439533+dolibali@users.noreply.github.com> Date: Wed, 18 Sep 2024 10:34:22 +0800 Subject: [PATCH] fix bugs and enhance unit test coverage for all functions in pkg/util/validator.go (#4505) * Fix bugs in validator.go and enhance unit test coverage for all functions --------- Signed-off-by: dolibali Co-authored-by: dolibali --- pkg/util/validator.go | 11 +- pkg/util/validator_test.go | 459 ++++++++++++++++++++++++++++++++++++- 2 files changed, 464 insertions(+), 6 deletions(-) diff --git a/pkg/util/validator.go b/pkg/util/validator.go index 81d8fced78b..d8334a13e55 100644 --- a/pkg/util/validator.go +++ b/pkg/util/validator.go @@ -70,7 +70,7 @@ func ValidateSubnet(subnet kubeovnv1.Subnet) error { for _, cidr := range strings.Split(subnet.Spec.CIDRBlock, ",") { // v6 ip address can not use upper case if ContainsUppercase(subnet.Spec.CIDRBlock) { - err := fmt.Errorf("subnet cidr block %s v6 ip address can not contain upper case", subnet.Spec.Gateway) + err := fmt.Errorf("subnet cidr block %s v6 ip address can not contain upper case", subnet.Spec.CIDRBlock) klog.Error(err) return err } @@ -86,7 +86,7 @@ func ValidateSubnet(subnet kubeovnv1.Subnet) error { allow := subnet.Spec.AllowSubnets for _, cidr := range allow { // v6 ip address can not use upper case - if ContainsUppercase(subnet.Spec.CIDRBlock) { + if ContainsUppercase(cidr) { err := fmt.Errorf("subnet %s allow subnet %s v6 ip address can not contain upper case", subnet.Name, cidr) klog.Error(err) return err @@ -205,10 +205,11 @@ func validateNatOutgoingPolicyRules(subnet kubeovnv1.Subnet) error { } func validateNatOutGoingPolicyRuleIPs(matchIPStr string) (string, error) { - ipItems := strings.Split(matchIPStr, ",") - if len(ipItems) == 0 { - return "", errors.New("MatchIPStr format error") + if matchIPStr = strings.TrimSpace(matchIPStr); matchIPStr == "" { + return "", errors.New("IPStr should not be empty") } + + ipItems := strings.Split(matchIPStr, ",") lastProtocol := "" checkProtocolConsistent := func(ipCidr string) bool { currentProtocol := CheckProtocol(ipCidr) diff --git a/pkg/util/validator_test.go b/pkg/util/validator_test.go index 4124d1b36ad..956918060c6 100644 --- a/pkg/util/validator_test.go +++ b/pkg/util/validator_test.go @@ -71,7 +71,306 @@ func TestValidateSubnet(t *testing.T) { err: "", }, { - name: "gatewayErr", + name: "GatewayUppercaseErr", + asubnet: kubeovnv1.Subnet{ + TypeMeta: metav1.TypeMeta{Kind: "Subnet", APIVersion: "kubeovn.io/v1"}, + ObjectMeta: metav1.ObjectMeta{ + Name: "ut-gateway-uppercase-err", + }, + Spec: kubeovnv1.SubnetSpec{ + Default: true, + Vpc: "ovn-cluster", + Protocol: kubeovnv1.ProtocolIPv6, + Namespaces: nil, + CIDRBlock: "2001:db8::/32", + Gateway: "2001:Db8::1", + ExcludeIps: []string{"2001:db8::a"}, + Provider: "ovn", + GatewayType: kubeovnv1.GWDistributedType, + }, + Status: kubeovnv1.SubnetStatus{}, + }, + err: "subnet gateway 2001:Db8::1 v6 ip address can not contain upper case", + }, + { + name: "CICDblockFormalErr", + asubnet: kubeovnv1.Subnet{ + TypeMeta: metav1.TypeMeta{Kind: "Subnet", APIVersion: "kubeovn.io/v1"}, + ObjectMeta: metav1.ObjectMeta{ + Name: "ut-cicd-block-formal-err", + }, + Spec: kubeovnv1.SubnetSpec{ + Default: true, + Vpc: "ovn-cluster", + Protocol: kubeovnv1.ProtocolIPv4, + Namespaces: nil, + CIDRBlock: "", + Gateway: "", + ExcludeIps: []string{"10.16.0.1"}, + Provider: "ovn", + GatewayType: kubeovnv1.GWDistributedType, + }, + Status: kubeovnv1.SubnetStatus{}, + }, + err: "CIDRBlock: formal error", + }, + { + name: "ExcludeIpsUppercaseErr", + asubnet: kubeovnv1.Subnet{ + TypeMeta: metav1.TypeMeta{Kind: "Subnet", APIVersion: "kubeovn.io/v1"}, + ObjectMeta: metav1.ObjectMeta{ + Name: "ut-exclude-ips-uppercase-err", + }, + Spec: kubeovnv1.SubnetSpec{ + Default: true, + Vpc: "ovn-cluster", + Protocol: kubeovnv1.ProtocolIPv6, + Namespaces: nil, + CIDRBlock: "2001:db8::/32", + Gateway: "2001:db8::1", + ExcludeIps: []string{"2001:db8::A"}, + Provider: "ovn", + GatewayType: kubeovnv1.GWDistributedType, + }, + Status: kubeovnv1.SubnetStatus{}, + }, + err: "subnet exclude ip 2001:db8::A can not contain upper case", + }, + { + name: "CidrBlockUppercaseErr", + asubnet: kubeovnv1.Subnet{ + TypeMeta: metav1.TypeMeta{Kind: "Subnet", APIVersion: "kubeovn.io/v1"}, + ObjectMeta: metav1.ObjectMeta{ + Name: "ut-cidr-block-uppercase-err", + }, + Spec: kubeovnv1.SubnetSpec{ + Default: true, + Vpc: "ovn-cluster", + Protocol: kubeovnv1.ProtocolIPv6, + Namespaces: nil, + CIDRBlock: "2001:Db8::/32", + Gateway: "2001:db8::1", + ExcludeIps: []string{"2001:db8::a"}, + Provider: "ovn", + GatewayType: kubeovnv1.GWDistributedType, + }, + Status: kubeovnv1.SubnetStatus{}, + }, + err: "subnet cidr block 2001:Db8::/32 v6 ip address can not contain upper case", + }, + { + name: "InvalidZeroCIDRErr", + asubnet: kubeovnv1.Subnet{ + TypeMeta: metav1.TypeMeta{Kind: "Subnet", APIVersion: "kubeovn.io/v1"}, + ObjectMeta: metav1.ObjectMeta{ + Name: "ut-invalid-zero-cidr-err", + }, + Spec: kubeovnv1.SubnetSpec{ + Default: true, + Vpc: "ovn-cluster", + Protocol: kubeovnv1.ProtocolIPv4, + Namespaces: nil, + CIDRBlock: "0.0.0.0", + Gateway: "", + ExcludeIps: []string{"10.16.0.1"}, + Provider: "ovn", + GatewayType: kubeovnv1.GWDistributedType, + }, + Status: kubeovnv1.SubnetStatus{}, + }, + err: "invalid zero cidr \"0.0.0.0\"", + }, + { + name: "InvalidCIDRErr", + asubnet: kubeovnv1.Subnet{ + TypeMeta: metav1.TypeMeta{Kind: "Subnet", APIVersion: "kubeovn.io/v1"}, + ObjectMeta: metav1.ObjectMeta{ + Name: "ut-invalid-cidr-err", + }, + Spec: kubeovnv1.SubnetSpec{ + Default: true, + Vpc: "ovn-cluster", + Protocol: kubeovnv1.ProtocolIPv4, + Namespaces: nil, + CIDRBlock: "192.168.1.0/invalid", + Gateway: "", + ExcludeIps: []string{"10.16.0.1"}, + Provider: "ovn", + GatewayType: kubeovnv1.GWDistributedType, + }, + Status: kubeovnv1.SubnetStatus{}, + }, + err: "subnet ut-invalid-cidr-err cidr 192.168.1.0/invalid is invalid", + }, + { + name: "ProtocolInvalidErr", + asubnet: kubeovnv1.Subnet{ + TypeMeta: metav1.TypeMeta{Kind: "Subnet", APIVersion: "kubeovn.io/v1"}, + ObjectMeta: metav1.ObjectMeta{ + Name: "ut-protocol-invalid-err", + }, + Spec: kubeovnv1.SubnetSpec{ + Default: true, + Vpc: "ovn-cluster", + Protocol: "ipv5", + Namespaces: nil, + CIDRBlock: "10.16.0.0/16", + Gateway: "10.16.0.1", + ExcludeIps: []string{"10.16.0.1"}, + Provider: "ovn", + + GatewayType: kubeovnv1.GWDistributedType, + }, + Status: kubeovnv1.SubnetStatus{}, + }, + err: "ipv5 is not a valid protocol type", + }, + { + name: "ExternalEgressGatewayUpperCaseErr", + asubnet: kubeovnv1.Subnet{ + TypeMeta: metav1.TypeMeta{Kind: "Subnet", APIVersion: "kubeovn.io/v1"}, + ObjectMeta: metav1.ObjectMeta{ + Name: "ut-external-egress-gateway-uppercase-err", + }, + Spec: kubeovnv1.SubnetSpec{ + Default: true, + Vpc: "ovn-cluster", + Protocol: kubeovnv1.ProtocolIPv6, + Namespaces: nil, + CIDRBlock: "2001:db8::/32", + Gateway: "2001:db8::1", + ExcludeIps: []string{"2001:db8::a"}, + Provider: "ovn", + ExternalEgressGateway: "2001:dB8::2", + GatewayType: kubeovnv1.GWDistributedType, + }, + Status: kubeovnv1.SubnetStatus{}, + }, + err: "subnet ut-external-egress-gateway-uppercase-err external egress gateway 2001:dB8::2 v6 ip address can not contain upper case", + }, + { + name: "VipsUpperCaseErr", + asubnet: kubeovnv1.Subnet{ + TypeMeta: metav1.TypeMeta{Kind: "Subnet", APIVersion: "kubeovn.io/v1"}, + ObjectMeta: metav1.ObjectMeta{ + Name: "ut-vips-uppercase-err", + }, + Spec: kubeovnv1.SubnetSpec{ + Default: true, + Vpc: "ovn-cluster", + Protocol: kubeovnv1.ProtocolIPv6, + Namespaces: nil, + CIDRBlock: "2001:db8::/32", + Gateway: "2001:db8::1", + ExcludeIps: []string{"2001:db8::a"}, + Provider: "ovn", + Vips: []string{"2001:dB8::2"}, + GatewayType: kubeovnv1.GWDistributedType, + }, + Status: kubeovnv1.SubnetStatus{}, + }, + err: "subnet ut-vips-uppercase-err vips 2001:dB8::2 v6 ip address can not contain upper case", + }, + { + name: "LogicalGatewayU2OInterconnectionSametimeTrueErr", + asubnet: kubeovnv1.Subnet{ + TypeMeta: metav1.TypeMeta{Kind: "Subnet", APIVersion: "kubeovn.io/v1"}, + ObjectMeta: metav1.ObjectMeta{ + Name: "ut-logical-gateway-u2o-interconnection-sametime-true-err", + }, + Spec: kubeovnv1.SubnetSpec{ + Default: true, + Vpc: "ovn-cluster", + Protocol: kubeovnv1.ProtocolIPv6, + Namespaces: nil, + CIDRBlock: "2001:db8::/32", + Gateway: "2001:db8::1", + ExcludeIps: []string{"2001:db8::a"}, + Provider: "ovn", + GatewayType: kubeovnv1.GWDistributedType, + LogicalGateway: true, + U2OInterconnection: true, + }, + Status: kubeovnv1.SubnetStatus{}, + }, + err: "logicalGateway and u2oInterconnection can't be opened at the same time", + }, + { + name: "ValidateNatOutgoingPolicyRulesErr", + asubnet: kubeovnv1.Subnet{ + TypeMeta: metav1.TypeMeta{Kind: "Subnet", APIVersion: "kubeovn.io/v1"}, + ObjectMeta: metav1.ObjectMeta{ + Name: "ut-validate-nat-outgoing-policy-rules-err", + }, + Spec: kubeovnv1.SubnetSpec{ + Default: true, + Vpc: "ovn-cluster", + Protocol: kubeovnv1.ProtocolIPv6, + Namespaces: nil, + CIDRBlock: "2001:db8::/32", + Gateway: "2001:db8::1", + ExcludeIps: []string{"2001:db8::a"}, + Provider: "ovn", + GatewayType: kubeovnv1.GWDistributedType, + NatOutgoingPolicyRules: []kubeovnv1.NatOutgoingPolicyRule{ + { + Match: kubeovnv1.NatOutGoingPolicyMatch{SrcIPs: "2001:db8::/32,192.168.0.1/24", DstIPs: "2001:db8::/32"}, + Action: "ACCEPT", + }, + }, + }, + Status: kubeovnv1.SubnetStatus{}, + }, + err: "validate nat policy rules src ips 2001:db8::/32,192.168.0.1/24 failed with err match ips 2001:db8::/32,192.168.0.1/24 protocol is not consistent", + }, + { + name: "U2oInterconnectionIpUppercaseErr", + asubnet: kubeovnv1.Subnet{ + TypeMeta: metav1.TypeMeta{Kind: "Subnet", APIVersion: "kubeovn.io/v1"}, + ObjectMeta: metav1.ObjectMeta{ + Name: "ut-u2o-interconnection-ip-uppercase-err", + }, + Spec: kubeovnv1.SubnetSpec{ + Default: true, + Vpc: "ovn-cluster", + Protocol: kubeovnv1.ProtocolIPv6, + Namespaces: nil, + CIDRBlock: "2001:db8::/32", + Gateway: "2001:db8::1", + ExcludeIps: []string{"2001:db8::a"}, + Provider: "ovn", + GatewayType: kubeovnv1.GWDistributedType, + U2OInterconnectionIP: "2001:dB8::2", + }, + Status: kubeovnv1.SubnetStatus{}, + }, + err: "subnet ut-u2o-interconnection-ip-uppercase-err U2O interconnection ip 2001:dB8::2 v6 ip address can not contain upper case", + }, + { + name: "U2oInterConnectionIpNotInCidrErr", + asubnet: kubeovnv1.Subnet{ + TypeMeta: metav1.TypeMeta{Kind: "Subnet", APIVersion: "kubeovn.io/v1"}, + ObjectMeta: metav1.ObjectMeta{ + Name: "ut-u2o-interconnection-ip-not-in-cidr-err", + }, + Spec: kubeovnv1.SubnetSpec{ + Default: true, + Vpc: "ovn-cluster", + Protocol: kubeovnv1.ProtocolIPv6, + Namespaces: nil, + CIDRBlock: "2001:db8::/32", + Gateway: "2001:db8::1", + ExcludeIps: []string{"2001:db8::a"}, + Provider: "ovn", + GatewayType: kubeovnv1.GWDistributedType, + U2OInterconnectionIP: "3001:db8::2", + }, + Status: kubeovnv1.SubnetStatus{}, + }, + err: "u2oInterconnectionIP 3001:db8::2 is not in subnet ut-u2o-interconnection-ip-not-in-cidr-err cidr 2001:db8::/32", + }, + { + name: "GatewayErr", asubnet: kubeovnv1.Subnet{ TypeMeta: metav1.TypeMeta{Kind: "Subnet", APIVersion: "kubeovn.io/v1"}, ObjectMeta: metav1.ObjectMeta{ @@ -270,6 +569,30 @@ func TestValidateSubnet(t *testing.T) { }, err: "10.18.0/16 in allowSubnets is not a valid address", }, + { + name: "AllowSubnetsUppercaseErr", + asubnet: kubeovnv1.Subnet{ + TypeMeta: metav1.TypeMeta{Kind: "Subnet", APIVersion: "kubeovn.io/v1"}, + ObjectMeta: metav1.ObjectMeta{ + Name: "ut-allow-subnets-uppercase-err", + }, + Spec: kubeovnv1.SubnetSpec{ + Default: true, + Vpc: "ovn-cluster", + Protocol: kubeovnv1.ProtocolIPv6, + Namespaces: nil, + CIDRBlock: "2001:db8::/32", + Gateway: "2001:db8::1", + ExcludeIps: []string{"2001:db8::a"}, + Provider: OvnProvider, + GatewayType: kubeovnv1.GWDistributedType, + Private: true, + AllowSubnets: []string{"2001:dB8::/32"}, + }, + Status: kubeovnv1.SubnetStatus{}, + }, + err: "subnet ut-allow-subnets-uppercase-err allow subnet 2001:dB8::/32 v6 ip address can not contain upper case", + }, { name: "gatewaytypeErr", asubnet: kubeovnv1.Subnet{ @@ -521,6 +844,7 @@ func TestValidateSubnet(t *testing.T) { err: "validate gateway 10.16.0.0 for cidr 10.16.0.0/32 failed: subnet 10.16.0.0/32 is configured with /32 netmask", }, } + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { ret := ValidateSubnet(tt.asubnet) @@ -955,6 +1279,7 @@ func TestValidateVpc(t *testing.T) { name string vpc *kubeovnv1.Vpc wantErr bool + errMsg string }{ { name: "valid vpc", @@ -996,6 +1321,33 @@ func TestValidateVpc(t *testing.T) { }, wantErr: true, }, + { + name: "invalid vpc static route CIDR", + vpc: &kubeovnv1.Vpc{ + Spec: kubeovnv1.VpcSpec{ + StaticRoutes: []*kubeovnv1.StaticRoute{ + { + CIDR: "192.168.%.0/24", + NextHopIP: "10.0.0.1", + }, + }, + PolicyRoutes: []*kubeovnv1.PolicyRoute{ + { + Action: kubeovnv1.PolicyRouteActionAllow, + NextHopIP: "10.0.0.1", + }, + }, + VpcPeerings: []*kubeovnv1.VpcPeering{ + { + LocalConnectIP: "192.168.1.0/24", + }, + }, + }, + }, + + wantErr: true, + errMsg: "invalid cidr 192.168.%.0/24: invalid CIDR address: 192.168.%.0/24", + }, { name: "invalid static route CIDR", vpc: &kubeovnv1.Vpc{ @@ -1073,6 +1425,111 @@ func TestValidateVpc(t *testing.T) { if (err != nil) != tt.wantErr { t.Errorf("got error = %v, but wantErr %v", err, tt.wantErr) } + if tt.errMsg != "" && err != nil && err.Error() != tt.errMsg { + t.Errorf("expected error message %q, but got %q", tt.errMsg, err.Error()) + } + }) + } +} + +func TestValidateNatOutGoingPolicyRuleIPs(t *testing.T) { + tests := []struct { + name string + input string + want string + expectErr bool + }{ + { + name: "Valid IPv4", + input: "192.168.1.1,10.0.0.1", + want: "IPv4", + expectErr: false, + }, + { + name: "Valid IPv6", + input: "2001:0db8::1,2001:0db8::2", + want: "IPv6", + expectErr: false, + }, + { + name: "Mixed IPv4 and IPv6", + input: "192.168.1.1,2001:0db8::1", + want: "", + expectErr: true, + }, + { + name: "Invalid IP", + input: "invalid_ip", + want: "", + expectErr: true, + }, + { + name: "Empty string", + input: "", + want: "", + expectErr: true, + }, + { + name: "Valid CIDR", + input: "192.168.1.0/24,10.0.0.0/8", + want: "IPv4", + expectErr: false, + }, + { + name: "Mixed IP and CIDR", + input: "192.168.1.1,10.0.0.0/8", + want: "IPv4", + expectErr: false, + }, + { + name: "Invalid CIDR", + input: "192.168.1.0/33", + want: "", + expectErr: true, + }, + { + name: "Single IPv4", + input: "10.0.0.1", + want: "IPv4", + expectErr: false, + }, + { + name: "Single IPv6", + input: "2001:0db8::1", + want: "IPv6", + expectErr: false, + }, + { + name: "Single Invalid IP", + input: "300.300.300.300", + want: "", + expectErr: true, + }, + { + name: "Empty after split", + input: ",", + want: "", + expectErr: true, + }, + + { + name: "Valid CIDR with IPv6", + input: "192.168.1.0/24,2001:0db8::1", + want: "", + expectErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := validateNatOutGoingPolicyRuleIPs(tt.input) + if (err != nil) != tt.expectErr { + t.Errorf("validateNatOutGoingPolicyRuleIPs() error = %v, wantErr %v", err, tt.expectErr) + return + } + if got != tt.want { + t.Errorf("validateNatOutGoingPolicyRuleIPs() = %v, want %v", got, tt.want) + } }) } }