From 52db699a6634529e2de202565fdce70936ccea9e Mon Sep 17 00:00:00 2001 From: Quan Tian Date: Thu, 25 Aug 2022 08:10:31 +0800 Subject: [PATCH] Fix unit tests that rely on pseudo-random numbers (#4148) Test_ofPacketOutBuilder_Done relies on specific pseudo-random numbers to be generated and will fail if another test also calls the pseudo-random number generator. This patch creates a dedicated random number generator for ofPacketOutBuilder and specifies a hardcoded seed for unit tests to make test outputs predictable. Signed-off-by: Quan Tian --- pkg/ovs/openflow/ofctrl_packetout.go | 22 +++++++--- pkg/ovs/openflow/ofctrl_packetout_test.go | 52 ++++++++--------------- 2 files changed, 32 insertions(+), 42 deletions(-) diff --git a/pkg/ovs/openflow/ofctrl_packetout.go b/pkg/ovs/openflow/ofctrl_packetout.go index 77baf18d716..d119667261e 100644 --- a/pkg/ovs/openflow/ofctrl_packetout.go +++ b/pkg/ovs/openflow/ofctrl_packetout.go @@ -18,6 +18,7 @@ import ( "encoding/binary" "math/rand" "net" + "time" "antrea.io/libOpenflow/openflow15" "antrea.io/libOpenflow/protocol" @@ -26,6 +27,9 @@ import ( "k8s.io/klog/v2" ) +// #nosec G404: random number generator not used for security purposes +var pktRand = rand.New(rand.NewSource(time.Now().UnixNano())) + type ofPacketOutBuilder struct { pktOut *ofctrl.PacketOut icmpID *uint16 @@ -341,11 +345,13 @@ func (b *ofPacketOutBuilder) Done() *ofctrl.PacketOut { b.pktOut.IPHeader.Length = 20 + b.pktOut.ICMPHeader.Len() } else if b.pktOut.TCPHeader != nil { b.pktOut.TCPHeader.HdrLen = 5 - // #nosec G404: random number generator not used for security purposes - b.pktOut.TCPHeader.SeqNum = rand.Uint32() + if b.pktOut.TCPHeader.SeqNum == 0 { + // #nosec G404: random number generator not used for security purposes + b.pktOut.TCPHeader.SeqNum = pktRand.Uint32() + } if b.pktOut.TCPHeader.AckNum == 0 { // #nosec G404: random number generator not used for security purposes - b.pktOut.TCPHeader.AckNum = rand.Uint32() + b.pktOut.TCPHeader.AckNum = pktRand.Uint32() } b.pktOut.TCPHeader.Checksum = b.tcpHeaderChecksum() b.pktOut.IPHeader.Length = 20 + b.pktOut.TCPHeader.Len() @@ -368,7 +374,7 @@ func (b *ofPacketOutBuilder) Done() *ofctrl.PacketOut { } if b.pktOut.IPHeader.Id == 0 { // #nosec G404: random number generator not used for security purposes - b.pktOut.IPHeader.Id = uint16(rand.Uint32()) + b.pktOut.IPHeader.Id = uint16(pktRand.Uint32()) } // Set IP version in the IP Header. b.pktOut.IPHeader.Version = 0x4 @@ -382,11 +388,13 @@ func (b *ofPacketOutBuilder) Done() *ofctrl.PacketOut { b.pktOut.IPv6Header.Length = b.pktOut.ICMPHeader.Len() } else if b.pktOut.TCPHeader != nil { b.pktOut.TCPHeader.HdrLen = 5 - // #nosec G404: random number generator not used for security purposes - b.pktOut.TCPHeader.SeqNum = rand.Uint32() + if b.pktOut.TCPHeader.SeqNum == 0 { + // #nosec G404: random number generator not used for security purposes + b.pktOut.TCPHeader.SeqNum = pktRand.Uint32() + } if b.pktOut.TCPHeader.AckNum == 0 { // #nosec G404: random number generator not used for security purposes - b.pktOut.TCPHeader.AckNum = rand.Uint32() + b.pktOut.TCPHeader.AckNum = pktRand.Uint32() } b.pktOut.TCPHeader.Checksum = b.tcpHeaderChecksum() b.pktOut.IPv6Header.Length = b.pktOut.TCPHeader.Len() diff --git a/pkg/ovs/openflow/ofctrl_packetout_test.go b/pkg/ovs/openflow/ofctrl_packetout_test.go index 6340347f285..a5d93d5e310 100644 --- a/pkg/ovs/openflow/ofctrl_packetout_test.go +++ b/pkg/ovs/openflow/ofctrl_packetout_test.go @@ -15,12 +15,14 @@ package openflow import ( + "math/rand" "net" "reflect" "testing" "antrea.io/libOpenflow/protocol" "antrea.io/ofnet/ofctrl" + "github.com/stretchr/testify/assert" ) func Test_ofPacketOutBuilder_SetSrcIP(t *testing.T) { @@ -644,6 +646,7 @@ func Test_ofPacketOutBuilder_Done(t *testing.T) { Version: 0x4, Length: 28, Checksum: 46753, + Id: 1090, }, ICMPHeader: &protocol.ICMP{ Type: 3, @@ -669,13 +672,16 @@ func Test_ofPacketOutBuilder_Done(t *testing.T) { IPHeader: &protocol.IPv4{ Version: 0x4, Length: 40, - Checksum: 45409, + Checksum: 8009, + Id: 39822, }, TCPHeader: &protocol.TCP{ PortSrc: 10000, PortDst: 10001, HdrLen: 5, - Checksum: 63286, + Checksum: 40408, + SeqNum: 2596996162, + AckNum: 4039455774, }, }, }, @@ -694,7 +700,8 @@ func Test_ofPacketOutBuilder_Done(t *testing.T) { IPHeader: &protocol.IPv4{ Version: 0x4, Length: 28, - Checksum: 45025, + Checksum: 46753, + Id: 1090, }, UDPHeader: &protocol.UDP{ PortSrc: 10000, @@ -751,7 +758,9 @@ func Test_ofPacketOutBuilder_Done(t *testing.T) { PortSrc: 10000, PortDst: 10001, HdrLen: 5, - Checksum: 26538, + Checksum: 40408, + SeqNum: 2596996162, + AckNum: 4039455774, }, }, }, @@ -782,43 +791,16 @@ func Test_ofPacketOutBuilder_Done(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + // Specify a hardcoded seed for testing to make output predictable. + // #nosec G404: random number generator not used for security purposes + pktRand = rand.New(rand.NewSource(1)) b := &ofPacketOutBuilder{ pktOut: tt.fields.pktOut, icmpID: tt.fields.icmpID, icmpSeq: tt.fields.icmpSeq, } got := b.Done() - if got == nil { - if tt.want != nil { - t.Errorf("Done() = %v, want %v", got, tt.want) - } - return - } - if got.IPHeader != nil { - got.IPHeader.Id = 0 - } - if got.TCPHeader != nil { - got.TCPHeader.SeqNum = 0 - got.TCPHeader.AckNum = 0 - } - if !reflect.DeepEqual(got.ICMPHeader, tt.want.ICMPHeader) { - t.Errorf("Done() = %v, want %v", got.ICMPHeader, tt.want.ICMPHeader) - } - if !reflect.DeepEqual(got.TCPHeader, tt.want.TCPHeader) { - t.Errorf("Done() = %+v, want %+v", got.TCPHeader, tt.want.TCPHeader) - } - if !reflect.DeepEqual(got.UDPHeader, tt.want.UDPHeader) { - t.Errorf("Done() = %v, want %v", got.UDPHeader, tt.want.UDPHeader) - } - if !reflect.DeepEqual(got.IPHeader, tt.want.IPHeader) { - t.Errorf("Done() = %v, want %v", got.IPHeader, tt.want.IPHeader) - } - if !reflect.DeepEqual(got.IPv6Header, tt.want.IPv6Header) { - t.Errorf("Done() = %v, want %v", got.IPv6Header, tt.want.IPv6Header) - } - if !reflect.DeepEqual(got, tt.want) { - t.Errorf("Done() = %+v, want %+v", got, tt.want) - } + assert.Equal(t, tt.want, got) }) } }