[SEBA-820] Preventing the ONU from going to dhcp_started if we don't have the DHCP flow

Change-Id: Ia9c136830a6781fa69d7ead5406c3e354ad35a06
diff --git a/internal/bbsim/devices/onu.go b/internal/bbsim/devices/onu.go
index 0f93e0d..cc467b8 100644
--- a/internal/bbsim/devices/onu.go
+++ b/internal/bbsim/devices/onu.go
@@ -18,6 +18,7 @@
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"github.com/cboling/omci"
 	"github.com/google/gopacket/layers"
@@ -46,10 +47,13 @@
 	// PortNo comes with flows and it's used when sending packetIndications,
 	// There is one PortNo per UNI Port, for now we're only storing the first one
 	// FIXME add support for multiple UNIs
-	PortNo        uint32
 	HwAddress     net.HardwareAddr
 	InternalState *fsm.FSM
 
+	// ONU State
+	PortNo           uint32
+	DhcpFlowReceived bool
+
 	OperState    *fsm.FSM
 	SerialNumber *openolt.SerialNumber
 
@@ -64,25 +68,26 @@
 	DoneChannel chan bool // this channel is used to signal once the onu is complete (when the struct is used by BBR)
 }
 
-func (o Onu) Sn() string {
+func (o *Onu) Sn() string {
 	return common.OnuSnToString(o.SerialNumber)
 }
 
 func CreateONU(olt OltDevice, pon PonPort, id uint32, sTag int, cTag int) *Onu {
 
 	o := Onu{
-		ID:          id,
-		PonPortID:   pon.ID,
-		PonPort:     pon,
-		STag:        sTag,
-		CTag:        cTag,
-		HwAddress:   net.HardwareAddr{0x2e, 0x60, 0x70, 0x13, byte(pon.ID), byte(id)},
-		PortNo:      0,
-		Channel:     make(chan Message, 2048),
-		tid:         0x1,
-		hpTid:       0x8000,
-		seqNumber:   0,
-		DoneChannel: make(chan bool, 1),
+		ID:               id,
+		PonPortID:        pon.ID,
+		PonPort:          pon,
+		STag:             sTag,
+		CTag:             cTag,
+		HwAddress:        net.HardwareAddr{0x2e, 0x60, 0x70, 0x13, byte(pon.ID), byte(id)},
+		PortNo:           0,
+		Channel:          make(chan Message, 2048),
+		tid:              0x1,
+		hpTid:            0x8000,
+		seqNumber:        0,
+		DoneChannel:      make(chan bool, 1),
+		DhcpFlowReceived: false,
 	}
 	o.SerialNumber = o.NewSN(olt.ID, pon.ID, o.ID)
 
@@ -167,6 +172,11 @@
 					"OnuSn":  o.Sn(),
 				}).Errorf("ONU failed to authenticate!")
 			},
+			"before_start_dhcp": func(e *fsm.Event) {
+				if o.DhcpFlowReceived == false {
+					e.Cancel(errors.New("cannot-go-to-dhcp-started-as-dhcp-flow-is-missing"))
+				}
+			},
 			"enter_dhcp_started": func(e *fsm.Event) {
 				msg := Message{
 					Type: StartDHCP,
@@ -209,7 +219,7 @@
 	}).Debugf("Changing ONU InternalState from %s to %s", src, dst)
 }
 
-func (o Onu) ProcessOnuMessages(stream openolt.Openolt_EnableIndicationServer, client openolt.OpenoltClient) {
+func (o *Onu) ProcessOnuMessages(stream openolt.Openolt_EnableIndicationServer, client openolt.OpenoltClient) {
 	onuLogger.WithFields(log.Fields{
 		"onuID": o.ID,
 		"onuSN": o.Sn(),
@@ -358,6 +368,7 @@
 
 	if err := stream.Send(&openolt.Indication{Data: discoverData}); err != nil {
 		log.Errorf("Failed to send Indication_OnuDiscInd: %v", err)
+		return
 	}
 
 	if err := o.InternalState.Event("discover"); err != nil {
@@ -507,9 +518,12 @@
 	} else if msg.Flow.Classifier.EthType == uint32(layers.EthernetTypeIPv4) &&
 		msg.Flow.Classifier.SrcPort == uint32(68) &&
 		msg.Flow.Classifier.DstPort == uint32(67) {
+
+		// keep track that we reveived the DHCP Flows so that we can transition the state to dhcp_started
+		o.DhcpFlowReceived = true
 		// NOTE we are receiving mulitple DHCP flows but we shouldn't call the transition multiple times
 		if err := o.InternalState.Event("start_dhcp"); err != nil {
-			log.Warnf("Can't go to dhcp_started: %v", err)
+			log.Errorf("Can't go to dhcp_started: %v", err)
 		}
 	}
 }
diff --git a/internal/bbsim/devices/onu_flow_test.go b/internal/bbsim/devices/onu_flow_test.go
index 1e47034..4194aa5 100644
--- a/internal/bbsim/devices/onu_flow_test.go
+++ b/internal/bbsim/devices/onu_flow_test.go
@@ -17,110 +17,13 @@
 package devices
 
 import (
-	"context"
-	"errors"
 	"github.com/google/gopacket/layers"
 	"github.com/looplab/fsm"
 	"github.com/opencord/voltha-protos/go/openolt"
-	"github.com/opencord/voltha-protos/go/tech_profile"
-	"google.golang.org/grpc"
 	"gotest.tools/assert"
-	"net"
 	"testing"
 )
 
-type FlowAddSpy struct {
-	CallCount int
-	Calls     map[int]*openolt.Flow
-}
-
-type mockClient struct {
-	FlowAddSpy
-	fail bool
-}
-
-func (s *mockClient) DisableOlt(ctx context.Context, in *openolt.Empty, opts ...grpc.CallOption) (*openolt.Empty, error) {
-	return nil, errors.New("unimplemented-in-mock-client")
-}
-func (s *mockClient) ReenableOlt(ctx context.Context, in *openolt.Empty, opts ...grpc.CallOption) (*openolt.Empty, error) {
-	return nil, errors.New("unimplemented-in-mock-client")
-}
-func (s *mockClient) ActivateOnu(ctx context.Context, in *openolt.Onu, opts ...grpc.CallOption) (*openolt.Empty, error) {
-	return nil, errors.New("unimplemented-in-mock-client")
-}
-func (s *mockClient) DeactivateOnu(ctx context.Context, in *openolt.Onu, opts ...grpc.CallOption) (*openolt.Empty, error) {
-	return nil, errors.New("unimplemented-in-mock-client")
-}
-func (s *mockClient) DeleteOnu(ctx context.Context, in *openolt.Onu, opts ...grpc.CallOption) (*openolt.Empty, error) {
-	return nil, errors.New("unimplemented-in-mock-client")
-}
-func (s *mockClient) OmciMsgOut(ctx context.Context, in *openolt.OmciMsg, opts ...grpc.CallOption) (*openolt.Empty, error) {
-	return nil, errors.New("unimplemented-in-mock-client")
-}
-func (s *mockClient) OnuPacketOut(ctx context.Context, in *openolt.OnuPacket, opts ...grpc.CallOption) (*openolt.Empty, error) {
-	return nil, errors.New("unimplemented-in-mock-client")
-}
-func (s *mockClient) UplinkPacketOut(ctx context.Context, in *openolt.UplinkPacket, opts ...grpc.CallOption) (*openolt.Empty, error) {
-	return nil, errors.New("unimplemented-in-mock-client")
-}
-func (s *mockClient) FlowAdd(ctx context.Context, in *openolt.Flow, opts ...grpc.CallOption) (*openolt.Empty, error) {
-	s.FlowAddSpy.CallCount++
-	if s.fail {
-		return nil, errors.New("fake-error")
-	}
-	s.FlowAddSpy.Calls[s.FlowAddSpy.CallCount] = in
-	return &openolt.Empty{}, nil
-}
-func (s *mockClient) FlowRemove(ctx context.Context, in *openolt.Flow, opts ...grpc.CallOption) (*openolt.Empty, error) {
-	return nil, errors.New("unimplemented-in-mock-client")
-}
-func (s *mockClient) HeartbeatCheck(ctx context.Context, in *openolt.Empty, opts ...grpc.CallOption) (*openolt.Heartbeat, error) {
-	return nil, errors.New("unimplemented-in-mock-client")
-}
-func (s *mockClient) EnablePonIf(ctx context.Context, in *openolt.Interface, opts ...grpc.CallOption) (*openolt.Empty, error) {
-	return nil, errors.New("unimplemented-in-mock-client")
-}
-func (s *mockClient) DisablePonIf(ctx context.Context, in *openolt.Interface, opts ...grpc.CallOption) (*openolt.Empty, error) {
-	return nil, errors.New("unimplemented-in-mock-client")
-}
-func (s *mockClient) GetDeviceInfo(ctx context.Context, in *openolt.Empty, opts ...grpc.CallOption) (*openolt.DeviceInfo, error) {
-	return nil, errors.New("unimplemented-in-mock-client")
-}
-func (s *mockClient) Reboot(ctx context.Context, in *openolt.Empty, opts ...grpc.CallOption) (*openolt.Empty, error) {
-	return nil, errors.New("unimplemented-in-mock-client")
-}
-func (s *mockClient) CollectStatistics(ctx context.Context, in *openolt.Empty, opts ...grpc.CallOption) (*openolt.Empty, error) {
-	return nil, errors.New("unimplemented-in-mock-client")
-}
-func (s *mockClient) CreateTrafficSchedulers(ctx context.Context, in *tech_profile.TrafficSchedulers, opts ...grpc.CallOption) (*openolt.Empty, error) {
-	return nil, errors.New("unimplemented-in-mock-client")
-}
-func (s *mockClient) RemoveTrafficSchedulers(ctx context.Context, in *tech_profile.TrafficSchedulers, opts ...grpc.CallOption) (*openolt.Empty, error) {
-	return nil, errors.New("unimplemented-in-mock-client")
-}
-func (s *mockClient) CreateTrafficQueues(ctx context.Context, in *tech_profile.TrafficQueues, opts ...grpc.CallOption) (*openolt.Empty, error) {
-	return nil, errors.New("unimplemented-in-mock-client")
-}
-func (s *mockClient) RemoveTrafficQueues(ctx context.Context, in *tech_profile.TrafficQueues, opts ...grpc.CallOption) (*openolt.Empty, error) {
-	return nil, errors.New("unimplemented-in-mock-client")
-}
-func (s *mockClient) EnableIndication(ctx context.Context, in *openolt.Empty, opts ...grpc.CallOption) (openolt.Openolt_EnableIndicationClient, error) {
-	return nil, errors.New("unimplemented-in-mock-client")
-}
-
-func createMockOnu(id uint32, ponPortId uint32, sTag int, cTag int) Onu {
-	o := Onu{
-		ID:        id,
-		PonPortID: ponPortId,
-		STag:      sTag,
-		CTag:      cTag,
-		HwAddress: net.HardwareAddr{0x2e, 0x60, 0x70, 0x13, byte(ponPortId), byte(id)},
-		PortNo:    0,
-	}
-	o.SerialNumber = o.NewSN(0, ponPortId, o.ID)
-	return o
-}
-
 func Test_Onu_SendEapolFlow(t *testing.T) {
 	onu := createMockOnu(1, 1, 900, 900)
 
diff --git a/internal/bbsim/devices/onu_state_machine_test.go b/internal/bbsim/devices/onu_state_machine_test.go
new file mode 100644
index 0000000..239869b
--- /dev/null
+++ b/internal/bbsim/devices/onu_state_machine_test.go
@@ -0,0 +1,166 @@
+/*
+ * Copyright 2018-present Open Networking Foundation
+
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+
+ * http://www.apache.org/licenses/LICENSE-2.0
+
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package devices
+
+import (
+	"gotest.tools/assert"
+	"testing"
+)
+
+func Test_Onu_StateMachine_enable(t *testing.T) {
+	olt := OltDevice{
+		ID: 0,
+	}
+	pon := PonPort{
+		ID: 1,
+	}
+	onu := CreateONU(olt, pon, 1, 900, 900)
+
+	assert.Equal(t, onu.InternalState.Current(), "created")
+	onu.InternalState.Event("discover")
+	assert.Equal(t, onu.InternalState.Current(), "discovered")
+	onu.InternalState.Event("enable")
+	assert.Equal(t, onu.InternalState.Current(), "enabled")
+}
+
+func Test_Onu_StateMachine_eapol_start_eap_flow(t *testing.T) {
+	olt := OltDevice{
+		ID: 0,
+	}
+	pon := PonPort{
+		ID: 1,
+	}
+	onu := CreateONU(olt, pon, 1, 900, 900)
+
+	onu.InternalState.SetState("enabled")
+
+	// TODO we need to add a check so that you can't go from eapol_flow_received
+	// to auth_started without passing through gem_port_added
+	// (see start_dhcp for an example)
+
+	assert.Equal(t, onu.InternalState.Current(), "enabled")
+	onu.InternalState.Event("receive_eapol_flow")
+	assert.Equal(t, onu.InternalState.Current(), "eapol_flow_received")
+	onu.InternalState.Event("add_gem_port")
+	assert.Equal(t, onu.InternalState.Current(), "gem_port_added")
+	onu.InternalState.Event("start_auth")
+	assert.Equal(t, onu.InternalState.Current(), "auth_started")
+}
+
+func Test_Onu_StateMachine_eapol_start_gem_port(t *testing.T) {
+	olt := OltDevice{
+		ID: 0,
+	}
+	pon := PonPort{
+		ID: 1,
+	}
+	onu := CreateONU(olt, pon, 1, 900, 900)
+
+	onu.InternalState.SetState("enabled")
+
+	// TODO we need to add a check so that you can't go from gem_port_added
+	// to auth_started without passing through eapol_flow_received
+	// (see start_dhcp for an example)
+
+	assert.Equal(t, onu.InternalState.Current(), "enabled")
+	onu.InternalState.Event("add_gem_port")
+	assert.Equal(t, onu.InternalState.Current(), "gem_port_added")
+	onu.InternalState.Event("receive_eapol_flow")
+	assert.Equal(t, onu.InternalState.Current(), "eapol_flow_received")
+	onu.InternalState.Event("start_auth")
+	assert.Equal(t, onu.InternalState.Current(), "auth_started")
+}
+
+func Test_Onu_StateMachine_eapol_states(t *testing.T) {
+	olt := OltDevice{
+		ID: 0,
+	}
+	pon := PonPort{
+		ID: 1,
+	}
+	onu := CreateONU(olt, pon, 1, 900, 900)
+
+	onu.InternalState.SetState("auth_started")
+
+	assert.Equal(t, onu.InternalState.Current(), "auth_started")
+	onu.InternalState.Event("eap_start_sent")
+	assert.Equal(t, onu.InternalState.Current(), "eap_start_sent")
+	onu.InternalState.Event("eap_response_identity_sent")
+	assert.Equal(t, onu.InternalState.Current(), "eap_response_identity_sent")
+	onu.InternalState.Event("eap_response_challenge_sent")
+	assert.Equal(t, onu.InternalState.Current(), "eap_response_challenge_sent")
+	onu.InternalState.Event("eap_response_success_received")
+	assert.Equal(t, onu.InternalState.Current(), "eap_response_success_received")
+}
+
+func Test_Onu_StateMachine_dhcp_start(t *testing.T) {
+	olt := OltDevice{
+		ID: 0,
+	}
+	pon := PonPort{
+		ID: 1,
+	}
+	onu := CreateONU(olt, pon, 1, 900, 900)
+	onu.DhcpFlowReceived = true
+
+	onu.InternalState.SetState("eap_response_success_received")
+	assert.Equal(t, onu.InternalState.Current(), "eap_response_success_received")
+
+	onu.InternalState.Event("start_dhcp")
+
+	assert.Equal(t, onu.InternalState.Current(), "dhcp_started")
+}
+
+func Test_Onu_StateMachine_dhcp_start_error(t *testing.T) {
+	olt := OltDevice{
+		ID: 0,
+	}
+	pon := PonPort{
+		ID: 1,
+	}
+	onu := CreateONU(olt, pon, 1, 900, 900)
+
+	onu.InternalState.SetState("eap_response_success_received")
+	assert.Equal(t, onu.InternalState.Current(), "eap_response_success_received")
+
+	err := onu.InternalState.Event("start_dhcp")
+
+	assert.Equal(t, onu.InternalState.Current(), "eap_response_success_received")
+	assert.Equal(t, err.Error(), "transition canceled with error: cannot-go-to-dhcp-started-as-dhcp-flow-is-missing")
+}
+
+func Test_Onu_StateMachine_dhcp_states(t *testing.T) {
+	olt := OltDevice{
+		ID: 0,
+	}
+	pon := PonPort{
+		ID: 1,
+	}
+	onu := CreateONU(olt, pon, 1, 900, 900)
+
+	onu.DhcpFlowReceived = false
+
+	onu.InternalState.SetState("dhcp_started")
+
+	assert.Equal(t, onu.InternalState.Current(), "dhcp_started")
+	onu.InternalState.Event("dhcp_discovery_sent")
+	assert.Equal(t, onu.InternalState.Current(), "dhcp_discovery_sent")
+	onu.InternalState.Event("dhcp_request_sent")
+	assert.Equal(t, onu.InternalState.Current(), "dhcp_request_sent")
+	onu.InternalState.Event("dhcp_ack_received")
+	assert.Equal(t, onu.InternalState.Current(), "dhcp_ack_received")
+}
diff --git a/internal/bbsim/devices/onu_test_helpers.go b/internal/bbsim/devices/onu_test_helpers.go
new file mode 100644
index 0000000..1044913
--- /dev/null
+++ b/internal/bbsim/devices/onu_test_helpers.go
@@ -0,0 +1,118 @@
+/*
+ * Copyright 2018-present Open Networking Foundation
+
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+
+ * http://www.apache.org/licenses/LICENSE-2.0
+
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package devices
+
+import (
+	"context"
+	"errors"
+	"github.com/opencord/voltha-protos/go/openolt"
+	"github.com/opencord/voltha-protos/go/tech_profile"
+	"google.golang.org/grpc"
+	"net"
+)
+
+type FlowAddSpy struct {
+	CallCount int
+	Calls     map[int]*openolt.Flow
+}
+
+type mockClient struct {
+	FlowAddSpy
+	fail bool
+}
+
+func (s *mockClient) DisableOlt(ctx context.Context, in *openolt.Empty, opts ...grpc.CallOption) (*openolt.Empty, error) {
+	return nil, errors.New("unimplemented-in-mock-client")
+}
+func (s *mockClient) ReenableOlt(ctx context.Context, in *openolt.Empty, opts ...grpc.CallOption) (*openolt.Empty, error) {
+	return nil, errors.New("unimplemented-in-mock-client")
+}
+func (s *mockClient) ActivateOnu(ctx context.Context, in *openolt.Onu, opts ...grpc.CallOption) (*openolt.Empty, error) {
+	return nil, errors.New("unimplemented-in-mock-client")
+}
+func (s *mockClient) DeactivateOnu(ctx context.Context, in *openolt.Onu, opts ...grpc.CallOption) (*openolt.Empty, error) {
+	return nil, errors.New("unimplemented-in-mock-client")
+}
+func (s *mockClient) DeleteOnu(ctx context.Context, in *openolt.Onu, opts ...grpc.CallOption) (*openolt.Empty, error) {
+	return nil, errors.New("unimplemented-in-mock-client")
+}
+func (s *mockClient) OmciMsgOut(ctx context.Context, in *openolt.OmciMsg, opts ...grpc.CallOption) (*openolt.Empty, error) {
+	return nil, errors.New("unimplemented-in-mock-client")
+}
+func (s *mockClient) OnuPacketOut(ctx context.Context, in *openolt.OnuPacket, opts ...grpc.CallOption) (*openolt.Empty, error) {
+	return nil, errors.New("unimplemented-in-mock-client")
+}
+func (s *mockClient) UplinkPacketOut(ctx context.Context, in *openolt.UplinkPacket, opts ...grpc.CallOption) (*openolt.Empty, error) {
+	return nil, errors.New("unimplemented-in-mock-client")
+}
+func (s *mockClient) FlowAdd(ctx context.Context, in *openolt.Flow, opts ...grpc.CallOption) (*openolt.Empty, error) {
+	s.FlowAddSpy.CallCount++
+	if s.fail {
+		return nil, errors.New("fake-error")
+	}
+	s.FlowAddSpy.Calls[s.FlowAddSpy.CallCount] = in
+	return &openolt.Empty{}, nil
+}
+func (s *mockClient) FlowRemove(ctx context.Context, in *openolt.Flow, opts ...grpc.CallOption) (*openolt.Empty, error) {
+	return nil, errors.New("unimplemented-in-mock-client")
+}
+func (s *mockClient) HeartbeatCheck(ctx context.Context, in *openolt.Empty, opts ...grpc.CallOption) (*openolt.Heartbeat, error) {
+	return nil, errors.New("unimplemented-in-mock-client")
+}
+func (s *mockClient) EnablePonIf(ctx context.Context, in *openolt.Interface, opts ...grpc.CallOption) (*openolt.Empty, error) {
+	return nil, errors.New("unimplemented-in-mock-client")
+}
+func (s *mockClient) DisablePonIf(ctx context.Context, in *openolt.Interface, opts ...grpc.CallOption) (*openolt.Empty, error) {
+	return nil, errors.New("unimplemented-in-mock-client")
+}
+func (s *mockClient) GetDeviceInfo(ctx context.Context, in *openolt.Empty, opts ...grpc.CallOption) (*openolt.DeviceInfo, error) {
+	return nil, errors.New("unimplemented-in-mock-client")
+}
+func (s *mockClient) Reboot(ctx context.Context, in *openolt.Empty, opts ...grpc.CallOption) (*openolt.Empty, error) {
+	return nil, errors.New("unimplemented-in-mock-client")
+}
+func (s *mockClient) CollectStatistics(ctx context.Context, in *openolt.Empty, opts ...grpc.CallOption) (*openolt.Empty, error) {
+	return nil, errors.New("unimplemented-in-mock-client")
+}
+func (s *mockClient) CreateTrafficSchedulers(ctx context.Context, in *tech_profile.TrafficSchedulers, opts ...grpc.CallOption) (*openolt.Empty, error) {
+	return nil, errors.New("unimplemented-in-mock-client")
+}
+func (s *mockClient) RemoveTrafficSchedulers(ctx context.Context, in *tech_profile.TrafficSchedulers, opts ...grpc.CallOption) (*openolt.Empty, error) {
+	return nil, errors.New("unimplemented-in-mock-client")
+}
+func (s *mockClient) CreateTrafficQueues(ctx context.Context, in *tech_profile.TrafficQueues, opts ...grpc.CallOption) (*openolt.Empty, error) {
+	return nil, errors.New("unimplemented-in-mock-client")
+}
+func (s *mockClient) RemoveTrafficQueues(ctx context.Context, in *tech_profile.TrafficQueues, opts ...grpc.CallOption) (*openolt.Empty, error) {
+	return nil, errors.New("unimplemented-in-mock-client")
+}
+func (s *mockClient) EnableIndication(ctx context.Context, in *openolt.Empty, opts ...grpc.CallOption) (openolt.Openolt_EnableIndicationClient, error) {
+	return nil, errors.New("unimplemented-in-mock-client")
+}
+
+func createMockOnu(id uint32, ponPortId uint32, sTag int, cTag int) Onu {
+	o := Onu{
+		ID:        id,
+		PonPortID: ponPortId,
+		STag:      sTag,
+		CTag:      cTag,
+		HwAddress: net.HardwareAddr{0x2e, 0x60, 0x70, 0x13, byte(ponPortId), byte(id)},
+		PortNo:    0,
+	}
+	o.SerialNumber = o.NewSN(0, ponPortId, o.ID)
+	return o
+}