Added support for openflow 1.5. #13

ashish-varma · 2021-09-24T03:27:48Z

Added package openflow15 which implements the Openflow 1.5 protocol
Marshal and Unmarshal functions.
Added the unit test (openflow15_test.go) file to test the Marshal/
Unmarshal of every Openflow 1.5 message.
Modified the README.md file to include support of Openflow 1.5.
Updated the VERSION file.

Signed-off-by: Ashish Varma ashishvarma.ovs@gmail.com

antoninbas

Before I review this, I'd like to understand why there seems to be no code reuse at all between the OF 1.3 implementation and the OF 1.5 implementation? I would expect OF 1.5 to incrementally build on previous OF versions and there should be a way to have mostly common code, while letting the library users import either openflow13 or openflow15.

wenyingd · 2021-09-27T01:35:31Z

Before I review this, I'd like to understand why there seems to be no code reuse at all between the OF 1.3 implementation and the OF 1.5 implementation? I would expect OF 1.5 to incrementally build on previous OF versions and there should be a way to have mostly common code, while letting the library users import either openflow13 or openflow15.

Hi Antonin, we used to have a discussion about how to support OF1.5, and we come to some agreements: 1) Antrea will switch to OF 1.5 completely after it is ready, and not use OF1.3 any more; 2) it is easier and quicker to support OF 1.5 only than support both OF 1.3 and OF 1.5 in libOpenflow. So Ashish chose to add a pure support OF 1.5 but not modify common code to support both.

antoninbas · 2021-09-27T19:08:16Z

Thanks for the context @wenyingd. I did discuss with @ashish-varma offline on Friday and he explained the same thing to me. I am fine with the approach.

openflow15/action.go

openflow15/bundles.go

openflow15/openflow15.go

openflow15/bundles.go

wenyingd · 2021-11-02T03:34:21Z

openflow15/flowmod.go

+	}
+	n += int(f.Stats.Len())
+
+	return err


s/return err/return nil/

I generally prefer to return err so that in case the 'err' gets set, we return it.
I understand that in this particular function, we check for 'err != nil' and return immediately, but keeping 'return err' would make sure we don't accidentally return nil for an actual error.

wenyingd · 2021-11-02T03:35:17Z

openflow15/group.go

+func (g *GroupMod) Len() (n uint16) {
+	n = g.Header.Len()
+	n += 16
+	//	if g.Command == OFPGC_DELETE {


Need to remove these lines?

Sure. Would remove.

wenyingd · 2021-11-02T03:42:17Z

openflow15/group.go

+}
+
+// Add a bucket to group mod
+func (g *GroupMod) AddBucket(bkt Bucket) {


Could you help provide functions to support InsertBucket/RemoveBucket in a group?

Yes. Would add.

Would add as another commit.

Is support for InsertBucket/RemoveBucket in a group added?

wenyingd · 2021-11-02T03:46:42Z

openflow15/match.go

+		n += len(b)
+	}
+
+	/* See if we need to pad it to make it align to 64bit boundary


Do you want to keep these line here or remove it?

wenyingd · 2021-11-02T04:03:34Z

openflow15/multipart.go

+		if err != nil {
+			return err
+		}
+		if req == nil {


For the case that the request body is empty (e.g., MultipartType_ControllerStatus, MultipartType_MeterFeatures, MultipartType_TableDesc), req should be nil, then this error is not expected, correct?

That is right. I will fix this.

openflow15/action.go

tnqn · 2021-12-16T17:30:40Z

openflow15/bundles.go

+	n += 2
+	binary.BigEndian.PutUint32(data[n:], e.ExperimenterID)
+	n += 4
+	headerBytes, err = e.Data.MarshalBinary()


Sure. Would do.

tnqn · 2021-12-16T17:39:42Z

openflow15/group.go

+	GroupId        uint32 /* Group identifier. */
+	BucketArrayLen uint16 /* Length of action buckets data. */
+	Pad            uint16
+	ComandBucketId uint32 /* Bucket Id used as part of


Suggested change

ComandBucketId uint32 /* Bucket Id used as part of

CommandBucketId uint32 /* Bucket Id used as part of

tnqn · 2021-12-16T17:51:14Z

openflow15/openflow15.go

+)
+
+func Parse(b []byte) (message util.Message, err error) {
+	fmt.Println("Parse_Type=", b[1])


Forgot to remove?

Yeah. Had added during unit test. Would remove.

tnqn · 2021-12-16T17:52:55Z

openflow15/openflow15.go

+	AuxilaryId   uint8
+	pad          []uint8 // Size 2
+	Capabilities uint32
+	//Actions      uint32


Do we need this?

No. Would Remove.

tnqn · 2021-12-16T17:53:13Z

openflow15/openflow15.go

+	n += uint16(len(s.DPID))
+	n += 16
+	/*
+		for _, p := range s.Ports {


Would take care. Thanks.

tnqn · 2021-12-16T17:55:10Z

openflow15_test.go

+	f := openflow15.NewInPortField(5)
+	flowMod.Match.AddField(*f)
+	//f = openflow15.NewEthTypeField(0x800)
+	//flowMod.Match.AddField(*f)


remove them?

wenyingd · 2022-04-20T06:30:23Z

openflow15/openflow15.go

+	Cookie   uint64
+	Match    Match
+	pad      []uint8
+	Data     util.Message


Why do you change the typd of Data from Ethernet to util.Message?

As per Openflow 1.5, 7.2.3.11
"Packet-in messages must include the Packet Type Match Field in the match, unless the packet type is Ethernet."

This means that the packet included in the Packet In message could be Ethernet or other types. Does this break any existing functionality?

Not a blocking issue. I asked it because Antrea thought the Data filed type is Ethernet with OF1.3, but it changes when switching to OF1.5. This caused a compilation issue. But if it is as designed, I could change Antrea code to be compatible with the new field type.

wenyingd · 2022-04-26T11:04:13Z

openflow15/match.go

+}
+
+// oxm_id - OXM TLV Header
+type OxmId struct {


Is this expected to use in flow action to represent a different field? In OpenFlow13, I use MatchField for this purpose, please add some functions to help users to create the target OxmId with a name string (e.g., the existing function FindFieldHeaderByName ) if you want it for the same purpose. It is because we can't expect all dev know every field's class, field, length, experimenterID.

Added a helper function to get the OxmId from a string. 'FindOxmIdByName'.

wenyingd · 2022-05-11T09:26:49Z

openflow15/match.go

+
+// IP_DSCP field
+type IpDscpField struct {
+	dscp uint8


Hi @ashish-varma , would you change this field as public? I know it is not introduced in your patch, but we need to set a masked field for IpDscpField, but the existing struct doesn't support this.

openflow15/openflow15.go

wenyingd · 2022-06-10T07:09:06Z

openflow15/openflow15.go

+	if p.Data != nil {
+		n += p.Data.Len()
+	}
+	//if n < 72 { return 72 }


Is this line needed?

Would remove it.

wenyingd · 2022-06-10T07:14:49Z

openflow15/openflow15.go

+	copy(b[0:], p.pad)
+	data = append(data, b...)
+
+	b, err = p.Data.MarshalBinary()


Misse err check?

wenyingd · 2022-06-10T07:19:18Z

openflow15/openflow15.go

+	var err error
+	n := 0
+
+	err = c.Header.UnmarshalBinary(data[n:])


ditto, err check

wenyingd · 2022-06-10T07:23:25Z

openflow15/openflow15.go

+	AuxilaryId   uint8
+	pad          []uint8 // Size 2
+	Capabilities uint32
+	Reserved     uint32


So Ports and Actions are removed from SwitchFeatures? Then how shall we get these information?

wenyingd · 2022-06-10T08:03:11Z

openflow15/match.go

+
+// IP_PROTO field
+type IpProtoField struct {
+	protocol uint8


Not introduced by you, but it should be better to use a public property for such a library, then it is convenient for callers to reset the values if necessary.

wenyingd · 2022-06-10T08:03:35Z

openflow15/match.go

+
+// Common struct for all port fields
+type PortField struct {
+	port uint16


wenyingd · 2022-06-10T08:04:14Z

openflow15/match.go

+
+// PACKET_TYPE field
+type PacketTypeField struct {
+	namespace uint16


wenyingd · 2022-06-10T08:14:52Z

openflow15/multipart.go

+)
+
+func NewTableFeatures(t uint8) *TableFeatures {
+	n := new(TableFeatures)


use tf as the variable ( instead of n ) ?

wenyingd · 2022-06-10T08:20:59Z

openflow15/nx_util.go

+	}, nil
+}
+
+func FindOxmIdByName(fieldName string, hasMask bool) (*OxmId, error) {


First, I am confuse if we could use OxmId instead of MatchField directly? Second, it looks the code is almost the same for these to functions, I think you can call FindFieldHeaderByName inside FindOxmIdByName, and generate an OxmId pointer object with *MatchField properties.

Changed the function to use FindFieldHeaderByName inside FindOxmIdByName.

hongliangl

Not finished yet.

hongliangl · 2022-06-15T02:34:53Z

openflow15/action.go

+	n += 2
+	copy(a.pad, data[n:n+6])
+	n += 6
+	return err


Suggested change

return err

return nil

hongliangl · 2022-06-15T02:36:15Z

openflow15/action.go

+	a.GroupId = binary.BigEndian.Uint32(data[n:])
+	n += 4
+
+	return err


Suggested change

return err

return nil

hongliangl · 2022-06-15T02:37:34Z

openflow15/action.go

+	}
+	n += int(a.Field.Len())
+
+	return err


Suggested change

return err

return nil

hongliangl · 2022-06-15T02:39:08Z

openflow15/action.go

+	}
+	n += a.OxmIdDst.Len()
+
+	return err


Suggested change

return err

return nil

hongliangl · 2022-06-15T03:25:40Z

openflow15/group.go

+	data = append(data, bytes...)
+
+	for _, bkt := range g.Buckets {
+		bytes, err = bkt.MarshalBinary()


Why not handling the err?

hongliangl · 2022-06-15T03:39:29Z

openflow15/group.go

+	}
+
+	for _, p := range b.Properties {
+		bytes, err = p.MarshalBinary()


hongliangl · 2022-06-15T03:53:06Z

openflow15/group.go

+}
+
+// Add a bucket to group mod
+func (g *GroupMod) AddBucket(bkt Bucket) {


Is support for InsertBucket/RemoveBucket in a group added?

* Add unit tests for ClickHouse monitor Signed-off-by: Yanjun Zhou <zhouya@vmware.com>

wenyingd · 2022-06-24T14:46:09Z

openflow15/match.go

+// PACKET_TYPE field
+type PacketTypeField struct {
+	Namespace uint16
+	Ns_type   uint16


nit: do not use charactors like _ or - in the name of a property/type/object/variable/constant.

wenyingd

LGTM

wenyingd · 2022-06-29T02:34:58Z

@hongliangl @tnqn Would you give a last round review on this PR?

hongliangl · 2022-07-04T09:54:54Z

LGTM, no further comment.

tnqn

Thanks @ashish-varma. LGTM overall, a few errors not handled and a message length issue.

openflow15/flowmod.go

tnqn · 2022-07-05T03:50:56Z

openflow15/flowstats.go

+func (s *Stats) MarshalBinary() (data []byte, err error) {
+	data = make([]byte, int(s.Len()))
+	n := 2
+	s.Length = s.Len() - 8 // 'Pad', 'Reserved' & 'Length' not part of Length


The above comment says Length is length of ofp_stats excluding padding and length of Fields should be length - 4, which means the length should include Reserved & Length.
I also checked spec of Flow Stats Header, it should include Reserved & Length.

You are right. Fixed.

openflow15/group.go

* Added package openflow15 which implements the Openflow 1.5 protocol Marshal and Unmarshal functions. * Added the unit test (openflow15_test.go) file to test the Marshal/ Unmarshal of every Openflow 1.5 message. * Modified the README.md file to include support of Openflow 1.5. * Updated the VERSION file. Signed-off-by: Ashish Varma <ashishvarma.ovs@gmail.com>

tnqn

LGTM

ashish-varma requested review from wenyingd and antoninbas September 24, 2021 04:55

antoninbas reviewed Sep 24, 2021

View reviewed changes

wenyingd requested a review from tnqn September 27, 2021 01:28

wenyingd reviewed Oct 14, 2021

View reviewed changes

wenyingd mentioned this pull request Oct 19, 2021

OpenFlow 1.5 support antrea-io/antrea#2115

Closed

ashish-varma force-pushed the main branch 2 times, most recently from 18deaf1 to 14e83a5 Compare October 21, 2021 07:30

wenyingd requested review from jianjuns and salv-orlando November 2, 2021 03:31

wenyingd reviewed Nov 2, 2021

View reviewed changes

tnqn reviewed Dec 16, 2021

View reviewed changes

ashish-varma force-pushed the main branch from 14e83a5 to ac17379 Compare March 28, 2022 21:09

ashish-varma force-pushed the main branch 2 times, most recently from d719e11 to d753d62 Compare April 6, 2022 07:23

wenyingd reviewed Apr 20, 2022

View reviewed changes

wenyingd reviewed Apr 26, 2022

View reviewed changes

wenyingd reviewed May 11, 2022

View reviewed changes

ashish-varma force-pushed the main branch 2 times, most recently from 3a4010a to ed02932 Compare May 12, 2022 00:14

wenyingd reviewed May 17, 2022

View reviewed changes

openflow15/openflow15.go Show resolved Hide resolved

ashish-varma force-pushed the main branch 3 times, most recently from b78a7aa to 5dc6e23 Compare May 25, 2022 04:59

wenyingd reviewed Jun 10, 2022

View reviewed changes

hongliangl reviewed Jun 15, 2022

View reviewed changes

ruicao93 pushed a commit to ruicao93/libOpenflow that referenced this pull request Jun 19, 2022

Add unit tests for ClickHouse monitor (antrea-io#13)

34915e7

* Add unit tests for ClickHouse monitor Signed-off-by: Yanjun Zhou <zhouya@vmware.com>

ashish-varma force-pushed the main branch from 5dc6e23 to 17b81d7 Compare June 24, 2022 02:02

wenyingd reviewed Jun 24, 2022

View reviewed changes

ashish-varma force-pushed the main branch from 17b81d7 to 0f4f2a5 Compare June 24, 2022 18:03

wenyingd previously approved these changes Jun 27, 2022

View reviewed changes

tnqn reviewed Jul 5, 2022

View reviewed changes

ashish-varma dismissed wenyingd’s stale review via 8943551 July 6, 2022 03:39

ashish-varma force-pushed the main branch from 0f4f2a5 to 8943551 Compare July 6, 2022 03:39

wenyingd requested a review from tnqn July 8, 2022 00:00

tnqn approved these changes Jul 8, 2022

View reviewed changes

wenyingd approved these changes Jul 8, 2022

View reviewed changes

wenyingd merged commit 02150d8 into antrea-io:main Jul 8, 2022

	ComandBucketId uint32 /* Bucket Id used as part of
	CommandBucketId uint32 /* Bucket Id used as part of

Added support for openflow 1.5. #13

Added support for openflow 1.5. #13

Conversation

ashish-varma commented Sep 24, 2021

antoninbas left a comment

Choose a reason for hiding this comment

wenyingd commented Sep 27, 2021

antoninbas commented Sep 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashish-varma Jan 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenyingd Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hongliangl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenyingd left a comment

Choose a reason for hiding this comment

wenyingd commented Jun 29, 2022

hongliangl commented Jul 4, 2022

tnqn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

ashish-varma Jan 5, 2022 •

edited

Loading

wenyingd Apr 26, 2022 •

edited

Loading